Re: [libvirt] [PATCH 1/3] lxc: Implement pin emulator for container startup

On 09/04/2014 03:52 AM, Wang Rui wrote: > From: Yue Wenyuan <yuewenyuan(a)huawei.com&gt; > > This patch implements libvirt_lxc process pin with emulatorpin > specified in xml. > > Signed-off-by: Wang Rui <moon.wangrui(a)huawei.com&gt; > Signed-off-by: Yue Wenyuan <yuewenyuan(a)huawei.com&gt; > --- > 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?

> +{ > + 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)

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.