[libvirt] [PATCH v2 0/2] Handle non-sequential NUMA node numbers
v2 *Add test case as suggested by Daniel *Minor change in comments Pradipta Kr. Banerjee (2): Handle non-sequential NUMA node numbers vircapstest: Introduce virCapabilitiesGetCpusForNodemask test src/conf/capabilities.c | 12 ++++- src/qemu/qemu_driver.c | 5 +- src/qemu/qemu_process.c | 2 +- tests/Makefile.am | 5 ++ tests/vircapstest.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 148 insertions(+), 5 deletions(-) create mode 100644 tests/vircapstest.c -- 1.8.5.3
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@linux.vnet.ibm.com> Signed-off-by: Pradipta Kr. Banerjee <bpradip@in.ibm.com> --- 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
On Sat, Feb 08, 2014 at 12:21:39PM +0530, Pradipta Kr. Banerjee wrote:
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@linux.vnet.ibm.com> Signed-off-by: Pradipta Kr. Banerjee <bpradip@in.ibm.com> --- src/conf/capabilities.c | 12 ++++++++++-- src/qemu/qemu_driver.c | 5 +++-- src/qemu/qemu_process.c | 2 +- 3 files changed, 14 insertions(+), 5 deletions(-)
ACK and will push shortly. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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@linux.vnet.ibm.com> Signed-off-by: Pradipta Kr. Banerjee <bpradip@in.ibm.com> --- 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
On Sat, Feb 08, 2014 at 12:21:40PM +0530, Pradipta Kr. Banerjee wrote:
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@linux.vnet.ibm.com> Signed-off-by: Pradipta Kr. Banerjee <bpradip@in.ibm.com> --- 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)
nit-pick: inconsistent indentation
diff --git a/tests/vircapstest.c b/tests/vircapstest.c new file mode 100644 index 0000000..dab8f3b --- /dev/null +++ b/tests/vircapstest.c +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++;
Indentation off by 4
+ + 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
We prefer /* */ over // for comments
+ 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
ACK, I'll fix style nitpicks when pushing. Thanks for taking the time to write the test case. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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 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@linux.vnet.ibm.com> Signed-off-by: Pradipta Kr. Banerjee <bpradip@in.ibm.com> --- 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;
^^^^ This will need to be initialized to NULL too
+ 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;
'cell_cpus' is allocated here
+ + 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;
If we jump to error here, it'll be leaked, as would '.siblings' for each that failed before.
+ 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;
If we jump to error here it'll be leaked
+ + cell_cpus = NULL; + } + + return caps; + +error:
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.
+ 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)
On 02/12/2014 12:43 PM, John Ferlan wrote:
Coverity has complained this morning about this change... see below.
Please send a patch to resolve.
A patch has been sent this morning, please review: https://www.redhat.com/archives/libvir-list/2014-February/msg00660.html
participants (4)
-
Daniel P. Berrange -
John Ferlan -
Ján Tomko -
Pradipta Kr. Banerjee