Since virBitmapSubtract is unused modify it to perform bitmap intersection. --- src/libvirt_private.syms | 2 +- src/util/virbitmap.c | 14 +++++++------- src/util/virbitmap.h | 2 +- tests/virbitmaptest.c | 14 +++++++------- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3f94521df..4ad1f7a60 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1295,6 +1295,7 @@ virBitmapEqual; virBitmapFormat; virBitmapFree; virBitmapGetBit; +virBitmapIntersect; virBitmapIsAllClear; virBitmapIsAllSet; virBitmapIsBitSet; @@ -1315,7 +1316,6 @@ virBitmapSetBit; virBitmapSetBitExpand; virBitmapSize; virBitmapString; -virBitmapSubtract; virBitmapToData; virBitmapToDataBuf; diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index eac63d997..a5077fe7c 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -1091,15 +1091,15 @@ virBitmapOverlaps(virBitmapPtr b1, } /** - * virBitmapSubtract: - * @a: minuend/result - * @b: subtrahend + * virBitmapIntersect: + * @a: bitmap, modified to contain result + * @b: bitmap * - * Performs bitwise subtraction: a = a - b + * Performs intersection of two bitmaps: a = intersect(a, b) */ void -virBitmapSubtract(virBitmapPtr a, - virBitmapPtr b) +virBitmapIntersect(virBitmapPtr a, + virBitmapPtr b) { size_t i; size_t max = a->map_len; @@ -1108,5 +1108,5 @@ virBitmapSubtract(virBitmapPtr a, max = b->map_len; for (i = 0; i < max; i++) - a->map[i] &= ~b->map[i]; + a->map[i] &= b->map[i]; } diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index 36282af1c..ffa3c42d7 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -143,7 +143,7 @@ bool virBitmapOverlaps(virBitmapPtr b1, virBitmapPtr b2) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -void virBitmapSubtract(virBitmapPtr a, virBitmapPtr b) +void virBitmapIntersect(virBitmapPtr a, virBitmapPtr b) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); #endif diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c index e5305d022..882c71544 100644 --- a/tests/virbitmaptest.c +++ b/tests/virbitmaptest.c @@ -589,7 +589,7 @@ test11(const void *opaque) virBitmapParse(data->res, &resmap, 256) < 0) goto cleanup; - virBitmapSubtract(amap, bmap); + virBitmapIntersect(amap, bmap); if (!virBitmapEqual(amap, resmap)) { fprintf(stderr, "\n bitmap subtraction failed: '%s'-'%s'!='%s'\n", @@ -700,13 +700,13 @@ mymain(void) ret = -1; virTestCounterReset("test11-"); - TESTBINARYOP("0", "0", "0,^0", test11); - TESTBINARYOP("0-3", "0", "1-3", test11); - TESTBINARYOP("0-3", "0,3", "1-2", test11); + TESTBINARYOP("0", "0", "0", test11); + TESTBINARYOP("0-3", "0", "0", test11); + TESTBINARYOP("0-3", "0,3", "0,3", test11); TESTBINARYOP("0,^0", "0", "0,^0", test11); - TESTBINARYOP("0-3", "0-3", "0,^0", test11); - TESTBINARYOP("0-3", "0,^0", "0-3", test11); - TESTBINARYOP("0,2", "1,3", "0,2", test11); + TESTBINARYOP("0-3", "0-3", "0-3", test11); + TESTBINARYOP("0-3", "0,^0", "0,^0", test11); + TESTBINARYOP("0,2", "1,3", "0,^0", test11); if (virTestRun("test12", test12, NULL) < 0) ret = -1; -- 2.12.2

Move the code to separate functions to avoid complicating the existing ones with changes. --- src/qemu/qemu_domain.c | 84 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 60 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 6772f1ae2..92e5609ad 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1761,6 +1761,29 @@ qemuDomainObjPrivateXMLFormatVcpus(virBufferPtr buf, static int +qemuDomainObjPtrivateXMLFormatAutomaticPlacement(virBufferPtr buf, + qemuDomainObjPrivatePtr priv) +{ + char *nodeset = NULL; + int ret = -1; + + if (!priv->autoNodeset) + return 0; + + if (!(nodeset = virBitmapFormat(priv->autoNodeset))) + goto cleanup; + + virBufferAsprintf(buf, "<numad nodeset='%s'/>\n", nodeset); + + ret = 0; + + cleanup: + VIR_FREE(nodeset); + return ret; +} + + +static int qemuDomainObjPrivateXMLFormat(virBufferPtr buf, virDomainObjPtr vm) { @@ -1869,15 +1892,8 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, virBufferAddLit(buf, "</devices>\n"); } - if (priv->autoNodeset) { - char *nodeset = virBitmapFormat(priv->autoNodeset); - - if (!nodeset) - return -1; - - virBufferAsprintf(buf, "<numad nodeset='%s'/>\n", nodeset); - VIR_FREE(nodeset); - } + if (qemuDomainObjPtrivateXMLFormatAutomaticPlacement(buf, priv) < 0) + return -1; /* Various per-domain paths */ virBufferEscapeString(buf, "<libDir path='%s'/>\n", priv->libDir); @@ -1936,6 +1952,40 @@ qemuDomainObjPrivateXMLParseVcpu(xmlNodePtr node, static int +qemuDomainObjPrivateXMLParseAutomaticPlacement(virQEMUDriverPtr driver, + qemuDomainObjPrivatePtr priv, + xmlXPathContextPtr ctxt) +{ + virCapsPtr caps = NULL; + char *nodeset; + int ret = -1; + + nodeset = virXPathString("string(./numad/@nodeset)", ctxt); + + if (!nodeset) + return 0; + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + if (virBitmapParse(nodeset, &priv->autoNodeset, caps->host.nnumaCell_max) < 0) + goto cleanup; + + if (!(priv->autoCpuset = virCapabilitiesGetCpusForNodemask(caps, + priv->autoNodeset))) + goto cleanup; + + ret = 0; + + cleanup: + virObjectUnref(caps); + VIR_FREE(nodeset); + + return ret; +} + + +static int qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, virDomainObjPtr vm, virDomainDefParserConfigPtr config) @@ -1949,7 +1999,6 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, xmlNodePtr *nodes = NULL; xmlNodePtr node = NULL; virQEMUCapsPtr qemuCaps = NULL; - virCapsPtr caps = NULL; if (VIR_ALLOC(priv->monConfig) < 0) goto error; @@ -2133,21 +2182,9 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, } VIR_FREE(nodes); - if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + if (qemuDomainObjPrivateXMLParseAutomaticPlacement(driver, priv, ctxt) < 0) goto error; - if ((tmp = virXPathString("string(./numad/@nodeset)", ctxt))) { - if (virBitmapParse(tmp, &priv->autoNodeset, - caps->host.nnumaCell_max) < 0) - goto error; - - if (!(priv->autoCpuset = virCapabilitiesGetCpusForNodemask(caps, - priv->autoNodeset))) - goto error; - } - virObjectUnref(caps); - VIR_FREE(tmp); - if ((tmp = virXPathString("string(./libDir/@path)", ctxt))) priv->libDir = tmp; if ((tmp = virXPathString("string(./channelTargetDir/@path)", ctxt))) @@ -2175,7 +2212,6 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, virStringListFree(priv->qemuDevices); priv->qemuDevices = NULL; virObjectUnref(qemuCaps); - virObjectUnref(caps); return -1; } -- 2.12.2

Decouple them by storing them in the XML separately rather than regenerating them. This will simplify upcoming fixes. --- src/qemu/qemu_domain.c | 32 +++++++++++++++++++++++++------- tests/qemuxml2xmltest.c | 2 +- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 92e5609ad..8c29db15f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1765,20 +1765,30 @@ qemuDomainObjPtrivateXMLFormatAutomaticPlacement(virBufferPtr buf, qemuDomainObjPrivatePtr priv) { char *nodeset = NULL; + char *cpuset = NULL; int ret = -1; - if (!priv->autoNodeset) + if (!priv->autoNodeset && !priv->autoCpuset) return 0; - if (!(nodeset = virBitmapFormat(priv->autoNodeset))) + if (priv->autoNodeset && + !((nodeset = virBitmapFormat(priv->autoNodeset)))) goto cleanup; - virBufferAsprintf(buf, "<numad nodeset='%s'/>\n", nodeset); + if (priv->autoCpuset && + !((cpuset = virBitmapFormat(priv->autoCpuset)))) + goto cleanup; + + virBufferAddLit(buf, "<numad"); + virBufferEscapeString(buf, " nodeset='%s'", nodeset); + virBufferEscapeString(buf, " cpuset='%s'", cpuset); + virBufferAddLit(buf, "/>\n"); ret = 0; cleanup: VIR_FREE(nodeset); + VIR_FREE(cpuset); return ret; } @@ -1958,11 +1968,13 @@ qemuDomainObjPrivateXMLParseAutomaticPlacement(virQEMUDriverPtr driver, { virCapsPtr caps = NULL; char *nodeset; + char *cpuset; int ret = -1; nodeset = virXPathString("string(./numad/@nodeset)", ctxt); + cpuset = virXPathString("string(./numad/@cpuset)", ctxt); - if (!nodeset) + if (!nodeset && !cpuset) return 0; if (!(caps = virQEMUDriverGetCapabilities(driver, false))) @@ -1971,15 +1983,21 @@ qemuDomainObjPrivateXMLParseAutomaticPlacement(virQEMUDriverPtr driver, if (virBitmapParse(nodeset, &priv->autoNodeset, caps->host.nnumaCell_max) < 0) goto cleanup; - if (!(priv->autoCpuset = virCapabilitiesGetCpusForNodemask(caps, - priv->autoNodeset))) - goto cleanup; + if (cpuset) { + if (virBitmapParse(cpuset, &priv->autoCpuset, VIR_DOMAIN_CPUMASK_LEN) < 0) + goto cleanup; + } else { + if (!(priv->autoCpuset = virCapabilitiesGetCpusForNodemask(caps, + priv->autoNodeset))) + goto cleanup; + } ret = 0; cleanup: virObjectUnref(caps); VIR_FREE(nodeset); + VIR_FREE(cpuset); return ret; } diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 79347671f..24d1eb33c 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -95,7 +95,7 @@ static const char testStatusXMLPrefixFooter[] = " <device alias='net0'/>\n" " <device alias='usb'/>\n" " </devices>\n" -" <numad nodeset='0-2'/>\n" +" <numad nodeset='0-2' cpuset='1,3'/>\n" " <libDir path='/tmp'/>\n" " <channelTargetDir path='/tmp/channel'/>\n"; -- 2.12.2

On Thu, Jul 20, 2017 at 14:12:44 +0200, Andrea Bolognani wrote: If you call virBitmapFormat on an empty or NULL bitmap you still get a (empty) string allocated so that condition is basically identical to the one that's already there due to how the bitmaps are formatted: if (!priv->autoNodeset && !priv->autoCpuset) return 0; if (priv->autoNodeset && !((nodeset = virBitmapFormat(priv->autoNodeset)))) goto cleanup; if (priv->autoCpuset && !((cpuset = virBitmapFormat(priv->autoCpuset)))) goto cleanup; That shouldn't ever happen (tm) :D I'll can add a condition that if nodeset is not in the XML the parsing will be skipped. So in that case only cpuset can be present (for future use). I'll also add a note that accessing autoNodeset in the else branch of if (cpuset) is safe.

Remove the code from qemuProcessPrepareDomain so that it won't get even more bloated. --- src/qemu/qemu_process.c | 61 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 40 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e6522a294..01fe33c92 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5410,6 +5410,44 @@ qemuProcessUpdateGuestCPU(virDomainDefPtr def, } +static int +qemuProcessPrepareDomainNUMAPlacement(virDomainObjPtr vm, + virCapsPtr caps) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + char *nodesetstr = NULL; + int ret = -1; + + /* Get the advisory nodeset from numad if 'placement' of + * either <vcpu> or <numatune> is 'auto'. + */ + if (!virDomainDefNeedsPlacementAdvice(vm->def)) + return 0; + + nodesetstr = virNumaGetAutoPlacementAdvice(virDomainDefGetVcpus(vm->def), + virDomainDefGetMemoryTotal(vm->def)); + + if (!nodesetstr) + goto cleanup; + + VIR_DEBUG("Nodeset returned from numad: %s", nodesetstr); + + if (virBitmapParse(nodesetstr, &priv->autoNodeset, + VIR_DOMAIN_CPUMASK_LEN) < 0) + goto cleanup; + + if (!(priv->autoCpuset = virCapabilitiesGetCpusForNodemask(caps, + priv->autoNodeset))) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(nodesetstr); + return ret; +} + + /** * qemuProcessPrepareDomain * @@ -5430,7 +5468,6 @@ qemuProcessPrepareDomain(virConnectPtr conn, { int ret = -1; size_t i; - char *nodeset = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virCapsPtr caps; @@ -5448,25 +5485,8 @@ qemuProcessPrepareDomain(virConnectPtr conn, } virDomainAuditSecurityLabel(vm, true); - /* Get the advisory nodeset from numad if 'placement' of - * either <vcpu> or <numatune> is 'auto'. - */ - if (virDomainDefNeedsPlacementAdvice(vm->def)) { - nodeset = virNumaGetAutoPlacementAdvice(virDomainDefGetVcpus(vm->def), - virDomainDefGetMemoryTotal(vm->def)); - if (!nodeset) - goto cleanup; - - VIR_DEBUG("Nodeset returned from numad: %s", nodeset); - - if (virBitmapParse(nodeset, &priv->autoNodeset, - VIR_DOMAIN_CPUMASK_LEN) < 0) - goto cleanup; - - if (!(priv->autoCpuset = virCapabilitiesGetCpusForNodemask(caps, - priv->autoNodeset))) - goto cleanup; - } + if (qemuProcessPrepareDomainNUMAPlacement(vm, caps) < 0) + goto cleanup; } /* Whether we should use virtlogd as stdio handler for character @@ -5537,7 +5557,6 @@ qemuProcessPrepareDomain(virConnectPtr conn, ret = 0; cleanup: - VIR_FREE(nodeset); virObjectUnref(caps); virObjectUnref(cfg); return ret; -- 2.12.2

'numad' may return a nodeset which contains NUMA nodes without memory for certain configurations. Since cgroups code will not be happy using nodes without memory we need to store only numa nodes with memory in autoNodeset. On the other hand autoCpuset should contain cpus also for nodes which do not have any memory. --- src/qemu/qemu_process.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 01fe33c92..78c5692a0 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5416,6 +5416,8 @@ qemuProcessPrepareDomainNUMAPlacement(virDomainObjPtr vm, { qemuDomainObjPrivatePtr priv = vm->privateData; char *nodesetstr = NULL; + virBitmapPtr numadNodeset = NULL; + virBitmapPtr hostMemoryNodeset = NULL; int ret = -1; /* Get the advisory nodeset from numad if 'placement' of @@ -5430,20 +5432,30 @@ qemuProcessPrepareDomainNUMAPlacement(virDomainObjPtr vm, if (!nodesetstr) goto cleanup; + if (!(hostMemoryNodeset = virNumaGetHostMemoryNodeset())) + goto cleanup; + VIR_DEBUG("Nodeset returned from numad: %s", nodesetstr); - if (virBitmapParse(nodesetstr, &priv->autoNodeset, - VIR_DOMAIN_CPUMASK_LEN) < 0) + if (virBitmapParse(nodesetstr, &numadNodeset, VIR_DOMAIN_CPUMASK_LEN) < 0) goto cleanup; - if (!(priv->autoCpuset = virCapabilitiesGetCpusForNodemask(caps, - priv->autoNodeset))) + /* numad may return a nodeset that only contains cpus but cgroups don't play + * well with that. Set the autoCpuset from all cpus from that nodeset, but + * assign autoNodeset only with nodes containing memory. */ + if (!(priv->autoCpuset = virCapabilitiesGetCpusForNodemask(caps, numadNodeset))) goto cleanup; + virBitmapIntersect(numadNodeset, hostMemoryNodeset); + + VIR_STEAL_PTR(priv->autoNodeset, numadNodeset); + ret = 0; cleanup: VIR_FREE(nodesetstr); + virBitmapFree(numadNodeset); + virBitmapFree(hostMemoryNodeset); return ret; } -- 2.12.2