On some platforms like IBM PowerNV the NUMA node numbers can be non-sequential. For eg. numactl --hardware o/p from such a machine looks as given below node distances: node 0 1 16 17 0: 10 40 40 40 1: 40 10 40 40 16: 40 40 10 40 17: 40 40 40 10 The NUMA nodes are 0,1,16,17 Libvirt uses sequential index as NUMA node numbers and this can result in crash or incorrect results. Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com&gt; Signed-off-by: Pradipta Kr. Banerjee <bpradip(a)in.ibm.com&gt; --- src/conf/capabilities.c | 12 ++++++++++-- src/qemu/qemu_driver.c | 5 +++-- src/qemu/qemu_process.c | 2 +- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 726ec3f..78f65cb 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -1000,10 +1000,18 @@ virCapabilitiesGetCpusForNode(virCapsPtr caps, size_t node, virBitmapPtr cpumask) { - virCapsHostNUMACellPtr cell = caps->host.numaCell[node]; + virCapsHostNUMACellPtr cell = NULL; size_t cpu; + size_t index; + /* The numa node numbers can be non-contiguous. Ex: 0,1,16,17. */ + for (index = 0; index < caps->host.nnumaCell; index++) { + if (caps->host.numaCell[index]->num == node) { + cell = caps->host.numaCell[index]; + break; + } + } - for (cpu = 0; cpu < cell->ncpus; cpu++) { + for (cpu = 0; cell && cpu < cell->ncpus; cpu++) { if (virBitmapSetBit(cpumask, cell->cpus[cpu].id) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cpu '%u' in node '%zu' is out of range " diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 944facb..cb63b60 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8566,12 +8566,13 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm, for (i = 0; i < caps->host.nnumaCell; i++) { bool result; - if (virBitmapGetBit(nodeset, i, &result) < 0) { + virCapsHostNUMACellPtr cell = caps->host.numaCell[i]; + if (virBitmapGetBit(nodeset, cell->num, &result) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to get cpuset bit values")); goto cleanup; } - if (result && (virBitmapSetBit(temp_nodeset, i) < 0)) { + if (result && (virBitmapSetBit(temp_nodeset, cell->num) < 0)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to set temporary cpuset bit values")); goto cleanup; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 8bcd98e..c28f84d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2002,7 +2002,7 @@ qemuPrepareCpumap(virQEMUDriverPtr driver, size_t j; int cur_ncpus = caps->host.numaCell[i]->ncpus; bool result; - if (virBitmapGetBit(nodemask, i, &result) < 0) { + if (virBitmapGetBit(nodemask, caps->host.numaCell[i]->num, &result) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to convert nodeset to cpuset")); virBitmapFree(cpumap); -- 1.8.5.3

This test creates a Fake NUMA topology with non-sequential cell ids to check if libvirt properly handles the same Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com&gt; Signed-off-by: Pradipta Kr. Banerjee <bpradip(a)in.ibm.com&gt; --- tests/Makefile.am | 5 ++ tests/vircapstest.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+) diff --git a/tests/Makefile.am b/tests/Makefile.am index eb96f38..bb50ac5 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -144,6 +144,7 @@ test_programs = virshtest sockettest \ virstoragetest \ virnetdevbandwidthtest \ virkmodtest \ + vircapstest \ $(NULL) if WITH_REMOTE @@ -672,6 +673,10 @@ virkmodtest_SOURCES = \ virkmodtest.c testutils.h testutils.c virkmodtest_LDADD = $(LDADDS) +vircapstest_SOURCES = \ + vircapstest.c testutils.h testutils.c +vircapstest_LDADD = $(LDADDS) + if WITH_LIBVIRTD libvirtdconftest_SOURCES = \ libvirtdconftest.c testutils.h testutils.c \ diff --git a/tests/vircapstest.c b/tests/vircapstest.c new file mode 100644 index 0000000..dab8f3b --- /dev/null +++ b/tests/vircapstest.c @@ -0,0 +1,129 @@ +/* + * Copyright (C) IBM Corp 2014 + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>;. + * + */ + +#include <config.h> +#include <stdlib.h> + +#include "testutils.h" +#include "capabilities.h" +#include "virbitmap.h" + + +#define VIR_FROM_THIS VIR_FROM_NONE + +#define MAX_CELLS 4 +#define MAX_CPUS_IN_CELL 2 +#define MAX_MEM_IN_CELL 2097152 + + +/* + * Build NUMA Toplogy with cell id starting from (0 + seq) + * for testing +*/ +static virCapsPtr +buildNUMATopology(int seq) +{ + virCapsPtr caps; + virCapsHostNUMACellCPUPtr cell_cpus; + int core_id, cell_id; + int id; + + if ((caps = virCapabilitiesNew(VIR_ARCH_X86_64, 0, 0)) == NULL) + goto error; + + id = 0; + for (cell_id = 0; cell_id < MAX_CELLS; cell_id++) { + if (VIR_ALLOC_N(cell_cpus, MAX_CPUS_IN_CELL) < 0) + goto error; + + 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; + if (!(cell_cpus[core_id].siblings = + virBitmapNew(MAX_CPUS_IN_CELL))) + goto error; + ignore_value(virBitmapSetBit(cell_cpus[core_id].siblings, id)); + } + id++; + + if (virCapabilitiesAddHostNUMACell(caps, cell_id + seq, + MAX_CPUS_IN_CELL, + MAX_MEM_IN_CELL, + cell_cpus) < 0) + goto error; + + cell_cpus = NULL; + } + + return caps; + +error: + virObjectUnref(caps); + return NULL; + +} + + +static int +test_virCapabilitiesGetCpusForNodemask(const void *data ATTRIBUTE_UNUSED) +{ + const char *nodestr = "3,4,5,6"; + virBitmapPtr nodemask = NULL; + virBitmapPtr cpumap = NULL; + virCapsPtr caps; + int mask_size = 8; + int ret = -1; + + //Build a NUMA topology with cell_id (NUMA node id + //being 3(0 + 3),4(1 + 3), 5 and 6 + if (!(caps = buildNUMATopology(3))) + goto error; + + if (virBitmapParse(nodestr, 0, &nodemask, mask_size) < 0) + goto error; + + if (!(cpumap = virCapabilitiesGetCpusForNodemask(caps, nodemask))) + goto error; + + ret = 0; + +error: + virBitmapFree(nodemask); + virBitmapFree(cpumap); + return ret; + +} + + +static int +mymain(void) +{ + int ret = 0; + + if (virtTestRun("test_virCapabilitiesGetCpusForNodemask", + test_virCapabilitiesGetCpusForNodemask, NULL) < 0) + ret = -1; + + return ret; + + +} + +VIRT_TEST_MAIN(mymain) -- 1.8.5.3

Coverity has complained this morning about this change... see below. Please send a patch to resolve. On 02/08/2014 01:51 AM, Pradipta Kr. Banerjee wrote: ^^^^ This will need to be initialized to NULL too 'cell_cpus' is allocated here If we jump to error here, it'll be leaked, as would '.siblings' for each that failed before. If we jump to error here it'll be leaked We cannot just VIR_FREE() here since I see the virBitmapNew() above for the array element. So it seems there needs to be a free routine called.