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

Re: [libvirt] [PATCH 3/4] remove the redundant codes



On 2013年03月01日 14:52, Gao feng wrote:
Intend to reduce the redundant code,use virSetupNumaMemoryPolicy
to replace virLXCControllerSetupNUMAPolicy and
qemuProcessInitNumaMemoryPolicy.

Signed-off-by: Gao feng<gaofeng cn fujitsu com>
---
  src/conf/domain_conf.h   |  23 +--------
  src/libvirt_private.syms |   1 +
  src/lxc/lxc_controller.c | 114 +-------------------------------------------
  src/qemu/qemu_process.c  | 121 +----------------------------------------------
  src/util/virnuma.c       | 114 ++++++++++++++++++++++++++++++++++++++++++++
  src/util/virnuma.h       |  24 ++++++++++
  6 files changed, 143 insertions(+), 254 deletions(-)

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 5828ae2..2a8dff3 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -47,6 +47,7 @@
  # include "device_conf.h"
  # include "virbitmap.h"
  # include "virstoragefile.h"
+# include "virnuma.h"

  /* forward declarations of all device types, required by
   * virDomainDeviceDef
@@ -1589,14 +1590,6 @@ enum virDomainCpuPlacementMode {
      VIR_DOMAIN_CPU_PLACEMENT_MODE_LAST
  };

-enum virDomainNumatuneMemPlacementMode {
-    VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_DEFAULT = 0,
-    VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_STATIC,
-    VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_AUTO,
-
-    VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_LAST
-};
-

Given that you move this into virnuma.h, VIR_ENUM_DECL and
VIR_ENUM_IMPL also need to be moved. And I don't see changes
on things like this:

virDomainNumatuneMemPlacementModeTypeFromString

in domain_conf.c, I bet the domain conf parsing and formating
are now broken with this patch applied.

  typedef struct _virDomainTimerCatchupDef virDomainTimerCatchupDef;
  typedef virDomainTimerCatchupDef *virDomainTimerCatchupDefPtr;
  struct _virDomainTimerCatchupDef {
@@ -1685,18 +1678,6 @@ virDomainVcpuPinDefPtr virDomainVcpuPinFindByVcpu(virDomainVcpuPinDefPtr *def,
                                                    int nvcpupin,
                                                    int vcpu);

-typedef struct _virDomainNumatuneDef virDomainNumatuneDef;
-typedef virDomainNumatuneDef *virDomainNumatuneDefPtr;
-struct _virDomainNumatuneDef {
-    struct {
-        virBitmapPtr nodemask;
-        int mode;
-        int placement_mode; /* enum virDomainNumatuneMemPlacementMode */
-    } memory;
-
-    /* Future NUMA tuning related stuff should go here. */
-};
-
  typedef struct _virBlkioDeviceWeight virBlkioDeviceWeight;
  typedef virBlkioDeviceWeight *virBlkioDeviceWeightPtr;
  struct _virBlkioDeviceWeight {
@@ -1784,7 +1765,7 @@ struct _virDomainDef {
          virDomainVcpuPinDefPtr emulatorpin;
      } cputune;

-    virDomainNumatuneDef numatune;
+    virNumaTuneParams numatune;

A bad new name, why not virNumatuneDef? the new name can be confused,
because we use params for other meaning in the project.


      /* These 3 are based on virDomainLifeCycleAction enum flags */
      int onReboot;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6aee6fa..56c466a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1567,6 +1567,7 @@ virNodeSuspendGetTargetMask;

  # util/virnuma.h
  virGetNumadAdvice;
+virSetupNumaMemoryPolicy;

Generally we want to use virNuma As the prefix for the helpers. This
applies to virGetNumadAdvice too (I didn't realized it when reviewing
1/4).


  # util/virobject.h
  virClassForObject;
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index b6c1fe8..3db0a88 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -46,11 +46,6 @@
  # include<cap-ng.h>
  #endif

-#if WITH_NUMACTL
-# define NUMA_VERSION1_COMPATIBILITY 1
-# include<numa.h>
-#endif
-
  #include "virerror.h"
  #include "virlog.h"
  #include "virutil.h"
@@ -409,113 +404,6 @@ cleanup:
      return ret;
  }

-#if WITH_NUMACTL
-static int virLXCControllerSetupNUMAPolicy(virLXCControllerPtr ctrl,
-                                           virBitmapPtr nodemask)
-{
-    nodemask_t mask;
-    int mode = -1;
-    int node = -1;
-    int ret = -1;
-    int i = 0;
-    int maxnode = 0;
-    bool warned = false;
-    virDomainNumatuneDef numatune = ctrl->def->numatune;
-    virBitmapPtr tmp_nodemask = NULL;
-
-    if (numatune.memory.placement_mode ==
-        VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_STATIC) {
-        if (!numatune.memory.nodemask)
-            return 0;
-        VIR_DEBUG("Set NUMA memory policy with specified nodeset");
-        tmp_nodemask = numatune.memory.nodemask;
-    } else if (numatune.memory.placement_mode ==
-               VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_AUTO) {
-        VIR_DEBUG("Set NUMA memory policy with advisory nodeset from numad");
-        tmp_nodemask = nodemask;
-    } else {
-        return 0;
-    }
-
-    VIR_DEBUG("Setting NUMA memory policy");
-
-    if (numa_available()<  0) {
-        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                       "%s", _("Host kernel is not aware of NUMA."));
-        return -1;
-    }
-
-    maxnode = numa_max_node() + 1;
-
-    /* Convert nodemask to NUMA bitmask. */
-    nodemask_zero(&mask);
-    i = -1;
-    while ((i = virBitmapNextSetBit(tmp_nodemask, i))>= 0) {
-        if (i>  NUMA_NUM_NODES) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("Host cannot support NUMA node %d"), i);
-            return -1;
-        }
-        if (i>  maxnode&&  !warned) {
-            VIR_WARN("nodeset is out of range, there is only %d NUMA "
-                     "nodes on host", maxnode);
-            warned = true;
-        }
-        nodemask_set(&mask, i);
-    }
-
-    mode = ctrl->def->numatune.memory.mode;
-
-    if (mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT) {
-        numa_set_bind_policy(1);
-        numa_set_membind(&mask);
-        numa_set_bind_policy(0);
-    } else if (mode == VIR_DOMAIN_NUMATUNE_MEM_PREFERRED) {
-        int nnodes = 0;
-        for (i = 0; i<  NUMA_NUM_NODES; i++) {
-            if (nodemask_isset(&mask, i)) {
-                node = i;
-                nnodes++;
-            }
-        }
-
-        if (nnodes != 1) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           "%s", _("NUMA memory tuning in 'preferred' mode "
-                                   "only supports single node"));
-            goto cleanup;
-        }
-
-        numa_set_bind_policy(0);
-        numa_set_preferred(node);
-    } else if (mode == VIR_DOMAIN_NUMATUNE_MEM_INTERLEAVE) {
-        numa_set_interleave_mask(&mask);
-    } else {
-        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                       _("Unable to set NUMA policy %s"),
-                       virDomainNumatuneMemModeTypeToString(mode));
-        goto cleanup;
-    }
-
-    ret = 0;
-
-cleanup:
-    return ret;
-}
-#else
-static int virLXCControllerSetupNUMAPolicy(virLXCControllerPtr ctrl,
-                                           virBitmapPtr nodemask ATTRIBUTE_UNUSED)
-{
-    if (ctrl->def->numatune.memory.nodemask) {
-        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                       _("NUMA policy is not available on this platform"));
-        return -1;
-    }
-
-    return 0;
-}
-#endif
-

  /*
   * To be run while still single threaded
@@ -621,7 +509,7 @@ static int virLXCControllerSetupResourceLimits(virLXCControllerPtr ctrl)
      if (ret<  0)
          goto cleanup;

-    ret = virLXCControllerSetupNUMAPolicy(ctrl, nodemask);
+    ret = virSetupNumaMemoryPolicy(ctrl->def->numatune, nodemask);
      if (ret<  0)
          goto cleanup;

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 20d41e3..2fa85aa 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -45,11 +45,6 @@
  #include "qemu_bridge_filter.h"
  #include "qemu_migration.h"

-#if WITH_NUMACTL
-# define NUMA_VERSION1_COMPATIBILITY 1
-# include<numa.h>
-#endif
-
  #include "datatypes.h"
  #include "virlog.h"
  #include "virerror.h"
@@ -1869,120 +1864,6 @@ qemuProcessDetectVcpuPIDs(virQEMUDriverPtr driver,
  }


-/*
- * Set NUMA memory policy for qemu process, to be run between
- * fork/exec of QEMU only.
- */
-#if WITH_NUMACTL
-static int
-qemuProcessInitNumaMemoryPolicy(virDomainObjPtr vm,
-                                virBitmapPtr nodemask)
-{
-    nodemask_t mask;
-    int mode = -1;
-    int node = -1;
-    int ret = -1;
-    int i = 0;
-    int maxnode = 0;
-    bool warned = false;
-    virDomainNumatuneDef numatune = vm->def->numatune;
-    virBitmapPtr tmp_nodemask = NULL;
-
-    if (numatune.memory.placement_mode ==
-        VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_STATIC) {
-        if (!numatune.memory.nodemask)
-            return 0;
-        VIR_DEBUG("Set NUMA memory policy with specified nodeset");
-        tmp_nodemask = numatune.memory.nodemask;
-    } else if (numatune.memory.placement_mode ==
-               VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_AUTO) {
-        VIR_DEBUG("Set NUMA memory policy with advisory nodeset from numad");
-        tmp_nodemask = nodemask;
-    } else {
-        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() + 1;
-    /* Convert nodemask to NUMA bitmask. */
-    nodemask_zero(&mask);
-    i = -1;
-    while ((i = virBitmapNextSetBit(tmp_nodemask, i))>= 0) {
-        if (i>  NUMA_NUM_NODES) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("Host cannot support NUMA node %d"), i);
-            return -1;
-        }
-        if (i>  maxnode&&  !warned) {
-            VIR_WARN("nodeset is out of range, there is only %d NUMA "
-                     "nodes on host", maxnode);
-            warned = true;
-        }
-        nodemask_set(&mask, i);
-    }
-
-    mode = numatune.memory.mode;
-
-    if (mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT) {
-        numa_set_bind_policy(1);
-        numa_set_membind(&mask);
-        numa_set_bind_policy(0);
-    } else if (mode == VIR_DOMAIN_NUMATUNE_MEM_PREFERRED) {
-        int nnodes = 0;
-        for (i = 0; i<  NUMA_NUM_NODES; i++) {
-            if (nodemask_isset(&mask, i)) {
-                node = i;
-                nnodes++;
-            }
-        }
-
-        if (nnodes != 1) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           "%s", _("NUMA memory tuning in 'preferred' mode "
-                                   "only supports single node"));
-            goto cleanup;
-        }
-
-        numa_set_bind_policy(0);
-        numa_set_preferred(node);
-    } else if (mode == VIR_DOMAIN_NUMATUNE_MEM_INTERLEAVE) {
-        numa_set_interleave_mask(&mask);
-    } else {
-        /* XXX: Shouldn't go here, as we already do checking when
-         * parsing domain XML.
-         */
-        virReportError(VIR_ERR_XML_ERROR,
-                       "%s", _("Invalid mode for memory NUMA tuning."));
-        goto cleanup;
-    }
-
-    ret = 0;
-
-cleanup:
-    return ret;
-}
-#else
-static int
-qemuProcessInitNumaMemoryPolicy(virDomainObjPtr vm,
-                                virBitmapPtr nodemask ATTRIBUTE_UNUSED)
-{
-    if (vm->def->numatune.memory.nodemask) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                       _("libvirt is compiled without NUMA tuning support"));
-
-        return -1;
-    }
-
-    return 0;
-}
-#endif
-
-
  /* Helper to prepare cpumap for affinity setting, convert
   * NUMA nodeset into cpuset if @nodemask is not NULL, otherwise
   * just return a new allocated bitmap.
@@ -2732,7 +2613,7 @@ static int qemuProcessHook(void *data)
          qemuProcessInitCpuAffinity(h->driver, h->vm, h->nodemask)<  0)
          goto cleanup;

-    if (qemuProcessInitNumaMemoryPolicy(h->vm, h->nodemask)<  0)
+    if (virSetupNumaMemoryPolicy(h->vm->def->numatune, h->nodemask)<  0)
          goto cleanup;

      ret = 0;
diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index 37931fe..75dcede 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -21,9 +21,15 @@

  #include<config.h>

+#if WITH_NUMACTL
+# define NUMA_VERSION1_COMPATIBILITY 1
+# include<numa.h>
+#endif
+
  #include "virnuma.h"
  #include "vircommand.h"
  #include "virerror.h"
+#include "virlog.h"

  #define VIR_FROM_THIS VIR_FROM_NONE

@@ -58,3 +64,111 @@ virGetNumadAdvice(unsigned short vcpus ATTRIBUTE_UNUSED,
      return NULL;
  }
  #endif
+
+#if WITH_NUMACTL
+int
+virSetupNumaMemoryPolicy(virNumaTuneParams numatune,
+                         virBitmapPtr nodemask)
+{
+    nodemask_t mask;
+    int mode = -1;
+    int node = -1;
+    int ret = -1;
+    int i = 0;
+    int maxnode = 0;
+    bool warned = false;
+    virBitmapPtr tmp_nodemask = NULL;
+
+    if (numatune.memory.placement_mode ==
+        VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_STATIC) {
+        if (!numatune.memory.nodemask)
+            return 0;
+        VIR_DEBUG("Set NUMA memory policy with specified nodeset");
+        tmp_nodemask = numatune.memory.nodemask;
+    } else if (numatune.memory.placement_mode ==
+               VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_AUTO) {
+        VIR_DEBUG("Set NUMA memory policy with advisory nodeset from numad");
+        tmp_nodemask = nodemask;
+    } else {
+        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() + 1;
+    /* Convert nodemask to NUMA bitmask. */
+    nodemask_zero(&mask);
+    i = -1;
+    while ((i = virBitmapNextSetBit(tmp_nodemask, i))>= 0) {
+        if (i>  NUMA_NUM_NODES) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Host cannot support NUMA node %d"), i);
+            return -1;
+        }
+        if (i>  maxnode&&  !warned) {
+            VIR_WARN("nodeset is out of range, there is only %d NUMA "
+                     "nodes on host", maxnode);
+            warned = true;
+        }
+        nodemask_set(&mask, i);
+    }
+
+    mode = numatune.memory.mode;
+
+    if (mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT) {
+        numa_set_bind_policy(1);
+        numa_set_membind(&mask);
+        numa_set_bind_policy(0);
+    } else if (mode == VIR_DOMAIN_NUMATUNE_MEM_PREFERRED) {
+        int nnodes = 0;
+        for (i = 0; i<  NUMA_NUM_NODES; i++) {
+            if (nodemask_isset(&mask, i)) {
+                node = i;
+                nnodes++;
+            }
+        }
+
+        if (nnodes != 1) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           "%s", _("NUMA memory tuning in 'preferred' mode "
+                                   "only supports single node"));
+            goto cleanup;
+        }
+
+        numa_set_bind_policy(0);
+        numa_set_preferred(node);
+    } else if (mode == VIR_DOMAIN_NUMATUNE_MEM_INTERLEAVE) {
+        numa_set_interleave_mask(&mask);
+    } else {
+        /* XXX: Shouldn't go here, as we already do checking when
+         * parsing domain XML.
+         */
+        virReportError(VIR_ERR_XML_ERROR,
+                       "%s", _("Invalid mode for memory NUMA tuning."));
+        goto cleanup;
+    }
+
+    ret = 0;
+
+cleanup:
+    return ret;
+}
+#else
+int
+virSetupNumaMemoryPolicy(virNumaTuneParams numatune,
+                         virBitmapPtr nodemask ATTRIBUTE_UNUSED)
+{
+    if (numatune.memory.nodemask) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("libvirt is compiled without NUMA tuning support"));
+
+        return -1;
+    }
+
+    return 0;
+}
+#endif
diff --git a/src/util/virnuma.h b/src/util/virnuma.h
index b9046c2..8d9f14d 100644
--- a/src/util/virnuma.h
+++ b/src/util/virnuma.h
@@ -22,7 +22,31 @@
  #ifndef __VIR_NUMA_H__
  # define __VIR_NUMA_H__

+#include "virbitmap.h"
+
+enum virDomainNumatuneMemPlacementMode {
+    VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_DEFAULT = 0,
+    VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_STATIC,
+    VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_AUTO,
+
+    VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_LAST
+};
+
+typedef struct _virNumaTuneParams virNumaTuneParams;
+typedef virNumaTuneParams *virNumaTuneParamsPtr;
+struct _virNumaTuneParams {
+    struct {
+        virBitmapPtr nodemask;
+        int mode;
+        int placement_mode; /* enum virDomainNumatuneMemPlacementMode */
+    } memory;
+
+    /* Future NUMA tuning related stuff should go here. */
+};
+

Except the pointed out nits, others are simply code moving, looks good
to me. This needs a v2 too.

Osier


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