Re: [libvirt] [PATCH v2 09/16] numatune: add support for per-node memory bindings in private APIs

2014-07-15 Thread Martin Kletzander 

On Fri, Jul 11, 2014 at 05:11:02PM +0200, Michal Privoznik wrote:

On 08.07.2014 13:50, Martin Kletzander wrote:

Signed-off-by: Martin Kletzander 
---
  src/conf/numatune_conf.c | 111 ---
  src/conf/numatune_conf.h |  14 --
  src/libvirt_private.syms |   1 +
  src/lxc/lxc_cgroup.c |   3 +-
  src/qemu/qemu_cgroup.c   |   2 +-
  src/qemu/qemu_driver.c   |  10 ++---
  src/util/virnuma.c   |   6 +--
  7 files changed, 117 insertions(+), 30 deletions(-)

diff --git a/src/conf/numatune_conf.c b/src/conf/numatune_conf.c
index 67fc799..60b6867 100644
--- a/src/conf/numatune_conf.c
+++ b/src/conf/numatune_conf.c
@@ -63,6 +63,18 @@ struct _virDomainNumatune {
  };


+static inline bool
+numa_cell_specified(virDomainNumatunePtr numatune,


Whoa, when I met this function call I thought to myself that this is
some libnuma function. Please name it differently to match our
virSomethingShiny pattern.



I wanted this function to stand out as it is a macro (or static
inline) and I've seen it somewhere else in the code.  I changed it to
virDomainNumatuneNodeSpecified(ewww, gross) and all the calls too
(with proper wrapping as well).


+int cellid)
+{
+if (numatune &&
+cellid >= 0 &&
+cellid < numatune->nmem_nodes)
+return numatune->mem_nodes[cellid].nodeset;
+
+return false;
+}
+
  static int
  virDomainNumatuneNodeParseXML(virDomainDefPtr def,
xmlXPathContextPtr ctxt)
@@ -312,6 +324,7 @@ void
  virDomainNumatuneFree(virDomainNumatunePtr numatune)
  {
  size_t i = 0;
+


This change is spurious. Either move it to 8/16 or drop this one.



Done.

[...]

@@ -469,6 +500,35 @@ virDomainNumatuneSet(virDomainDefPtr def,
  return ret;
  }

+static bool
+virDomainNumatuneNodesEqual(virDomainNumatunePtr n1,
+virDomainNumatunePtr n2)
+{
+size_t i = 0;
+
+if (n1->nmem_nodes != n2->nmem_nodes)
+return false;
+
+for (i = 0; i < n1->nmem_nodes; i++) {
+virDomainNumatuneNodePtr nd1 = &n1->mem_nodes[i];
+virDomainNumatuneNodePtr nd2 = &n2->mem_nodes[i];
+
+if (!nd1->nodeset && !nd2->nodeset)
+continue;


So if both are missing nodeset, they are considered equal? What if they
differ in mode?



Yes, because !nd->nodeset means there was no  with that
cellid.  Therefore mode doesn't make sense (and is not set anyway).

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 v2 09/16] numatune: add support for per-node memory bindings in private APIs

2014-07-11 Thread Michal Privoznik 

On 08.07.2014 13:50, Martin Kletzander wrote:

Signed-off-by: Martin Kletzander 
---
  src/conf/numatune_conf.c | 111 ---
  src/conf/numatune_conf.h |  14 --
  src/libvirt_private.syms |   1 +
  src/lxc/lxc_cgroup.c |   3 +-
  src/qemu/qemu_cgroup.c   |   2 +-
  src/qemu/qemu_driver.c   |  10 ++---
  src/util/virnuma.c   |   6 +--
  7 files changed, 117 insertions(+), 30 deletions(-)

diff --git a/src/conf/numatune_conf.c b/src/conf/numatune_conf.c
index 67fc799..60b6867 100644
--- a/src/conf/numatune_conf.c
+++ b/src/conf/numatune_conf.c
@@ -63,6 +63,18 @@ struct _virDomainNumatune {
  };


+static inline bool
+numa_cell_specified(virDomainNumatunePtr numatune,


Whoa, when I met this function call I thought to myself that this is 
some libnuma function. Please name it differently to match our 
virSomethingShiny pattern.



+int cellid)
+{
+if (numatune &&
+cellid >= 0 &&
+cellid < numatune->nmem_nodes)
+return numatune->mem_nodes[cellid].nodeset;
+
+return false;
+}
+
  static int
  virDomainNumatuneNodeParseXML(virDomainDefPtr def,
xmlXPathContextPtr ctxt)
@@ -312,6 +324,7 @@ void
  virDomainNumatuneFree(virDomainNumatunePtr numatune)
  {
  size_t i = 0;
+


This change is spurious. Either move it to 8/16 or drop this one.


  if (!numatune)
  return;

@@ -324,26 +337,37 @@ virDomainNumatuneFree(virDomainNumatunePtr numatune)
  }

  virDomainNumatuneMemMode
-virDomainNumatuneGetMode(virDomainNumatunePtr numatune)
+virDomainNumatuneGetMode(virDomainNumatunePtr numatune,
+ int cellid)
  {
-return (numatune && numatune->memory.specified) ? numatune->memory.mode : 
0;
+if (!numatune)
+return 0;
+
+if (numa_cell_specified(numatune, cellid))
+return numatune->mem_nodes[cellid].mode;
+
+if (numatune->memory.specified)
+return numatune->memory.mode;
+
+return 0;
  }

  virBitmapPtr
  virDomainNumatuneGetNodeset(virDomainNumatunePtr numatune,
-virBitmapPtr auto_nodeset)
+virBitmapPtr auto_nodeset,
+int cellid)
  {
  if (!numatune)
  return NULL;

-if (numatune->memory.placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO)
+if (numatune->memory.specified &&
+numatune->memory.placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO)
  return auto_nodeset;

-/*
- * This weird logic has the same meaning as switch with
- * auto/static/default, but can be more readably changed later.
- */
-if (numatune->memory.placement != VIR_DOMAIN_NUMATUNE_PLACEMENT_STATIC)
+if (numa_cell_specified(numatune, cellid))
+return numatune->mem_nodes[cellid].nodeset;
+
+if (!numatune->memory.specified)
  return NULL;

  return numatune->memory.nodeset;
@@ -351,23 +375,30 @@ virDomainNumatuneGetNodeset(virDomainNumatunePtr numatune,

  char *
  virDomainNumatuneFormatNodeset(virDomainNumatunePtr numatune,
-   virBitmapPtr auto_nodeset)
+   virBitmapPtr auto_nodeset,
+   int cellid)
  {
  return virBitmapFormat(virDomainNumatuneGetNodeset(numatune,
-   auto_nodeset));
+   auto_nodeset,
+   cellid));
  }

  int
  virDomainNumatuneMaybeFormatNodeset(virDomainNumatunePtr numatune,
  virBitmapPtr auto_nodeset,
-char **mask)
+char **mask,
+int cellid)
  {
  *mask = NULL;

  if (!numatune)
  return 0;

-if (numatune->memory.placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO &&
+if (!numa_cell_specified(numatune, cellid) && !numatune->memory.specified)
+return 0;
+
+if (numatune->memory.specified &&
+numatune->memory.placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO &&
  !auto_nodeset) {
  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
 _("Advice from numad is needed in case of "
@@ -375,7 +406,7 @@ virDomainNumatuneMaybeFormatNodeset(virDomainNumatunePtr 
numatune,
  return -1;
  }

-*mask = virDomainNumatuneFormatNodeset(numatune, auto_nodeset);
+*mask = virDomainNumatuneFormatNodeset(numatune, auto_nodeset, cellid);
  if (!*mask)
  return -1;

@@ -469,6 +500,35 @@ virDomainNumatuneSet(virDomainDefPtr def,
  return ret;
  }

+static bool
+virDomainNumatuneNodesEqual(virDomainNumatunePtr n1,
+virDomainNumatunePtr n2)
+{
+size_t i = 0;
+
+if (n1->nmem_nodes != n2->nmem_nodes)
+return false;
+
+for (i = 0; i < n1->nmem_nodes; i++) {
+