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

[Wang Rui]

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