[libvirt] [PATCH 3/4] remove the redundant codes
Osier Yang
jyang at redhat.com
Fri Mar 15 09:03:47 UTC 2013
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 at 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
More information about the libvir-list
mailing list