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; } -- 1.7.7.6

On 2012年10月10日 22:59, Martin Kletzander wrote: 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: s/explictly/explicitely/, but... ... 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 :-) Maybe I didn't understand the description above correctly, but this doesn't seem connected with it. Anyway, don't hesitate to correct me. You can use VIR_REALLOC_N with NULL, it works as a VIR_ALLOC_N. 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. 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: s/enforce/inherit/ should be a good addition. 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. Okay, 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 IIUC, this can be easily sort out by not printing <vcpupin> which has same cpuset as def->cpuset. Thanks for the points, hopefully my explanation is readable, waiting for a conclusion, and will post v2 after that. Regards, Osier

The onlined vcpu pinning policy should inherit def->cpuset if it's not specified explicitly, and the affinity should be set in this case. Oppositely, the offlined vcpu pinning policy should be free()'ed. --- src/conf/domain_conf.c | 21 +++++++++++++ src/conf/domain_conf.h | 3 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 75 ++++++++++++++++++++++++++++++++++++++++----- 4 files changed, 91 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2d14489..d82d5ba 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8360,6 +8360,27 @@ error: goto cleanup; } +/* + * Return the vcpupin related with the vcpu id on SUCCESS, or + * NULL on failure. + */ +virDomainVcpuPinDefPtr +virDomainLookupVcpuPin(virDomainDefPtr def, + int vcpuid) +{ + int i; + + if (!def->cputune.vcpupin) + return NULL; + + for (i = 0; i < def->cputune.nvcpupin; i++) { + if (def->cputune.vcpupin[i]->vcpuid == vcpuid) + return def->cputune.vcpupin[i]; + } + + return NULL; +} + static int virDomainDefMaybeAddController(virDomainDefPtr def, int type, int idx) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 14dead3..852440b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2284,4 +2284,7 @@ virDomainNetDefPtr virDomainNetFind(virDomainDefPtr def, int virDomainList(virConnectPtr conn, virHashTablePtr domobjs, virDomainPtr **domains, unsigned int flags); +virDomainVcpuPinDefPtr virDomainLookupVcpuPin(virDomainDefPtr def, + int vcpuid); + #endif /* __DOMAIN_CONF_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a8c81e7..210f283 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -421,6 +421,7 @@ virDomainLifecycleTypeFromString; virDomainLifecycleTypeToString; virDomainLiveConfigHelperMethod; virDomainLoadAllConfigs; +virDomainLookupVcpuPin; virDomainMemballoonModelTypeFromString; virDomainMemballoonModelTypeToString; virDomainMemDumpTypeFromString; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b8ceca1..350b352 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3603,6 +3603,7 @@ static int qemudDomainHotplugVcpus(struct qemud_driver *driver, int ncpupids; virCgroupPtr cgroup = NULL; virCgroupPtr cgroup_vcpu = NULL; + bool cgroup_available = false; qemuDomainObjEnterMonitor(driver, vm); @@ -3656,11 +3657,13 @@ static int qemudDomainHotplugVcpus(struct qemud_driver *driver, goto cleanup; } - if (virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0) == 0) { - int rv = -1; + cgroup_available = (virCgroupForDomain(driver->cgroup, vm->def->name, + &cgroup, 0) == 0); - if (nvcpus > oldvcpus) { - for (i = oldvcpus; i < nvcpus; i++) { + if (nvcpus > oldvcpus) { + for (i = oldvcpus; i < nvcpus; i++) { + if (cgroup_available) { + int rv = -1; /* Create cgroup for the onlined vcpu */ rv = virCgroupForVcpu(cgroup, i, &cgroup_vcpu, 1); if (rv < 0) { @@ -3680,11 +3683,62 @@ static int qemudDomainHotplugVcpus(struct qemud_driver *driver, virCgroupRemove(cgroup_vcpu); goto cleanup; } + } - virCgroupFree(&cgroup_vcpu); + /* Inherit def->cpuset */ + if (vm->def->cpumask) { + /* vm->def->cputune.vcpupin can't be NULL if + * vm->def->cpumask is not NULL. + */ + virDomainVcpuPinDefPtr vcpupin = NULL; + + if (VIR_REALLOC_N(vm->def->cputune.vcpupin, + vm->def->cputune.nvcpupin + 1) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (VIR_ALLOC(vcpupin) < 0) { + virReportOOMError(); + goto cleanup; + } + + vcpupin->cpumask = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN); + virBitmapCopy(vcpupin->cpumask, vm->def->cpumask); + vcpupin->vcpuid = i; + vm->def->cputune.vcpupin[vm->def->cputune.nvcpupin++] = vcpupin; + + if (cgroup_available) { + if (qemuSetupCgroupVcpuPin(cgroup_vcpu, + vm->def->cputune.vcpupin, + vm->def->cputune.nvcpupin, i) < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("failed to set cpuset.cpus in cgroup" + " for vcpu %d"), i); + ret = -1; + goto cleanup; + } + } else { + if (virProcessInfoSetAffinity(cpupids[i], + vcpupin->cpumask) < 0) { + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to set cpu affinity for vcpu %d"), + i); + ret = -1; + goto cleanup; + } + } } - } else { - for (i = oldvcpus - 1; i >= nvcpus; i--) { + + virCgroupFree(&cgroup_vcpu); + } + } else { + for (i = oldvcpus - 1; i >= nvcpus; i--) { + virDomainVcpuPinDefPtr vcpupin = NULL; + + if (cgroup_available) { + int rv = -1; + rv = virCgroupForVcpu(cgroup, i, &cgroup_vcpu, 0); if (rv < 0) { virReportSystemError(-rv, @@ -3698,9 +3752,12 @@ static int qemudDomainHotplugVcpus(struct qemud_driver *driver, virCgroupRemove(cgroup_vcpu); virCgroupFree(&cgroup_vcpu); } - } - virCgroupFree(&cgroup); + /* Free vcpupin setting */ + if ((vcpupin = virDomainLookupVcpuPin(vm->def, i))) { + VIR_FREE(vcpupin); + } + } } priv->nvcpupids = ncpupids; -- 1.7.7.6