On 2014/10/22 23:34, John Ferlan wrote:
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?
Hi, John. Thank you for your review. These patches are only the implementation of emulator pin. It is ture that <emulator_period> and <emulator_quota> are not supported by lxc. I think It can be handled in the later patch if these patches are ACKed. [...]
+{ + 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.
Thanks to point that. I think virLXCControllerGetNumadAdvice which is called in virLXCControllerSetupResourceLimits is the comparable function to qemuPrepareCpumap. Maybe it's another bug.
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)
[...]
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.
Some code is not well aligned. I can fix that as your suggestion. I'll send patches V2 after release 1.2.10.