[libvirt] [PATCH 0/5] Fix vcpu pinning info propagation on vcpu cold-unplug
Remove the vcpupin info on cold cpuunplug. Also refactor a few unpleasant-to-look-at functions found while looking for the problem. Peter Krempa (5): qemu: process: Remove unnecessary argument and rename function conf: cpupin: Remove useless checking of vcpupin element count conf: Refactor virDomainVcpuPinDefParseXML qemu: cpu: unplug: Remove vcpu pinning on cold cpu unplug qemu: process: Pin on per-vcpu basis instead of per-vcpupin element src/conf/domain_conf.c | 64 +++++++++++++++++++++---------------------------- src/qemu/qemu_driver.c | 7 ++++++ src/qemu/qemu_process.c | 20 +++++++++------- 3 files changed, 46 insertions(+), 45 deletions(-) -- 2.0.2
We set just one affinity of the emulator and the virConnectPtr isn't needed for that function. --- src/qemu/qemu_process.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 52d9052..fe8fd2a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2258,8 +2258,7 @@ qemuProcessSetVcpuAffinities(virDomainObjPtr vm) /* Set CPU affinities for emulator threads. */ static int -qemuProcessSetEmulatorAffinities(virConnectPtr conn ATTRIBUTE_UNUSED, - virDomainObjPtr vm) +qemuProcessSetEmulatorAffinity(virDomainObjPtr vm) { virBitmapPtr cpumask; virDomainDefPtr def = vm->def; @@ -4263,7 +4262,7 @@ int qemuProcessStart(virConnectPtr conn, goto cleanup; VIR_DEBUG("Setting affinity of emulator threads"); - if (qemuProcessSetEmulatorAffinities(conn, vm) < 0) + if (qemuProcessSetEmulatorAffinity(vm) < 0) goto cleanup; VIR_DEBUG("Setting any required VM passwords"); -- 2.0.2
The check doesn't make much sense as right below it the entries are either checked for duplicity or ignored in some cases. Having this check doesn't actually forbid passing invalid values. --- src/conf/domain_conf.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5c762fa..82f3ee6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11993,12 +11993,6 @@ virDomainDefParseXML(xmlDocPtr xml, if (n && VIR_ALLOC_N(def->cputune.vcpupin, n) < 0) goto error; - if (n > def->maxvcpus) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("vcpupin nodes must be less than maxvcpus")); - goto error; - } - for (i = 0; i < n; i++) { virDomainVcpuPinDefPtr vcpupin = NULL; vcpupin = virDomainVcpuPinDefParseXML(nodes[i], ctxt, def->maxvcpus, 0); -- 2.0.2
Tidy up control flow, change boolean argument to use 'bool', improve error message in case the function is used to parse emulator pinning info and avoid a few temp variables that made no sense. Also when the function is called to parse emulator pinning info, there's no need to check the processor ID in that case. --- src/conf/domain_conf.c | 58 +++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 31 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 82f3ee6..25d33c7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11308,11 +11308,9 @@ virDomainPanicDefParseXML(xmlNodePtr node) /* Parse the XML definition for a vcpupin or emulatorpin. * * vcpupin has the form of - * * <vcpupin vcpu='0' cpuset='0'/> * * and emulatorpin has the form of - * * <emulatorpin cpuset='0'/> * * A vcpuid of -1 is valid and only valid for emulatorpin. So callers @@ -11322,7 +11320,7 @@ static virDomainVcpuPinDefPtr virDomainVcpuPinDefParseXML(xmlNodePtr node, xmlXPathContextPtr ctxt, int maxvcpus, - int emulator) + bool emulator) { virDomainVcpuPinDefPtr def; xmlNodePtr oldnode = ctxt->node; @@ -11335,46 +11333,43 @@ virDomainVcpuPinDefParseXML(xmlNodePtr node, ctxt->node = node; - if (emulator == 0) { + if (!emulator) { ret = virXPathInt("string(./@vcpu)", ctxt, &vcpuid); if ((ret == -2) || (vcpuid < -1)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("vcpu id must be an unsigned integer or -1")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("vcpu id must be an unsigned integer or -1")); goto error; } else if (vcpuid == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("vcpu id value -1 is not allowed for vcpupin")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("vcpu id value -1 is not allowed for vcpupin")); goto error; } - } - if (vcpuid >= maxvcpus) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("vcpu id must be less than maxvcpus")); - goto error; - } - - def->vcpuid = vcpuid; + if (vcpuid >= maxvcpus) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("vcpu id must be less than maxvcpus")); + goto error; + } - tmp = virXMLPropString(node, "cpuset"); + def->vcpuid = vcpuid; + } - if (tmp) { - char *set = tmp; - int cpumasklen = VIR_DOMAIN_CPUMASK_LEN; + if (!(tmp = virXMLPropString(node, "cpuset"))) { + if (emulator) + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing cpuset for emulatorpin")); + else + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing cpuset for vcpupin")); - if (virBitmapParse(set, 0, &def->cpumask, - cpumasklen) < 0) { - VIR_FREE(tmp); - goto error; - } - VIR_FREE(tmp); - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("missing cpuset for vcpupin")); goto error; } + if (virBitmapParse(tmp, 0, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) + goto error; + cleanup: + VIR_FREE(tmp); ctxt->node = oldnode; return def; @@ -11995,7 +11990,8 @@ virDomainDefParseXML(xmlDocPtr xml, for (i = 0; i < n; i++) { virDomainVcpuPinDefPtr vcpupin = NULL; - vcpupin = virDomainVcpuPinDefParseXML(nodes[i], ctxt, def->maxvcpus, 0); + vcpupin = virDomainVcpuPinDefParseXML(nodes[i], ctxt, + def->maxvcpus, false); if (!vcpupin) goto error; @@ -12069,7 +12065,7 @@ virDomainDefParseXML(xmlDocPtr xml, } def->cputune.emulatorpin = virDomainVcpuPinDefParseXML(nodes[0], ctxt, - def->maxvcpus, 1); + 0, true); if (!def->cputune.emulatorpin) goto error; -- 2.0.2
Remove the pinning info when removing to CPU, otherwise when the VM will be started our code will try to pin non-existing vcpus as the definition wasn't updated. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1129372 --- src/qemu/qemu_driver.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ac0717c..bff8f77 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4277,6 +4277,7 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, qemuAgentCPUInfoPtr cpuinfo = NULL; int ncpuinfo; qemuDomainObjPrivatePtr priv; + size_t i; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | @@ -4386,6 +4387,12 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + /* remove vcpupin entries for vcpus that were unplugged */ + if (nvcpus < persistentDef->vcpus) { + for (i = persistentDef->vcpus; i >= nvcpus; i--) + virDomainVcpuPinDel(persistentDef, i); + } + if (maximum) { persistentDef->maxvcpus = nvcpus; if (nvcpus < persistentDef->vcpus) -- 2.0.2
Pin existing vcpus rather than existing vcpu pinning infos. This increases the complexity of the lookup, but avoids pinning cpus that are not enabled actually. --- src/qemu/qemu_process.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index fe8fd2a..cfd7ee1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2230,7 +2230,8 @@ qemuProcessSetVcpuAffinities(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr def = vm->def; - int vcpu, n; + virDomainVcpuPinDefPtr pininfo; + int n; int ret = -1; if (!def->cputune.nvcpupin) @@ -2242,11 +2243,15 @@ qemuProcessSetVcpuAffinities(virDomainObjPtr vm) return -1; } - for (n = 0; n < def->cputune.nvcpupin; n++) { - vcpu = def->cputune.vcpupin[n]->vcpuid; + for (n = 0; n < def->vcpus; n++) { + /* set affinity only for existing vcpus */ + if (!(pininfo = virDomainVcpuPinFindByVcpu(def->cputune.vcpupin, + def->cputune.nvcpupin, + n))) + continue; - if (virProcessSetAffinity(priv->vcpupids[vcpu], - def->cputune.vcpupin[n]->cpumask) < 0) { + if (virProcessSetAffinity(priv->vcpupids[n], + pininfo->cpumask) < 0) { goto cleanup; } } -- 2.0.2
On 08/15/2014 04:49 PM, Peter Krempa wrote:
Remove the vcpupin info on cold cpuunplug. Also refactor a few unpleasant-to-look-at functions found while looking for the problem.
Peter Krempa (5): qemu: process: Remove unnecessary argument and rename function conf: cpupin: Remove useless checking of vcpupin element count conf: Refactor virDomainVcpuPinDefParseXML qemu: cpu: unplug: Remove vcpu pinning on cold cpu unplug qemu: process: Pin on per-vcpu basis instead of per-vcpupin element
src/conf/domain_conf.c | 64 +++++++++++++++++++++---------------------------- src/qemu/qemu_driver.c | 7 ++++++ src/qemu/qemu_process.c | 20 +++++++++------- 3 files changed, 46 insertions(+), 45 deletions(-)
ACK series Jan
On 08/18/14 17:39, Ján Tomko wrote:
On 08/15/2014 04:49 PM, Peter Krempa wrote:
Remove the vcpupin info on cold cpuunplug. Also refactor a few unpleasant-to-look-at functions found while looking for the problem.
Peter Krempa (5): qemu: process: Remove unnecessary argument and rename function conf: cpupin: Remove useless checking of vcpupin element count conf: Refactor virDomainVcpuPinDefParseXML qemu: cpu: unplug: Remove vcpu pinning on cold cpu unplug qemu: process: Pin on per-vcpu basis instead of per-vcpupin element
src/conf/domain_conf.c | 64 +++++++++++++++++++++---------------------------- src/qemu/qemu_driver.c | 7 ++++++ src/qemu/qemu_process.c | 20 +++++++++------- 3 files changed, 46 insertions(+), 45 deletions(-)
ACK series
Jan
Pushed; Thanks. Peter
participants (2)
-
Ján Tomko -
Peter Krempa