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

Re: [libvirt] [PATCH v3 3/4] NUMA: cleanup for numa related codes



On Mon, Mar 18, 2013 at 05:04:03PM +0800, Gao feng wrote:
> Intend to reduce the redundant code,use virNumaSetupMemoryPolicy
> to replace virLXCControllerSetupNUMAPolicy and
> qemuProcessInitNumaMemoryPolicy.
> 
> This patch also moves the numa related codes to the
> file virnuma.c and virnuma.h
> 
> Signed-off-by: Gao feng <gaofeng cn fujitsu com>
> ---
>  include/libvirt/libvirt.h.in |  15 ------
>  src/conf/domain_conf.c       |  25 +++------
>  src/conf/domain_conf.h       |  17 +-----
>  src/libvirt_private.syms     |   9 ++--
>  src/lxc/lxc_controller.c     | 114 +--------------------------------------
>  src/qemu/qemu_cgroup.c       |   6 +--
>  src/qemu/qemu_driver.c       |   2 +-
>  src/qemu/qemu_process.c      | 121 +----------------------------------------
>  src/util/virnuma.c           | 125 +++++++++++++++++++++++++++++++++++++++++++
>  src/util/virnuma.h           |  45 ++++++++++++++++
>  tools/virsh-domain.c         |   4 +-
>  11 files changed, 192 insertions(+), 291 deletions(-)
> 
> diff --git PATCH v3include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index f6a7aff..b3bfd1d 100644
> --- PATCH v3include/libvirt/libvirt.h.in	
> +++ b/include/libvirt/libvirt.h.in
> @@ -1762,21 +1762,6 @@ typedef enum {
>  /* Manage numa parameters */
>  
>  /**
> - * virDomainNumatuneMemMode:
> - * Representation of the various modes in the <numatune> element of
> - * a domain.
> - */
> -typedef enum {
> -    VIR_DOMAIN_NUMATUNE_MEM_STRICT      = 0,
> -    VIR_DOMAIN_NUMATUNE_MEM_PREFERRED   = 1,
> -    VIR_DOMAIN_NUMATUNE_MEM_INTERLEAVE  = 2,
> -
> -#ifdef VIR_ENUM_SENTINELS
> -    VIR_DOMAIN_NUMATUNE_MEM_LAST /* This constant is subject to change */
> -#endif
> -} virDomainNumatuneMemMode;
> -

NACK, as Osier said, it is forbidden to delete or rename any
structs / enums / etc in libvirt.h.in

> diff --git PATCH v3src/libvirt_private.syms b/src/libvirt_private.syms
> index 1374470..9bf35a8 100644
> --- PATCH v3src/libvirt_private.syms	
> +++ b/src/libvirt_private.syms
> @@ -252,10 +252,10 @@ virDomainNetRemove;
>  virDomainNetTypeToString;
>  virDomainNostateReasonTypeFromString;
>  virDomainNostateReasonTypeToString;
> -virDomainNumatuneMemModeTypeFromString;
> -virDomainNumatuneMemModeTypeToString;
> -virDomainNumatuneMemPlacementModeTypeFromString;
> -virDomainNumatuneMemPlacementModeTypeToString;
> +virNumatuneMemModeTypeFromString;
> +virNumatuneMemModeTypeToString;
> +virNumatuneMemPlacementModeTypeFromString;
> +virNumatuneMemPlacementModeTypeToString;
>  virDomainObjAssignDef;
>  virDomainObjCopyPersistentDef;
>  virDomainObjGetPersistentDef;

If you had run 'make check' you'd see that this change is not valid 
because it is not alphabetically sorted anymore.

> diff --git PATCH v3src/util/virnuma.c b/src/util/virnuma.c
> index f6a6eb2..0601dcc 100644
> --- PATCH v3src/util/virnuma.c	
> +++ b/src/util/virnuma.c
> @@ -21,12 +21,29 @@
>  
>  #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
>  
> +VIR_ENUM_IMPL(virNumatuneMemPlacementMode,
> +              VIR_NUMATUNE_MEM_PLACEMENT_MODE_LAST,
> +              "default",
> +              "static",
> +              "auto");
> +
> +VIR_ENUM_IMPL(virNumatuneMemMode, VIR_NUMATUNE_MEM_LAST,
> +              "strict",
> +              "preferred",
> +              "interleave");
> +
>  #if HAVE_NUMAD
>  char *
>  virNumaGetAutoPlacementAdvice(unsigned short vcpus,
> @@ -59,3 +76,111 @@ virNumaGetAutoPlacementAdvice(unsigned short vcpus ATTRIBUTE_UNUSED,
>      return NULL;
>  }
>  #endif
> +
> +#if WITH_NUMACTL
> +int
> +virNumaSetupMemoryPolicy(virNumatuneDef 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_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_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_NUMATUNE_MEM_STRICT) {
> +        numa_set_bind_policy(1);
> +        numa_set_membind(&mask);
> +        numa_set_bind_policy(0);
> +    } else if (mode == VIR_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_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
> +virNumaSetupMemoryPolicy(virNumatuneDef 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 PATCH v3src/util/virnuma.h b/src/util/virnuma.h
> index d3d7d3e..5fdf39e 100644
> --- PATCH v3src/util/virnuma.h	
> +++ b/src/util/virnuma.h
> @@ -22,7 +22,52 @@
>  #ifndef __VIR_NUMA_H__
>  # define __VIR_NUMA_H__
>  
> +# include "internal.h"
> +# include "virbitmap.h"
> +# include "virutil.h"
> +
> +enum virNumatuneMemPlacementMode {
> +    VIR_NUMATUNE_MEM_PLACEMENT_MODE_DEFAULT = 0,
> +    VIR_NUMATUNE_MEM_PLACEMENT_MODE_STATIC,
> +    VIR_NUMATUNE_MEM_PLACEMENT_MODE_AUTO,
> +
> +    VIR_NUMATUNE_MEM_PLACEMENT_MODE_LAST
> +};
> +
> +VIR_ENUM_DECL(virNumatuneMemPlacementMode)
> +
> +/**
> + * virNumatuneMemMode:
> + * Representation of the various modes in the <numatune> element of
> + * a domain.
> + */
> +typedef enum {
> +    VIR_NUMATUNE_MEM_STRICT      = 0,
> +    VIR_NUMATUNE_MEM_PREFERRED   = 1,
> +    VIR_NUMATUNE_MEM_INTERLEAVE  = 2,
> +
> +# ifdef VIR_ENUM_SENTINELS
> +    VIR_NUMATUNE_MEM_LAST /* This constant is subject to change */
> +# endif
> +} virNumatuneMemMode;
> +
> +VIR_ENUM_DECL(virNumatuneMemMode)
> +
> +typedef struct _virNumatuneDef virNumatuneDef;
> +typedef virNumatuneDef *virNumatuneDefPtr;
> +struct _virNumatuneDef {
> +    struct {
> +        virBitmapPtr nodemask;
> +        int mode;
> +        int placement_mode; /* enum virNumatuneMemPlacementMode */
> +    } memory;
> +
> +    /* Future NUMA tuning related stuff should go here. */
> +};

Bad capitalization in all this stuff - the letter immediately
following the prefix 'virNuma' must be capitalized. eg

  virNumaTuneDef


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


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