On 2014/10/22 23:34, John Ferlan wrote:
>
>
> On 09/04/2014 03:52 AM, Wang Rui wrote:
>> From: Yue Wenyuan <yuewenyuan@xxxxxxxxxx>
>>
>> This patch implements libvirt_lxc process pin with emulatorpin
>> specified in xml.
>>
>> Signed-off-by: Wang Rui <moon.wangrui@xxxxxxxxxx>
>> Signed-off-by: Yue Wenyuan <yuewenyuan@xxxxxxxxxx>
>> ---
>> 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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list