[libvirt] [PATCH 2/3] conf: Initialize the pinning policy for vcpus

[Martin Kletzander]

On 10/10/2012 01:14 PM, Osier Yang wrote:
> There are two branches to specifiy the pinning policy for vcpus.
> 
> 1) def->cpuset:
> 
>    <vcpu cpuset='0-10,^6'>6</vcpu>
> 
> 2) def->cputune.vcpupins:
> 
>    <cputune>
>      <vcpupin vcpuid='1' cpuset='0-5'/>
>    </cputune>
> 
> def->cputune.vcpupins only maintains the pinning policy only for
> vcpus have <vcpupin> specified explictly, this works fine before

s/explictly/explicitely/, but...

> cgroup is widely used for domain. But since now cgroup uses
> def->cputune.vcpupins to setup the cgroups for each vcpus thread,
> it means the vcpus which doesn't has <vcpupin> specified will
> be ignored for cgroup setting.
> 

... anyway, I didn't quite get that for the first time. However IIUC for
the second time I think it's enough to mention that in case there are
both cpuset and cputune with vcpupin, we don't enforce the cpuset for
vcpus not specified in the cputune.

OK, maybe mine version is not that readable either :-)

> To fix the problem, the patch initialize vcpupin for each vcpu
> which doesn't has <vcpupin> specified as def->cpuset.
> 
> Problem example without this patch:
> 
> 1. % virsh start dom
> Domain dom started
> 
> 
> 2. % virsh dumpxml dom
> <vcpu placement='static' cpuset='3'>4</vcpu>
> 
> 3. % virsh vcpuinfo dom
> VCPU: 0
> CPU: 0
> State: running
> CPU time: 8.3s
> CPU Affinity: yyyyyyyy
> 
> VCPU: 1
> CPU: 4
> State: running
> CPU time: 2.4s
> CPU Affinity: yyyyyyyy
> 
> VCPU: 2
> CPU: 0
> State: running
> CPU time: 4.1s
> CPU Affinity: yyyyyyyy
> 
> VCPU: 3
> CPU: 1
> State: running
> CPU time: 2.8s
> CPU Affinity: yyyyyyyy
> 

Maybe I didn't understand the description above correctly, but this
doesn't seem connected with it.


Anyway, don't hesitate to correct me.

> ---
>  src/conf/domain_conf.c |   35 +++++++++++++++++++++++++++++++++++
>  1 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 87e0c6b..2d14489 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8772,6 +8772,41 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>      }
>      VIR_FREE(nodes);
>  
> +    /* Initialize the pinning policy for vcpus which doesn't has
> +     * the policy specified explicitly as def->cpuset.
> +     */
> +    if (def->cpumask) {
> +        if (!def->cputune.vcpupin) {
> +            if (VIR_ALLOC_N(def->cputune.vcpupin, def->vcpus) < 0) {
> +                virReportOOMError();
> +                goto error;
> +            }
> +        } else {
> +            if (VIR_REALLOC_N(def->cputune.vcpupin, def->vcpus) < 0) {
> +                virReportOOMError();
> +                goto error;
> +            }
> +        }

You can use VIR_REALLOC_N with NULL, it works as a VIR_ALLOC_N.

> +
> +        for (i = 0; i < def->vcpus; i++) {
> +            if (!virDomainVcpuPinIsDuplicate(def->cputune.vcpupin,
> +                                             def->cputune.nvcpupin,
> +                                             i)) {
> +                virDomainVcpuPinDefPtr vcpupin = NULL;
> +
> +                if (VIR_ALLOC(vcpupin) < 0) {
> +                    virReportOOMError();
> +                    goto error;
> +                }
> +
> +                vcpupin->cpumask = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN);
> +                virBitmapCopy(vcpupin->cpumask, def->cpumask);
> +                vcpupin->vcpuid = i;
> +                def->cputune.vcpupin[def->cputune.nvcpupin++] = vcpupin;

I don't know if we should update the cpu pinning in the definition.
That will collide with documentation and mostly it will create a huge
XML and thus harden changing the cpuset for all vcpus without pinning.

To explain myself a little easier, imagine this machine:
...
    <vcpu cpuset='0-10,^6'>16</vcpu>
    <cputune>
      <vcpupin vcpuid='1' cpuset='0-10,^5'/>
      <vcpupin vcpuid='7' cpuset='0-10,^7'/>
      <vcpupin vcpuid='11' cpuset='0-10,^9'/>
      <vcpupin vcpuid='13' cpuset='0-10,^10'/>
    </cputune>
...

and imagine you want to change the cpuset to for example something like
'0-10,^2,^6' (I know it doesn't make sense). You'd have to remove all
the cpus that got allocated there before changing the cpuset.

> +            }
> +        }
> +    }
> +
>      if ((n = virXPathNodeSet("./cputune/emulatorpin", ctxt, &nodes)) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("cannot extract emulatorpin nodes"));
> 

I'd rather handle this as I said, but I'm willing to discuss it and
opened for ideas.

Martin