There's an if-else statement in libxlCapsInitNuma() that can really be just two standalone if()-s. Writing it as such helps with code readability. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/libxl/libxl_capabilities.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index ea4f370a6d..a64a9266c4 100644 --- a/src/libxl/libxl_capabilities.c +++ b/src/libxl/libxl_capabilities.c @@ -260,13 +260,13 @@ libxlCapsInitNuma(libxl_ctx *ctx, virCaps *caps) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("libxl_get_numainfo failed")); goto cleanup; - } else { - cpu_topo = libxl_get_cpu_topology(ctx, &nr_cpus); - if (cpu_topo == NULL || nr_cpus == 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("libxl_get_cpu_topology failed")); - goto cleanup; - } + } + + cpu_topo = libxl_get_cpu_topology(ctx, &nr_cpus); + if (cpu_topo == NULL || nr_cpus == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("libxl_get_cpu_topology failed")); + goto cleanup; } cpus = g_new0(virCapsHostNUMACellCPU *, nr_nodes); -- 2.26.3

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(a)redhat.com&gt; --- 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