For memnode in numatune element, the range of attribute 'nodeset' was not validated. on my host maxnodes was 1, but when setting nodeset to '0-2' or more, guest also started succuss. there probably was qemu's bug too. Signed-off-by: Chen Fan <chen.fan.fnst(a)cn.fujitsu.com&gt; --- src/conf/numatune_conf.c | 21 --------- src/conf/numatune_conf.h | 19 ++++++++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 3 ++ src/qemu/qemu_command.h | 1 + src/util/virnuma.c | 55 ++++++++++++++++++++++ src/util/virnuma.h | 2 + ...rgv-numatune-static-nodeset-exceed-hostnode.xml | 35 ++++++++++++++ tests/qemuxml2argvmock.c | 9 ++++ tests/qemuxml2argvtest.c | 1 + 10 files changed, 126 insertions(+), 21 deletions(-) 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..d440b86 100644 --- a/src/conf/numatune_conf.c +++ b/src/conf/numatune_conf.c @@ -42,27 +42,6 @@ VIR_ENUM_IMPL(virDomainNumatunePlacement, "static", "auto"); -typedef struct _virDomainNumatuneNode virDomainNumatuneNode; -typedef virDomainNumatuneNode *virDomainNumatuneNodePtr; - -struct _virDomainNumatune { - struct { - bool specified; - virBitmapPtr nodeset; - virDomainNumatuneMemMode mode; - virDomainNumatunePlacement placement; - } memory; /* pinning for all the memory */ - - struct _virDomainNumatuneNode { - virBitmapPtr nodeset; - virDomainNumatuneMemMode mode; - } *mem_nodes; /* fine tuning per guest node */ - size_t nmem_nodes; - - /* Future NUMA tuning related stuff should go here. */ -}; - - static inline bool virDomainNumatuneNodeSpecified(virDomainNumatunePtr numatune, int cellid) diff --git a/src/conf/numatune_conf.h b/src/conf/numatune_conf.h index 5254629..650b6e7 100644 --- a/src/conf/numatune_conf.h +++ b/src/conf/numatune_conf.h @@ -45,6 +45,25 @@ typedef enum { VIR_ENUM_DECL(virDomainNumatunePlacement) VIR_ENUM_DECL(virDomainNumatuneMemMode) +typedef struct _virDomainNumatuneNode virDomainNumatuneNode; +typedef virDomainNumatuneNode *virDomainNumatuneNodePtr; + +struct _virDomainNumatune { + struct { + bool specified; + virBitmapPtr nodeset; + virDomainNumatuneMemMode mode; + virDomainNumatunePlacement placement; + } memory; /* pinning for all the memory */ + + struct _virDomainNumatuneNode { + virBitmapPtr nodeset; + virDomainNumatuneMemMode mode; + } *mem_nodes; /* fine tuning per guest node */ + size_t nmem_nodes; + + /* Future NUMA tuning related stuff should go here. */ +}; void virDomainNumatuneFree(virDomainNumatunePtr numatune); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d63a8f0..16a5864 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1728,6 +1728,7 @@ virNumaGetPageInfo; virNumaGetPages; virNumaIsAvailable; virNumaNodeIsAvailable; +virNumaNodesetIsAvailable; virNumaSetPagePoolSize; virNumaSetupMemoryPolicy; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2e5af4f..9757d3e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6663,6 +6663,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/qemu/qemu_command.h b/src/qemu/qemu_command.h index aa40c9e..f263665 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -27,6 +27,7 @@ # include "domain_addr.h" # include "domain_conf.h" # include "vircommand.h" +# include "virnuma.h" # include "capabilities.h" # include "qemu_conf.h" # include "qemu_domain.h" diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 690615f..411719d 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -312,6 +312,55 @@ virNumaGetNodeCPUs(int node, return ret; } + +bool +virNumaNodesetIsAvailable(virDomainNumatunePtr numatune) +{ + int maxnode; + int bit = -1; + size_t i; + virBitmapPtr nodemask = NULL; + + if (!numatune) + return true; + + if ((maxnode = virNumaGetMaxNode()) < 0) + return false; + + maxnode = maxnode < NUMA_NUM_NODES ? maxnode : NUMA_NUM_NODES; + + /* verify <memory> and <memnode> element in <numatune> */ + nodemask = virDomainNumatuneGetNodeset(numatune, NULL, -1); + if (nodemask) { + while ((bit = virBitmapNextSetBit(nodemask, bit)) >= 0) { + if (bit > maxnode) { + goto error; + } + } + } + + for (i = 0; i < numatune->nmem_nodes; i++) { + nodemask = virDomainNumatuneGetNodeset(numatune, NULL, i); + + if (!nodemask) + continue; + + bit = -1; + while ((bit = virBitmapNextSetBit(nodemask, bit)) >= 0) { + if (bit > maxnode) { + goto error; + } + } + } + + return true; + + error: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("NUMA node %d is out of range"), bit); + return false; +} + # undef MASK_CPU_ISSET # undef n_bits @@ -373,6 +422,12 @@ virNumaGetNodeCPUs(int node ATTRIBUTE_UNUSED, _("NUMA isn't available on this host")); return -1; } + +bool +virNumaNodesetIsAvailable(virDomainNumatunePtr numatune) +{ + return true; +} #endif diff --git a/src/util/virnuma.h b/src/util/virnuma.h index 04b6b8c..e129bb2 100644 --- a/src/util/virnuma.h +++ b/src/util/virnuma.h @@ -34,6 +34,8 @@ 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..84a560a --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml @@ -0,0 +1,35 @@ +<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..316bf0b 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 maxnodes = 7; + + return maxnodes; +} 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 Wed, 2014-10-29 at 07:58 +0100, Martin Kletzander wrote: Got it! Thanks for your suggestion. I will make a try. Thanks, Chen

On Tue, Oct 28, 2014 at 04:22:22PM +0800, Chen Fan wrote: This would mean it will succeed if the numa node is not available on the host. Don't you want to error out? By the way, it would make sense to make the check in virNumaSetupMemoryPolicy() itself. Martin

Signed-off-by: Chen Fan <chen.fan.fnst(a)cn.fujitsu.com&gt; --- src/util/virnuma.c | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 411719d..8431b3c 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -95,31 +95,16 @@ virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, int ret = -1; int bit = 0; size_t i; - int maxnode = 0; virBitmapPtr tmp_nodemask = NULL; 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); } -- 1.9.3