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

[Osier Yang]

On 2012年10月10日 23:34, Martin Kletzander wrote:
> 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.

s/enforce/inherit/ should be a good addition.

>
> 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.


Actually related, when the domain starting, we setup cgroup or affinity
for each vcpu by its pinnng policy, but because we ignored the global
def->cpuset for vcpus which doesn't has pinning policy specified, they
are pinnined to all available pCPUs by default.

The similiar problem existed for vcpupin (8 y'es in above example).

So there are two problems to resolve here.

1) vcpuinfo get the pinning by virProcessInfoGetAffinity, so we need to
    make sure affinity for each vcpu setup correctly by either cgroup
    setting or virProcessInfoSetAffinity.

2) vcpupin get the pinning from def->cputune.vcpupin, (NB, it supports
    get the pinnng policy for inactive domain too), so we need to make
    sure the def->cputune.vcpupin is correct.

There are two ways to fix the problems:

1) inherit the def->cpuset for vcpus which doesn't have pinning policy
specified when parsing domain conf (just like this patch does).

2) inherit def->cpuset internally (inside driver) for the APIs. E.g.
    when setup cgroup, when qemudDomainGetVcpuPinInfo.

I choosed 1), because we never known how many new codes will take use
of the vcpu pinning policy in future, and it's a bad idea to use the
implicit inheritance rule across. On the other hand, the internal data
struct should be consistent with what domain conf presents.

>
>
> 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.

Okay,

>
>> +
>> +        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

Documentation can be updated. :-)

Mention that vcpu cpuset inherits def->cpuset if it's not specified, (
well, actually we have a bad logic design currently, <vcpupin> is
independant with <vcpu cpuset='4'>, it doesn't hornor the global
def->cpuset).


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.

IIUC, this can be easily sort out by not printing <vcpupin> which has
same cpuset as def->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.
>

Thanks for the points, hopefully my explanation is readable, waiting
for a conclusion, and will post v2 after that.

Regards,
Osier