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

2014-10-30 Thread Martin Kletzander 

On Thu, Oct 30, 2014 at 02:23:00AM +, 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 :)

Martin


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2014-10-30 Thread Chen, Fan 
On Thu, 2014-10-30 at 07:55 +0100, Martin Kletzander wrote: 
 On Thu, Oct 30, 2014 at 02:23:00AM +, 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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2014-10-29 Thread Martin Kletzander 

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.


Signed-off-by: Chen Fan chen.fan.f...@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.


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

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

2014-10-29 Thread Chen, Fan 
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.f...@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