On Tue, Oct 28, 2014 at 04:22:21PM +0800, Chen Fan wrote:

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@cn.fujitsu.com> --- 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);

NACK to these two hunks. The point of the structure being hidden in the .c file was to abstract it. You can provide accessors to those members you need if they are not available already.

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; + } + } + } +

It will even look better if you abstract this to virDomainNumatuneMaxNode() for example. You can also create virBotmapLastSetBit() that would make it even more modular, but that's not a requirement. Martin

+ 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 08:00 +0100, Martin Kletzander wrote:

On Tue, Oct 28, 2014 at 04:22:22PM +0800, Chen Fan wrote:

...

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/lxc/lxc_controller.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 1861dd6..1ee89ab 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -689,7 +689,8 @@ static int virLXCControllerSetupResourceLimits(virLXCControllerPtr ctrl) int ret = -1;

if (virLXCControllerGetNumadAdvice(ctrl, &nodemask) < 0 || - virNumaSetupMemoryPolicy(ctrl->def->numatune, nodemask) < 0) + (virNumaNodesetIsAvailable (ctrl->def->numatune) && + virNumaSetupMemoryPolicy(ctrl->def->numatune, nodemask) < 0)) goto cleanup;

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.

Oh, you are right. As for output because virNumaNodesetIsAvailable is self error output. so I think it not necessary. I think the check !virNumaNodesetIsAvailable (ctrl->def->numatune) || virNumaSetupMemoryPolicy(ctrl->def->numatune, nodemask) < 0 would be OK. Thanks, Chen

Martin