[libvirt] [PATCH 04/13] Enable cpuset cgroup and synchronous vcpupin info to cgroup

[Eric Blake]

On 06/05/2012 02:17 AM, tangchen wrote:
> This patch enables cpuset cgroup, and synchronous vcpupin info set by sched_setaffinity() to cgroup.
> 

This doesn't really give many details about what you are trying to do here.

> Signed-off-by: Tang Chen <tangchen at cn.fujitsu.com>
> ---
>  src/libvirt_private.syms |    2 +
>  src/qemu/qemu_cgroup.c   |   51 ++++++++++++++++++++++++++++++++++++++++++++-
>  src/qemu/qemu_cgroup.h   |    2 +
>  src/qemu/qemu_driver.c   |   43 +++++++++++++++++++++++++++++++-------
>  src/util/cgroup.c        |   35 ++++++++++++++++++++++++++++++-
>  src/util/cgroup.h        |    3 ++
>  6 files changed, 125 insertions(+), 11 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 6ff1a3b..88cc37a 100644
> --- a/src/libvirt_private.syms

> +++ b/src/qemu/qemu_cgroup.c
> @@ -473,18 +473,57 @@ cleanup:
>          rc = virCgroupSetCpuCfsPeriod(cgroup, old_period);
>          if (rc < 0)
>              virReportSystemError(-rc,
> -                                 _("%s"),
> -                                 "Unable to rollback cpu bandwidth period");
> +                                 "%s",
> +                                 _("Unable to rollback cpu bandwidth period"));

This hunk is an independent bug fix, and should be pushed separately.  I
will take care of that shortly.

>      }
>  
>      return -1;
>  }
>  
> +int qemuSetupCgroupVcpuPin(virCgroupPtr cgroup, virDomainDefPtr def,
> +                           int vcpuid)
> +{
> +    int i, rc;
> +    char *new_cpus = NULL;
> +
> +    if (vcpuid < 0 || vcpuid >= def->vcpus) {
> +        virReportSystemError(EINVAL,
> +                             "%s: %d", _("invalid vcpuid"), vcpuid);

I would write this:

virReportSystemError(EINVAL,
                     _("invalid vcpuid: %d"), vcpuid);

> +        return -1;
> +    }
> +
> +    for (i = 0; i < def->cputune.nvcpupin; i++) {
> +        if (vcpuid == def->cputune.vcpupin[i]->vcpuid) {
> +            new_cpus = virDomainCpuSetFormat(def->cputune.vcpupin[i]->cpumask,
> +                                             VIR_DOMAIN_CPUMASK_LEN);
> +            if (!new_cpus) {
> +                qemuReportError(VIR_ERR_INTERNAL_ERROR,
> +                                _("failed to convert cpu mask"));
> +                goto cleanup;
> +            }
> +            rc = virCgroupSetCpusetCpus(cgroup, new_cpus);
> +            if (rc < 0) {
> +                virReportSystemError(-rc,
> +                                     "%s", _("Unable to set cpuset.cpus"));
> +                goto cleanup;
> +            }
> +        }
> +    }
> +    VIR_FREE(new_cpus);
> +    return 0;
> +
> +cleanup:
> +    if (new_cpus)
> +        VIR_FREE(new_cpus);

This fails 'make syntax-check':

src/qemu/qemu_cgroup.c: if (new_cpus)
        VIR_FREE(new_cpus);
maint.mk: found useless "if" before "free" above

> +    return -1;
> +}

And since you call VIR_FREE(new_cpus) on both success and failure path,
I'd consolidate things.  Declare this up front:

int ret = -1;

then the tail of the function becomes:

    ret = 0;
cleanup:
    VIR_FREE(new_cpus);
    return ret;
}

> @@ -556,6 +595,14 @@ int qemuSetupCgroupForVcpu(struct qemud_driver *driver, virDomainObjPtr vm)
>              }
>          }
>  
> +        /* Set vcpupin in cgroup if vcpupin xml is provided */
> +        if (def->cputune.nvcpupin) {
> +            if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPUSET)) {
> +                if (qemuSetupCgroupVcpuPin(cgroup_vcpu, def, i) < 0)
> +                    goto cleanup;

Rather than nesting this deeply, you could use &&, as in:

if (def->cputune.nvcpupin &&
    qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPUSET) &&
    qemuSetupCgroupVcpuPin(cgroup_vcpu, def, i) < 0)
    goto cleanup;


> @@ -3605,9 +3607,37 @@ qemudDomainPinVcpuFlags(virDomainPtr dom,
>      if (flags & VIR_DOMAIN_AFFECT_LIVE) {
>  
>          if (priv->vcpupids != NULL) {
> +            /* Add config to vm->def first, because cgroup APIs need it. */
> +            if (virDomainVcpuPinAdd(vm->def, cpumap, maplen, vcpu) < 0) {
> +                qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                                _("failed to update or add vcpupin xml of "
> +                                  "a running domain"));
> +                goto cleanup;
> +            }
> +
> +            /* Configure the corresponding cpuset cgroup before set affinity. */
> +            if (qemuCgroupControllerActive(driver,
> +                                           VIR_CGROUP_CONTROLLER_CPUSET)) {
> +                if (virCgroupForDomain(driver->cgroup, vm->def->name,
> +                                       &cgroup_dom, 0) == 0) {
> +                    if (virCgroupForVcpu(cgroup_dom, vcpu, &cgroup_vcpu, 0) == 0) {
> +                        if (qemuSetupCgroupVcpuPin(cgroup_vcpu, vm->def, vcpu) < 0) {
> +                            qemuReportError(VIR_ERR_OPERATION_INVALID, "%s %d",
> +                                            _("failed to set cpuset.cpus in cgroup"
> +                                              " for vcpu"), vcpu);

Another place where I would embed the %d into the message, as in:

_("failed to set cpuset.cpus in cgroup for vcpu %d"), vcpu

> +                            goto cleanup;
> +                        }
> +                    }
> +                }
> +            }
> +
>              if (virProcessInfoSetAffinity(priv->vcpupids[vcpu],
> -                                          cpumap, maplen, maxcpu) < 0)
> +                                          cpumap, maplen, maxcpu) < 0) {
> +                qemuReportError(VIR_ERR_SYSTEM_ERROR, "%s %d",
> +                                _("failed to set cpu affinity for vcpu"),
> +                                vcpu);

and again

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120702/35a1a880/attachment-0001.sig>