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

[John Ferlan]

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?

> 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