On Wed, 2014-10-29 at 14:20 +0100, Martin Kletzander wrote:

On Wed, Oct 29, 2014 at 08:33:32PM +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.

How about rewording this as:

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.

Look good to me.

...

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 | 2 + src/qemu/qemu_command.c | 3 ++ src/qemu/qemu_command.h | 1 + src/util/virbitmap.c | 49 ++++++++++++++++++++++ src/util/virbitmap.h | 3 ++ src/util/virnuma.c | 33 +++++++++++++++ src/util/virnuma.h | 2 + ...rgv-numatune-static-nodeset-exceed-hostnode.xml | 35 ++++++++++++++++ tests/qemuxml2argvmock.c | 9 ++++ tests/qemuxml2argvtest.c | 1 + tests/virbitmaptest.c | 26 +++++++++++- 13 files changed, 192 insertions(+), 1 deletion(-) 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..54f309a 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; + + if (!numatune) + return ret; + + nodemask = virDomainNumatuneGetNodeset(numatune, NULL, -1); + if (nodemask) { + ret = virBitmapLastSetBit(nodemask, -1); + } + for (i = 0; i < numatune->nmem_nodes; i++) {

Well, you're using the advantage of accessible structure members here (numatune->nmem_nodes), but using accessors around. These particular ones are useless here when you don't need any of the logic they provide.

right, I should use numatune->mem_nodes[i].nodeset directly.

...

+ int bit = -1; + nodemask = virDomainNumatuneGetNodeset(numatune, NULL, i); + if (!nodemask) + continue; + + bit = virBitmapLastSetBit(nodemask, -1); + 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 d63a8f0..4a30ad7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1011,6 +1011,7 @@ virBitmapFree; virBitmapGetBit; virBitmapIsAllClear; virBitmapIsAllSet; +virBitmapLastSetBit; virBitmapNew; virBitmapNewCopy; virBitmapNewData; @@ -1728,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..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/virbitmap.c b/src/util/virbitmap.c index b6bd074..aed525a 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -651,6 +651,55 @@ virBitmapNextSetBit(virBitmapPtr bitmap, ssize_t pos) }

/** + * virBitmapLastSetBit: + * @bitmap: the bitmap + * @pos: the position after which to search for a set bit + * + * Search for the last set bit before position @pos in bitmap @bitmap. + * @pos can be -1 to search for the last set bit. Position starts + * at max_bit. + * + * Returns the position of the found bit, or -1 if no bit found. + */ +ssize_t +virBitmapLastSetBit(virBitmapPtr bitmap, ssize_t pos) +{ + size_t nl; + size_t nb; + unsigned long bits; + size_t i; + + if (pos < 0) + pos = bitmap->max_bit; + + pos--; + + if (pos >= bitmap->max_bit) + return -1; + + nl = pos / VIR_BITMAP_BITS_PER_UNIT; + nb = pos % VIR_BITMAP_BITS_PER_UNIT; +

You can use VIR_BITMAP_UNIT_OFFSET and VIR_BITMAP_BIT_OFFSET macros here.

...

+ bits = bitmap->map[nl] & ((1UL << (nb + 1)) - 1); +

VIR_BITMAP_BIT(nb + 1) - 1

...

+ while (bits == 0 && --nl < bitmap->map_len) { + bits = bitmap->map[nl]; + } +

Reading this is weird, especially when you're using underflowing of size_t, eww. I think you made this unnecessarily complicated by keeping "pos" attribute there. If (and only if) you really need to have @pos there, I rather suggest making this a wrapper using virBitmapNextSetBit(), but I hope you don't.

I will delete the "pos" attribute.

...

+ if (bits == 0) + return -1; + + i = VIR_BITMAP_BITS_PER_UNIT - 1; + for (; i < VIR_BITMAP_BITS_PER_UNIT; i--) { + if (bits & 1UL << i) { + return i + nl * 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..c0fad55 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, ssize_t pos) + ATTRIBUTE_NONNULL(1); + ssize_t virBitmapNextClearBit(virBitmapPtr bitmap, ssize_t pos) ATTRIBUTE_NONNULL(1);

Could you separate these virbitmap.* hunks and the tests for them into their own patch, please?

Sure.

...

diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 690615f..fbe8fd1 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -312,6 +312,33 @@ virNumaGetNodeCPUs(int node,

return ret; } + +bool +virNumaNodesetIsAvailable(virDomainNumatunePtr numatune) +{ + int maxnode; + int bit; + + if (!numatune) + return true; + + if ((maxnode = virNumaGetMaxNode()) < 0) + return false; + + maxnode = maxnode < NUMA_NUM_NODES ? maxnode : NUMA_NUM_NODES; + + bit = virDomainNumatuneSpecifiedMaxNode(numatune); + 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 +400,12 @@ virNumaGetNodeCPUs(int node ATTRIBUTE_UNUSED, _("NUMA isn't available on this host")); return -1; } + +bool +virNumaNodesetIsAvailable(virDomainNumatunePtr numatune) +{ + return true; +} #endif

In what case would you like this to return true?

when libvirt does not support numa, we can not check it. maybe we should return false if setting nodeset. Thanks, Chen

...

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); diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c index ea832ad..21e8611 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) != -1) + goto error; + for (i = 0; i < size; i++) { if (virBitmapNextClearBit(bitmap, i - 1) != i) goto error; @@ -232,6 +235,18 @@ test4(const void *data ATTRIBUTE_UNUSED) if (virBitmapNextSetBit(bitmap, i) != -1) goto error;

+ j = sizeof(bitsPos)/sizeof(int) - 1; + i = -1; + + while (j < ARRAY_CARDINALITY(bitsPos)) { + i = virBitmapLastSetBit(bitmap, i); + if (i != bitsPos[j--]) + goto error; + } + + if (virBitmapLastSetBit(bitmap, i) != -1) + goto error; + j = 0; i = -1;

@@ -255,6 +270,15 @@ test4(const void *data ATTRIBUTE_UNUSED) if (virBitmapNextSetBit(bitmap, i) != -1) goto error;

+ for (i = size; i > 0; i--) { + if (virBitmapLastSetBit(bitmap, i) != i - 1) + goto error; + } + + if (virBitmapLastSetBit(bitmap, i) != -1) + goto error; + + if (virBitmapNextClearBit(bitmap, -1) != -1) goto error;

-- 1.9.3

On Thu, 2014-10-30 at 07:55 +0100, Martin Kletzander wrote:

On Thu, Oct 30, 2014 at 02:23:00AM +0000, Chen, Fan wrote:

...

On Wed, 2014-10-29 at 14:20 +0100, Martin Kletzander wrote:

...

On Wed, Oct 29, 2014 at 08:33:32PM +0800, Chen Fan wrote:

...

diff --git a/src/util/virnuma.c b/src/util/virnuma.c @@ -373,6 +400,12 @@ virNumaGetNodeCPUs(int node ATTRIBUTE_UNUSED, _("NUMA isn't available on this host")); return -1; } + +bool +virNumaNodesetIsAvailable(virDomainNumatunePtr numatune) +{ + return true; +} #endif

In what case would you like this to return true?

when libvirt does not support numa, we can not check it. maybe we should return false if setting nodeset.

That was my idea, I just wanted to make sure we're on the same page. The thing is that if you want something that's not available, it makes more sense to say "NO" then just allow it because libvirt doesn't know. Make the user fix it :) Yes, I had made a v4 and sent it, please help to review.

Thanks, Chen

Martin

On Wed, 2014-10-29 at 14:23 +0100, Martin Kletzander wrote:

On Wed, Oct 29, 2014 at 08:33:34PM +0800, Chen Fan wrote:

...

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/util/virnuma.c | 15 --------------- 1 file changed, 15 deletions(-)

I think this harmless check may prevent future problems (if SetupMemoryPolicy is called from some new codepath. Either keep it here or call virNumaNodesetIsAvailable() in the start of the function. call virNumaNodesetIsAvailable() in the start of the function will be fine.

Thanks, Chen

...

diff --git a/src/util/virnuma.c b/src/util/virnuma.c index fbe8fd1..5a08049 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