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

2 Jul 2012

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@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@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

    