On Wed, 2014-10-29 at 07:58 +0100, Martin Kletzander wrote:
> 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@xxxxxxxxxxxxxx>
> >---
> > 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.
Got it!
>
> >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.
Thanks for your suggestion. I will make a try.
Thanks,
Chen
>
> 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
> >
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list