Re: [PATCH v2 1/3] numatune: add check for numatune nodeset range

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]