[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/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 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.
> 

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.


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