[libvirt] [PATCH v4 0/3] add nodeset check in numatune
when setting elements memnode and nodeset in attribute numatune more than the host nodes in XML file, VM boot should fail. so add check for that. Chen Fan (3): bitmap: add virBitmapLastSetBit for finding the last bit position of bitmap numatune: add check for numatune nodeset range virnuma: use virNumaNodesetIsAvailable checking nodeset in virNumaSetupMemoryPolicy src/conf/numatune_conf.c | 28 ++++++++++ src/conf/numatune_conf.h | 1 + src/libvirt_private.syms | 2 + src/qemu/qemu_command.c | 4 ++ src/util/virbitmap.c | 45 ++++++++++++++++ src/util/virbitmap.h | 3 ++ src/util/virnuma.c | 61 +++++++++++++++------- src/util/virnuma.h | 1 + ...rgv-numatune-static-nodeset-exceed-hostnode.xml | 36 +++++++++++++ tests/qemuxml2argvmock.c | 9 ++++ tests/qemuxml2argvtest.c | 1 + tests/virbitmaptest.c | 13 ++++- 12 files changed, 184 insertions(+), 20 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml -- 1.9.3
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/libvirt_private.syms | 1 + src/util/virbitmap.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ src/util/virbitmap.h | 3 +++ tests/virbitmaptest.c | 13 ++++++++++++- 4 files changed, 61 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d63a8f0..1e2bc0a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1011,6 +1011,7 @@ virBitmapFree; virBitmapGetBit; virBitmapIsAllClear; virBitmapIsAllSet; +virBitmapLastSetBit; virBitmapNew; virBitmapNewCopy; virBitmapNewData; diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index b6bd074..3e7269e 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -651,6 +651,51 @@ virBitmapNextSetBit(virBitmapPtr bitmap, ssize_t pos) } /** + * virBitmapLastSetBit: + * @bitmap: the bitmap + * + * Search for the last set bit in bitmap @bitmap. + * + * Returns the position of the found bit, or -1 if no bit found. + */ +ssize_t +virBitmapLastSetBit(virBitmapPtr bitmap) +{ + ssize_t i; + int unusedBits; + ssize_t sz; + unsigned long bits; + + unusedBits = bitmap->map_len * VIR_BITMAP_BITS_PER_UNIT - bitmap->max_bit; + + sz = bitmap->map_len - 1; + if (unusedBits > 0) { + bits = bitmap->map[sz] & (VIR_BITMAP_BIT(VIR_BITMAP_BITS_PER_UNIT - unusedBits) - 1); + if (bits != 0) + goto found; + + sz--; + } + + for (; sz >= 0; sz--) { + bits = bitmap->map[sz]; + if (bits != 0) + goto found; + } + + if (bits == 0) + return -1; + +found: + for (i = VIR_BITMAP_BITS_PER_UNIT - 1; i >= 0; i--) { + if (bits & 1UL << i) + return i + sz * VIR_BITMAP_BITS_PER_UNIT; + } + + return -1; +} + +/** * virBitmapNextClearBit: * @bitmap: the bitmap * @pos: the position after which to search for a clear bit diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index 4493cc9..565264c 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -105,6 +105,9 @@ bool virBitmapIsAllClear(virBitmapPtr bitmap) ssize_t virBitmapNextSetBit(virBitmapPtr bitmap, ssize_t pos) ATTRIBUTE_NONNULL(1); +ssize_t virBitmapLastSetBit(virBitmapPtr bitmap) + ATTRIBUTE_NONNULL(1); + ssize_t virBitmapNextClearBit(virBitmapPtr bitmap, ssize_t pos) ATTRIBUTE_NONNULL(1); diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c index ea832ad..ac5f298 100644 --- a/tests/virbitmaptest.c +++ b/tests/virbitmaptest.c @@ -171,7 +171,7 @@ test3(const void *data ATTRIBUTE_UNUSED) return ret; } -/* test for virBitmapNextSetBit, virBitmapNextClearBit */ +/* test for virBitmapNextSetBit, virBitmapLastSetBit, virBitmapNextClearBit */ static int test4(const void *data ATTRIBUTE_UNUSED) { @@ -200,6 +200,9 @@ test4(const void *data ATTRIBUTE_UNUSED) if (virBitmapNextSetBit(bitmap, -1) != -1) goto error; + if (virBitmapLastSetBit(bitmap) != -1) + goto error; + for (i = 0; i < size; i++) { if (virBitmapNextClearBit(bitmap, i - 1) != i) goto error; @@ -232,6 +235,11 @@ test4(const void *data ATTRIBUTE_UNUSED) if (virBitmapNextSetBit(bitmap, i) != -1) goto error; + j = sizeof(bitsPos)/sizeof(int) - 1; + + if (virBitmapLastSetBit(bitmap) != bitsPos[j]) + goto error; + j = 0; i = -1; @@ -255,6 +263,9 @@ test4(const void *data ATTRIBUTE_UNUSED) if (virBitmapNextSetBit(bitmap, i) != -1) goto error; + if (virBitmapLastSetBit(bitmap) != size - 1) + goto error; + if (virBitmapNextClearBit(bitmap, -1) != -1) goto error; -- 1.9.3
On Thu, Oct 30, 2014 at 01:44:17PM +0800, Chen Fan wrote:
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/libvirt_private.syms | 1 + src/util/virbitmap.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ src/util/virbitmap.h | 3 +++ tests/virbitmaptest.c | 13 ++++++++++++- 4 files changed, 61 insertions(+), 1 deletion(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d63a8f0..1e2bc0a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1011,6 +1011,7 @@ virBitmapFree; virBitmapGetBit; virBitmapIsAllClear; virBitmapIsAllSet; +virBitmapLastSetBit; virBitmapNew; virBitmapNewCopy; virBitmapNewData; diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index b6bd074..3e7269e 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -651,6 +651,51 @@ virBitmapNextSetBit(virBitmapPtr bitmap, ssize_t pos) }
/** + * virBitmapLastSetBit: + * @bitmap: the bitmap + * + * Search for the last set bit in bitmap @bitmap. + * + * Returns the position of the found bit, or -1 if no bit found.
s/found/is set/
+ */ +ssize_t +virBitmapLastSetBit(virBitmapPtr bitmap) +{ + ssize_t i; + int unusedBits; + ssize_t sz; + unsigned long bits; + + unusedBits = bitmap->map_len * VIR_BITMAP_BITS_PER_UNIT - bitmap->max_bit; + + sz = bitmap->map_len - 1; + if (unusedBits > 0) { + bits = bitmap->map[sz] & (VIR_BITMAP_BIT(VIR_BITMAP_BITS_PER_UNIT - unusedBits) - 1); + if (bits != 0) + goto found; + + sz--; + } + + for (; sz >= 0; sz--) { + bits = bitmap->map[sz]; + if (bits != 0) + goto found; + } + + if (bits == 0) + return -1; + +found:
Missing space before label. "make syntax-check" would tell you that instead of me ;)
On Mon, 2014-11-03 at 14:18 +0100, Martin Kletzander wrote:
On Thu, Oct 30, 2014 at 01:44:17PM +0800, Chen Fan wrote:
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/libvirt_private.syms | 1 + src/util/virbitmap.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ src/util/virbitmap.h | 3 +++ tests/virbitmaptest.c | 13 ++++++++++++- 4 files changed, 61 insertions(+), 1 deletion(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d63a8f0..1e2bc0a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1011,6 +1011,7 @@ virBitmapFree; virBitmapGetBit; virBitmapIsAllClear; virBitmapIsAllSet; +virBitmapLastSetBit; virBitmapNew; virBitmapNewCopy; virBitmapNewData; diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index b6bd074..3e7269e 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -651,6 +651,51 @@ virBitmapNextSetBit(virBitmapPtr bitmap, ssize_t pos) }
/** + * virBitmapLastSetBit: + * @bitmap: the bitmap + * + * Search for the last set bit in bitmap @bitmap. + * + * Returns the position of the found bit, or -1 if no bit found.
s/found/is set/
+ */ +ssize_t +virBitmapLastSetBit(virBitmapPtr bitmap) +{ + ssize_t i; + int unusedBits; + ssize_t sz; + unsigned long bits; + + unusedBits = bitmap->map_len * VIR_BITMAP_BITS_PER_UNIT - bitmap->max_bit; + + sz = bitmap->map_len - 1; + if (unusedBits > 0) { + bits = bitmap->map[sz] & (VIR_BITMAP_BIT(VIR_BITMAP_BITS_PER_UNIT - unusedBits) - 1); + if (bits != 0) + goto found; + + sz--; + } + + for (; sz >= 0; sz--) { + bits = bitmap->map[sz]; + if (bits != 0) + goto found; + } + + if (bits == 0) + return -1; + +found:
Missing space before label. "make syntax-check" would tell you that instead of me ;)
Thanks for your remind. I will do that. Chen
There was no check for 'nodeset' attribute in numatune-related elements. This patch adds validation that any nodeset specified does not exceed maximum host node. Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/conf/numatune_conf.c | 28 ++++++++++++++++ src/conf/numatune_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 4 +++ src/util/virnuma.c | 38 ++++++++++++++++++++++ src/util/virnuma.h | 1 + ...rgv-numatune-static-nodeset-exceed-hostnode.xml | 36 ++++++++++++++++++++ tests/qemuxml2argvmock.c | 9 +++++ tests/qemuxml2argvtest.c | 1 + 9 files changed, 119 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml diff --git a/src/conf/numatune_conf.c b/src/conf/numatune_conf.c index 21d9a64..6986739 100644 --- a/src/conf/numatune_conf.c +++ b/src/conf/numatune_conf.c @@ -612,3 +612,31 @@ virDomainNumatuneHasPerNodeBinding(virDomainNumatunePtr numatune) return false; } + +int +virDomainNumatuneSpecifiedMaxNode(virDomainNumatunePtr numatune) +{ + int ret = -1; + virBitmapPtr nodemask = NULL; + size_t i; + int bit; + + if (!numatune) + return ret; + + nodemask = virDomainNumatuneGetNodeset(numatune, NULL, -1); + if (nodemask) + ret = virBitmapLastSetBit(nodemask); + + for (i = 0; i < numatune->nmem_nodes; i++) { + nodemask = numatune->mem_nodes[i].nodeset; + if (!nodemask) + continue; + + bit = virBitmapLastSetBit(nodemask); + if (bit > ret) + ret = bit; + } + + return ret; +} diff --git a/src/conf/numatune_conf.h b/src/conf/numatune_conf.h index 5254629..15dc0d6 100644 --- a/src/conf/numatune_conf.h +++ b/src/conf/numatune_conf.h @@ -102,4 +102,5 @@ bool virDomainNumatuneHasPlacementAuto(virDomainNumatunePtr numatune); bool virDomainNumatuneHasPerNodeBinding(virDomainNumatunePtr numatune); +int virDomainNumatuneSpecifiedMaxNode(virDomainNumatunePtr numatune); #endif /* __NUMATUNE_CONF_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1e2bc0a..4a30ad7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1729,6 +1729,7 @@ virNumaGetPageInfo; virNumaGetPages; virNumaIsAvailable; virNumaNodeIsAvailable; +virNumaNodesetIsAvailable; virNumaSetPagePoolSize; virNumaSetupMemoryPolicy; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2e5af4f..d2c5f0c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -53,6 +53,7 @@ #include "virstoragefile.h" #include "virtpm.h" #include "virscsi.h" +#include "virnuma.h" #if defined(__linux__) # include <linux/capability.h> #endif @@ -6663,6 +6664,9 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, goto cleanup; } + if (!virNumaNodesetIsAvailable(def->numatune)) + goto cleanup; + for (i = 0; i < def->mem.nhugepages; i++) { ssize_t next_bit, pos = 0; diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 690615f..4188ef5 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -165,6 +165,33 @@ virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, return ret; } +bool +virNumaNodesetIsAvailable(virDomainNumatunePtr numatune) +{ + int maxnode; + int bit; + + if (!numatune) + return true; + + bit = virDomainNumatuneSpecifiedMaxNode(numatune); + if (bit == -1) + return true; + + if ((maxnode = virNumaGetMaxNode()) < 0) + return false; + + maxnode = maxnode < NUMA_NUM_NODES ? maxnode : NUMA_NUM_NODES; + if (bit > maxnode) + goto error; + + return true; + + error: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("NUMA node %d is out of range"), bit); + return false; +} bool virNumaIsAvailable(void) @@ -330,6 +357,17 @@ virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, return 0; } +bool +virNumaNodesetIsAvailable(virDomainNumatunePtr numatune) +{ + if (virDomainNumatuneSpecifiedMaxNode(numatune) != -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("libvirt is compiled without NUMA tuning support")); + return false; + } + + return true; +} bool virNumaIsAvailable(void) diff --git a/src/util/virnuma.h b/src/util/virnuma.h index 04b6b8c..5bb37b8 100644 --- a/src/util/virnuma.h +++ b/src/util/virnuma.h @@ -34,6 +34,7 @@ char *virNumaGetAutoPlacementAdvice(unsigned short vcups, int virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, virBitmapPtr nodemask); +bool virNumaNodesetIsAvailable(virDomainNumatunePtr numatune); bool virNumaIsAvailable(void); int virNumaGetMaxNode(void); bool virNumaNodeIsAvailable(int node); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml b/tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml new file mode 100644 index 0000000..b7564fe --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml @@ -0,0 +1,36 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>4</vcpu> + <numatune> + <memnode cellid='0' mode='strict' nodeset='0-8'/> + <memnode cellid='1' mode='strict' nodeset='0'/> + </numatune> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <cpu> + <numa> + <cell id='0' cpus='0-1' memory='524288'/> + <cell id='1' cpus='2-3' memory='524288'/> + </numa> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> + diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index bff3b0f..7218747 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -21,6 +21,7 @@ #include <config.h> #include "internal.h" +#include "virnuma.h" #include <time.h> time_t time(time_t *t) @@ -30,3 +31,11 @@ time_t time(time_t *t) *t = ret; return ret; } + +int +virNumaGetMaxNode(void) +{ + const int maxnodesNum = 7; + + return maxnodesNum; +} diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0e9fab9..bab6d0d 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1250,6 +1250,7 @@ mymain(void) DO_TEST_FAILURE("numatune-memnode-no-memory", NONE); DO_TEST("numatune-auto-nodeset-invalid", NONE); + DO_TEST_FAILURE("numatune-static-nodeset-exceed-hostnode", QEMU_CAPS_OBJECT_MEMORY_RAM); DO_TEST_PARSE_ERROR("numatune-memnode-nocpu", NONE); DO_TEST_PARSE_ERROR("numatune-memnodes-problematic", NONE); DO_TEST("numad", NONE); -- 1.9.3
On Thu, Oct 30, 2014 at 01:44:18PM +0800, Chen Fan wrote:
There was no check for 'nodeset' attribute in numatune-related elements. This patch adds validation that any nodeset specified does not exceed maximum host node.
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/conf/numatune_conf.c | 28 ++++++++++++++++ src/conf/numatune_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 4 +++ src/util/virnuma.c | 38 ++++++++++++++++++++++ src/util/virnuma.h | 1 + ...rgv-numatune-static-nodeset-exceed-hostnode.xml | 36 ++++++++++++++++++++ tests/qemuxml2argvmock.c | 9 +++++ tests/qemuxml2argvtest.c | 1 + 9 files changed, 119 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml
diff --git a/src/conf/numatune_conf.c b/src/conf/numatune_conf.c index 21d9a64..6986739 100644 --- a/src/conf/numatune_conf.c +++ b/src/conf/numatune_conf.c @@ -612,3 +612,31 @@ virDomainNumatuneHasPerNodeBinding(virDomainNumatunePtr numatune)
return false; } + +int +virDomainNumatuneSpecifiedMaxNode(virDomainNumatunePtr numatune) +{ + int ret = -1; + virBitmapPtr nodemask = NULL; + size_t i; + int bit; + + if (!numatune) + return ret; + + nodemask = virDomainNumatuneGetNodeset(numatune, NULL, -1); + if (nodemask) + ret = virBitmapLastSetBit(nodemask); + + for (i = 0; i < numatune->nmem_nodes; i++) { + nodemask = numatune->mem_nodes[i].nodeset; + if (!nodemask) + continue; + + bit = virBitmapLastSetBit(nodemask); + if (bit > ret) + ret = bit; + } + + return ret; +} diff --git a/src/conf/numatune_conf.h b/src/conf/numatune_conf.h index 5254629..15dc0d6 100644 --- a/src/conf/numatune_conf.h +++ b/src/conf/numatune_conf.h @@ -102,4 +102,5 @@ bool virDomainNumatuneHasPlacementAuto(virDomainNumatunePtr numatune);
bool virDomainNumatuneHasPerNodeBinding(virDomainNumatunePtr numatune);
+int virDomainNumatuneSpecifiedMaxNode(virDomainNumatunePtr numatune); #endif /* __NUMATUNE_CONF_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1e2bc0a..4a30ad7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1729,6 +1729,7 @@ virNumaGetPageInfo; virNumaGetPages; virNumaIsAvailable; virNumaNodeIsAvailable; +virNumaNodesetIsAvailable; virNumaSetPagePoolSize; virNumaSetupMemoryPolicy;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2e5af4f..d2c5f0c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -53,6 +53,7 @@ #include "virstoragefile.h" #include "virtpm.h" #include "virscsi.h" +#include "virnuma.h" #if defined(__linux__) # include <linux/capability.h> #endif @@ -6663,6 +6664,9 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, goto cleanup; }
+ if (!virNumaNodesetIsAvailable(def->numatune)) + goto cleanup; + for (i = 0; i < def->mem.nhugepages; i++) { ssize_t next_bit, pos = 0;
diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 690615f..4188ef5 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -165,6 +165,33 @@ virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, return ret; }
+bool +virNumaNodesetIsAvailable(virDomainNumatunePtr numatune) +{ + int maxnode; + int bit; + + if (!numatune) + return true; + + bit = virDomainNumatuneSpecifiedMaxNode(numatune); + if (bit == -1)
(bit < 0) would go with the rest of the code, the functions can be then modified to report various kinds of errors more easily.
+ return true; + + if ((maxnode = virNumaGetMaxNode()) < 0) + return false; + + maxnode = maxnode < NUMA_NUM_NODES ? maxnode : NUMA_NUM_NODES; + if (bit > maxnode) + goto error; + + return true; + + error: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("NUMA node %d is out of range"), bit); + return false; +}
bool virNumaIsAvailable(void) @@ -330,6 +357,17 @@ virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, return 0; }
+bool +virNumaNodesetIsAvailable(virDomainNumatunePtr numatune) +{ + if (virDomainNumatuneSpecifiedMaxNode(numatune) != -1) {
similarly " >= 0" here.
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("libvirt is compiled without NUMA tuning support")); + return false; + } + + return true; +}
bool virNumaIsAvailable(void) diff --git a/src/util/virnuma.h b/src/util/virnuma.h index 04b6b8c..5bb37b8 100644 --- a/src/util/virnuma.h +++ b/src/util/virnuma.h @@ -34,6 +34,7 @@ char *virNumaGetAutoPlacementAdvice(unsigned short vcups, int virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, virBitmapPtr nodemask);
+bool virNumaNodesetIsAvailable(virDomainNumatunePtr numatune); bool virNumaIsAvailable(void); int virNumaGetMaxNode(void); bool virNumaNodeIsAvailable(int node); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml b/tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml new file mode 100644 index 0000000..b7564fe --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml @@ -0,0 +1,36 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>4</vcpu> + <numatune> + <memnode cellid='0' mode='strict' nodeset='0-8'/> + <memnode cellid='1' mode='strict' nodeset='0'/> + </numatune> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <cpu> + <numa> + <cell id='0' cpus='0-1' memory='524288'/> + <cell id='1' cpus='2-3' memory='524288'/> + </numa> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> +
Empty line at EOF. "make syntax-check" would find that for you ;)
diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index bff3b0f..7218747 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -21,6 +21,7 @@ #include <config.h>
#include "internal.h" +#include "virnuma.h" #include <time.h>
time_t time(time_t *t) @@ -30,3 +31,11 @@ time_t time(time_t *t) *t = ret; return ret; } + +int +virNumaGetMaxNode(void) +{ + const int maxnodesNum = 7; + + return maxnodesNum; +}
Why not just "return 7;" ???
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0e9fab9..bab6d0d 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1250,6 +1250,7 @@ mymain(void) DO_TEST_FAILURE("numatune-memnode-no-memory", NONE);
DO_TEST("numatune-auto-nodeset-invalid", NONE); + DO_TEST_FAILURE("numatune-static-nodeset-exceed-hostnode", QEMU_CAPS_OBJECT_MEMORY_RAM);
Very long line.
DO_TEST_PARSE_ERROR("numatune-memnode-nocpu", NONE); DO_TEST_PARSE_ERROR("numatune-memnodes-problematic", NONE); DO_TEST("numad", NONE); -- 1.9.3
On Mon, 2014-11-03 at 14:18 +0100, Martin Kletzander wrote:
On Thu, Oct 30, 2014 at 01:44:18PM +0800, Chen Fan wrote:
There was no check for 'nodeset' attribute in numatune-related elements. This patch adds validation that any nodeset specified does not exceed maximum host node.
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/conf/numatune_conf.c | 28 ++++++++++++++++ src/conf/numatune_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 4 +++ src/util/virnuma.c | 38 ++++++++++++++++++++++ src/util/virnuma.h | 1 + ...rgv-numatune-static-nodeset-exceed-hostnode.xml | 36 ++++++++++++++++++++ tests/qemuxml2argvmock.c | 9 +++++ tests/qemuxml2argvtest.c | 1 + 9 files changed, 119 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml
diff --git a/src/conf/numatune_conf.c b/src/conf/numatune_conf.c index 21d9a64..6986739 100644 --- a/src/conf/numatune_conf.c +++ b/src/conf/numatune_conf.c @@ -612,3 +612,31 @@ virDomainNumatuneHasPerNodeBinding(virDomainNumatunePtr numatune)
return false; } + +int +virDomainNumatuneSpecifiedMaxNode(virDomainNumatunePtr numatune) +{ + int ret = -1; + virBitmapPtr nodemask = NULL; + size_t i; + int bit; + + if (!numatune) + return ret; + + nodemask = virDomainNumatuneGetNodeset(numatune, NULL, -1); + if (nodemask) + ret = virBitmapLastSetBit(nodemask); + + for (i = 0; i < numatune->nmem_nodes; i++) { + nodemask = numatune->mem_nodes[i].nodeset; + if (!nodemask) + continue; + + bit = virBitmapLastSetBit(nodemask); + if (bit > ret) + ret = bit; + } + + return ret; +} diff --git a/src/conf/numatune_conf.h b/src/conf/numatune_conf.h index 5254629..15dc0d6 100644 --- a/src/conf/numatune_conf.h +++ b/src/conf/numatune_conf.h @@ -102,4 +102,5 @@ bool virDomainNumatuneHasPlacementAuto(virDomainNumatunePtr numatune);
bool virDomainNumatuneHasPerNodeBinding(virDomainNumatunePtr numatune);
+int virDomainNumatuneSpecifiedMaxNode(virDomainNumatunePtr numatune); #endif /* __NUMATUNE_CONF_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1e2bc0a..4a30ad7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1729,6 +1729,7 @@ virNumaGetPageInfo; virNumaGetPages; virNumaIsAvailable; virNumaNodeIsAvailable; +virNumaNodesetIsAvailable; virNumaSetPagePoolSize; virNumaSetupMemoryPolicy;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2e5af4f..d2c5f0c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -53,6 +53,7 @@ #include "virstoragefile.h" #include "virtpm.h" #include "virscsi.h" +#include "virnuma.h" #if defined(__linux__) # include <linux/capability.h> #endif @@ -6663,6 +6664,9 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, goto cleanup; }
+ if (!virNumaNodesetIsAvailable(def->numatune)) + goto cleanup; + for (i = 0; i < def->mem.nhugepages; i++) { ssize_t next_bit, pos = 0;
diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 690615f..4188ef5 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -165,6 +165,33 @@ virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, return ret; }
+bool +virNumaNodesetIsAvailable(virDomainNumatunePtr numatune) +{ + int maxnode; + int bit; + + if (!numatune) + return true; + + bit = virDomainNumatuneSpecifiedMaxNode(numatune); + if (bit == -1)
(bit < 0) would go with the rest of the code, the functions can be then modified to report various kinds of errors more easily.
+ return true; + + if ((maxnode = virNumaGetMaxNode()) < 0) + return false; + + maxnode = maxnode < NUMA_NUM_NODES ? maxnode : NUMA_NUM_NODES; + if (bit > maxnode) + goto error; + + return true; + + error: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("NUMA node %d is out of range"), bit); + return false; +}
bool virNumaIsAvailable(void) @@ -330,6 +357,17 @@ virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, return 0; }
+bool +virNumaNodesetIsAvailable(virDomainNumatunePtr numatune) +{ + if (virDomainNumatuneSpecifiedMaxNode(numatune) != -1) {
similarly " >= 0" here.
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("libvirt is compiled without NUMA tuning support")); + return false; + } + + return true; +}
bool virNumaIsAvailable(void) diff --git a/src/util/virnuma.h b/src/util/virnuma.h index 04b6b8c..5bb37b8 100644 --- a/src/util/virnuma.h +++ b/src/util/virnuma.h @@ -34,6 +34,7 @@ char *virNumaGetAutoPlacementAdvice(unsigned short vcups, int virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, virBitmapPtr nodemask);
+bool virNumaNodesetIsAvailable(virDomainNumatunePtr numatune); bool virNumaIsAvailable(void); int virNumaGetMaxNode(void); bool virNumaNodeIsAvailable(int node); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml b/tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml new file mode 100644 index 0000000..b7564fe --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml @@ -0,0 +1,36 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>4</vcpu> + <numatune> + <memnode cellid='0' mode='strict' nodeset='0-8'/> + <memnode cellid='1' mode='strict' nodeset='0'/> + </numatune> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <cpu> + <numa> + <cell id='0' cpus='0-1' memory='524288'/> + <cell id='1' cpus='2-3' memory='524288'/> + </numa> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> +
Empty line at EOF. "make syntax-check" would find that for you ;)
diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index bff3b0f..7218747 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -21,6 +21,7 @@ #include <config.h>
#include "internal.h" +#include "virnuma.h" #include <time.h>
time_t time(time_t *t) @@ -30,3 +31,11 @@ time_t time(time_t *t) *t = ret; return ret; } + +int +virNumaGetMaxNode(void) +{ + const int maxnodesNum = 7; + + return maxnodesNum; +}
Why not just "return 7;" ???
I just think magic number may be not proper. Thanks, Chen
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0e9fab9..bab6d0d 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1250,6 +1250,7 @@ mymain(void) DO_TEST_FAILURE("numatune-memnode-no-memory", NONE);
DO_TEST("numatune-auto-nodeset-invalid", NONE); + DO_TEST_FAILURE("numatune-static-nodeset-exceed-hostnode", QEMU_CAPS_OBJECT_MEMORY_RAM);
Very long line.
DO_TEST_PARSE_ERROR("numatune-memnode-nocpu", NONE); DO_TEST_PARSE_ERROR("numatune-memnodes-problematic", NONE); DO_TEST("numad", NONE); -- 1.9.3
On Tue, Nov 04, 2014 at 01:57:52AM +0000, Chen, Fan wrote:
On Mon, 2014-11-03 at 14:18 +0100, Martin Kletzander wrote:
On Thu, Oct 30, 2014 at 01:44:18PM +0800, Chen Fan wrote:
diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index bff3b0f..7218747 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -21,6 +21,7 @@ #include <config.h>
#include "internal.h" +#include "virnuma.h" #include <time.h>
time_t time(time_t *t) @@ -30,3 +31,11 @@ time_t time(time_t *t) *t = ret; return ret; } + +int +virNumaGetMaxNode(void) +{ + const int maxnodesNum = 7; + + return maxnodesNum; +}
Why not just "return 7;" ???
I just think magic number may be not proper.
Probably a matter of taste, I'd use a comment in that case, but proper compiler should optimize it even without that const, so no problem here ;) Martin
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/util/virnuma.c | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-) diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 4188ef5..613a43c 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -95,31 +95,19 @@ virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, int ret = -1; int bit = 0; size_t i; - int maxnode = 0; virBitmapPtr tmp_nodemask = NULL; + if (!virNumaNodesetIsAvailable(numatune)) + return -1; + tmp_nodemask = virDomainNumatuneGetNodeset(numatune, nodemask, -1); if (!tmp_nodemask) return 0; - if (numa_available() < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Host kernel is not aware of NUMA.")); - return -1; - } - - maxnode = numa_max_node(); - maxnode = maxnode < NUMA_NUM_NODES ? maxnode : NUMA_NUM_NODES; - /* Convert nodemask to NUMA bitmask. */ nodemask_zero(&mask); bit = -1; while ((bit = virBitmapNextSetBit(tmp_nodemask, bit)) >= 0) { - if (bit > maxnode) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("NUMA node %d is out of range"), bit); - return -1; - } nodemask_set(&mask, bit); } @@ -347,10 +335,7 @@ int virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, virBitmapPtr nodemask ATTRIBUTE_UNUSED) { - if (virDomainNumatuneGetNodeset(numatune, NULL, -1)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("libvirt is compiled without NUMA tuning support")); - + if (!virNumaNodesetIsAvailable(numatune)) { return -1; } -- 1.9.3
On Thu, Oct 30, 2014 at 01:44:19PM +0800, Chen Fan wrote:
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/util/virnuma.c | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-)
diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 4188ef5..613a43c 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -95,31 +95,19 @@ virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, int ret = -1; int bit = 0; size_t i; - int maxnode = 0; virBitmapPtr tmp_nodemask = NULL;
+ if (!virNumaNodesetIsAvailable(numatune)) + return -1; + tmp_nodemask = virDomainNumatuneGetNodeset(numatune, nodemask, -1); if (!tmp_nodemask) return 0;
- if (numa_available() < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Host kernel is not aware of NUMA.")); - return -1; - } - - maxnode = numa_max_node(); - maxnode = maxnode < NUMA_NUM_NODES ? maxnode : NUMA_NUM_NODES; - /* Convert nodemask to NUMA bitmask. */ nodemask_zero(&mask); bit = -1; while ((bit = virBitmapNextSetBit(tmp_nodemask, bit)) >= 0) { - if (bit > maxnode) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("NUMA node %d is out of range"), bit); - return -1; - } nodemask_set(&mask, bit); }
I think this is safe, "numad" returning nodeset that's not on the host would be weird error and it is easy to find in the logs.
@@ -347,10 +335,7 @@ int virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, virBitmapPtr nodemask ATTRIBUTE_UNUSED) { - if (virDomainNumatuneGetNodeset(numatune, NULL, -1)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("libvirt is compiled without NUMA tuning support")); - + if (!virNumaNodesetIsAvailable(numatune)) { return -1; }
This makes the square brackets obsolete.
On Mon, 2014-11-03 at 14:18 +0100, Martin Kletzander wrote:
On Thu, Oct 30, 2014 at 01:44:19PM +0800, Chen Fan wrote:
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/util/virnuma.c | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-)
diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 4188ef5..613a43c 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -95,31 +95,19 @@ virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, int ret = -1; int bit = 0; size_t i; - int maxnode = 0; virBitmapPtr tmp_nodemask = NULL;
+ if (!virNumaNodesetIsAvailable(numatune)) + return -1; + tmp_nodemask = virDomainNumatuneGetNodeset(numatune, nodemask, -1); if (!tmp_nodemask) return 0;
- if (numa_available() < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Host kernel is not aware of NUMA.")); - return -1; - } - - maxnode = numa_max_node(); - maxnode = maxnode < NUMA_NUM_NODES ? maxnode : NUMA_NUM_NODES; - /* Convert nodemask to NUMA bitmask. */ nodemask_zero(&mask); bit = -1; while ((bit = virBitmapNextSetBit(tmp_nodemask, bit)) >= 0) { - if (bit > maxnode) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("NUMA node %d is out of range"), bit); - return -1; - } nodemask_set(&mask, bit); }
I think this is safe, "numad" returning nodeset that's not on the host would be weird error and it is easy to find in the logs. I think virNumaNodesetIsAvailable() has checked the case, but retain it here is ok.
@@ -347,10 +335,7 @@ int virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, virBitmapPtr nodemask ATTRIBUTE_UNUSED) { - if (virDomainNumatuneGetNodeset(numatune, NULL, -1)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("libvirt is compiled without NUMA tuning support")); - + if (!virNumaNodesetIsAvailable(numatune)) { return -1; }
This makes the square brackets obsolete.
Thanks, Chen
On Tue, Nov 04, 2014 at 02:05:16AM +0000, Chen, Fan wrote:
On Mon, 2014-11-03 at 14:18 +0100, Martin Kletzander wrote:
On Thu, Oct 30, 2014 at 01:44:19PM +0800, Chen Fan wrote:
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/util/virnuma.c | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-)
diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 4188ef5..613a43c 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -95,31 +95,19 @@ virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, int ret = -1; int bit = 0; size_t i; - int maxnode = 0; virBitmapPtr tmp_nodemask = NULL;
+ if (!virNumaNodesetIsAvailable(numatune))
Here you call virNumaNodesetIsAvailable() with @numatune, but ...
+ return -1; + tmp_nodemask = virDomainNumatuneGetNodeset(numatune, nodemask, -1);
... here you can get the automatic one ...
I think this is safe, "numad" returning nodeset that's not on the host would be weird error and it is easy to find in the logs. I think virNumaNodesetIsAvailable() has checked the case, but retain it here is ok.
... and that's what I meant here that it might be missed. I would be OK with the check removed though, since that should create no new problems, but since you added it in the next version, I'll keep it there ;)
On Thu, Oct 30, 2014 at 01:44:16PM +0800, Chen Fan wrote:
when setting elements memnode and nodeset in attribute numatune more than the host nodes in XML file, VM boot should fail. so add check for that.
You should run "make syntax-check" and "make check" on those patches, it would find at least two things ;) Anyway, ACK series with the changes I mentioned. If you're OK with them, I'll push the series. Martin
On Mon, 2014-11-03 at 14:18 +0100, Martin Kletzander wrote:
On Thu, Oct 30, 2014 at 01:44:16PM +0800, Chen Fan wrote:
when setting elements memnode and nodeset in attribute numatune more than the host nodes in XML file, VM boot should fail. so add check for that.
You should run "make syntax-check" and "make check" on those patches, it would find at least two things ;)
Anyway, ACK series with the changes I mentioned. If you're OK with them, I'll push the series.
I will send a new series after change them. and Thanks for your review. Chen
Martin
participants (3)
-
Chen Fan -
Chen, Fan
-
Martin Kletzander