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(a)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.