On 09/04/2014 03:52 AM, Wang Rui wrote:
From: Yue Wenyuan <yuewenyuan@huawei.com>
This patch implements libvirt_lxc process pin with emulatorpin specified in xml.
Signed-off-by: Wang Rui <moon.wangrui@huawei.com> Signed-off-by: Yue Wenyuan <yuewenyuan@huawei.com> --- src/lxc/lxc_cgroup.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++ src/lxc/lxc_cgroup.h | 4 +++ src/lxc/lxc_controller.c | 4 +++ 3 files changed, 76 insertions(+)
I'm not an LXC expert, but I'll give this a go since it's been sitting around a while. I am curious why only the CPUSET (eg, <vcpuset ... cpuset="1-4,^3,6"> or <emulatorpin cpuset="1-3"/>) were handled and the <emulator_period> and <emulator_quota> were not?
diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index f9af31c..f696bf8 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -530,3 +530,71 @@ int virLXCCgroupSetup(virDomainDefPtr def, cleanup: return ret; } + +static int virLXCCgroupSetupCpusetTuneForEmulator(virDomainDefPtr def,
Change name virLXCCgroupSetupCpusetTuneForEmulator to virLXCSetupCgroupCpusetTunesForEmulator [1] note the (s) on Tune as well... Could have also gone with "CpusetCpusMems", but that really seemed too long
+ virCgroupPtr cgroup, + virBitmapPtr nodemask)
Although lxc_cgroup doesn't do it for the majority of the functions use: static int virLXCSetupCgroupEmulatorCpusetCpusMems(...) make sure to align arguments properly too.
+{ + int ret = -1; + char *mask = NULL; + + if (def->placement_mode != VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) { + if (def->cputune.emulatorpin) { + if (!(mask = virBitmapFormat(def->cputune.emulatorpin->cpumask))) + return ret; + } else if (def->cpumask) { + if (!(mask = virBitmapFormat(def->cpumask))) + return ret; + }
At this point mask could still be NULL, thus you need a "if (mask && " prior to call...
+ if (virCgroupSetCpusetCpus(cgroup, mask) < 0) + goto cleanup; + }
And if MODE_AUTO, then does virLXCControllerSetupCpuAffinity called prior to this function satisfy the doc requirement to query numad? If so, then perhaps noting that as a comment prior to this hunk of code would be beneficial. If not, then that needs to be handled. It doesn't seem that way since I don't see comparable code in LXC to the qemuPrepareCpumap code that handles the host.numaCell. Also since you're reusing 'mask' below you'll need a VIR_FREE(mask); prior to the next set of calls; otherwise, you'll leak it.
+ + if (virDomainNumatuneMaybeFormatNodeset(def->numatune, nodemask, + &mask, -1) < 0) + goto cleanup; + + if (mask && virCgroupSetCpusetMems(cgroup, mask) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(mask); + return ret; +} + +int virLXCCgroupSetupForEmulator(virDomainDefPtr def, + virCgroupPtr cgroup, + virBitmapPtr nodemask)
The qemu function is qemuSetupCgroupForEmulator, thus this should be virLXCSetupCgroupForEmulator. [2] The function arguments need to be aligned and like above: int virLXCSetupCgroupForEmulator(...)
+{ + virCgroupPtr cgroup_emulator = NULL; + + if (!virCgroupHasController(cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) + return 0; + + if (cgroup == NULL) + return 0; /* Not supported, so claim success */
This seems to be superfluous as the virCgroupHasController() will return false if (!cgroup), although I do see that it's a cut-n-paste of the similar qemu code.
+ + if (virCgroupNewEmulator(cgroup, true, &cgroup_emulator) < 0) + goto error; + + if (virCgroupMoveTask(cgroup, cgroup_emulator) < 0) + goto error; + + if (virCgroupHasController(cgroup_emulator, VIR_CGROUP_CONTROLLER_CPUSET) && ^^^^^^^^^^^^^^^ qemu code uses priv->cgroup, so should this code use 'cgroup' instead? or is the qemu code incorrect and should be fixed?
+ virLXCCgroupSetupCpusetTuneForEmulator(def, cgroup_emulator, nodemask) < 0)
[1] Use new name...
+ goto error; + + virCgroupFree(&cgroup_emulator); + return 0; + + error: + + if (cgroup_emulator) { + virCgroupRemove(cgroup_emulator); + virCgroupFree(&cgroup_emulator); + } + + return -1; +} diff --git a/src/lxc/lxc_cgroup.h b/src/lxc/lxc_cgroup.h index 0e78126..32086c5 100644 --- a/src/lxc/lxc_cgroup.h +++ b/src/lxc/lxc_cgroup.h @@ -33,6 +33,10 @@ int virLXCCgroupSetup(virDomainDefPtr def, virCgroupPtr cgroup, virBitmapPtr nodemask);
+int virLXCCgroupSetupForEmulator(virDomainDefPtr def, + virCgroupPtr cgroup, + virBitmapPtr nodemask); + [2] See note above about name change s/CgroupSetup/SetupCgroup
Also the argument alignment is off - yes I see the previous function prototype is too, but lets at least do this one right and fix the other one later in either a follow up or separate patch prior to this patch.
int virLXCCgroupGetMeminfo(virLXCMeminfoPtr meminfo);
int diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 1861dd6..1a62e20 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -698,6 +698,10 @@ static int virLXCControllerSetupResourceLimits(virLXCControllerPtr ctrl) if (virLXCCgroupSetup(ctrl->def, ctrl->cgroup, nodemask) < 0) goto cleanup;
+ VIR_DEBUG("Setting cgroup for lxc emulator"); + if (virLXCCgroupSetupForEmulator(ctrl->def, ctrl->cgroup, nodemask) < 0)
[2] See note above about name change s/CgroupSetup/SetupCgroup
+ goto cleanup; + ret = 0; cleanup: virBitmapFree(nodemask);
You need to update docs/formatdomain.html.in as well to add the LXC Since 1.2.x (whenever this gets in) to various bits and parts of the <cputune> and <vcpus> descriptions. John