[libvirt PATCH 00/25] cgroup related fixes and cleanup
Pavel Hrdina (25): qemu_cgroup: remove unused @empty variable qemu: remove dead code that setup cgroups for helper processes qemu_dbus: use emulator cgroup for dbus-daemon vircgroupv2: properly detect empty tasks vircgroupv2: properly detect placement of running VM vircgroupv2: detect controllers enabled in parent cgroup vircgroup: remove useless cgroup->path variable vircgroup: introduce virCgroupSetBackends helper vircgroup: introduce virCgroupCopyMounts helper vircgroup: introduce virCgroupCopyPlacement helper vircgroup: introduce virCgroupValidatePlacement helper vircgroup: introduce virCgroupDetectControllers helper vircgroup: extract virCgroupNewDetect from virCgroupNew vircgroup: introduce virCgroupNewParent vircgroup: drop @parent from virCgroupNew vircgroup: virCgroupNew is now always called with absolute path vircgroup: expand virCgroupDetect into virCgroupNew vircgroup: no need to use PID in virCgroupEnableMissingControllers vircgroup: drop @pid argument from virCgroupNew vircgroup: introduce virCgroupSetPlacement vircgroup: drop @create from virCgroupNewDomainPartition vircgroup: refactor virCgroupEnableMissingControllers vircgroup: move parentPath declaration vircgroup: refactor virCgroupNewPartition vircgroup: drop condition for absolute path from copyPlacement callbacks src/qemu/qemu_cgroup.c | 5 +- src/qemu/qemu_dbus.c | 9 +- src/qemu/qemu_dbus.h | 3 +- src/qemu/qemu_extdevice.c | 2 +- src/qemu/qemu_slirp.c | 4 - src/util/vircgroup.c | 337 ++++++++++++++++++++++-------------- src/util/vircgroupbackend.h | 5 + src/util/vircgrouppriv.h | 7 +- src/util/vircgroupv1.c | 57 +++--- src/util/vircgroupv2.c | 61 ++++--- tests/vircgrouptest.c | 33 ++-- 11 files changed, 309 insertions(+), 214 deletions(-) -- 2.26.2
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_cgroup.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 210cac9866..10fdc7444d 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -988,7 +988,6 @@ static void qemuRestoreCgroupState(virDomainObjPtr vm) { g_autofree char *mem_mask = NULL; - int empty = -1; qemuDomainObjPrivatePtr priv = vm->privateData; size_t i = 0; g_autoptr(virBitmap) all_nodes = NULL; @@ -1003,8 +1002,8 @@ qemuRestoreCgroupState(virDomainObjPtr vm) if (!(mem_mask = virBitmapFormat(all_nodes))) goto error; - if ((empty = virCgroupHasEmptyTasks(priv->cgroup, - VIR_CGROUP_CONTROLLER_CPUSET)) <= 0) + if (virCgroupHasEmptyTasks(priv->cgroup, + VIR_CGROUP_CONTROLLER_CPUSET) <= 0) goto error; if (virCgroupSetCpusetMems(priv->cgroup, mem_mask) < 0) -- 2.26.2
In both cases priv->cgroup will always be NULL because it is called before the QEMU process is started and cgroups are configured. In qemuProcessLaunch() the call order is following: qemuExtDevicesStart() ... virCommandRun() ... qemuSetupCgroup() where qemuDBusStart() is called from qemuExtDevicesStart() but we cgroups are created in qemuSetupCgroup(). Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_dbus.c | 4 ---- src/qemu/qemu_slirp.c | 4 ---- 2 files changed, 8 deletions(-) diff --git a/src/qemu/qemu_dbus.c b/src/qemu/qemu_dbus.c index 81042876fe..a0567e55a3 100644 --- a/src/qemu/qemu_dbus.c +++ b/src/qemu/qemu_dbus.c @@ -268,10 +268,6 @@ qemuDBusStart(virQEMUDriverPtr driver, goto cleanup; } - if (priv->cgroup && - virCgroupAddProcess(priv->cgroup, cpid) < 0) - goto cleanup; - if (qemuSecurityDomainSetPathLabel(driver, vm, sockpath, false) < 0) goto cleanup; diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c index dfb36125f0..4c4949f88b 100644 --- a/src/qemu/qemu_slirp.c +++ b/src/qemu/qemu_slirp.c @@ -251,7 +251,6 @@ qemuSlirpStart(qemuSlirpPtr slirp, virDomainNetDefPtr net, bool incoming) { - qemuDomainObjPrivatePtr priv = vm->privateData; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autoptr(virCommand) cmd = NULL; g_autofree char *pidfile = NULL; @@ -356,9 +355,6 @@ qemuSlirpStart(qemuSlirpPtr slirp, slirp->pid = pid; - if (priv->cgroup && qemuSlirpSetupCgroup(slirp, priv->cgroup) < 0) - goto error; - return 0; error: -- 2.26.2
All other helper processes are moved to cgroup with QEMU emulator thread as we keep the root VM cgroup without any processes. This assumption is validated in qemuRestoreCgroupState() which is called when libvirtd is restarted and reconnected to all running VMs. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_dbus.c | 5 +++-- src/qemu/qemu_dbus.h | 3 ++- src/qemu/qemu_extdevice.c | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_dbus.c b/src/qemu/qemu_dbus.c index a0567e55a3..ffcf83e5da 100644 --- a/src/qemu/qemu_dbus.c +++ b/src/qemu/qemu_dbus.c @@ -148,7 +148,8 @@ qemuDBusStop(virQEMUDriverPtr driver, int qemuDBusSetupCgroup(virQEMUDriverPtr driver, - virDomainObjPtr vm) + virDomainObjPtr vm, + virCgroupPtr cgroup) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData; @@ -167,7 +168,7 @@ qemuDBusSetupCgroup(virQEMUDriverPtr driver, return -1; } - return virCgroupAddProcess(priv->cgroup, cpid); + return virCgroupAddProcess(cgroup, cpid); } int diff --git a/src/qemu/qemu_dbus.h b/src/qemu/qemu_dbus.h index 3c2145a223..e3ce1330fd 100644 --- a/src/qemu/qemu_dbus.h +++ b/src/qemu/qemu_dbus.h @@ -35,4 +35,5 @@ int qemuDBusVMStateAdd(virDomainObjPtr vm, const char *id); void qemuDBusVMStateRemove(virDomainObjPtr vm, const char *id); int qemuDBusSetupCgroup(virQEMUDriverPtr driver, - virDomainObjPtr vm); + virDomainObjPtr vm, + virCgroupPtr cgroup); diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c index 2fb71dd334..8fe7ceaa10 100644 --- a/src/qemu/qemu_extdevice.c +++ b/src/qemu/qemu_extdevice.c @@ -280,7 +280,7 @@ qemuExtDevicesSetupCgroup(virQEMUDriverPtr driver, virDomainDefPtr def = vm->def; size_t i; - if (qemuDBusSetupCgroup(driver, vm) < 0) + if (qemuDBusSetupCgroup(driver, vm, cgroup) < 0) return -1; for (i = 0; i < def->nvideos; i++) { -- 2.26.2
With cgroups v2 the file cgroup.procs will never be empty if threading is enabled as it will always have ID of all processes even if all threads of the processes are moved to sub-cgroups. If that happens the file cgroup.threads will be empty. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroupv2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index 285b7675d9..65f5cc6bd0 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -521,7 +521,7 @@ virCgroupV2HasEmptyTasks(virCgroupPtr cgroup, int ret = -1; g_autofree char *content = NULL; - ret = virCgroupGetValueStr(cgroup, controller, "cgroup.procs", &content); + ret = virCgroupGetValueStr(cgroup, controller, "cgroup.threads", &content); if (ret == 0 && content[0] == '\0') ret = 1; -- 2.26.2
When libvirtd starts a VM it internally stores a path to the main cgroup. When we restart libvirtd we should get to the same state. When we start a VM on host with systemd the cgroup is created for us and the process is already placed into that cgroup and we detect the path created by systemd using /proc/$PID/cgroup. After that we create sub-cgroups and move all threads there. Once libvirtd is restarted we again detect the cgroup path using /proc/$PID/cgroup, but in this case we will get a different path because the main thread was moved to a "emulator" cgroup. Instead of ignoring the "emulator" directory when validating cgroups remove it completely when detecting cgroup otherwise cgroups will not work properly when libvirtd is restarted. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroupv2.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index 65f5cc6bd0..fb97b7fd2a 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -122,12 +122,6 @@ virCgroupV2ValidateMachineGroup(virCgroupPtr group, if (!(tmp = strrchr(group->unified.placement, '/'))) return false; - if (STREQ(tmp, "/emulator")) { - *tmp = '\0'; - - if (!(tmp = strrchr(group->unified.placement, '/'))) - return false; - } tmp++; if (STRNEQ(tmp, partmachinename) && @@ -198,6 +192,9 @@ virCgroupV2DetectPlacement(virCgroupPtr group, const char *controllers, const char *selfpath) { + g_autofree char *placement = g_strdup(selfpath); + char *tmp = NULL; + if (group->unified.placement) return 0; @@ -208,12 +205,18 @@ virCgroupV2DetectPlacement(virCgroupPtr group, if (STRNEQ(controllers, "")) return 0; + /* Running VM will have the main thread placed in emulator cgroup + * but we need to get the main cgroup. */ + tmp = g_strrstr(placement, "/emulator"); + if (tmp) + *tmp = '\0'; + /* * selfpath == "/" + path="" -> "/" * selfpath == "/libvirt.service" + path == "" -> "/libvirt.service" * selfpath == "/libvirt.service" + path == "foo" -> "/libvirt.service/foo" */ - group->unified.placement = g_strdup_printf("%s%s%s", selfpath, + group->unified.placement = g_strdup_printf("%s%s%s", placement, (STREQ(selfpath, "/") || STREQ(path, "") ? "" : "/"), path); return 0; -- 2.26.2
With cgroups v2 working with controllers is a bit more complicated then with cgroups v1 where the controller had to be mounted. There are two files, cgroups.controllers and cgroup.subtree_control. The file cgroup.controllers lists all controllers enabled in the current cgroup and cgroups.subtree_control, as the name suggest, controls which controllers are enabled for a subtree of cgroups. Now the issue here is that the current code doesn't make any difference if the @parent variable is NULL or not because ../cgroup.subtree_control will list the same controllers as ./cgroup.controllers. The whole point of the @parent variable is when we are building the cgroup topology ourselves without systemd help we need to detect which controllers are enabled in the parent cgroup in order to enable them for the current cgroup as well and for that we need to check cgroup.controllers of the parent group. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroupv2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index fb97b7fd2a..530e5d2ce9 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -255,7 +255,7 @@ virCgroupV2ParseControllersFile(virCgroupPtr group, char **tmp; if (parent) { - contFile = g_strdup_printf("%s%s/cgroup.subtree_control", + contFile = g_strdup_printf("%s%s/cgroup.controllers", parent->unified.mountPoint, NULLSTR_EMPTY(parent->unified.placement)); } else { -- 2.26.2
It is only used for debug and error purposes which can be easily replaced by @placement. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 20 ++++++-------------- src/util/vircgrouppriv.h | 2 -- src/util/vircgroupv1.c | 8 +++----- src/util/vircgroupv2.c | 4 +--- tests/vircgrouptest.c | 29 ++++++++++------------------- 5 files changed, 20 insertions(+), 43 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index e3c45e93cb..7ff23e1218 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -669,13 +669,6 @@ virCgroupNew(pid_t pid, *group = NULL; newGroup = g_new0(virCgroup, 1); - if (path[0] == '/' || !parent) { - newGroup->path = g_strdup(path); - } else { - newGroup->path = g_strdup_printf("%s%s%s", parent->path, - STREQ(parent->path, "") ? "" : "/", path); - } - if (virCgroupDetect(newGroup, pid, controllers, path, parent) < 0) return -1; @@ -2388,8 +2381,8 @@ virCgroupKillInternal(virCgroupPtr group, g_autofree char *keypath = NULL; bool done = false; FILE *fp = NULL; - VIR_DEBUG("group=%p path=%s signum=%d pids=%p", - group, group->path, signum, pids); + VIR_DEBUG("group=%p signum=%d pids=%p", + group, signum, pids); if (virCgroupPathOfController(group, controller, taskFile, &keypath) < 0) return -1; @@ -2471,8 +2464,8 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, g_autoptr(DIR) dp = NULL; struct dirent *ent; int direrr; - VIR_DEBUG("group=%p path=%s signum=%d pids=%p", - group, group->path, signum, pids); + VIR_DEBUG("group=%p signum=%d pids=%p", + group, signum, pids); if (virCgroupPathOfController(group, controller, "", &keypath) < 0) return -1; @@ -2532,7 +2525,7 @@ virCgroupKillRecursive(virCgroupPtr group, int signum) virCgroupBackendPtr *backends = virCgroupBackendGetAll(); g_autoptr(GHashTable) pids = g_hash_table_new_full(g_int64_hash, g_int64_equal, g_free, NULL); - VIR_DEBUG("group=%p path=%s signum=%d", group, group->path, signum); + VIR_DEBUG("group=%p signum=%d", group, signum); for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { if (backends && backends[i] && backends[i]->available()) { @@ -2563,7 +2556,7 @@ virCgroupKillPainfully(virCgroupPtr group) { size_t i; int ret; - VIR_DEBUG("cgroup=%p path=%s", group, group->path); + VIR_DEBUG("cgroup=%p", group); for (i = 0; i < 15; i++) { int signum; if (i == 0) @@ -3491,7 +3484,6 @@ virCgroupFree(virCgroupPtr group) VIR_FREE(group->unified.mountPoint); VIR_FREE(group->unified.placement); - VIR_FREE(group->path); VIR_FREE(group); } diff --git a/src/util/vircgrouppriv.h b/src/util/vircgrouppriv.h index f85a36efdb..af8bf99888 100644 --- a/src/util/vircgrouppriv.h +++ b/src/util/vircgrouppriv.h @@ -60,8 +60,6 @@ typedef struct _virCgroupV2Controller virCgroupV2Controller; typedef virCgroupV2Controller *virCgroupV2ControllerPtr; struct _virCgroup { - char *path; - virCgroupBackendPtr backends[VIR_CGROUP_BACKEND_TYPE_LAST]; virCgroupV1Controller legacy[VIR_CGROUP_CONTROLLER_LAST]; diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c index 61fb359f8d..6bb819410b 100644 --- a/src/util/vircgroupv1.c +++ b/src/util/vircgroupv1.c @@ -545,7 +545,9 @@ virCgroupV1CpuSetInherit(virCgroupPtr parent, "cpuset.memory_migrate", }; - VIR_DEBUG("Setting up inheritance %s -> %s", parent->path, group->path); + VIR_DEBUG("Setting up inheritance %s -> %s", + parent->legacy[VIR_CGROUP_CONTROLLER_CPUSET].placement, + group->legacy[VIR_CGROUP_CONTROLLER_CPUSET].placement); for (i = 0; i < G_N_ELEMENTS(inherit_values); i++) { g_autofree char *value = NULL; @@ -583,7 +585,6 @@ virCgroupV1SetMemoryUseHierarchy(virCgroupPtr group) if (value == 1) return 0; - VIR_DEBUG("Setting up %s/%s", group->path, filename); if (virCgroupSetValueU64(group, VIR_CGROUP_CONTROLLER_MEMORY, filename, 1) < 0) @@ -601,7 +602,6 @@ virCgroupV1MakeGroup(virCgroupPtr parent, { size_t i; - VIR_DEBUG("Make group %s", group->path); for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { g_autofree char *path = NULL; @@ -671,7 +671,6 @@ virCgroupV1Remove(virCgroupPtr group) int rc = 0; size_t i; - VIR_DEBUG("Removing cgroup %s", group->path); for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { g_autofree char *grppath = NULL; @@ -697,7 +696,6 @@ virCgroupV1Remove(virCgroupPtr group) VIR_DEBUG("Removing cgroup %s and all child cgroups", grppath); rc = virCgroupRemoveRecursively(grppath); } - VIR_DEBUG("Done removing cgroup %s", group->path); return rc; } diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index 530e5d2ce9..0467a72566 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -413,8 +413,6 @@ virCgroupV2MakeGroup(virCgroupPtr parent, return 0; } - VIR_DEBUG("Make group %s", group->path); - controller = virCgroupV2GetAnyController(group); if (virCgroupV2PathOfController(group, controller, "", &path) < 0) return -1; @@ -424,7 +422,7 @@ virCgroupV2MakeGroup(virCgroupPtr parent, if (!virFileExists(path) && (!create || (mkdir(path, 0755) < 0 && errno != EEXIST))) { virReportSystemError(errno, _("Failed to create v2 cgroup '%s'"), - group->path); + path); return -1; } diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c index 6c1a766fa2..0dd5e3f0b4 100644 --- a/tests/vircgrouptest.c +++ b/tests/vircgrouptest.c @@ -38,7 +38,6 @@ VIR_LOG_INIT("tests.cgrouptest"); static int validateCgroup(virCgroupPtr cgroup, - const char *expectPath, const char **expectMountPoint, const char **expectLinkPoint, const char **expectPlacement, @@ -48,12 +47,6 @@ static int validateCgroup(virCgroupPtr cgroup, { size_t i; - if (STRNEQ(cgroup->path, expectPath)) { - fprintf(stderr, "Wrong path '%s', expected '%s'\n", - cgroup->path, expectPath); - return -1; - } - for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { if (STRNEQ_NULLABLE(expectMountPoint[i], cgroup->legacy[i].mountPoint)) { @@ -240,7 +233,7 @@ static int testCgroupNewForSelf(const void *args G_GNUC_UNUSED) return -1; } - return validateCgroup(cgroup, "", mountsFull, links, placement, NULL, NULL, 0); + return validateCgroup(cgroup, mountsFull, links, placement, NULL, NULL, 0); } @@ -314,14 +307,14 @@ static int testCgroupNewForPartition(const void *args G_GNUC_UNUSED) fprintf(stderr, "Cannot create /virtualmachines cgroup: %d\n", -rv); return -1; } - rv = validateCgroup(cgroup, "/virtualmachines.partition", mountsSmall, links, placementSmall, NULL, NULL, 0); + rv = validateCgroup(cgroup, mountsSmall, links, placementSmall, NULL, NULL, 0); virCgroupFree(cgroup); if ((rv = virCgroupNewPartition("/virtualmachines", true, -1, &cgroup)) != 0) { fprintf(stderr, "Cannot create /virtualmachines cgroup: %d\n", -rv); return -1; } - return validateCgroup(cgroup, "/virtualmachines.partition", mountsFull, links, placementFull, NULL, NULL, 0); + return validateCgroup(cgroup, mountsFull, links, placementFull, NULL, NULL, 0); } @@ -365,8 +358,7 @@ static int testCgroupNewForPartitionNested(const void *args G_GNUC_UNUSED) return -1; } - return validateCgroup(cgroup, "/deployment.partition/production.partition", - mountsFull, links, placementFull, NULL, NULL, 0); + return validateCgroup(cgroup, mountsFull, links, placementFull, NULL, NULL, 0); } @@ -416,8 +408,7 @@ static int testCgroupNewForPartitionNestedDeep(const void *args G_GNUC_UNUSED) return -1; } - return validateCgroup(cgroup, "/user/berrange.user/production.partition", - mountsFull, links, placementFull, NULL, NULL, 0); + return validateCgroup(cgroup, mountsFull, links, placementFull, NULL, NULL, 0); } @@ -448,7 +439,7 @@ static int testCgroupNewForPartitionDomain(const void *args G_GNUC_UNUSED) return -1; } - return validateCgroup(domaincgroup, "/production.partition/foo.libvirt-lxc", mountsFull, links, placement, NULL, NULL, 0); + return validateCgroup(domaincgroup, mountsFull, links, placement, NULL, NULL, 0); } static int testCgroupNewForPartitionDomainEscaped(const void *args G_GNUC_UNUSED) @@ -493,7 +484,7 @@ static int testCgroupNewForPartitionDomainEscaped(const void *args G_GNUC_UNUSED * since our fake /proc/cgroups pretends this controller * isn't compiled into the kernel */ - return validateCgroup(domaincgroup, "/_cgroup.evil/net_cls.evil/__evil.evil/_cpu.foo.libvirt-lxc", mountsFull, links, placement, NULL, NULL, 0); + return validateCgroup(domaincgroup, mountsFull, links, placement, NULL, NULL, 0); } static int testCgroupNewForSelfAllInOne(const void *args G_GNUC_UNUSED) @@ -514,7 +505,7 @@ static int testCgroupNewForSelfAllInOne(const void *args G_GNUC_UNUSED) return -1; } - return validateCgroup(cgroup, "", mountsAllInOne, linksAllInOne, placement, NULL, NULL, 0); + return validateCgroup(cgroup, mountsAllInOne, linksAllInOne, placement, NULL, NULL, 0); } @@ -547,7 +538,7 @@ static int testCgroupNewForSelfUnified(const void *args G_GNUC_UNUSED) return -1; } - return validateCgroup(cgroup, "", empty, empty, empty, + return validateCgroup(cgroup, empty, empty, empty, "/not/really/sys/fs/cgroup", "/", controllers); } @@ -580,7 +571,7 @@ static int testCgroupNewForSelfHybrid(const void *args G_GNUC_UNUSED) return -1; } - return validateCgroup(cgroup, "", mounts, empty, placement, + return validateCgroup(cgroup, mounts, empty, placement, "/not/really/sys/fs/cgroup/unified", "/", controllers); } -- 2.26.2
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 7ff23e1218..3db20ed262 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -212,6 +212,33 @@ virCgroupPartitionEscape(char **path) } +static int +virCgroupSetBackends(virCgroupPtr group) +{ + virCgroupBackendPtr *backends = virCgroupBackendGetAll(); + bool backendAvailable = false; + size_t i; + + if (!backends) + return -1; + + for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { + if (backends[i] && backends[i]->available()) { + group->backends[i] = backends[i]; + backendAvailable = true; + } + } + + if (!backendAvailable) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("no cgroup backend available")); + return -1; + } + + return 0; +} + + /* * Process /proc/mounts figuring out what controllers are * mounted and where @@ -339,29 +366,14 @@ virCgroupDetect(virCgroupPtr group, virCgroupPtr parent) { size_t i; - bool backendAvailable = false; int controllersAvailable = 0; - virCgroupBackendPtr *backends = virCgroupBackendGetAll(); VIR_DEBUG("group=%p controllers=%d path=%s parent=%p", group, controllers, path, parent); - if (!backends) + if (virCgroupSetBackends(group) < 0) return -1; - for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { - if (backends[i] && backends[i]->available()) { - group->backends[i] = backends[i]; - backendAvailable = true; - } - } - - if (!backendAvailable) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("no cgroup backend available")); - return -1; - } - if (parent) { for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { if (group->backends[i] && -- 2.26.2
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 3db20ed262..a30aec4e5e 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -239,6 +239,23 @@ virCgroupSetBackends(virCgroupPtr group) } +static int +virCgroupCopyMounts(virCgroupPtr group, + virCgroupPtr parent) +{ + size_t i; + + for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { + if (group->backends[i] && + group->backends[i]->copyMounts(group, parent) < 0) { + return -1; + } + } + + return 0; +} + + /* * Process /proc/mounts figuring out what controllers are * mounted and where @@ -375,12 +392,8 @@ virCgroupDetect(virCgroupPtr group, return -1; if (parent) { - for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { - if (group->backends[i] && - group->backends[i]->copyMounts(group, parent) < 0) { - return -1; - } - } + if (virCgroupCopyMounts(group, parent) < 0) + return -1; } else { if (virCgroupDetectMounts(group) < 0) return -1; -- 2.26.2
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index a30aec4e5e..b5f38210fb 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -294,6 +294,24 @@ virCgroupDetectMounts(virCgroupPtr group) } +static int +virCgroupCopyPlacement(virCgroupPtr group, + const char *path, + virCgroupPtr parent) +{ + size_t i; + + for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { + if (group->backends[i] && + group->backends[i]->copyPlacement(group, path, parent) < 0) { + return -1; + } + } + + return 0; +} + + /* * virCgroupDetectPlacement: * @group: the group to process @@ -403,12 +421,8 @@ virCgroupDetect(virCgroupPtr group, * based on the parent cgroup... */ if (parent || path[0] == '/') { - for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { - if (group->backends[i] && - group->backends[i]->copyPlacement(group, path, parent) < 0) { - return -1; - } - } + if (virCgroupCopyPlacement(group, path, parent) < 0) + return -1; } /* ... but use /proc/cgroups to fill in the rest */ -- 2.26.2
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index b5f38210fb..55fa49a398 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -393,6 +393,23 @@ virCgroupDetectPlacement(virCgroupPtr group, } +static int +virCgroupValidatePlacement(virCgroupPtr group, + pid_t pid) +{ + size_t i; + + for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { + if (group->backends[i] && + group->backends[i]->validatePlacement(group, pid) < 0) { + return -1; + } + } + + return 0; +} + + static int virCgroupDetect(virCgroupPtr group, pid_t pid, @@ -430,12 +447,8 @@ virCgroupDetect(virCgroupPtr group, return -1; /* Check that for every mounted controller, we found our placement */ - for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { - if (group->backends[i] && - group->backends[i]->validatePlacement(group, pid) < 0) { - return -1; - } - } + if (virCgroupValidatePlacement(group, pid) < 0) + return -1; for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { if (group->backends[i]) { -- 2.26.2
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 51 ++++++++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 18 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 55fa49a398..f21a581946 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -410,6 +410,38 @@ virCgroupValidatePlacement(virCgroupPtr group, } +static int +virCgroupDetectControllers(virCgroupPtr group, + int controllers, + virCgroupPtr parent) +{ + size_t i; + int controllersAvailable = 0; + + for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { + int rc; + + if (!group->backends[i]) + continue; + + rc = group->backends[i]->detectControllers(group, controllers, parent, + controllersAvailable); + if (rc < 0) + return -1; + controllersAvailable |= rc; + } + + /* Check that at least 1 controller is available */ + if (controllersAvailable == 0) { + virReportSystemError(ENXIO, "%s", + _("At least one cgroup controller is required")); + return -1; + } + + return 0; +} + + static int virCgroupDetect(virCgroupPtr group, pid_t pid, @@ -417,9 +449,6 @@ virCgroupDetect(virCgroupPtr group, const char *path, virCgroupPtr parent) { - size_t i; - int controllersAvailable = 0; - VIR_DEBUG("group=%p controllers=%d path=%s parent=%p", group, controllers, path, parent); @@ -450,22 +479,8 @@ virCgroupDetect(virCgroupPtr group, if (virCgroupValidatePlacement(group, pid) < 0) return -1; - for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { - if (group->backends[i]) { - int rc = group->backends[i]->detectControllers(group, controllers, parent, - controllersAvailable); - if (rc < 0) - return -1; - controllersAvailable |= rc; - } - } - - /* Check that at least 1 controller is available */ - if (controllersAvailable == 0) { - virReportSystemError(ENXIO, "%s", - _("At least one cgroup controller is required")); + if (virCgroupDetectControllers(group, controllers, parent) < 0) return -1; - } return 0; } -- 2.26.2
The current code uses virCgroupNew() as a single point of entry and calls into virCgroupDetect() as well. Both have logic for several paths which is difficult to figure out. Extract the actually used code path from the two functions to make it obvious what's happening in this case. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index f21a581946..97e15c0ab3 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -1048,7 +1048,28 @@ virCgroupNewDetect(pid_t pid, int controllers, virCgroupPtr *group) { - return virCgroupNew(pid, "", NULL, controllers, group); + g_autoptr(virCgroup) new = g_new0(virCgroup, 1); + + VIR_DEBUG("pid=%lld controllers=%d group=%p", + (long long) pid, controllers, group); + + if (virCgroupSetBackends(new) < 0) + return -1; + + if (virCgroupDetectMounts(new) < 0) + return -1; + + if (virCgroupDetectPlacement(new, pid, "") < 0) + return -1; + + if (virCgroupValidatePlacement(new, pid) < 0) + return -1; + + if (virCgroupDetectControllers(new, controllers, NULL) < 0) + return -1; + + *group = g_steal_pointer(&new); + return 0; } -- 2.26.2
The current code uses virCgroupNew() as a single point of entry and calls into virCgroupDetect() as well. Both have logic for several paths which is difficult to figure out. Extract the actually used code path from the two functions to make it obvious what's happening in this case. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 51 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 42 insertions(+), 9 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 97e15c0ab3..b11139bef1 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -861,6 +861,40 @@ virCgroupSetPartitionSuffix(const char *path, char **res) } +static int +virCgroupNewFromParent(virCgroupPtr parent, + const char *path, + int controllers, + virCgroupPtr *group) +{ + g_autoptr(virCgroup) new = g_new0(virCgroup, 1); + + VIR_DEBUG("parent=%p path=%s controllers=%d group=%p", + parent, path, controllers, group); + + if (virCgroupSetBackends(new) < 0) + return -1; + + if (virCgroupCopyMounts(new, parent) < 0) + return -1; + + if (virCgroupCopyPlacement(new, path, parent) < 0) + return -1; + + if (virCgroupDetectPlacement(new, -1, path) < 0) + return -1; + + if (virCgroupValidatePlacement(new, -1) < 0) + return -1; + + if (virCgroupDetectControllers(new, controllers, parent) < 0) + return -1; + + *group = g_steal_pointer(&new); + return 0; +} + + /** * virCgroupNewPartition: * @path: path for the partition @@ -910,7 +944,7 @@ virCgroupNewPartition(const char *path, return -1; } - if (virCgroupNew(-1, newPath, parent, controllers, &newGroup) < 0) + if (virCgroupNewFromParent(parent, newPath, controllers, &newGroup) < 0) return -1; if (parent) { @@ -965,7 +999,7 @@ virCgroupNewDomainPartition(virCgroupPtr partition, if (virCgroupPartitionEscape(&grpname) < 0) return -1; - if (virCgroupNew(-1, grpname, partition, -1, &newGroup) < 0) + if (virCgroupNewFromParent(partition, grpname, -1, &newGroup) < 0) return -1; /* @@ -1032,7 +1066,7 @@ virCgroupNewThread(virCgroupPtr domain, (1 << VIR_CGROUP_CONTROLLER_CPUACCT) | (1 << VIR_CGROUP_CONTROLLER_CPUSET)); - if (virCgroupNew(-1, name, domain, controllers, &newGroup) < 0) + if (virCgroupNewFromParent(domain, name, controllers, &newGroup) < 0) return -1; if (virCgroupMakeGroup(domain, newGroup, create, VIR_CGROUP_THREAD) < 0) @@ -1133,11 +1167,10 @@ virCgroupEnableMissingControllers(char *path, if (t) *t = '\0'; - if (virCgroupNew(pidleader, - path, - parent, - controllers, - &tmp) < 0) + if (virCgroupNewFromParent(parent, + path, + controllers, + &tmp) < 0) return -1; if (virCgroupMakeGroup(parent, tmp, true, VIR_CGROUP_SYSTEMD) < 0) @@ -2583,7 +2616,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, VIR_DEBUG("Process subdir %s", ent->d_name); - if (virCgroupNew(-1, ent->d_name, group, -1, &subgroup) < 0) + if (virCgroupNewFromParent(group, ent->d_name, -1, &subgroup) < 0) return -1; if ((rc = virCgroupKillRecursiveInternal(subgroup, signum, pids, -- 2.26.2
Now it is always NULL. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 40 +++++++++++++--------------------------- src/util/vircgrouppriv.h | 1 - src/util/vircgroupv1.c | 2 +- 3 files changed, 14 insertions(+), 29 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index b11139bef1..56aa9cc064 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -446,28 +446,19 @@ static int virCgroupDetect(virCgroupPtr group, pid_t pid, int controllers, - const char *path, - virCgroupPtr parent) + const char *path) { - VIR_DEBUG("group=%p controllers=%d path=%s parent=%p", - group, controllers, path, parent); + VIR_DEBUG("group=%p controllers=%d path=%s", + group, controllers, path); if (virCgroupSetBackends(group) < 0) return -1; - if (parent) { - if (virCgroupCopyMounts(group, parent) < 0) - return -1; - } else { - if (virCgroupDetectMounts(group) < 0) - return -1; - } + if (virCgroupDetectMounts(group) < 0) + return -1; - /* In some cases we can copy part of the placement info - * based on the parent cgroup... - */ - if (parent || path[0] == '/') { - if (virCgroupCopyPlacement(group, path, parent) < 0) + if (path[0] == '/') { + if (virCgroupCopyPlacement(group, path, NULL) < 0) return -1; } @@ -479,7 +470,7 @@ virCgroupDetect(virCgroupPtr group, if (virCgroupValidatePlacement(group, pid) < 0) return -1; - if (virCgroupDetectControllers(group, controllers, parent) < 0) + if (virCgroupDetectControllers(group, controllers, NULL) < 0) return -1; return 0; @@ -708,15 +699,12 @@ virCgroupMakeGroup(virCgroupPtr parent, /** * virCgroupNew: * @path: path for the new group - * @parent: parent group, or NULL * @controllers: bitmask of controllers to activate * * Create a new cgroup storing it in @group. * * If @path starts with a '/' it is treated as an - * absolute path, and @parent is ignored. Otherwise - * it is treated as being relative to @parent. If - * @parent is NULL, then the placement of the current + * absolute path. Otherwise then the placement of the current * process is used. * * Returns 0 on success, -1 on error @@ -724,19 +712,18 @@ virCgroupMakeGroup(virCgroupPtr parent, int virCgroupNew(pid_t pid, const char *path, - virCgroupPtr parent, int controllers, virCgroupPtr *group) { g_autoptr(virCgroup) newGroup = NULL; - VIR_DEBUG("pid=%lld path=%s parent=%p controllers=%d group=%p", - (long long) pid, path, parent, controllers, group); + VIR_DEBUG("pid=%lld path=%s controllers=%d group=%p", + (long long) pid, path, controllers, group); *group = NULL; newGroup = g_new0(virCgroup, 1); - if (virCgroupDetect(newGroup, pid, controllers, path, parent) < 0) + if (virCgroupDetect(newGroup, pid, controllers, path) < 0) return -1; *group = g_steal_pointer(&newGroup); @@ -940,7 +927,7 @@ virCgroupNewPartition(const char *path, tmp++; *tmp = '\0'; - if (virCgroupNew(-1, parentPath, NULL, controllers, &parent) < 0) + if (virCgroupNew(-1, parentPath, controllers, &parent) < 0) return -1; } @@ -1156,7 +1143,6 @@ virCgroupEnableMissingControllers(char *path, if (virCgroupNew(pidleader, "/", - NULL, controllers, &parent) < 0) return -1; diff --git a/src/util/vircgrouppriv.h b/src/util/vircgrouppriv.h index af8bf99888..0ac62882a5 100644 --- a/src/util/vircgrouppriv.h +++ b/src/util/vircgrouppriv.h @@ -112,7 +112,6 @@ int virCgroupGetValueForBlkDev(const char *str, int virCgroupNew(pid_t pid, const char *path, - virCgroupPtr parent, int controllers, virCgroupPtr *group); diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c index 6bb819410b..b3e0f733b9 100644 --- a/src/util/vircgroupv1.c +++ b/src/util/vircgroupv1.c @@ -1468,7 +1468,7 @@ virCgroupV1MemoryOnceInit(void) g_autoptr(virCgroup) group = NULL; unsigned long long int mem_unlimited = 0ULL; - if (virCgroupNew(-1, "/", NULL, -1, &group) < 0) + if (virCgroupNew(-1, "/", -1, &group) < 0) return; if (!virCgroupV1HasController(group, VIR_CGROUP_CONTROLLER_MEMORY)) -- 2.26.2
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 56aa9cc064..794680d696 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -457,10 +457,8 @@ virCgroupDetect(virCgroupPtr group, if (virCgroupDetectMounts(group) < 0) return -1; - if (path[0] == '/') { - if (virCgroupCopyPlacement(group, path, NULL) < 0) - return -1; - } + if (virCgroupCopyPlacement(group, path, NULL) < 0) + return -1; /* ... but use /proc/cgroups to fill in the rest */ if (virCgroupDetectPlacement(group, pid, path) < 0) @@ -703,10 +701,6 @@ virCgroupMakeGroup(virCgroupPtr parent, * * Create a new cgroup storing it in @group. * - * If @path starts with a '/' it is treated as an - * absolute path. Otherwise then the placement of the current - * process is used. - * * Returns 0 on success, -1 on error */ int -- 2.26.2
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 52 +++++++++++++++----------------------------- 1 file changed, 18 insertions(+), 34 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 794680d696..81aa3ee791 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -442,39 +442,6 @@ virCgroupDetectControllers(virCgroupPtr group, } -static int -virCgroupDetect(virCgroupPtr group, - pid_t pid, - int controllers, - const char *path) -{ - VIR_DEBUG("group=%p controllers=%d path=%s", - group, controllers, path); - - if (virCgroupSetBackends(group) < 0) - return -1; - - if (virCgroupDetectMounts(group) < 0) - return -1; - - if (virCgroupCopyPlacement(group, path, NULL) < 0) - return -1; - - /* ... but use /proc/cgroups to fill in the rest */ - if (virCgroupDetectPlacement(group, pid, path) < 0) - return -1; - - /* Check that for every mounted controller, we found our placement */ - if (virCgroupValidatePlacement(group, pid) < 0) - return -1; - - if (virCgroupDetectControllers(group, controllers, NULL) < 0) - return -1; - - return 0; -} - - char * virCgroupGetBlockDevString(const char *path) { @@ -717,7 +684,24 @@ virCgroupNew(pid_t pid, *group = NULL; newGroup = g_new0(virCgroup, 1); - if (virCgroupDetect(newGroup, pid, controllers, path) < 0) + if (virCgroupSetBackends(newGroup) < 0) + return -1; + + if (virCgroupDetectMounts(newGroup) < 0) + return -1; + + if (virCgroupCopyPlacement(newGroup, path, NULL) < 0) + return -1; + + /* ... but use /proc/cgroups to fill in the rest */ + if (virCgroupDetectPlacement(newGroup, pid, path) < 0) + return -1; + + /* Check that for every mounted controller, we found our placement */ + if (virCgroupValidatePlacement(newGroup, pid) < 0) + return -1; + + if (virCgroupDetectControllers(newGroup, controllers, NULL) < 0) return -1; *group = g_steal_pointer(&newGroup); -- 2.26.2
This function is relevant only with cgroups v1 where it creates hierarchy for controllers that are not managed by systemd. PID is used to detect a placement of current process but in this situation we are building the hierarchy for already known placement. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 81aa3ee791..2c1d6f9f01 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -1112,14 +1112,13 @@ virCgroupNewDetectMachine(const char *name, static int virCgroupEnableMissingControllers(char *path, - pid_t pidleader, int controllers, virCgroupPtr *group) { g_autoptr(virCgroup) parent = NULL; char *offset = path; - if (virCgroupNew(pidleader, + if (virCgroupNew(-1, "/", controllers, &parent) < 0) @@ -1213,10 +1212,8 @@ virCgroupNewMachineSystemd(const char *name, return -2; } - if (virCgroupEnableMissingControllers(path, pidleader, - controllers, &newGroup) < 0) { + if (virCgroupEnableMissingControllers(path, controllers, &newGroup) < 0) return -1; - } if (virCgroupAddProcess(newGroup, pidleader) < 0) { virErrorPtr saved; -- 2.26.2
Now it is always -1. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 16 +++++++--------- src/util/vircgrouppriv.h | 3 +-- src/util/vircgroupv1.c | 2 +- 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 2c1d6f9f01..6a34eb4037 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -671,15 +671,14 @@ virCgroupMakeGroup(virCgroupPtr parent, * Returns 0 on success, -1 on error */ int -virCgroupNew(pid_t pid, - const char *path, +virCgroupNew(const char *path, int controllers, virCgroupPtr *group) { g_autoptr(virCgroup) newGroup = NULL; - VIR_DEBUG("pid=%lld path=%s controllers=%d group=%p", - (long long) pid, path, controllers, group); + VIR_DEBUG("path=%s controllers=%d group=%p", + path, controllers, group); *group = NULL; newGroup = g_new0(virCgroup, 1); @@ -694,11 +693,11 @@ virCgroupNew(pid_t pid, return -1; /* ... but use /proc/cgroups to fill in the rest */ - if (virCgroupDetectPlacement(newGroup, pid, path) < 0) + if (virCgroupDetectPlacement(newGroup, -1, path) < 0) return -1; /* Check that for every mounted controller, we found our placement */ - if (virCgroupValidatePlacement(newGroup, pid) < 0) + if (virCgroupValidatePlacement(newGroup, -1) < 0) return -1; if (virCgroupDetectControllers(newGroup, controllers, NULL) < 0) @@ -905,7 +904,7 @@ virCgroupNewPartition(const char *path, tmp++; *tmp = '\0'; - if (virCgroupNew(-1, parentPath, controllers, &parent) < 0) + if (virCgroupNew(parentPath, controllers, &parent) < 0) return -1; } @@ -1118,8 +1117,7 @@ virCgroupEnableMissingControllers(char *path, g_autoptr(virCgroup) parent = NULL; char *offset = path; - if (virCgroupNew(-1, - "/", + if (virCgroupNew("/", controllers, &parent) < 0) return -1; diff --git a/src/util/vircgrouppriv.h b/src/util/vircgrouppriv.h index 0ac62882a5..7248bc1d35 100644 --- a/src/util/vircgrouppriv.h +++ b/src/util/vircgrouppriv.h @@ -110,8 +110,7 @@ int virCgroupGetValueForBlkDev(const char *str, const char *devPath, char **value); -int virCgroupNew(pid_t pid, - const char *path, +int virCgroupNew(const char *path, int controllers, virCgroupPtr *group); diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c index b3e0f733b9..98b5aa047d 100644 --- a/src/util/vircgroupv1.c +++ b/src/util/vircgroupv1.c @@ -1468,7 +1468,7 @@ virCgroupV1MemoryOnceInit(void) g_autoptr(virCgroup) group = NULL; unsigned long long int mem_unlimited = 0ULL; - if (virCgroupNew(-1, "/", -1, &group) < 0) + if (virCgroupNew("/", -1, &group) < 0) return; if (!virCgroupV1HasController(group, VIR_CGROUP_CONTROLLER_MEMORY)) -- 2.26.2
Currently this task is done by virCgroupCopyPlacement when the @path starts with "/". virCgroupNew is always called with @path starting with "/" and there is no parent to copy path from. To make it obvious what the code is doing introduce new helper. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 19 ++++++++++++++++++- src/util/vircgroupbackend.h | 5 +++++ src/util/vircgroupv1.c | 21 +++++++++++++++++++++ src/util/vircgroupv2.c | 11 +++++++++++ 4 files changed, 55 insertions(+), 1 deletion(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 6a34eb4037..00967ea5fa 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -393,6 +393,23 @@ virCgroupDetectPlacement(virCgroupPtr group, } +static int +virCgroupSetPlacement(virCgroupPtr group, + const char *path) +{ + size_t i; + + for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { + if (group->backends[i] && + group->backends[i]->setPlacement(group, path) < 0) { + return -1; + } + } + + return 0; +} + + static int virCgroupValidatePlacement(virCgroupPtr group, pid_t pid) @@ -689,7 +706,7 @@ virCgroupNew(const char *path, if (virCgroupDetectMounts(newGroup) < 0) return -1; - if (virCgroupCopyPlacement(newGroup, path, NULL) < 0) + if (virCgroupSetPlacement(newGroup, path) < 0) return -1; /* ... but use /proc/cgroups to fill in the rest */ diff --git a/src/util/vircgroupbackend.h b/src/util/vircgroupbackend.h index f677157a91..4ca2e38af2 100644 --- a/src/util/vircgroupbackend.h +++ b/src/util/vircgroupbackend.h @@ -88,6 +88,10 @@ typedef int const char *controllers, const char *selfpath); +typedef int +(*virCgroupSetPlacementCB)(virCgroupPtr group, + const char *path); + typedef int (*virCgroupValidatePlacementCB)(virCgroupPtr group, pid_t pid); @@ -369,6 +373,7 @@ struct _virCgroupBackend { virCgroupCopyPlacementCB copyPlacement; virCgroupDetectMountsCB detectMounts; virCgroupDetectPlacementCB detectPlacement; + virCgroupSetPlacementCB setPlacement; virCgroupValidatePlacementCB validatePlacement; virCgroupStealPlacementCB stealPlacement; virCgroupDetectControllersCB detectControllers; diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c index 98b5aa047d..ed4b0813f2 100644 --- a/src/util/vircgroupv1.c +++ b/src/util/vircgroupv1.c @@ -364,6 +364,26 @@ virCgroupV1DetectPlacement(virCgroupPtr group, } +static int +virCgroupV1SetPlacement(virCgroupPtr group, + const char *path) +{ + size_t i; + + for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { + if (!group->legacy[i].mountPoint) + continue; + + if (i == VIR_CGROUP_CONTROLLER_SYSTEMD) + continue; + + group->legacy[i].placement = g_strdup(path); + } + + return 0; +} + + static int virCgroupV1ValidatePlacement(virCgroupPtr group, pid_t pid) @@ -2101,6 +2121,7 @@ virCgroupBackend virCgroupV1Backend = { .copyPlacement = virCgroupV1CopyPlacement, .detectMounts = virCgroupV1DetectMounts, .detectPlacement = virCgroupV1DetectPlacement, + .setPlacement = virCgroupV1SetPlacement, .validatePlacement = virCgroupV1ValidatePlacement, .stealPlacement = virCgroupV1StealPlacement, .detectControllers = virCgroupV1DetectControllers, diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index 0467a72566..8e058ca9c6 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -223,6 +223,16 @@ virCgroupV2DetectPlacement(virCgroupPtr group, } +static int +virCgroupV2SetPlacement(virCgroupPtr group, + const char *path) +{ + group->unified.placement = g_strdup(path); + + return 0; +} + + static int virCgroupV2ValidatePlacement(virCgroupPtr group, pid_t pid G_GNUC_UNUSED) @@ -1845,6 +1855,7 @@ virCgroupBackend virCgroupV2Backend = { .copyPlacement = virCgroupV2CopyPlacement, .detectMounts = virCgroupV2DetectMounts, .detectPlacement = virCgroupV2DetectPlacement, + .setPlacement = virCgroupV2SetPlacement, .validatePlacement = virCgroupV2ValidatePlacement, .stealPlacement = virCgroupV2StealPlacement, .detectControllers = virCgroupV2DetectControllers, -- 2.26.2
All callers pass true. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 5 +---- src/util/vircgrouppriv.h | 1 - tests/vircgrouptest.c | 4 ++-- 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 00967ea5fa..1e18b84b54 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -969,7 +969,6 @@ int virCgroupNewDomainPartition(virCgroupPtr partition, const char *driver, const char *name, - bool create, virCgroupPtr *group) { g_autofree char *grpname = NULL; @@ -993,7 +992,7 @@ virCgroupNewDomainPartition(virCgroupPtr partition, * a group for driver, is to avoid overhead to track * cumulative usage that we don't need. */ - if (virCgroupMakeGroup(partition, newGroup, create, + if (virCgroupMakeGroup(partition, newGroup, true, VIR_CGROUP_MEM_HIERACHY) < 0) { return -1; } @@ -1278,7 +1277,6 @@ virCgroupNewMachineManual(const char *name, if (virCgroupNewDomainPartition(parent, drivername, name, - true, &newGroup) < 0) return -1; @@ -2836,7 +2834,6 @@ int virCgroupNewDomainPartition(virCgroupPtr partition G_GNUC_UNUSED, const char *driver G_GNUC_UNUSED, const char *name G_GNUC_UNUSED, - bool create G_GNUC_UNUSED, virCgroupPtr *group G_GNUC_UNUSED) { virReportSystemError(ENXIO, "%s", diff --git a/src/util/vircgrouppriv.h b/src/util/vircgrouppriv.h index 7248bc1d35..77e01519b0 100644 --- a/src/util/vircgrouppriv.h +++ b/src/util/vircgrouppriv.h @@ -123,7 +123,6 @@ int virCgroupNewPartition(const char *path, int virCgroupNewDomainPartition(virCgroupPtr partition, const char *driver, const char *name, - bool create, virCgroupPtr *group) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5); diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c index 0dd5e3f0b4..1baa71e61c 100644 --- a/tests/vircgrouptest.c +++ b/tests/vircgrouptest.c @@ -434,7 +434,7 @@ static int testCgroupNewForPartitionDomain(const void *args G_GNUC_UNUSED) return -1; } - if ((rv = virCgroupNewDomainPartition(partitioncgroup, "lxc", "foo", true, &domaincgroup)) != 0) { + if ((rv = virCgroupNewDomainPartition(partitioncgroup, "lxc", "foo", &domaincgroup)) != 0) { fprintf(stderr, "Cannot create LXC cgroup: %d\n", -rv); return -1; } @@ -475,7 +475,7 @@ static int testCgroupNewForPartitionDomainEscaped(const void *args G_GNUC_UNUSED return -1; } - if ((rv = virCgroupNewDomainPartition(partitioncgroup3, "lxc", "cpu.foo", true, &domaincgroup)) != 0) { + if ((rv = virCgroupNewDomainPartition(partitioncgroup3, "lxc", "cpu.foo", &domaincgroup)) != 0) { fprintf(stderr, "Cannot create LXC cgroup: %d\n", -rv); return -1; } -- 2.26.2
On 11/3/20 7:41 AM, Pavel Hrdina wrote:
All callers pass true.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 5 +---- src/util/vircgrouppriv.h | 1 - tests/vircgrouptest.c | 4 ++-- 3 files changed, 3 insertions(+), 7 deletions(-)
[...]
diff --git a/src/util/vircgrouppriv.h b/src/util/vircgrouppriv.h index 7248bc1d35..77e01519b0 100644 --- a/src/util/vircgrouppriv.h +++ b/src/util/vircgrouppriv.h @@ -123,7 +123,6 @@ int virCgroupNewPartition(const char *path, int virCgroupNewDomainPartition(virCgroupPtr partition, const char *driver, const char *name, - bool create, virCgroupPtr *group) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5);
^^ FYI: This breaks the Coverity build - need to use ATTRIBUTE_NONNULL(4) John [...]
Use virStringSplit() to get the list of directories needed to be created. This improves readability of the code and stops passing absolute path to virCgroupNewFromParent(). Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 1e18b84b54..f4c1623567 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -1131,21 +1131,18 @@ virCgroupEnableMissingControllers(char *path, virCgroupPtr *group) { g_autoptr(virCgroup) parent = NULL; - char *offset = path; + VIR_AUTOSTRINGLIST tokens = virStringSplit(path, "/", 0); + size_t i; - if (virCgroupNew("/", - controllers, - &parent) < 0) + if (virCgroupNew("/", controllers, &parent) < 0) return -1; - for (;;) { + /* Skip the first token as it is empty string. */ + for (i = 1; tokens[i]; i++) { g_autoptr(virCgroup) tmp = NULL; - char *t = strchr(offset + 1, '/'); - if (t) - *t = '\0'; if (virCgroupNewFromParent(parent, - path, + tokens[i], controllers, &tmp) < 0) return -1; @@ -1153,17 +1150,10 @@ virCgroupEnableMissingControllers(char *path, if (virCgroupMakeGroup(parent, tmp, true, VIR_CGROUP_SYSTEMD) < 0) return -1; - if (t) { - *t = '/'; - offset = t; - virCgroupFree(parent); - parent = g_steal_pointer(&tmp); - } else { - *group = g_steal_pointer(&tmp); - break; - } + parent = g_steal_pointer(&tmp); } + *group = g_steal_pointer(&parent); return 0; } -- 2.26.2
It's used only inside the if condition. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index f4c1623567..6caeb2d7f4 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -893,7 +893,6 @@ virCgroupNewPartition(const char *path, int controllers, virCgroupPtr *group) { - g_autofree char *parentPath = NULL; g_autofree char *newPath = NULL; g_autoptr(virCgroup) parent = NULL; g_autoptr(virCgroup) newGroup = NULL; @@ -915,7 +914,7 @@ virCgroupNewPartition(const char *path, if (STRNEQ(newPath, "/")) { char *tmp; - parentPath = g_strdup(newPath); + g_autofree char *parentPath = g_strdup(newPath); tmp = strrchr(parentPath, '/'); tmp++; -- 2.26.2
The old code passed an absolute path to virCgroupNewFromParent() which is not necessary. The code can take the current placement of parent cgroup and append a relative path. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 6caeb2d7f4..e0fe1dbf3e 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -896,6 +896,7 @@ virCgroupNewPartition(const char *path, g_autofree char *newPath = NULL; g_autoptr(virCgroup) parent = NULL; g_autoptr(virCgroup) newGroup = NULL; + char *partition = NULL; VIR_DEBUG("path=%s create=%d controllers=%x", path, create, controllers); @@ -914,17 +915,26 @@ virCgroupNewPartition(const char *path, if (STRNEQ(newPath, "/")) { char *tmp; - g_autofree char *parentPath = g_strdup(newPath); + const char *parentPath; - tmp = strrchr(parentPath, '/'); - tmp++; + tmp = strrchr(newPath, '/'); *tmp = '\0'; + if (tmp == newPath) { + parentPath = "/"; + } else { + parentPath = newPath; + } + if (virCgroupNew(parentPath, controllers, &parent) < 0) return -1; + + partition = tmp + 1; + } else { + partition = newPath; } - if (virCgroupNewFromParent(parent, newPath, controllers, &newGroup) < 0) + if (virCgroupNewFromParent(parent, partition, controllers, &newGroup) < 0) return -1; if (parent) { -- 2.26.2
Now that every caller to copyPlacement doesn't pass absolute path there is no need to have a condition to handle that case. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroupv1.c | 26 ++++++++++++-------------- src/util/vircgroupv2.c | 25 +++++++++++-------------- 2 files changed, 23 insertions(+), 28 deletions(-) diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c index ed4b0813f2..731e9d61d4 100644 --- a/src/util/vircgroupv1.c +++ b/src/util/vircgroupv1.c @@ -190,26 +190,24 @@ virCgroupV1CopyPlacement(virCgroupPtr group, { size_t i; for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { + bool delim; + if (!group->legacy[i].mountPoint) continue; if (i == VIR_CGROUP_CONTROLLER_SYSTEMD) continue; - if (path[0] == '/') { - group->legacy[i].placement = g_strdup(path); - } else { - bool delim = STREQ(parent->legacy[i].placement, "/") || STREQ(path, ""); - /* - * parent == "/" + path="" => "/" - * parent == "/libvirt.service" + path == "" => "/libvirt.service" - * parent == "/libvirt.service" + path == "foo" => "/libvirt.service/foo" - */ - group->legacy[i].placement = g_strdup_printf("%s%s%s", - parent->legacy[i].placement, - delim ? "" : "/", - path); - } + delim = STREQ(parent->legacy[i].placement, "/") || STREQ(path, ""); + /* + * parent == "/" + path="" => "/" + * parent == "/libvirt.service" + path == "" => "/libvirt.service" + * parent == "/libvirt.service" + path == "foo" => "/libvirt.service/foo" + */ + group->legacy[i].placement = g_strdup_printf("%s%s%s", + parent->legacy[i].placement, + delim ? "" : "/", + path); } return 0; diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index 8e058ca9c6..2b32f614e4 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -149,22 +149,19 @@ virCgroupV2CopyPlacement(virCgroupPtr group, const char *path, virCgroupPtr parent) { + bool delim = STREQ(parent->unified.placement, "/") || STREQ(path, ""); + VIR_DEBUG("group=%p path=%s parent=%p", group, path, parent); - if (path[0] == '/') { - group->unified.placement = g_strdup(path); - } else { - bool delim = STREQ(parent->unified.placement, "/") || STREQ(path, ""); - /* - * parent == "/" + path="" => "/" - * parent == "/libvirt.service" + path == "" => "/libvirt.service" - * parent == "/libvirt.service" + path == "foo" => "/libvirt.service/foo" - */ - group->unified.placement = g_strdup_printf("%s%s%s", - parent->unified.placement, - delim ? "" : "/", - path); - } + /* + * parent == "/" + path="" => "/" + * parent == "/libvirt.service" + path == "" => "/libvirt.service" + * parent == "/libvirt.service" + path == "foo" => "/libvirt.service/foo" + */ + group->unified.placement = g_strdup_printf("%s%s%s", + parent->unified.placement, + delim ? "" : "/", + path); return 0; } -- 2.26.2
On 11/3/20 1:41 PM, Pavel Hrdina wrote:
Pavel Hrdina (25): qemu_cgroup: remove unused @empty variable qemu: remove dead code that setup cgroups for helper processes qemu_dbus: use emulator cgroup for dbus-daemon vircgroupv2: properly detect empty tasks vircgroupv2: properly detect placement of running VM vircgroupv2: detect controllers enabled in parent cgroup vircgroup: remove useless cgroup->path variable vircgroup: introduce virCgroupSetBackends helper vircgroup: introduce virCgroupCopyMounts helper vircgroup: introduce virCgroupCopyPlacement helper vircgroup: introduce virCgroupValidatePlacement helper vircgroup: introduce virCgroupDetectControllers helper vircgroup: extract virCgroupNewDetect from virCgroupNew vircgroup: introduce virCgroupNewParent vircgroup: drop @parent from virCgroupNew vircgroup: virCgroupNew is now always called with absolute path vircgroup: expand virCgroupDetect into virCgroupNew vircgroup: no need to use PID in virCgroupEnableMissingControllers vircgroup: drop @pid argument from virCgroupNew vircgroup: introduce virCgroupSetPlacement vircgroup: drop @create from virCgroupNewDomainPartition vircgroup: refactor virCgroupEnableMissingControllers vircgroup: move parentPath declaration vircgroup: refactor virCgroupNewPartition vircgroup: drop condition for absolute path from copyPlacement callbacks
src/qemu/qemu_cgroup.c | 5 +- src/qemu/qemu_dbus.c | 9 +- src/qemu/qemu_dbus.h | 3 +- src/qemu/qemu_extdevice.c | 2 +- src/qemu/qemu_slirp.c | 4 - src/util/vircgroup.c | 337 ++++++++++++++++++++++-------------- src/util/vircgroupbackend.h | 5 + src/util/vircgrouppriv.h | 7 +- src/util/vircgroupv1.c | 57 +++--- src/util/vircgroupv2.c | 61 ++++--- tests/vircgrouptest.c | 33 ++-- 11 files changed, 309 insertions(+), 214 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
On Tue, Nov 3, 2020 at 7:42 AM Pavel Hrdina <phrdina@redhat.com> wrote:
Pavel Hrdina (25): qemu_cgroup: remove unused @empty variable qemu: remove dead code that setup cgroups for helper processes qemu_dbus: use emulator cgroup for dbus-daemon vircgroupv2: properly detect empty tasks vircgroupv2: properly detect placement of running VM vircgroupv2: detect controllers enabled in parent cgroup vircgroup: remove useless cgroup->path variable vircgroup: introduce virCgroupSetBackends helper vircgroup: introduce virCgroupCopyMounts helper vircgroup: introduce virCgroupCopyPlacement helper vircgroup: introduce virCgroupValidatePlacement helper vircgroup: introduce virCgroupDetectControllers helper vircgroup: extract virCgroupNewDetect from virCgroupNew vircgroup: introduce virCgroupNewParent vircgroup: drop @parent from virCgroupNew vircgroup: virCgroupNew is now always called with absolute path vircgroup: expand virCgroupDetect into virCgroupNew vircgroup: no need to use PID in virCgroupEnableMissingControllers vircgroup: drop @pid argument from virCgroupNew vircgroup: introduce virCgroupSetPlacement vircgroup: drop @create from virCgroupNewDomainPartition vircgroup: refactor virCgroupEnableMissingControllers vircgroup: move parentPath declaration vircgroup: refactor virCgroupNewPartition vircgroup: drop condition for absolute path from copyPlacement callbacks
src/qemu/qemu_cgroup.c | 5 +- src/qemu/qemu_dbus.c | 9 +- src/qemu/qemu_dbus.h | 3 +- src/qemu/qemu_extdevice.c | 2 +- src/qemu/qemu_slirp.c | 4 - src/util/vircgroup.c | 337 ++++++++++++++++++++++-------------- src/util/vircgroupbackend.h | 5 + src/util/vircgrouppriv.h | 7 +- src/util/vircgroupv1.c | 57 +++--- src/util/vircgroupv2.c | 61 ++++--- tests/vircgrouptest.c | 33 ++-- 11 files changed, 309 insertions(+), 214 deletions(-)
-- 2.26.2
Series LGTM. Reviewed-by: Neal Gompa <ngompa13@gmail.com> -- 真実はいつも一つ！/ Always, there's only one truth!
participants (4)
-
John Ferlan -
Michal Privoznik -
Neal Gompa -
Pavel Hrdina