[PATCH 0/4] Couple of virnuma cleanups
I'm looking into NUMA code in capabilities and came up with a couple of cleanups. Technically, 3/4 is not needed yet, but I'll be introducing new data to vircaps2xmltest where a NUMA node has no CPUs and that's why the patch is needed. Michal Prívozník (4): numa_conf: Use virXMLFormatElement() in virDomainNumaDefFormatXML virnuma: Export virNumaGetMaxCPUs properly virnumamock: Allow CPU-less NUMA nodes virCapabilitiesHostNUMAFormat: Bring variables into loops src/conf/capabilities.c | 9 ++-- src/conf/numa_conf.c | 107 ++++++++++++++++++--------------------- src/libvirt_private.syms | 1 + src/util/virnuma.h | 2 +- tests/virnumamock.c | 7 ++- 5 files changed, 61 insertions(+), 65 deletions(-) -- 2.26.3
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/numa_conf.c | 107 +++++++++++++++++++------------------------ 1 file changed, 48 insertions(+), 59 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 525bc28962..537e515b5a 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -1105,92 +1105,81 @@ virDomainNumaDefFormatXML(virBuffer *buf, virBufferAdjustIndent(buf, 2); for (i = 0; i < ncells; i++) { virBitmap *cpumask = virDomainNumaGetNodeCpumask(def, i); - int ndistances; - size_t ncaches; + g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); + size_t j; memAccess = virDomainNumaGetNodeMemoryAccessMode(def, i); discard = virDomainNumaGetNodeDiscard(def, i); - virBufferAddLit(buf, "<cell"); - virBufferAsprintf(buf, " id='%zu'", i); + virBufferAsprintf(&attrBuf, " id='%zu'", i); if (cpumask) { g_autofree char *cpustr = virBitmapFormat(cpumask); if (!cpustr) return -1; - virBufferAsprintf(buf, " cpus='%s'", cpustr); + virBufferAsprintf(&attrBuf, " cpus='%s'", cpustr); } - virBufferAsprintf(buf, " memory='%llu'", + virBufferAsprintf(&attrBuf, " memory='%llu'", virDomainNumaGetNodeMemorySize(def, i)); - virBufferAddLit(buf, " unit='KiB'"); + virBufferAddLit(&attrBuf, " unit='KiB'"); if (memAccess) - virBufferAsprintf(buf, " memAccess='%s'", + virBufferAsprintf(&attrBuf, " memAccess='%s'", virDomainMemoryAccessTypeToString(memAccess)); if (discard) - virBufferAsprintf(buf, " discard='%s'", + virBufferAsprintf(&attrBuf, " discard='%s'", virTristateBoolTypeToString(discard)); - ndistances = def->mem_nodes[i].ndistances; - ncaches = def->mem_nodes[i].ncaches; - if (ndistances == 0 && ncaches == 0) { - virBufferAddLit(buf, "/>\n"); - } else { - size_t j; - - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); - - if (ndistances) { - virDomainNumaDistance *distances = def->mem_nodes[i].distances; - - virBufferAddLit(buf, "<distances>\n"); - virBufferAdjustIndent(buf, 2); - for (j = 0; j < ndistances; j++) { - if (distances[j].value) { - virBufferAddLit(buf, "<sibling"); - virBufferAsprintf(buf, " id='%d'", distances[j].cellid); - virBufferAsprintf(buf, " value='%d'", distances[j].value); - virBufferAddLit(buf, "/>\n"); - } + if (def->mem_nodes[i].ndistances) { + virDomainNumaDistance *distances = def->mem_nodes[i].distances; + + virBufferAddLit(&childBuf, "<distances>\n"); + virBufferAdjustIndent(&childBuf, 2); + for (j = 0; j < def->mem_nodes[i].ndistances; j++) { + if (distances[j].value) { + virBufferAddLit(&childBuf, "<sibling"); + virBufferAsprintf(&childBuf, " id='%d'", distances[j].cellid); + virBufferAsprintf(&childBuf, " value='%d'", distances[j].value); + virBufferAddLit(&childBuf, "/>\n"); } - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</distances>\n"); } + virBufferAdjustIndent(&childBuf, -2); + virBufferAddLit(&childBuf, "</distances>\n"); + } - for (j = 0; j < ncaches; j++) { - virDomainNumaCache *cache = &def->mem_nodes[i].caches[j]; - - virBufferAsprintf(buf, "<cache level='%u'", cache->level); - if (cache->associativity) { - virBufferAsprintf(buf, " associativity='%s'", - virDomainCacheAssociativityTypeToString(cache->associativity)); - } + for (j = 0; j < def->mem_nodes[i].ncaches; j++) { + virDomainNumaCache *cache = &def->mem_nodes[i].caches[j]; - if (cache->policy) { - virBufferAsprintf(buf, " policy='%s'", - virDomainCachePolicyTypeToString(cache->policy)); - } - virBufferAddLit(buf, ">\n"); + virBufferAsprintf(&childBuf, "<cache level='%u'", cache->level); + if (cache->associativity) { + virBufferAsprintf(&childBuf, " associativity='%s'", + virDomainCacheAssociativityTypeToString(cache->associativity)); + } - virBufferAdjustIndent(buf, 2); - virBufferAsprintf(buf, - "<size value='%u' unit='KiB'/>\n", - cache->size); + if (cache->policy) { + virBufferAsprintf(&childBuf, " policy='%s'", + virDomainCachePolicyTypeToString(cache->policy)); + } + virBufferAddLit(&childBuf, ">\n"); - if (cache->line) { - virBufferAsprintf(buf, - "<line value='%u' unit='B'/>\n", - cache->line); - } + virBufferAdjustIndent(&childBuf, 2); + virBufferAsprintf(&childBuf, + "<size value='%u' unit='KiB'/>\n", + cache->size); - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</cache>\n"); + if (cache->line) { + virBufferAsprintf(&childBuf, + "<line value='%u' unit='B'/>\n", + cache->line); } - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</cell>\n"); + + virBufferAdjustIndent(&childBuf, -2); + virBufferAddLit(&childBuf, "</cache>\n"); } + + virXMLFormatElement(buf, "cell", &attrBuf, &childBuf); } if (def->ninterconnects) { -- 2.26.3
This function will be used in virnumamock, shortly. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virnuma.h | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1df4b8cfe8..b7bbe46dc7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2872,6 +2872,7 @@ virNodeSuspendGetTargetMask; virNumaGetAutoPlacementAdvice; virNumaGetDistances; virNumaGetHostMemoryNodeset; +virNumaGetMaxCPUs; virNumaGetMaxNode; virNumaGetNodeCPUs; virNumaGetNodeMemory; diff --git a/src/util/virnuma.h b/src/util/virnuma.h index 45342ecf11..bc79bff891 100644 --- a/src/util/virnuma.h +++ b/src/util/virnuma.h @@ -43,7 +43,7 @@ int virNumaGetNodeMemory(int node, unsigned long long *memsize, unsigned long long *memfree) G_GNUC_NO_INLINE; -unsigned int virNumaGetMaxCPUs(void); +unsigned int virNumaGetMaxCPUs(void) G_GNUC_NO_INLINE; int virNumaGetNodeCPUs(int node, virBitmap **cpus) G_GNUC_NO_INLINE; int virNumaNodesetToCPUset(virBitmap *nodeset, -- 2.26.3
The original virNumaGetNodeCPUs() returns an empty virBitmap if given NUMA node has no CPUs. But that's not how our mock behaves - it looks under $fakesysfs/node/node$N/cpulist only to find an empty file which is then passed to virBitmapParseUnlimited() which threats such input as error. Fortunately, we don't have any fake sysfs data where this path is hit, but we might soon. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virnumamock.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/virnumamock.c b/tests/virnumamock.c index 3a203ded77..ff9c6e951d 100644 --- a/tests/virnumamock.c +++ b/tests/virnumamock.c @@ -172,7 +172,12 @@ virNumaGetNodeCPUs(int node, virBitmap **cpus) SYSFS_SYSTEM_PATH, node) < 0) return -1; - *cpus = virBitmapParseUnlimited(cpulist); + if (STREQ(cpulist, "")) { + unsigned int max_n_cpus = virNumaGetMaxCPUs(); + *cpus = virBitmapNew(max_n_cpus); + } else { + *cpus = virBitmapParseUnlimited(cpulist); + } if (!*cpus) goto cleanup; -- 2.26.3
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/capabilities.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 1dae6d38cc..915cd3149e 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -807,8 +807,6 @@ virCapabilitiesHostNUMAFormat(virBuffer *buf, virCapsHostNUMA *caps) { size_t i; - size_t j; - char *siblings; if (!caps) return 0; @@ -819,6 +817,8 @@ virCapabilitiesHostNUMAFormat(virBuffer *buf, virBufferAdjustIndent(buf, 2); for (i = 0; i < caps->cells->len; i++) { virCapsHostNUMACell *cell = g_ptr_array_index(caps->cells, i); + size_t j; + virBufferAsprintf(buf, "<cell id='%d'>\n", cell->num); virBufferAdjustIndent(buf, 2); @@ -847,10 +847,12 @@ virCapabilitiesHostNUMAFormat(virBuffer *buf, virBufferAsprintf(buf, "<cpus num='%d'>\n", cell->ncpus); virBufferAdjustIndent(buf, 2); - for (j = 0; j < cell->ncpus; j++) { + for (j = 0; j < cell->ncpus; ++j) { virBufferAsprintf(buf, "<cpu id='%d'", cell->cpus[j].id); if (cell->cpus[j].siblings) { + g_autofree char * siblings = NULL; + if (!(siblings = virBitmapFormat(cell->cpus[j].siblings))) return -1; @@ -860,7 +862,6 @@ virCapabilitiesHostNUMAFormat(virBuffer *buf, cell->cpus[j].die_id, cell->cpus[j].core_id, siblings); - VIR_FREE(siblings); } virBufferAddLit(buf, "/>\n"); } -- 2.26.3
On a Monday in 2021, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/capabilities.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 1dae6d38cc..915cd3149e 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -807,8 +807,6 @@ virCapabilitiesHostNUMAFormat(virBuffer *buf, virCapsHostNUMA *caps) { size_t i; - size_t j; - char *siblings;
if (!caps) return 0; @@ -819,6 +817,8 @@ virCapabilitiesHostNUMAFormat(virBuffer *buf, virBufferAdjustIndent(buf, 2); for (i = 0; i < caps->cells->len; i++) { virCapsHostNUMACell *cell = g_ptr_array_index(caps->cells, i); + size_t j; + virBufferAsprintf(buf, "<cell id='%d'>\n", cell->num); virBufferAdjustIndent(buf, 2);
@@ -847,10 +847,12 @@ virCapabilitiesHostNUMAFormat(virBuffer *buf,
virBufferAsprintf(buf, "<cpus num='%d'>\n", cell->ncpus); virBufferAdjustIndent(buf, 2); - for (j = 0; j < cell->ncpus; j++) { + for (j = 0; j < cell->ncpus; ++j) {
No need to change the post-increment to pre-increment. Jano
virBufferAsprintf(buf, "<cpu id='%d'", cell->cpus[j].id);
if (cell->cpus[j].siblings) { + g_autofree char * siblings = NULL; + if (!(siblings = virBitmapFormat(cell->cpus[j].siblings))) return -1;
On 5/17/21 3:46 PM, Ján Tomko wrote:
On a Monday in 2021, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/capabilities.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 1dae6d38cc..915cd3149e 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -807,8 +807,6 @@ virCapabilitiesHostNUMAFormat(virBuffer *buf, virCapsHostNUMA *caps) { size_t i; - size_t j; - char *siblings;
if (!caps) return 0; @@ -819,6 +817,8 @@ virCapabilitiesHostNUMAFormat(virBuffer *buf, virBufferAdjustIndent(buf, 2); for (i = 0; i < caps->cells->len; i++) { virCapsHostNUMACell *cell = g_ptr_array_index(caps->cells, i); + size_t j; + virBufferAsprintf(buf, "<cell id='%d'>\n", cell->num); virBufferAdjustIndent(buf, 2);
@@ -847,10 +847,12 @@ virCapabilitiesHostNUMAFormat(virBuffer *buf,
virBufferAsprintf(buf, "<cpus num='%d'>\n", cell->ncpus); virBufferAdjustIndent(buf, 2); - for (j = 0; j < cell->ncpus; j++) { + for (j = 0; j < cell->ncpus; ++j) {
No need to change the post-increment to pre-increment.
Huh, I have no idea how that slipped in there :-) Michal
On a Monday in 2021, Michal Privoznik wrote:
I'm looking into NUMA code in capabilities and came up with a couple of cleanups. Technically, 3/4 is not needed yet, but I'll be introducing new data to vircaps2xmltest where a NUMA node has no CPUs and that's why the patch is needed.
Michal Prívozník (4): numa_conf: Use virXMLFormatElement() in virDomainNumaDefFormatXML virnuma: Export virNumaGetMaxCPUs properly virnumamock: Allow CPU-less NUMA nodes virCapabilitiesHostNUMAFormat: Bring variables into loops
src/conf/capabilities.c | 9 ++-- src/conf/numa_conf.c | 107 ++++++++++++++++++--------------------- src/libvirt_private.syms | 1 + src/util/virnuma.h | 2 +- tests/virnumamock.c | 7 ++- 5 files changed, 61 insertions(+), 65 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Ján Tomko -
Michal Privoznik -
Michal Prívozník