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

[Eric Blake]

On 08/03/2012 12:36 AM, Hu Tao wrote:
> From: Tang Chen <tangchen at cn.fujitsu.com>
> 
> vcpu threads pin are implemented using sched_setaffinity(), but
> not controlled by cgroup. This patch does the following things:
> 
>     1) enable cpuset cgroup
>     2) reflect all the vcpu threads pin info to cgroup
> 
> Signed-off-by: Tang Chen <tangchen at cn.fujitsu.com>
> Signed-off-by: Hu Tao <hutao at cn.fujitsu.com>
> ---

> @@ -3667,9 +3669,38 @@ qemudDomainPinVcpuFlags(virDomainPtr dom,
>      if (flags & VIR_DOMAIN_AFFECT_LIVE) {
>  
>          if (priv->vcpupids != NULL) {
> +            /* Add config to vm->def first, because qemuSetupCgroupVcpuPin needs it. */
> +            if (virDomainVcpuPinAdd(vm->def, cpumap, maplen, vcpu) < 0) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                               _("failed to update or add vcpupin xml of "
> +                                 "a running domain"));
> +                goto cleanup;
> +            }

If this succeeds,

> +
> +            /* 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 &&
> +                    virCgroupForVcpu(cgroup_dom, vcpu, &cgroup_vcpu, 0) == 0 &&
> +                    qemuSetupCgroupVcpuPin(cgroup_vcpu, vm->def, vcpu) < 0) {
> +                    virReportError(VIR_ERR_OPERATION_INVALID,
> +                                   _("failed to set cpuset.cpus in cgroup"
> +                                     " for vcpu %d"), vcpu);
> +                    goto cleanup;

but this fails, then where do we roll vm->def back to its pre-attempt state?

> +                }
> +            } else {
> +                /* Here, we should go on because even if cgroup is not active,
> +                 * we can still use virProcessInfoSetAffinity.
> +                 */
> +                VIR_WARN("cpuset cgroup is not active");
> +            }
> +
>              if (virProcessInfoSetAffinity(priv->vcpupids[vcpu],
> -                                          cpumap, maplen, maxcpu) < 0)
> +                                          cpumap, maplen, maxcpu) < 0) {
> +                virReportError(VIR_ERR_SYSTEM_ERROR,
> +                               _("failed to set cpu affinity for vcpu %d"),
> +                               vcpu);
>                  goto cleanup;

Same situation - if set_affinity failed, where do we roll back to the
original vm->def state (also, should we roll back to the original cpuset
state)?

If we don't roll back, then a failure can leave the domain in an unknown
state for cpu pinning.  Or are we just declaring that if these functions
fail, the domain is in an unknown set of cpu pinning?

Do we need both cpuset and set_affinity?  Or is cpuset a superset of
affinity (that is, if I alter cpuset, can I completely skip out on
calling set_affinity and still get the same results)?  If cpuset gives
stronger pinning than affinity, then maybe we don't need to try both
methods, which gives us a bit better mechanism for rollbacks; the only
remaining thing is making sure you avoid altering vm->def except on
success (perhaps by using a temporary structure rather than vm->def at
the time you attempt the pinning).

> +            }
>          } else {
>              virReportError(VIR_ERR_OPERATION_INVALID,
>                             "%s", _("cpu affinity is not supported"));
> @@ -3683,13 +3714,6 @@ qemudDomainPinVcpuFlags(virDomainPtr dom,
>                                   "a running domain"));
>                  goto cleanup;
>              }
> -        } else {
> -            if (virDomainVcpuPinAdd(vm->def, cpumap, maplen, vcpu) < 0) {
> -                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                               _("failed to update or add vcpupin xml of "
> -                                 "a running domain"));
> -                goto cleanup;
> -            }

In other words, I think floating this code to occur before the pinning
is dangerous, unless you also add a rollback mechanism.

That said, if you ignore this particular corner case of failure
recovery, the rest of the patch seems fine, so it might be okay to add
failure recovery as a followup patch.  I'll continue reviewing the
series, and try actually playing with the results (which, in the common
case of success, should just work).

-- 
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/20120806/c5309b38/attachment-0001.sig>