[PATCH 0/4] conf: Deduplicate NUMA distance code
While the number of saved lines is not that great, it prepares the code for what I'm working on - exposing HMAT in capabilities XML. My idea is to reuse XML formatters there too so that mgmt app can just take whatever we produce in capabilities XML and paste it into domain XML. Michal Prívozník (4): capabilities: Rename siblings to distances numa_conf: Rename virDomainNumaDistance to virNumaDistance numa_conf: Expose virNumaDistance formatter conf: Deduplicate NUMA distance code src/conf/capabilities.c | 60 ++++++++++++++------------------ src/conf/capabilities.h | 13 +++---- src/conf/numa_conf.c | 63 ++++++++++++++++++---------------- src/conf/numa_conf.h | 10 ++++++ src/conf/virconftypes.h | 2 -- src/libvirt_private.syms | 1 + src/libxl/libxl_capabilities.c | 20 +++++------ 7 files changed, 83 insertions(+), 86 deletions(-) -- 2.26.3
The virCapsHostNUMACellSiblingInfo structure really represents distance to other NUMA node. Rename the structure and variables of that type to make it more obvious. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/capabilities.c | 52 +++++++++++++++++----------------- src/conf/capabilities.h | 10 +++---- src/conf/virconftypes.h | 2 +- src/libxl/libxl_capabilities.c | 20 ++++++------- 4 files changed, 42 insertions(+), 42 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 7fe282ad3a..926ecb5a24 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -119,7 +119,7 @@ virCapabilitiesFreeHostNUMACell(virCapsHostNUMACell *cell) virCapabilitiesClearHostNUMACellCPUTopology(cell->cpus, cell->ncpus); g_free(cell->cpus); - g_free(cell->siblings); + g_free(cell->distances); g_free(cell->pageinfo); g_free(cell); } @@ -331,8 +331,8 @@ virCapabilitiesSetNetPrefix(virCaps *caps, * @mem: Total size of memory in the NUMA node (in KiB) * @ncpus: number of CPUs in cell * @cpus: array of CPU definition structures - * @nsiblings: number of sibling NUMA nodes - * @siblings: info on sibling NUMA nodes + * @ndistances: number of sibling NUMA nodes + * @distances: NUMA distances to other nodes * @npageinfo: number of pages at node @num * @pageinfo: info on each single memory page * @@ -348,8 +348,8 @@ virCapabilitiesHostNUMAAddCell(virCapsHostNUMA *caps, unsigned long long mem, int ncpus, virCapsHostNUMACellCPU **cpus, - int nsiblings, - virCapsHostNUMACellSiblingInfo **siblings, + int ndistances, + virCapsHostNUMACellDistance **distances, int npageinfo, virCapsHostNUMACellPageInfo **pageinfo) { @@ -361,9 +361,9 @@ virCapabilitiesHostNUMAAddCell(virCapsHostNUMA *caps, cell->ncpus = ncpus; cell->cpus = g_steal_pointer(cpus); } - if (siblings) { - cell->nsiblings = nsiblings; - cell->siblings = g_steal_pointer(siblings); + if (distances) { + cell->ndistances = ndistances; + cell->distances = g_steal_pointer(distances); } if (pageinfo) { cell->npageinfo = npageinfo; @@ -833,13 +833,13 @@ virCapabilitiesHostNUMAFormat(virBuffer *buf, cell->pageinfo[j].avail); } - if (cell->nsiblings) { + if (cell->ndistances) { virBufferAddLit(buf, "<distances>\n"); virBufferAdjustIndent(buf, 2); - for (j = 0; j < cell->nsiblings; j++) { + for (j = 0; j < cell->ndistances; j++) { virBufferAsprintf(buf, "<sibling id='%d' value='%d'/>\n", - cell->siblings[j].node, - cell->siblings[j].distance); + cell->distances[j].node, + cell->distances[j].distance); } virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</distances>\n"); @@ -1456,11 +1456,11 @@ virCapabilitiesFillCPUInfo(int cpu_id G_GNUC_UNUSED, } static int -virCapabilitiesGetNUMASiblingInfo(int node, - virCapsHostNUMACellSiblingInfo **siblings, - int *nsiblings) +virCapabilitiesGetNUMADistances(int node, + virCapsHostNUMACellDistance **distancesRet, + int *ndistancesRet) { - virCapsHostNUMACellSiblingInfo *tmp = NULL; + virCapsHostNUMACellDistance *tmp = NULL; int tmp_size = 0; int ret = -1; int *distances = NULL; @@ -1471,12 +1471,12 @@ virCapabilitiesGetNUMASiblingInfo(int node, goto cleanup; if (!distances) { - *siblings = NULL; - *nsiblings = 0; + *distancesRet = NULL; + *ndistancesRet = 0; return 0; } - tmp = g_new0(virCapsHostNUMACellSiblingInfo, ndistances); + tmp = g_new0(virCapsHostNUMACellDistance, ndistances); for (i = 0; i < ndistances; i++) { if (!distances[i]) @@ -1489,8 +1489,8 @@ virCapabilitiesGetNUMASiblingInfo(int node, VIR_REALLOC_N(tmp, tmp_size); - *nsiblings = tmp_size; - *siblings = g_steal_pointer(&tmp); + *ndistancesRet = tmp_size; + *distancesRet = g_steal_pointer(&tmp); tmp_size = 0; ret = 0; cleanup: @@ -1607,8 +1607,8 @@ virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps) for (n = 0; n <= max_node; n++) { g_autoptr(virBitmap) cpumap = NULL; - g_autofree virCapsHostNUMACellSiblingInfo *siblings = NULL; - int nsiblings = 0; + g_autofree virCapsHostNUMACellDistance *distances = NULL; + int ndistances = 0; g_autofree virCapsHostNUMACellPageInfo *pageinfo = NULL; int npageinfo; unsigned long long memory; @@ -1632,8 +1632,8 @@ virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps) } } - if (virCapabilitiesGetNUMASiblingInfo(n, &siblings, &nsiblings) < 0) - goto cleanup; + if (virCapabilitiesGetNUMADistances(n, &distances, &ndistances) < 0) + return -1; if (virCapabilitiesGetNUMAPagesInfo(n, &pageinfo, &npageinfo) < 0) goto cleanup; @@ -1644,7 +1644,7 @@ virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps) virCapabilitiesHostNUMAAddCell(caps, n, memory, ncpus, &cpus, - nsiblings, &siblings, + ndistances, &distances, npageinfo, &pageinfo); } diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index ba863447c0..f11471ef6c 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -94,7 +94,7 @@ struct _virCapsHostNUMACellCPU { virBitmap *siblings; }; -struct _virCapsHostNUMACellSiblingInfo { +struct _virCapsHostNUMACellDistance { int node; /* foreign NUMA node */ unsigned int distance; /* distance to the node */ }; @@ -109,8 +109,8 @@ struct _virCapsHostNUMACell { int ncpus; unsigned long long mem; /* in kibibytes */ virCapsHostNUMACellCPU *cpus; - int nsiblings; - virCapsHostNUMACellSiblingInfo *siblings; + int ndistances; + virCapsHostNUMACellDistance *distances; int npageinfo; virCapsHostNUMACellPageInfo *pageinfo; }; @@ -255,8 +255,8 @@ virCapabilitiesHostNUMAAddCell(virCapsHostNUMA *caps, unsigned long long mem, int ncpus, virCapsHostNUMACellCPU **cpus, - int nsiblings, - virCapsHostNUMACellSiblingInfo **siblings, + int ndistances, + virCapsHostNUMACellDistance **distances, int npageinfo, virCapsHostNUMACellPageInfo **pageinfo); diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h index ff5d8145c3..d21d5a1be3 100644 --- a/src/conf/virconftypes.h +++ b/src/conf/virconftypes.h @@ -60,7 +60,7 @@ typedef struct _virCapsHostNUMACellCPU virCapsHostNUMACellCPU; typedef struct _virCapsHostNUMACellPageInfo virCapsHostNUMACellPageInfo; -typedef struct _virCapsHostNUMACellSiblingInfo virCapsHostNUMACellSiblingInfo; +typedef struct _virCapsHostNUMACellDistance virCapsHostNUMACellDistance; typedef struct _virCapsHostSecModel virCapsHostSecModel; diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index ea4f370a6d..d1a1241279 100644 --- a/src/libxl/libxl_capabilities.c +++ b/src/libxl/libxl_capabilities.c @@ -247,9 +247,9 @@ libxlCapsInitNuma(libxl_ctx *ctx, virCaps *caps) { libxl_numainfo *numa_info = NULL; libxl_cputopology *cpu_topo = NULL; - int nr_nodes = 0, nr_cpus = 0, nr_siblings = 0; + int nr_nodes = 0, nr_cpus = 0, nr_distances = 0; virCapsHostNUMACellCPU **cpus = NULL; - virCapsHostNUMACellSiblingInfo *siblings = NULL; + virCapsHostNUMACellDistance *distances = NULL; int *nr_cpus_node = NULL; size_t i; int ret = -1; @@ -316,22 +316,22 @@ libxlCapsInitNuma(libxl_ctx *ctx, virCaps *caps) if (numa_info[i].size == LIBXL_NUMAINFO_INVALID_ENTRY) continue; - nr_siblings = numa_info[i].num_dists; - if (nr_siblings) { + nr_distances = numa_info[i].num_dists; + if (nr_distances) { size_t j; - siblings = g_new0(virCapsHostNUMACellSiblingInfo, nr_siblings); + distances = g_new0(virCapsHostNUMACellDistance, nr_distances); - for (j = 0; j < nr_siblings; j++) { - siblings[j].node = j; - siblings[j].distance = numa_info[i].dists[j]; + for (j = 0; j < nr_distances; j++) { + distances[j].node = j; + distances[j].distance = numa_info[i].dists[j]; } } virCapabilitiesHostNUMAAddCell(caps->host.numa, i, numa_info[i].size / 1024, nr_cpus_node[i], &cpus[i], - nr_siblings, &siblings, + nr_distances, &distances, 0, NULL); /* This is safe, as the CPU list is now stored in the NUMA cell */ @@ -348,7 +348,7 @@ libxlCapsInitNuma(libxl_ctx *ctx, virCaps *caps) virCapabilitiesHostNUMAUnref(caps->host.numa); caps->host.numa = NULL; } - VIR_FREE(siblings); + VIR_FREE(distances); } VIR_FREE(cpus); -- 2.26.3
On Thu, May 20, 2021 at 17:24:53 +0200, Michal Privoznik wrote:
The virCapsHostNUMACellSiblingInfo structure really represents distance to other NUMA node. Rename the structure and variables of that type to make it more obvious.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/capabilities.c | 52 +++++++++++++++++----------------- src/conf/capabilities.h | 10 +++---- src/conf/virconftypes.h | 2 +- src/libxl/libxl_capabilities.c | 20 ++++++------- 4 files changed, 42 insertions(+), 42 deletions(-)
[...] I first wanted to complain that we might want to add other data related to NUMA siblings other than distances ...
@@ -833,13 +833,13 @@ virCapabilitiesHostNUMAFormat(virBuffer *buf, cell->pageinfo[j].avail); }
- if (cell->nsiblings) { + if (cell->ndistances) { virBufferAddLit(buf, "<distances>\n");
... but this clearly nests this as 'distances' so adding anything here would seem wrong.
virBufferAdjustIndent(buf, 2); - for (j = 0; j < cell->nsiblings; j++) { + for (j = 0; j < cell->ndistances; j++) { virBufferAsprintf(buf, "<sibling id='%d' value='%d'/>\n", - cell->siblings[j].node, - cell->siblings[j].distance); + cell->distances[j].node, + cell->distances[j].distance); } virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</distances>\n");
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On 5/21/21 9:46 AM, Peter Krempa wrote:
On Thu, May 20, 2021 at 17:24:53 +0200, Michal Privoznik wrote:
The virCapsHostNUMACellSiblingInfo structure really represents distance to other NUMA node. Rename the structure and variables of that type to make it more obvious.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/capabilities.c | 52 +++++++++++++++++----------------- src/conf/capabilities.h | 10 +++---- src/conf/virconftypes.h | 2 +- src/libxl/libxl_capabilities.c | 20 ++++++------- 4 files changed, 42 insertions(+), 42 deletions(-)
[...]
I first wanted to complain that we might want to add other data related to NUMA siblings other than distances ...
That was my intent when implementing NUMA distance reporting for capabilities, years ago. And I'm planning on extending capabilities for memory caches reporting (currently struct _virDomainNumaCache for domain NUMA nodes). But my code has it as another argument to virCapabilitiesHostNUMAAddCell() and another struct (I'm doing a deduplication similar to this one). And because of that code reuse (well, XML formater) - I prefer to follow _virDomainNuma struct in capabilities. Michal
There's nothing domain specific about NUMA distances. Rename the virDomainNumaDistance structure to just virNumaDistance. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/numa_conf.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 537e515b5a..fa39c1ccc0 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -82,7 +82,7 @@ VIR_ENUM_IMPL(virDomainMemoryLatency, "write" ); -typedef struct _virDomainNumaDistance virDomainNumaDistance; +typedef struct _virNumaDistance virNumaDistance; typedef struct _virDomainNumaCache virDomainNumaCache; @@ -106,7 +106,7 @@ struct _virDomainNuma { virDomainMemoryAccess memAccess; /* shared memory access configuration */ virTristateBool discard; /* discard-data for memory-backend-file */ - struct _virDomainNumaDistance { + struct _virNumaDistance { unsigned int value; /* locality value for node i->j or j->i */ unsigned int cellid; } *distances; /* remote node distances */ @@ -759,8 +759,8 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def, } for (i = 0; i < sibling; i++) { - virDomainNumaDistance *ldist; - virDomainNumaDistance *rdist; + virNumaDistance *ldist; + virNumaDistance *rdist; unsigned int sibling_id, sibling_value; if (virXMLPropUInt(nodes[i], "id", 10, VIR_XML_PROP_REQUIRED, @@ -799,7 +799,7 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def, /* Apply the local / remote distance */ ldist = def->mem_nodes[cur_cell].distances; if (!ldist) { - ldist = g_new0(virDomainNumaDistance, ndistances); + ldist = g_new0(virNumaDistance, ndistances); ldist[cur_cell].value = LOCAL_DISTANCE; ldist[cur_cell].cellid = cur_cell; def->mem_nodes[cur_cell].ndistances = ndistances; @@ -812,7 +812,7 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def, /* Apply symmetry if none given */ rdist = def->mem_nodes[sibling_id].distances; if (!rdist) { - rdist = g_new0(virDomainNumaDistance, ndistances); + rdist = g_new0(virNumaDistance, ndistances); rdist[sibling_id].value = LOCAL_DISTANCE; rdist[sibling_id].cellid = sibling_id; def->mem_nodes[sibling_id].ndistances = ndistances; @@ -1133,7 +1133,7 @@ virDomainNumaDefFormatXML(virBuffer *buf, virTristateBoolTypeToString(discard)); if (def->mem_nodes[i].ndistances) { - virDomainNumaDistance *distances = def->mem_nodes[i].distances; + virNumaDistance *distances = def->mem_nodes[i].distances; virBufferAddLit(&childBuf, "<distances>\n"); virBufferAdjustIndent(&childBuf, 2); @@ -1501,7 +1501,7 @@ virDomainNumaGetNodeDistance(virDomainNuma *numa, size_t node, size_t cellid) { - virDomainNumaDistance *distances = NULL; + virNumaDistance *distances = NULL; if (node < numa->nmem_nodes) distances = numa->mem_nodes[node].distances; @@ -1526,7 +1526,7 @@ virDomainNumaSetNodeDistance(virDomainNuma *numa, size_t cellid, unsigned int value) { - virDomainNumaDistance *distances; + virNumaDistance *distances; if (node >= numa->nmem_nodes) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1579,7 +1579,7 @@ virDomainNumaSetNodeDistanceCount(virDomainNuma *numa, size_t node, size_t ndistances) { - virDomainNumaDistance *distances; + virNumaDistance *distances; distances = numa->mem_nodes[node].distances; if (distances) { @@ -1589,7 +1589,7 @@ virDomainNumaSetNodeDistanceCount(virDomainNuma *numa, return 0; } - distances = g_new0(struct _virDomainNumaDistance, ndistances); + distances = g_new0(virNumaDistance, ndistances); numa->mem_nodes[node].distances = distances; numa->mem_nodes[node].ndistances = ndistances; -- 2.26.3
On Thu, May 20, 2021 at 17:24:54 +0200, Michal Privoznik wrote:
There's nothing domain specific about NUMA distances. Rename the virDomainNumaDistance structure to just virNumaDistance.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/numa_conf.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Expose virNumaDistance XML formatter so that it can be re-used by other parts of the code. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/numa_conf.c | 47 +++++++++++++++++++++------------------- src/conf/numa_conf.h | 10 +++++++++ src/libvirt_private.syms | 1 + 3 files changed, 36 insertions(+), 22 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index fa39c1ccc0..8fd5635aca 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -82,8 +82,6 @@ VIR_ENUM_IMPL(virDomainMemoryLatency, "write" ); -typedef struct _virNumaDistance virNumaDistance; - typedef struct _virDomainNumaCache virDomainNumaCache; typedef struct _virDomainNumaInterconnect virDomainNumaInterconnect; @@ -106,10 +104,7 @@ struct _virDomainNuma { virDomainMemoryAccess memAccess; /* shared memory access configuration */ virTristateBool discard; /* discard-data for memory-backend-file */ - struct _virNumaDistance { - unsigned int value; /* locality value for node i->j or j->i */ - unsigned int cellid; - } *distances; /* remote node distances */ + virNumaDistance *distances; /* remote node distances */ size_t ndistances; struct _virDomainNumaCache { @@ -1132,22 +1127,9 @@ virDomainNumaDefFormatXML(virBuffer *buf, virBufferAsprintf(&attrBuf, " discard='%s'", virTristateBoolTypeToString(discard)); - if (def->mem_nodes[i].ndistances) { - virNumaDistance *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(&childBuf, -2); - virBufferAddLit(&childBuf, "</distances>\n"); - } + virNumaDistanceFormat(&childBuf, + def->mem_nodes[i].distances, + def->mem_nodes[i].ndistances); for (j = 0; j < def->mem_nodes[i].ncaches; j++) { virDomainNumaCache *cache = &def->mem_nodes[i].caches[j]; @@ -1836,3 +1818,24 @@ virDomainNumaGetInterconnect(const virDomainNuma *numa, *value = l->value; return 0; } + + +void +virNumaDistanceFormat(virBuffer *buf, + const virNumaDistance *distances, + size_t ndistances) +{ + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); + size_t i; + + for (i = 0; i < ndistances; i++) { + if (distances[i].value == 0) + continue; + virBufferAddLit(&childBuf, "<sibling"); + virBufferAsprintf(&childBuf, " id='%d'", distances[i].cellid); + virBufferAsprintf(&childBuf, " value='%d'", distances[i].value); + virBufferAddLit(&childBuf, "/>\n"); + } + + virXMLFormatElement(buf, "distances", NULL, &childBuf); +} diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index 8f6597b0b7..6682580ded 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -249,3 +249,13 @@ int virDomainNumaGetInterconnect(const virDomainNuma *numa, unsigned int *cache, virDomainMemoryLatency *accessType, unsigned long *value); + +typedef struct _virNumaDistance virNumaDistance; +struct _virNumaDistance { + unsigned int value; /* locality value for node i->j or j->i */ + unsigned int cellid; +}; + +void virNumaDistanceFormat(virBuffer *buf, + const virNumaDistance *distances, + size_t ndistances); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6fbdee4124..fcf833429b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -913,6 +913,7 @@ virDomainNumatunePlacementTypeFromString; virDomainNumatunePlacementTypeToString; virDomainNumatuneSet; virDomainNumatuneSpecifiedMaxNode; +virNumaDistanceFormat; # conf/nwfilter_conf.h -- 2.26.3
On Thu, May 20, 2021 at 17:24:55 +0200, Michal Privoznik wrote:
Expose virNumaDistance XML formatter so that it can be re-used by other parts of the code.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/numa_conf.c | 47 +++++++++++++++++++++------------------- src/conf/numa_conf.h | 10 +++++++++ src/libvirt_private.syms | 1 + 3 files changed, 36 insertions(+), 22 deletions(-)
diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index fa39c1ccc0..8fd5635aca 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c
[...]
@@ -1132,22 +1127,9 @@ virDomainNumaDefFormatXML(virBuffer *buf, virBufferAsprintf(&attrBuf, " discard='%s'", virTristateBoolTypeToString(discard));
- if (def->mem_nodes[i].ndistances) { - virNumaDistance *distances = def->mem_nodes[i].distances; - - virBufferAddLit(&childBuf, "<distances>\n");
A slight semantic difference between the two impls is that this will format an empty <distances> pair tag ...
- virBufferAdjustIndent(&childBuf, 2); - for (j = 0; j < def->mem_nodes[i].ndistances; j++) { - if (distances[j].value) {
... if all of these are 0.
- virBufferAddLit(&childBuf, "<sibling"); - virBufferAsprintf(&childBuf, " id='%d'", distances[j].cellid); - virBufferAsprintf(&childBuf, " value='%d'", distances[j].value); - virBufferAddLit(&childBuf, "/>\n"); - } - } - virBufferAdjustIndent(&childBuf, -2); - virBufferAddLit(&childBuf, "</distances>\n"); - } + virNumaDistanceFormat(&childBuf, + def->mem_nodes[i].distances, + def->mem_nodes[i].ndistances);
for (j = 0; j < def->mem_nodes[i].ncaches; j++) { virDomainNumaCache *cache = &def->mem_nodes[i].caches[j]; @@ -1836,3 +1818,24 @@ virDomainNumaGetInterconnect(const virDomainNuma *numa, *value = l->value; return 0; } + + +void +virNumaDistanceFormat(virBuffer *buf, + const virNumaDistance *distances, + size_t ndistances) +{ + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); + size_t i; + + for (i = 0; i < ndistances; i++) { + if (distances[i].value == 0) + continue; + virBufferAddLit(&childBuf, "<sibling"); + virBufferAsprintf(&childBuf, " id='%d'", distances[i].cellid); + virBufferAsprintf(&childBuf, " value='%d'", distances[i].value); + virBufferAddLit(&childBuf, "/>\n"); + } + + virXMLFormatElement(buf, "distances", NULL, &childBuf);
while here we won't.
+}
In this case it shouldn't be a problem though unlike in the NBD part of the migration cookie ;). If you think it might be a problem you can use virXMLFormatElementEmpty. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
After previous patches we have two structures: virCapsHostNUMACellDistance and virNumaDistance which express the same thing. And have the exact same members (modulo their names). Drop the former in favor of the latter. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/capabilities.c | 26 ++++++++------------------ src/conf/capabilities.h | 11 +++-------- src/conf/virconftypes.h | 2 -- src/libxl/libxl_capabilities.c | 8 ++++---- 4 files changed, 15 insertions(+), 32 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 926ecb5a24..1290c9c15d 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -349,7 +349,7 @@ virCapabilitiesHostNUMAAddCell(virCapsHostNUMA *caps, int ncpus, virCapsHostNUMACellCPU **cpus, int ndistances, - virCapsHostNUMACellDistance **distances, + virNumaDistance **distances, int npageinfo, virCapsHostNUMACellPageInfo **pageinfo) { @@ -833,17 +833,7 @@ virCapabilitiesHostNUMAFormat(virBuffer *buf, cell->pageinfo[j].avail); } - if (cell->ndistances) { - virBufferAddLit(buf, "<distances>\n"); - virBufferAdjustIndent(buf, 2); - for (j = 0; j < cell->ndistances; j++) { - virBufferAsprintf(buf, "<sibling id='%d' value='%d'/>\n", - cell->distances[j].node, - cell->distances[j].distance); - } - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</distances>\n"); - } + virNumaDistanceFormat(buf, cell->distances, cell->ndistances); virBufferAsprintf(buf, "<cpus num='%d'>\n", cell->ncpus); virBufferAdjustIndent(buf, 2); @@ -1457,10 +1447,10 @@ virCapabilitiesFillCPUInfo(int cpu_id G_GNUC_UNUSED, static int virCapabilitiesGetNUMADistances(int node, - virCapsHostNUMACellDistance **distancesRet, + virNumaDistance **distancesRet, int *ndistancesRet) { - virCapsHostNUMACellDistance *tmp = NULL; + virNumaDistance *tmp = NULL; int tmp_size = 0; int ret = -1; int *distances = NULL; @@ -1476,14 +1466,14 @@ virCapabilitiesGetNUMADistances(int node, return 0; } - tmp = g_new0(virCapsHostNUMACellDistance, ndistances); + tmp = g_new0(virNumaDistance, ndistances); for (i = 0; i < ndistances; i++) { if (!distances[i]) continue; - tmp[tmp_size].node = i; - tmp[tmp_size].distance = distances[i]; + tmp[tmp_size].cellid = i; + tmp[tmp_size].value = distances[i]; tmp_size++; } @@ -1607,7 +1597,7 @@ virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps) for (n = 0; n <= max_node; n++) { g_autoptr(virBitmap) cpumap = NULL; - g_autofree virCapsHostNUMACellDistance *distances = NULL; + g_autofree virNumaDistance *distances = NULL; int ndistances = 0; g_autofree virCapsHostNUMACellPageInfo *pageinfo = NULL; int npageinfo; diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index f11471ef6c..4d4ac476ea 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -94,11 +94,6 @@ struct _virCapsHostNUMACellCPU { virBitmap *siblings; }; -struct _virCapsHostNUMACellDistance { - int node; /* foreign NUMA node */ - unsigned int distance; /* distance to the node */ -}; - struct _virCapsHostNUMACellPageInfo { unsigned int size; /* page size in kibibytes */ unsigned long long avail; /* the size of pool */ @@ -109,8 +104,8 @@ struct _virCapsHostNUMACell { int ncpus; unsigned long long mem; /* in kibibytes */ virCapsHostNUMACellCPU *cpus; - int ndistances; - virCapsHostNUMACellDistance *distances; + size_t ndistances; + virNumaDistance *distances; int npageinfo; virCapsHostNUMACellPageInfo *pageinfo; }; @@ -256,7 +251,7 @@ virCapabilitiesHostNUMAAddCell(virCapsHostNUMA *caps, int ncpus, virCapsHostNUMACellCPU **cpus, int ndistances, - virCapsHostNUMACellDistance **distances, + virNumaDistance **distances, int npageinfo, virCapsHostNUMACellPageInfo **pageinfo); diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h index d21d5a1be3..b21068486e 100644 --- a/src/conf/virconftypes.h +++ b/src/conf/virconftypes.h @@ -60,8 +60,6 @@ typedef struct _virCapsHostNUMACellCPU virCapsHostNUMACellCPU; typedef struct _virCapsHostNUMACellPageInfo virCapsHostNUMACellPageInfo; -typedef struct _virCapsHostNUMACellDistance virCapsHostNUMACellDistance; - typedef struct _virCapsHostSecModel virCapsHostSecModel; typedef struct _virCapsHostSecModelLabel virCapsHostSecModelLabel; diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index d1a1241279..a73f13f829 100644 --- a/src/libxl/libxl_capabilities.c +++ b/src/libxl/libxl_capabilities.c @@ -249,7 +249,7 @@ libxlCapsInitNuma(libxl_ctx *ctx, virCaps *caps) libxl_cputopology *cpu_topo = NULL; int nr_nodes = 0, nr_cpus = 0, nr_distances = 0; virCapsHostNUMACellCPU **cpus = NULL; - virCapsHostNUMACellDistance *distances = NULL; + virNumaDistance *distances = NULL; int *nr_cpus_node = NULL; size_t i; int ret = -1; @@ -320,11 +320,11 @@ libxlCapsInitNuma(libxl_ctx *ctx, virCaps *caps) if (nr_distances) { size_t j; - distances = g_new0(virCapsHostNUMACellDistance, nr_distances); + distances = g_new0(virNumaDistance, nr_distances); for (j = 0; j < nr_distances; j++) { - distances[j].node = j; - distances[j].distance = numa_info[i].dists[j]; + distances[j].cellid = j; + distances[j].value = numa_info[i].dists[j]; } } -- 2.26.3
On Thu, May 20, 2021 at 17:24:56 +0200, Michal Privoznik wrote:
After previous patches we have two structures: virCapsHostNUMACellDistance and virNumaDistance which express the same thing. And have the exact same members (modulo their names). Drop the former in favor of the latter.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/capabilities.c | 26 ++++++++------------------ src/conf/capabilities.h | 11 +++-------- src/conf/virconftypes.h | 2 -- src/libxl/libxl_capabilities.c | 8 ++++---- 4 files changed, 15 insertions(+), 32 deletions(-)
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 926ecb5a24..1290c9c15d 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c
[...]
@@ -833,17 +833,7 @@ virCapabilitiesHostNUMAFormat(virBuffer *buf, cell->pageinfo[j].avail); }
- if (cell->ndistances) { - virBufferAddLit(buf, "<distances>\n"); - virBufferAdjustIndent(buf, 2); - for (j = 0; j < cell->ndistances; j++) {
This code didn't skip printing the sibling if 'value' is 0 ...
- virBufferAsprintf(buf, "<sibling id='%d' value='%d'/>\n", - cell->distances[j].node, - cell->distances[j].distance); - } - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</distances>\n"); - } + virNumaDistanceFormat(buf, cell->distances, cell->ndistances);
... but this new implementation does that. I didn't check whether that's justified or not, but the commit message doesn't try to justify it either. Was that an expected change?
virBufferAsprintf(buf, "<cpus num='%d'>\n", cell->ncpus); virBufferAdjustIndent(buf, 2);
On 5/21/21 9:58 AM, Peter Krempa wrote:
On Thu, May 20, 2021 at 17:24:56 +0200, Michal Privoznik wrote:
After previous patches we have two structures: virCapsHostNUMACellDistance and virNumaDistance which express the same thing. And have the exact same members (modulo their names). Drop the former in favor of the latter.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/capabilities.c | 26 ++++++++------------------ src/conf/capabilities.h | 11 +++-------- src/conf/virconftypes.h | 2 -- src/libxl/libxl_capabilities.c | 8 ++++---- 4 files changed, 15 insertions(+), 32 deletions(-)
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 926ecb5a24..1290c9c15d 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c
[...]
@@ -833,17 +833,7 @@ virCapabilitiesHostNUMAFormat(virBuffer *buf, cell->pageinfo[j].avail); }
- if (cell->ndistances) { - virBufferAddLit(buf, "<distances>\n"); - virBufferAdjustIndent(buf, 2); - for (j = 0; j < cell->ndistances; j++) {
This code didn't skip printing the sibling if 'value' is 0 ...
- virBufferAsprintf(buf, "<sibling id='%d' value='%d'/>\n", - cell->distances[j].node, - cell->distances[j].distance); - } - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</distances>\n"); - } + virNumaDistanceFormat(buf, cell->distances, cell->ndistances);
... but this new implementation does that. I didn't check whether that's justified or not, but the commit message doesn't try to justify it either.
Was that an expected change?
Yes, I will adjust commit message. The point is that in domain XML we allow partial specification of distances. Each guest NUMA node will have an array of virDomainNumaDistance structs (#nodes long) and for each the .value member will either be in [10, 255] range or 0 (if not specified in XML). For capabilities - we query numa_distance() and store its output into an array. The numa_distance() comes from numactl package, and returns 0 to indicate an error (if either parsing distances from sysfs failed or we passed a non-existent node in): https://github.com/numactl/numactl/blob/master/distance.c#L110 Therefore, I think it's safe to ignore 0 - it's not valid distance anyway. However, what I forgot to squash in was: diff --git i/docs/schemas/capability.rng w/docs/schemas/capability.rng index c4cafc47ee..fb8203ad6d 100644 --- i/docs/schemas/capability.rng +++ w/docs/schemas/capability.rng @@ -157,16 +157,9 @@ <optional> <element name="distances"> - <zeroOrMore> - <element name="sibling"> - <attribute name="id"> - <ref name="unsignedInt"/> - </attribute> - <attribute name="value"> - <ref name="unsignedInt"/> - </attribute> - </element> - </zeroOrMore> + <oneOrMore> + <ref name="numaDistance"/> + </oneOrMore> </element> </optional> So let me resend v2. Thanks! Michal
After previous patches we have two structures: virCapsHostNUMACellDistance and virNumaDistance which express the same thing. And have the exact same members (modulo their names). Drop the former in favor of the latter. This change means that distances with value of 0 are no longer printed out into capabilities XML, because domain XML code allows partial distance specification and thus threats value of 0 as unspecified by user (see virDomainNumaGetNodeDistance() which returns the default LOCAL/REMOTE distance for value of 0). Also, from ACPI 6.1 specification, section 5.2.17 System Locality Distance Information Table (SLIT): Distance values of 0-9 are reserved and have no meaning. Thus we shouldn't be ever reporting 0 in neither domain nor capabilities XML. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- diff to v1: - Filled in justification to stop reporting 0 distance in capabilities XML (which we never did anyway). - Change capabilities RNG to make it obvious that NUMA distances are the same in domain and capabilities XMLs. docs/schemas/capability.rng | 13 +++---------- src/conf/capabilities.c | 26 ++++++++------------------ src/conf/capabilities.h | 11 +++-------- src/conf/virconftypes.h | 2 -- src/libxl/libxl_capabilities.c | 8 ++++---- 5 files changed, 18 insertions(+), 42 deletions(-) diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng index c4cafc47ee..fb8203ad6d 100644 --- a/docs/schemas/capability.rng +++ b/docs/schemas/capability.rng @@ -157,16 +157,9 @@ <optional> <element name="distances"> - <zeroOrMore> - <element name="sibling"> - <attribute name="id"> - <ref name="unsignedInt"/> - </attribute> - <attribute name="value"> - <ref name="unsignedInt"/> - </attribute> - </element> - </zeroOrMore> + <oneOrMore> + <ref name="numaDistance"/> + </oneOrMore> </element> </optional> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 926ecb5a24..1290c9c15d 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -349,7 +349,7 @@ virCapabilitiesHostNUMAAddCell(virCapsHostNUMA *caps, int ncpus, virCapsHostNUMACellCPU **cpus, int ndistances, - virCapsHostNUMACellDistance **distances, + virNumaDistance **distances, int npageinfo, virCapsHostNUMACellPageInfo **pageinfo) { @@ -833,17 +833,7 @@ virCapabilitiesHostNUMAFormat(virBuffer *buf, cell->pageinfo[j].avail); } - if (cell->ndistances) { - virBufferAddLit(buf, "<distances>\n"); - virBufferAdjustIndent(buf, 2); - for (j = 0; j < cell->ndistances; j++) { - virBufferAsprintf(buf, "<sibling id='%d' value='%d'/>\n", - cell->distances[j].node, - cell->distances[j].distance); - } - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</distances>\n"); - } + virNumaDistanceFormat(buf, cell->distances, cell->ndistances); virBufferAsprintf(buf, "<cpus num='%d'>\n", cell->ncpus); virBufferAdjustIndent(buf, 2); @@ -1457,10 +1447,10 @@ virCapabilitiesFillCPUInfo(int cpu_id G_GNUC_UNUSED, static int virCapabilitiesGetNUMADistances(int node, - virCapsHostNUMACellDistance **distancesRet, + virNumaDistance **distancesRet, int *ndistancesRet) { - virCapsHostNUMACellDistance *tmp = NULL; + virNumaDistance *tmp = NULL; int tmp_size = 0; int ret = -1; int *distances = NULL; @@ -1476,14 +1466,14 @@ virCapabilitiesGetNUMADistances(int node, return 0; } - tmp = g_new0(virCapsHostNUMACellDistance, ndistances); + tmp = g_new0(virNumaDistance, ndistances); for (i = 0; i < ndistances; i++) { if (!distances[i]) continue; - tmp[tmp_size].node = i; - tmp[tmp_size].distance = distances[i]; + tmp[tmp_size].cellid = i; + tmp[tmp_size].value = distances[i]; tmp_size++; } @@ -1607,7 +1597,7 @@ virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps) for (n = 0; n <= max_node; n++) { g_autoptr(virBitmap) cpumap = NULL; - g_autofree virCapsHostNUMACellDistance *distances = NULL; + g_autofree virNumaDistance *distances = NULL; int ndistances = 0; g_autofree virCapsHostNUMACellPageInfo *pageinfo = NULL; int npageinfo; diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index f11471ef6c..4d4ac476ea 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -94,11 +94,6 @@ struct _virCapsHostNUMACellCPU { virBitmap *siblings; }; -struct _virCapsHostNUMACellDistance { - int node; /* foreign NUMA node */ - unsigned int distance; /* distance to the node */ -}; - struct _virCapsHostNUMACellPageInfo { unsigned int size; /* page size in kibibytes */ unsigned long long avail; /* the size of pool */ @@ -109,8 +104,8 @@ struct _virCapsHostNUMACell { int ncpus; unsigned long long mem; /* in kibibytes */ virCapsHostNUMACellCPU *cpus; - int ndistances; - virCapsHostNUMACellDistance *distances; + size_t ndistances; + virNumaDistance *distances; int npageinfo; virCapsHostNUMACellPageInfo *pageinfo; }; @@ -256,7 +251,7 @@ virCapabilitiesHostNUMAAddCell(virCapsHostNUMA *caps, int ncpus, virCapsHostNUMACellCPU **cpus, int ndistances, - virCapsHostNUMACellDistance **distances, + virNumaDistance **distances, int npageinfo, virCapsHostNUMACellPageInfo **pageinfo); diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h index d21d5a1be3..b21068486e 100644 --- a/src/conf/virconftypes.h +++ b/src/conf/virconftypes.h @@ -60,8 +60,6 @@ typedef struct _virCapsHostNUMACellCPU virCapsHostNUMACellCPU; typedef struct _virCapsHostNUMACellPageInfo virCapsHostNUMACellPageInfo; -typedef struct _virCapsHostNUMACellDistance virCapsHostNUMACellDistance; - typedef struct _virCapsHostSecModel virCapsHostSecModel; typedef struct _virCapsHostSecModelLabel virCapsHostSecModelLabel; diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index d1a1241279..a73f13f829 100644 --- a/src/libxl/libxl_capabilities.c +++ b/src/libxl/libxl_capabilities.c @@ -249,7 +249,7 @@ libxlCapsInitNuma(libxl_ctx *ctx, virCaps *caps) libxl_cputopology *cpu_topo = NULL; int nr_nodes = 0, nr_cpus = 0, nr_distances = 0; virCapsHostNUMACellCPU **cpus = NULL; - virCapsHostNUMACellDistance *distances = NULL; + virNumaDistance *distances = NULL; int *nr_cpus_node = NULL; size_t i; int ret = -1; @@ -320,11 +320,11 @@ libxlCapsInitNuma(libxl_ctx *ctx, virCaps *caps) if (nr_distances) { size_t j; - distances = g_new0(virCapsHostNUMACellDistance, nr_distances); + distances = g_new0(virNumaDistance, nr_distances); for (j = 0; j < nr_distances; j++) { - distances[j].node = j; - distances[j].distance = numa_info[i].dists[j]; + distances[j].cellid = j; + distances[j].value = numa_info[i].dists[j]; } } -- 2.26.3
On Mon, May 24, 2021 at 14:03:09 +0200, Michal Privoznik wrote:
After previous patches we have two structures: virCapsHostNUMACellDistance and virNumaDistance which express the same thing. And have the exact same members (modulo their names). Drop the former in favor of the latter.
This change means that distances with value of 0 are no longer printed out into capabilities XML, because domain XML code allows partial distance specification and thus threats value of 0 as unspecified by user (see virDomainNumaGetNodeDistance() which returns the default LOCAL/REMOTE distance for value of 0).
Also, from ACPI 6.1 specification, section 5.2.17 System Locality Distance Information Table (SLIT):
Distance values of 0-9 are reserved and have no meaning.
Thus we shouldn't be ever reporting 0 in neither domain nor capabilities XML.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
diff to v1: - Filled in justification to stop reporting 0 distance in capabilities XML (which we never did anyway). - Change capabilities RNG to make it obvious that NUMA distances are the same in domain and capabilities XMLs.
docs/schemas/capability.rng | 13 +++---------- src/conf/capabilities.c | 26 ++++++++------------------ src/conf/capabilities.h | 11 +++-------- src/conf/virconftypes.h | 2 -- src/libxl/libxl_capabilities.c | 8 ++++---- 5 files changed, 18 insertions(+), 42 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (3)
-
Michal Privoznik -
Michal Prívozník
-
Peter Krempa