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?
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list