[libvirt] [PATCH 12/18] Refactor cgroups internal data structures

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; Currently the virCgroupPtr struct contains 3 pieces of information - path - path of the cgroup, relative to current process' cgroup placement - placement - current process' placement in each controller - mounts - mount point of each controller When reading/writing cgroup settings, the path & placement strings are combined to form the file path. This approach only works if we assume all cgroups will be relative to the current process' cgroup placement. To allow support for managing cgroups at any place in the heirarchy a change is needed. The 'placement' data should reflect the absolute path to the cgroup, and the 'path' value should no longer be used to form the paths to the cgroup attribute files. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/util/vircgroup.c | 222 +++++++++++++++++++++++++++++++++++--------------- tests/vircgrouptest.c | 53 +++++++----- 2 files changed, 188 insertions(+), 87 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 2f52c92..c336806 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -101,6 +101,23 @@ bool virCgroupHasController(virCgroupPtr cgroup, int controller) } #if defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R +static int virCgroupCopyMounts(virCgroupPtr group, + virCgroupPtr parent) +{ + int i; + for (i = 0 ; i < VIR_CGROUP_CONTROLLER_LAST ; i++) { + if (!parent->controllers[i].mountPoint) + continue; + + group->controllers[i].mountPoint = + strdup(parent->controllers[i].mountPoint); + + if (!group->controllers[i].mountPoint) + return -ENOMEM; + } + return 0; +} + /* * Process /proc/mounts figuring out what controllers are * mounted and where @@ -158,12 +175,61 @@ no_memory: } +static int virCgroupCopyPlacement(virCgroupPtr group, + const char *path, + virCgroupPtr parent) +{ + int i; + for (i = 0 ; i < VIR_CGROUP_CONTROLLER_LAST ; i++) { + if (!group->controllers[i].mountPoint) + continue; + + if (path[0] == '/') { + if (!(group->controllers[i].placement = strdup(path))) + return -ENOMEM; + } else { + /* + * parent=="/" + path="" => "/" + * parent=="/libvirt.service" + path="" => "/libvirt.service" + * parent=="/libvirt.service" + path="foo" => "/libvirt.service/foo" + */ + if (virAsprintf(&group->controllers[i].placement, + "%s%s%s", + parent->controllers[i].placement, + (STREQ(parent->controllers[i].placement, "/") || + STREQ(path, "") ? "" : "/"), + path) < 0) + return -ENOMEM; + } + } + + return 0; +} + + /* + * @group: the group to process + * @path: the relative path to append, not starting with '/' + * * Process /proc/self/cgroup figuring out what cgroup * sub-path the current process is assigned to. ie not - * necessarily in the root + * necessarily in the root. The contents of this file + * looks like + * + * 9:perf_event:/ + * 8:blkio:/ + * 7:net_cls:/ + * 6:freezer:/ + * 5:devices:/ + * 4:memory:/ + * 3:cpuacct,cpu:/ + * 2:cpuset:/ + * 1:name=systemd:/user/berrange/2 + * + * It then appends @path to each detected path. */ -static int virCgroupDetectPlacement(virCgroupPtr group) +static int virCgroupDetectPlacement(virCgroupPtr group, + const char *path) { int i; FILE *mapping = NULL; @@ -177,18 +243,18 @@ static int virCgroupDetectPlacement(virCgroupPtr group) while (fgets(line, sizeof(line), mapping) != NULL) { char *controllers = strchr(line, ':'); - char *path = controllers ? strchr(controllers+1, ':') : NULL; - char *nl = path ? strchr(path, '\n') : NULL; + char *selfpath = controllers ? strchr(controllers + 1, ':') : NULL; + char *nl = selfpath ? strchr(selfpath, '\n') : NULL; - if (!controllers || !path) + if (!controllers || !selfpath) continue; if (nl) *nl = '\0'; - *path = '\0'; + *selfpath = '\0'; controllers++; - path++; + selfpath++; for (i = 0 ; i < VIR_CGROUP_CONTROLLER_LAST ; i++) { const char *typestr = virCgroupControllerTypeToString(i); @@ -198,14 +264,25 @@ static int virCgroupDetectPlacement(virCgroupPtr group) char *next = strchr(tmp, ','); int len; if (next) { - len = next-tmp; + len = next - tmp; next++; } else { len = strlen(tmp); } - if (typelen == len && STREQLEN(typestr, tmp, len) && - !(group->controllers[i].placement = strdup(STREQ(path, "/") ? "" : path))) - goto no_memory; + + /* + * selfpath=="/" + path="" -> "/" + * selfpath=="/libvirt.service" + path="" -> "/libvirt.service" + * selfpath=="/libvirt.service" + path="foo" -> "/libvirt.service/foo" + */ + if (typelen == len && STREQLEN(typestr, tmp, len)) { + if (virAsprintf(&group->controllers[i].placement, + "%s%s%s", selfpath, + (STREQ(selfpath, "/") || + STREQ(path, "") ? "" : "/"), + path) < 0) + goto no_memory; + } tmp = next; } @@ -223,13 +300,20 @@ no_memory: } static int virCgroupDetect(virCgroupPtr group, - int controllers) + int controllers, + const char *path, + virCgroupPtr parent) { int rc; int i; int j; + VIR_DEBUG("group=%p controllers=%d path=%s parent=%p", + group, controllers, path, parent); - rc = virCgroupDetectMounts(group); + if (parent) + rc = virCgroupCopyMounts(group, parent); + else + rc = virCgroupDetectMounts(group); if (rc < 0) { VIR_ERROR(_("Failed to detect mounts for %s"), group->path); return rc; @@ -238,9 +322,10 @@ static int virCgroupDetect(virCgroupPtr group, if (controllers >= 0) { VIR_DEBUG("Validating controllers %d", controllers); for (i = 0 ; i < VIR_CGROUP_CONTROLLER_LAST ; i++) { - VIR_DEBUG("Controller '%s' wanted=%s", + VIR_DEBUG("Controller '%s' wanted=%s, mount='%s'", virCgroupControllerTypeToString(i), - (1 << i) & controllers ? "yes" : "no"); + (1 << i) & controllers ? "yes" : "no", + NULLSTR(group->controllers[i].mountPoint)); if (((1 << i) & controllers)) { /* Ensure requested controller is present */ if (!group->controllers[i].mountPoint) { @@ -282,10 +367,15 @@ static int virCgroupDetect(virCgroupPtr group, } /* Check that at least 1 controller is available */ - if (!controllers) + if (!controllers) { + VIR_DEBUG("No controllers set"); return -ENXIO; + } - rc = virCgroupDetectPlacement(group); + if (parent || path[0] == '/') + rc = virCgroupCopyPlacement(group, path, parent); + else + rc = virCgroupDetectPlacement(group, path); if (rc == 0) { /* Check that for every mounted controller, we found our placement */ @@ -339,10 +429,9 @@ int virCgroupPathOfController(virCgroupPtr group, if (group->controllers[controller].placement == NULL) return -ENOENT; - if (virAsprintf(path, "%s%s%s/%s", + if (virAsprintf(path, "%s%s/%s", group->controllers[controller].mountPoint, group->controllers[controller].placement, - STREQ(group->path, "/") ? "" : group->path, key ? key : "") == -1) return -ENOMEM; @@ -634,14 +723,31 @@ static int 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 + * process is used. + * + */ static int virCgroupNew(const char *path, + virCgroupPtr parent, int controllers, virCgroupPtr *group) { int rc = 0; char *typpath = NULL; - VIR_DEBUG("path=%s controllers=%d", path, controllers); + VIR_DEBUG("parent=%p path=%s controllers=%d", + parent, path, controllers); *group = NULL; if (VIR_ALLOC((*group)) != 0) { @@ -649,12 +755,22 @@ static int virCgroupNew(const char *path, goto err; } - if (!((*group)->path = strdup(path))) { - rc = -ENOMEM; - goto err; + if (path[0] == '/' || !parent) { + if (!((*group)->path = strdup(path))) { + rc = -ENOMEM; + goto err; + } + } else { + if (virAsprintf(&(*group)->path, "%s%s%s", + parent->path, + STREQ(parent->path, "") ? "" : "/", + path) < 0) { + rc = -ENOMEM; + goto err; + } } - rc = virCgroupDetect(*group, controllers); + rc = virCgroupDetect(*group, controllers, path, parent); if (rc < 0) goto err; @@ -673,15 +789,16 @@ static int virCgroupAppRoot(bool privileged, bool create, int controllers) { - virCgroupPtr rootgrp = NULL; + virCgroupPtr selfgrp = NULL; int rc; - rc = virCgroupNew("/", controllers, &rootgrp); + rc = virCgroupNewSelf(&selfgrp); + if (rc != 0) return rc; if (privileged) { - rc = virCgroupNew("/libvirt", controllers, group); + rc = virCgroupNew("libvirt", selfgrp, controllers, group); } else { char *rootname; char *username; @@ -690,23 +807,23 @@ static int virCgroupAppRoot(bool privileged, rc = -ENOMEM; goto cleanup; } - rc = virAsprintf(&rootname, "/libvirt-%s", username); + rc = virAsprintf(&rootname, "libvirt-%s", username); VIR_FREE(username); if (rc < 0) { rc = -ENOMEM; goto cleanup; } - rc = virCgroupNew(rootname, controllers, group); + rc = virCgroupNew(rootname, selfgrp, controllers, group); VIR_FREE(rootname); } if (rc != 0) goto cleanup; - rc = virCgroupMakeGroup(rootgrp, *group, create, VIR_CGROUP_NONE); + rc = virCgroupMakeGroup(selfgrp, *group, create, VIR_CGROUP_NONE); cleanup: - virCgroupFree(&rootgrp); + virCgroupFree(&selfgrp); return rc; } #endif @@ -942,7 +1059,6 @@ int virCgroupNewDriver(const char *name, virCgroupPtr *group) { int rc; - char *path = NULL; virCgroupPtr rootgrp = NULL; rc = virCgroupAppRoot(privileged, &rootgrp, @@ -950,20 +1066,12 @@ int virCgroupNewDriver(const char *name, if (rc != 0) goto out; - if (virAsprintf(&path, "%s/%s", rootgrp->path, name) < 0) { - rc = -ENOMEM; - goto out; - } - - rc = virCgroupNew(path, controllers, group); - VIR_FREE(path); - + rc = virCgroupNew(name, rootgrp, -1, group); if (rc == 0) { rc = virCgroupMakeGroup(rootgrp, *group, create, VIR_CGROUP_NONE); if (rc != 0) virCgroupFree(group); } - out: virCgroupFree(&rootgrp); @@ -994,7 +1102,7 @@ int virCgroupNewDriver(const char *name ATTRIBUTE_UNUSED, #if defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R int virCgroupNewSelf(virCgroupPtr *group) { - return virCgroupNew("/", -1, group); + return virCgroupNew("", NULL, -1, group); } #else int virCgroupNewSelf(virCgroupPtr *group ATTRIBUTE_UNUSED) @@ -1019,13 +1127,8 @@ int virCgroupNewDomain(virCgroupPtr driver, virCgroupPtr *group) { int rc; - char *path; - if (virAsprintf(&path, "%s/%s", driver->path, name) < 0) - return -ENOMEM; - - rc = virCgroupNew(path, -1, group); - VIR_FREE(path); + rc = virCgroupNew(name, driver, -1, group); if (rc == 0) { /* @@ -1071,18 +1174,18 @@ int virCgroupNewVcpu(virCgroupPtr domain, virCgroupPtr *group) { int rc; - char *path; + char *name; int controllers; - if (virAsprintf(&path, "%s/vcpu%d", domain->path, vcpuid) < 0) + if (virAsprintf(&name, "vcpu%d", vcpuid) < 0) return -ENOMEM; controllers = ((1 << VIR_CGROUP_CONTROLLER_CPU) | (1 << VIR_CGROUP_CONTROLLER_CPUACCT) | (1 << VIR_CGROUP_CONTROLLER_CPUSET)); - rc = virCgroupNew(path, controllers, group); - VIR_FREE(path); + rc = virCgroupNew(name, domain, controllers, group); + VIR_FREE(name); if (rc == 0) { rc = virCgroupMakeGroup(domain, *group, create, VIR_CGROUP_NONE); @@ -1116,18 +1219,13 @@ int virCgroupNewEmulator(virCgroupPtr domain, virCgroupPtr *group) { int rc; - char *path; int controllers; - if (virAsprintf(&path, "%s/emulator", domain->path) < 0) - return -ENOMEM; - controllers = ((1 << VIR_CGROUP_CONTROLLER_CPU) | (1 << VIR_CGROUP_CONTROLLER_CPUACCT) | (1 << VIR_CGROUP_CONTROLLER_CPUSET)); - rc = virCgroupNew(path, controllers, group); - VIR_FREE(path); + rc = virCgroupNew("emulator", domain, controllers, group); if (rc == 0) { rc = virCgroupMakeGroup(domain, *group, create, VIR_CGROUP_NONE); @@ -2015,8 +2113,6 @@ static int virCgroupKillRecursiveInternal(virCgroupPtr group, int signum, virHas } while ((ent = readdir(dp))) { - char *subpath; - if (STREQ(ent->d_name, ".")) continue; if (STREQ(ent->d_name, "..")) @@ -2025,12 +2121,8 @@ static int virCgroupKillRecursiveInternal(virCgroupPtr group, int signum, virHas continue; VIR_DEBUG("Process subdir %s", ent->d_name); - if (virAsprintf(&subpath, "%s/%s", group->path, ent->d_name) < 0) { - rc = -ENOMEM; - goto cleanup; - } - if ((rc = virCgroupNew(subpath, -1, &subgroup)) != 0) + if ((rc = virCgroupNew(ent->d_name, group, -1, &subgroup)) != 0) goto cleanup; if ((rc = virCgroupKillRecursiveInternal(subgroup, signum, pids, true)) < 0) diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c index a68aa88..3f35f2e 100644 --- a/tests/vircgrouptest.c +++ b/tests/vircgrouptest.c @@ -94,11 +94,11 @@ static int testCgroupNewForSelf(const void *args ATTRIBUTE_UNUSED) const char *placement[VIR_CGROUP_CONTROLLER_LAST] = { [VIR_CGROUP_CONTROLLER_CPU] = "/system", [VIR_CGROUP_CONTROLLER_CPUACCT] = "/system", - [VIR_CGROUP_CONTROLLER_CPUSET] = "", - [VIR_CGROUP_CONTROLLER_MEMORY] = "", + [VIR_CGROUP_CONTROLLER_CPUSET] = "/", + [VIR_CGROUP_CONTROLLER_MEMORY] = "/", [VIR_CGROUP_CONTROLLER_DEVICES] = NULL, - [VIR_CGROUP_CONTROLLER_FREEZER] = "", - [VIR_CGROUP_CONTROLLER_BLKIO] = "", + [VIR_CGROUP_CONTROLLER_FREEZER] = "/", + [VIR_CGROUP_CONTROLLER_BLKIO] = "/", }; if (virCgroupNewSelf(&cgroup) < 0) { @@ -106,7 +106,7 @@ static int testCgroupNewForSelf(const void *args ATTRIBUTE_UNUSED) goto cleanup; } - ret = validateCgroup(cgroup, "/", mountsFull, placement); + ret = validateCgroup(cgroup, "", mountsFull, placement); cleanup: virCgroupFree(&cgroup); @@ -119,14 +119,23 @@ static int testCgroupNewForDriver(const void *args ATTRIBUTE_UNUSED) virCgroupPtr cgroup = NULL; int ret = -1; int rv; - const char *placement[VIR_CGROUP_CONTROLLER_LAST] = { - [VIR_CGROUP_CONTROLLER_CPU] = "/system", - [VIR_CGROUP_CONTROLLER_CPUACCT] = "/system", - [VIR_CGROUP_CONTROLLER_CPUSET] = "", - [VIR_CGROUP_CONTROLLER_MEMORY] = "", + const char *placementSmall[VIR_CGROUP_CONTROLLER_LAST] = { + [VIR_CGROUP_CONTROLLER_CPU] = "/system/libvirt/lxc", + [VIR_CGROUP_CONTROLLER_CPUACCT] = "/system/libvirt/lxc", + [VIR_CGROUP_CONTROLLER_CPUSET] = NULL, + [VIR_CGROUP_CONTROLLER_MEMORY] = "/libvirt/lxc", [VIR_CGROUP_CONTROLLER_DEVICES] = NULL, - [VIR_CGROUP_CONTROLLER_FREEZER] = "", - [VIR_CGROUP_CONTROLLER_BLKIO] = "", + [VIR_CGROUP_CONTROLLER_FREEZER] = NULL, + [VIR_CGROUP_CONTROLLER_BLKIO] = NULL, + }; + const char *placementFull[VIR_CGROUP_CONTROLLER_LAST] = { + [VIR_CGROUP_CONTROLLER_CPU] = "/system/libvirt/lxc", + [VIR_CGROUP_CONTROLLER_CPUACCT] = "/system/libvirt/lxc", + [VIR_CGROUP_CONTROLLER_CPUSET] = "/libvirt/lxc", + [VIR_CGROUP_CONTROLLER_MEMORY] = "/libvirt/lxc", + [VIR_CGROUP_CONTROLLER_DEVICES] = NULL, + [VIR_CGROUP_CONTROLLER_FREEZER] = "/libvirt/lxc", + [VIR_CGROUP_CONTROLLER_BLKIO] = "/libvirt/lxc", }; if ((rv = virCgroupNewDriver("lxc", true, false, -1, &cgroup)) != -ENOENT) { @@ -159,14 +168,14 @@ static int testCgroupNewForDriver(const void *args ATTRIBUTE_UNUSED) fprintf(stderr, "Cannot create LXC cgroup: %d\n", -rv); goto cleanup; } - ret = validateCgroup(cgroup, "/libvirt/lxc", mountsSmall, placement); + ret = validateCgroup(cgroup, "libvirt/lxc", mountsSmall, placementSmall); virCgroupFree(&cgroup); if ((rv = virCgroupNewDriver("lxc", true, true, -1, &cgroup)) != 0) { fprintf(stderr, "Cannot create LXC cgroup: %d\n", -rv); goto cleanup; } - ret = validateCgroup(cgroup, "/libvirt/lxc", mountsFull, placement); + ret = validateCgroup(cgroup, "libvirt/lxc", mountsFull, placementFull); cleanup: virCgroupFree(&cgroup); @@ -181,13 +190,13 @@ static int testCgroupNewForDomain(const void *args ATTRIBUTE_UNUSED) int ret = -1; int rv; const char *placement[VIR_CGROUP_CONTROLLER_LAST] = { - [VIR_CGROUP_CONTROLLER_CPU] = "/system", - [VIR_CGROUP_CONTROLLER_CPUACCT] = "/system", - [VIR_CGROUP_CONTROLLER_CPUSET] = "", - [VIR_CGROUP_CONTROLLER_MEMORY] = "", + [VIR_CGROUP_CONTROLLER_CPU] = "/system/libvirt/lxc/wibble", + [VIR_CGROUP_CONTROLLER_CPUACCT] = "/system/libvirt/lxc/wibble", + [VIR_CGROUP_CONTROLLER_CPUSET] = "/libvirt/lxc/wibble", + [VIR_CGROUP_CONTROLLER_MEMORY] = "/libvirt/lxc/wibble", [VIR_CGROUP_CONTROLLER_DEVICES] = NULL, - [VIR_CGROUP_CONTROLLER_FREEZER] = "", - [VIR_CGROUP_CONTROLLER_BLKIO] = "", + [VIR_CGROUP_CONTROLLER_FREEZER] = "/libvirt/lxc/wibble", + [VIR_CGROUP_CONTROLLER_BLKIO] = "/libvirt/lxc/wibble", }; if ((rv = virCgroupNewDriver("lxc", true, false, -1, &drivercgroup)) != 0) { @@ -200,7 +209,7 @@ static int testCgroupNewForDomain(const void *args ATTRIBUTE_UNUSED) goto cleanup; } - ret = validateCgroup(domaincgroup, "/libvirt/lxc/wibble", mountsFull, placement); + ret = validateCgroup(domaincgroup, "libvirt/lxc/wibble", mountsFull, placement); cleanup: virCgroupFree(&drivercgroup); @@ -246,4 +255,4 @@ mymain(void) return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE; } -VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/libvircgroupmock.so") +VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/vircgroupmock.so") -- 1.8.1.4