On 2012年10月10日 22:59, Martin Kletzander wrote:

On 10/10/2012 01:14 PM, Osier Yang wrote:

...

Setting pinning policy for vcpu which exceed current vcpus number makes no sense, and it could cause problem for APIs which associate the vcpu thread id with cgroup. --- src/conf/domain_conf.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 33e1e7f..87e0c6b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8746,9 +8746,9 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, if (n&& VIR_ALLOC_N(def->cputune.vcpupin, n)< 0) goto no_memory;

- if (n> def->maxvcpus) { + if (n> def->vcpus) { virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("vcpupin nodes must be less than maxvcpus")); + "%s", _("vcpupin nodes must be less than current vcpus")); goto error; }

What about XML that specifies vcpu pinning for all the vcpus that might be there, but now enables only one. And other pinning just won't be used until those vcpus are in plugged. Or am I missing some problems this could cause?

That means we will keep the <vcpupin> for unplugged vcpus too. Assuming we go this way, then: It needs to maitain these unused yet <vcpupin> info in def->cputune, which cause a further work to do: sort the def->cputune.vcpupin by vcpuid, and then use def->vcpus instead of def->cputune.nvcpupin in various places to only work on the current onlined vcpus, and it's very likely missing the change in some corner. On one hand, personally I'd like manage the <vcpupin> dynamically, I.e. add it for plugged vcpu, remove it for unplugged vcpu. Keeping it always there looks strange. On the other hand, regardless of which way is better, better to keep it simple for us now, especially I won't want to see upcoming bugs bite me in the edge. :-) Regards, Osier

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

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