On 2013/03/15 17:03, Osier Yang wrote:
> 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@xxxxxxxxxxxxxx>
>> ---
>> 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.
>
Yes, I totally miss this, thanks for pointing it out,
will fix it :)
>> 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.
>
Ok, I prefer virNumatuneDef.
>>
>> /* 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).
>
Get it :)
>>
>> # 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.
>
Thanks for your review.
will send a v2 patch
Thanks.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list