[PATCH 2/3] capabilities: Turn @cpus in _virCapsHostNUMACell into GArray
Michal Privoznik
mprivozn at redhat.com
Wed May 12 13:23:30 UTC 2021
Currently, we are storing the list of CPUs belonging to a NUMA
cell as a regular C array of virCapsHostNUMACellCPU structs. The
length of the array is stored separately, next to the array.
This works, except for one case - freeing the array. Since the
array length is stored separately we can't use automatic
cleanup/free.
The solution is to rework how the array is stored -> switch to
GAarray which stores the length among with the array items and
allows us to define a clear function which is called over each
individual item in the array upon its release.
For many places this has no direct effect on how the array is
constructed, except for libxl. A multidimensional array is
constructed there and honestly, I did not want to touch it.
Therefore, instead of constructing CPU list directly into GArray
I'm just copying over the values after the multidimensional array
is created.
Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
---
src/conf/capabilities.c | 129 ++++++++++++++++++---------------
src/conf/capabilities.h | 11 +--
src/libvirt_private.syms | 2 +-
src/libxl/libxl_capabilities.c | 20 +++--
src/test/test_driver.c | 12 +--
tests/testutils.c | 23 +++---
6 files changed, 108 insertions(+), 89 deletions(-)
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index 1dae6d38cc..4c50f5d52a 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -95,30 +95,41 @@ virCapabilitiesNew(virArch hostarch,
return caps;
}
-void
-virCapabilitiesClearHostNUMACellCPUTopology(virCapsHostNUMACellCPU *cpus,
- size_t ncpus)
+
+static void
+virCapsHostNUMACellCPUClear(void *data)
{
- size_t i;
+ virCapsHostNUMACellCPU *cpu = data;
+
+ virBitmapFree(cpu->siblings);
+ memset(cpu, 0, sizeof(*cpu));
+}
- if (!cpus)
- return;
- for (i = 0; i < ncpus; i++) {
- virBitmapFree(cpus[i].siblings);
- cpus[i].siblings = NULL;
- }
+/**
+ * virCapsHostNUMACellCPUNew:
+ * @prealloc: number of items to allocate
+ *
+ * Creates a new array of host CPUs.
+ */
+GArray *
+virCapsHostNUMACellCPUNew(size_t prealloc)
+{
+ GArray *ret;
+
+ ret = g_array_sized_new(FALSE, FALSE, sizeof(virCapsHostNUMACellCPU), prealloc);
+ g_array_set_clear_func(ret, virCapsHostNUMACellCPUClear);
+ return ret;
}
+
static void
virCapabilitiesFreeHostNUMACell(virCapsHostNUMACell *cell)
{
if (cell == NULL)
return;
- virCapabilitiesClearHostNUMACellCPUTopology(cell->cpus, cell->ncpus);
-
- g_free(cell->cpus);
+ g_array_unref(cell->cpus);
g_free(cell->siblings);
g_free(cell->pageinfo);
g_free(cell);
@@ -329,7 +340,6 @@ virCapabilitiesSetNetPrefix(virCaps *caps,
* @caps: capabilities to extend
* @num: ID number of NUMA cell
* @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
@@ -346,8 +356,7 @@ void
virCapabilitiesHostNUMAAddCell(virCapsHostNUMA *caps,
int num,
unsigned long long mem,
- int ncpus,
- virCapsHostNUMACellCPU **cpus,
+ GArray **cpus,
int nsiblings,
virCapsHostNUMACellSiblingInfo **siblings,
int npageinfo,
@@ -357,9 +366,10 @@ virCapabilitiesHostNUMAAddCell(virCapsHostNUMA *caps,
cell->num = num;
cell->mem = mem;
- if (cpus) {
- cell->ncpus = ncpus;
+ if (cpus && *cpus) {
cell->cpus = g_steal_pointer(cpus);
+ } else {
+ cell->cpus = virCapsHostNUMACellCPUNew(0);
}
if (siblings) {
cell->nsiblings = nsiblings;
@@ -845,20 +855,22 @@ virCapabilitiesHostNUMAFormat(virBuffer *buf,
virBufferAddLit(buf, "</distances>\n");
}
- virBufferAsprintf(buf, "<cpus num='%d'>\n", cell->ncpus);
+ virBufferAsprintf(buf, "<cpus num='%d'>\n", cell->cpus->len);
virBufferAdjustIndent(buf, 2);
- for (j = 0; j < cell->ncpus; j++) {
- virBufferAsprintf(buf, "<cpu id='%d'", cell->cpus[j].id);
+ for (j = 0; j < cell->cpus->len; j++) {
+ virCapsHostNUMACellCPU *cpu = &g_array_index(cell->cpus, virCapsHostNUMACellCPU, j);
- if (cell->cpus[j].siblings) {
- if (!(siblings = virBitmapFormat(cell->cpus[j].siblings)))
+ virBufferAsprintf(buf, "<cpu id='%d'", cpu->id);
+
+ if (cpu->siblings) {
+ if (!(siblings = virBitmapFormat(cpu->siblings)))
return -1;
virBufferAsprintf(buf,
" socket_id='%d' die_id='%d' core_id='%d' siblings='%s'",
- cell->cpus[j].socket_id,
- cell->cpus[j].die_id,
- cell->cpus[j].core_id,
+ cpu->socket_id,
+ cpu->die_id,
+ cpu->core_id,
siblings);
VIR_FREE(siblings);
}
@@ -1341,14 +1353,16 @@ virCapabilitiesHostNUMAGetMaxcpu(virCapsHostNUMA *caps)
{
unsigned int maxcpu = 0;
size_t node;
- size_t cpu;
for (node = 0; node < caps->cells->len; node++) {
virCapsHostNUMACell *cell = g_ptr_array_index(caps->cells, node);
+ size_t j;
- for (cpu = 0; cpu < cell->ncpus; cpu++) {
- if (cell->cpus[cpu].id > maxcpu)
- maxcpu = cell->cpus[cpu].id;
+ for (j = 0; j < cell->cpus->len; j++) {
+ virCapsHostNUMACellCPU *cpu = &g_array_index(cell->cpus, virCapsHostNUMACellCPU, j);
+
+ if (cpu->id > maxcpu)
+ maxcpu = cpu->id;
}
}
@@ -1362,7 +1376,6 @@ virCapabilitiesHostNUMAGetCellCpus(virCapsHostNUMA *caps,
virBitmap *cpumask)
{
virCapsHostNUMACell *cell = NULL;
- size_t cpu;
size_t i;
/* The numa node numbers can be non-contiguous. Ex: 0,1,16,17. */
for (i = 0; i < caps->cells->len; i++) {
@@ -1372,12 +1385,14 @@ virCapabilitiesHostNUMAGetCellCpus(virCapsHostNUMA *caps,
cell = NULL;
}
- for (cpu = 0; cell && cpu < cell->ncpus; cpu++) {
- if (virBitmapSetBit(cpumask, cell->cpus[cpu].id) < 0) {
+ for (i = 0; cell && i < cell->cpus->len; i++) {
+ virCapsHostNUMACellCPU *cpu = &g_array_index(cell->cpus, virCapsHostNUMACellCPU, i);
+
+ if (virBitmapSetBit(cpumask, cpu->id) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Cpu '%u' in node '%zu' is out of range "
+ _("CPU '%u' in node '%zu' is out of range "
"of the provided bitmap"),
- cell->cpus[cpu].id, node);
+ cpu->id, node);
return -1;
}
}
@@ -1532,10 +1547,9 @@ static int
virCapabilitiesHostNUMAInitFake(virCapsHostNUMA *caps)
{
virNodeInfo nodeinfo;
- virCapsHostNUMACellCPU *cpus;
int ncpus;
int n, s, c, t;
- int id, cid;
+ int id;
int onlinecpus G_GNUC_UNUSED;
bool tmp;
@@ -1547,10 +1561,10 @@ virCapabilitiesHostNUMAInitFake(virCapsHostNUMA *caps)
id = 0;
for (n = 0; n < nodeinfo.nodes; n++) {
+ g_autoptr(GArray) cpus = NULL;
int nodecpus = nodeinfo.sockets * nodeinfo.cores * nodeinfo.threads;
- cid = 0;
- cpus = g_new0(virCapsHostNUMACellCPU, nodecpus);
+ cpus = virCapsHostNUMACellCPUNew(nodecpus);
for (s = 0; s < nodeinfo.sockets; s++) {
for (c = 0; c < nodeinfo.cores; c++) {
@@ -1559,15 +1573,17 @@ virCapabilitiesHostNUMAInitFake(virCapsHostNUMA *caps)
ignore_value(virBitmapSetBit(siblings, id + t));
for (t = 0; t < nodeinfo.threads; t++) {
+ virCapsHostNUMACellCPU cpu = { 0 };
+
if (virHostCPUGetOnline(id, &tmp) < 0)
goto error;
if (tmp) {
- cpus[cid].id = id;
- cpus[cid].die_id = 0;
- cpus[cid].socket_id = s;
- cpus[cid].core_id = c;
- cpus[cid].siblings = virBitmapNewCopy(siblings);
- cid++;
+ cpu.id = id;
+ cpu.die_id = 0;
+ cpu.socket_id = s;
+ cpu.core_id = c;
+ cpu.siblings = virBitmapNewCopy(siblings);
+ g_array_append_val(cpus, cpu);
}
id++;
@@ -1577,7 +1593,7 @@ virCapabilitiesHostNUMAInitFake(virCapsHostNUMA *caps)
virCapabilitiesHostNUMAAddCell(caps, 0,
nodeinfo.memory,
- cid, &cpus,
+ &cpus,
0, NULL,
0, NULL);
}
@@ -1585,9 +1601,6 @@ virCapabilitiesHostNUMAInitFake(virCapsHostNUMA *caps)
return 0;
error:
- for (; cid >= 0; cid--)
- virBitmapFree(cpus[cid].siblings);
- VIR_FREE(cpus);
return -1;
}
@@ -1596,9 +1609,7 @@ static int
virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps)
{
int n;
- virCapsHostNUMACellCPU *cpus = NULL;
int ret = -1;
- int ncpus = 0;
int max_node;
if ((max_node = virNumaGetMaxNode()) < 0)
@@ -1606,12 +1617,13 @@ virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps)
for (n = 0; n <= max_node; n++) {
g_autoptr(virBitmap) cpumap = NULL;
+ int ncpus;
+ g_autoptr(GArray) cpus = NULL;
g_autofree virCapsHostNUMACellSiblingInfo *siblings = NULL;
int nsiblings = 0;
g_autofree virCapsHostNUMACellPageInfo *pageinfo = NULL;
int npageinfo;
unsigned long long memory;
- int cpu;
size_t i;
if ((ncpus = virNumaGetNodeCPUs(n, &cpumap)) < 0) {
@@ -1621,13 +1633,16 @@ virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps)
goto cleanup;
}
- cpus = g_new0(virCapsHostNUMACellCPU, ncpus);
- cpu = 0;
+ cpus = virCapsHostNUMACellCPUNew(ncpus);
for (i = 0; i < virBitmapSize(cpumap); i++) {
if (virBitmapIsBitSet(cpumap, i)) {
- if (virCapabilitiesFillCPUInfo(i, cpus + cpu++) < 0)
+ virCapsHostNUMACellCPU cpu = { 0 };
+
+ if (virCapabilitiesFillCPUInfo(i, &cpu) < 0)
goto cleanup;
+
+ g_array_append_val(cpus, cpu);
}
}
@@ -1642,7 +1657,7 @@ virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps)
memory >>= 10;
virCapabilitiesHostNUMAAddCell(caps, n, memory,
- ncpus, &cpus,
+ &cpus,
nsiblings, &siblings,
npageinfo, &pageinfo);
}
@@ -1650,8 +1665,6 @@ virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps)
ret = 0;
cleanup:
- virCapabilitiesClearHostNUMACellCPUTopology(cpus, ncpus);
- VIR_FREE(cpus);
return ret;
}
diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
index ba863447c0..f1b4a8e800 100644
--- a/src/conf/capabilities.h
+++ b/src/conf/capabilities.h
@@ -106,9 +106,8 @@ struct _virCapsHostNUMACellPageInfo {
struct _virCapsHostNUMACell {
int num;
- int ncpus;
unsigned long long mem; /* in kibibytes */
- virCapsHostNUMACellCPU *cpus;
+ GArray *cpus; /* virCapsHostNUMACellCPU */
int nsiblings;
virCapsHostNUMACellSiblingInfo *siblings;
int npageinfo;
@@ -253,8 +252,7 @@ void
virCapabilitiesHostNUMAAddCell(virCapsHostNUMA *caps,
int num,
unsigned long long mem,
- int ncpus,
- virCapsHostNUMACellCPU **cpus,
+ GArray **cpus,
int nsiblings,
virCapsHostNUMACellSiblingInfo **siblings,
int npageinfo,
@@ -320,9 +318,8 @@ virCapabilitiesDomainSupported(virCaps *caps,
int domaintype);
-void
-virCapabilitiesClearHostNUMACellCPUTopology(virCapsHostNUMACellCPU *cpu,
- size_t ncpus);
+GArray *
+virCapsHostNUMACellCPUNew(size_t prealloc);
char *
virCapabilitiesFormatXML(virCaps *caps);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 1b12c49018..036c890f32 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -59,7 +59,6 @@ virCapabilitiesAddHostFeature;
virCapabilitiesAddHostMigrateTransport;
virCapabilitiesAddStoragePool;
virCapabilitiesAllocMachines;
-virCapabilitiesClearHostNUMACellCPUTopology;
virCapabilitiesDomainDataLookup;
virCapabilitiesDomainSupported;
virCapabilitiesFormatXML;
@@ -78,6 +77,7 @@ virCapabilitiesInitCaches;
virCapabilitiesInitPages;
virCapabilitiesNew;
virCapabilitiesSetNetPrefix;
+virCapsHostNUMACellCPUNew;
# conf/checkpoint_conf.h
diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c
index a64a9266c4..96edecf4b3 100644
--- a/src/libxl/libxl_capabilities.c
+++ b/src/libxl/libxl_capabilities.c
@@ -313,13 +313,20 @@ libxlCapsInitNuma(libxl_ctx *ctx, virCaps *caps)
caps->host.numa = virCapabilitiesHostNUMANew();
for (i = 0; i < nr_nodes; i++) {
+ g_autoptr(GArray) gcpus = NULL;
+ size_t j;
+
if (numa_info[i].size == LIBXL_NUMAINFO_INVALID_ENTRY)
continue;
+ gcpus = virCapsHostNUMACellCPUNew(nr_cpus_node[i]);
+
+ for (j = 0; j < nr_cpus_node[i]; j++) {
+ g_array_append_val(gcpus, cpus[i][j]);
+ }
+
nr_siblings = numa_info[i].num_dists;
if (nr_siblings) {
- size_t j;
-
siblings = g_new0(virCapsHostNUMACellSiblingInfo, nr_siblings);
for (j = 0; j < nr_siblings; j++) {
@@ -330,20 +337,15 @@ libxlCapsInitNuma(libxl_ctx *ctx, virCaps *caps)
virCapabilitiesHostNUMAAddCell(caps->host.numa, i,
numa_info[i].size / 1024,
- nr_cpus_node[i], &cpus[i],
+ &gcpus,
nr_siblings, &siblings,
0, NULL);
-
- /* This is safe, as the CPU list is now stored in the NUMA cell */
- cpus[i] = NULL;
}
ret = 0;
cleanup:
if (ret != 0) {
- for (i = 0; cpus && i < nr_nodes; i++)
- VIR_FREE(cpus[i]);
if (caps->host.numa) {
virCapabilitiesHostNUMAUnref(caps->host.numa);
caps->host.numa = NULL;
@@ -351,6 +353,8 @@ libxlCapsInitNuma(libxl_ctx *ctx, virCaps *caps)
VIR_FREE(siblings);
}
+ for (i = 0; cpus && i < nr_nodes; i++)
+ VIR_FREE(cpus[i]);
VIR_FREE(cpus);
VIR_FREE(nr_cpus_node);
libxl_cputopology_list_free(cpu_topo, nr_cpus);
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index ea5a5005e7..fb82eeb2a4 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -307,15 +307,17 @@ testBuildCapabilities(virConnectPtr conn)
caps->host.numa = virCapabilitiesHostNUMANew();
for (i = 0; i < privconn->numCells; i++) {
- virCapsHostNUMACellCPU *cpu_cells;
+ g_autoptr(GArray) cpus = NULL;
virCapsHostNUMACellPageInfo *pages;
size_t nPages = caps->host.nPagesSize - 1;
- cpu_cells = g_new0(virCapsHostNUMACellCPU, privconn->cells[i].numCpus);
+ cpus = virCapsHostNUMACellCPUNew(privconn->cells[i].numCpus);
+
pages = g_new0(virCapsHostNUMACellPageInfo, nPages);
- memcpy(cpu_cells, privconn->cells[i].cpus,
- sizeof(*cpu_cells) * privconn->cells[i].numCpus);
+ for (j = 0; j < privconn->cells[i].numCpus; j++) {
+ g_array_append_val(cpus, privconn->cells[i].cpus[j]);
+ }
if (i == 1)
pages[0].size = caps->host.pagesSize[1];
@@ -329,7 +331,7 @@ testBuildCapabilities(virConnectPtr conn)
virCapabilitiesHostNUMAAddCell(caps->host.numa,
i, privconn->cells[i].mem,
- privconn->cells[i].numCpus, &cpu_cells,
+ &cpus,
0, NULL,
nPages, &pages);
}
diff --git a/tests/testutils.c b/tests/testutils.c
index d6b7c2a580..89c4ed464d 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -916,30 +916,33 @@ virCapsHostNUMA *
virTestCapsBuildNUMATopology(int seq)
{
g_autoptr(virCapsHostNUMA) caps = virCapabilitiesHostNUMANew();
- virCapsHostNUMACellCPU *cell_cpus = NULL;
int core_id, cell_id;
int id;
id = 0;
for (cell_id = 0; cell_id < MAX_CELLS; cell_id++) {
- cell_cpus = g_new0(virCapsHostNUMACellCPU, MAX_CPUS_IN_CELL);
+ g_autoptr(GArray) cpus = NULL;
+
+ cpus = virCapsHostNUMACellCPUNew(MAX_CPUS_IN_CELL);
for (core_id = 0; core_id < MAX_CPUS_IN_CELL; core_id++) {
- cell_cpus[core_id].id = id + core_id;
- cell_cpus[core_id].socket_id = cell_id + seq;
- cell_cpus[core_id].core_id = id + core_id;
- cell_cpus[core_id].siblings = virBitmapNew(MAX_CPUS_IN_CELL);
- ignore_value(virBitmapSetBit(cell_cpus[core_id].siblings, id));
+ virCapsHostNUMACellCPU cpu = { 0 };
+
+ cpu.id = id + core_id;
+ cpu.socket_id = cell_id + seq;
+ cpu.core_id = id + core_id;
+ cpu.siblings = virBitmapNew(MAX_CPUS_IN_CELL);
+ ignore_value(virBitmapSetBit(cpu.siblings, id));
+
+ g_array_append_val(cpus, cpu);
}
id++;
virCapabilitiesHostNUMAAddCell(caps, cell_id + seq,
MAX_MEM_IN_CELL,
- MAX_CPUS_IN_CELL, &cell_cpus,
+ &cpus,
0, NULL,
0, NULL);
-
- cell_cpus = NULL;
}
return g_steal_pointer(&caps);
--
2.26.3
More information about the libvir-list
mailing list