[libvirt] [PATCH v2 09/16] numatune: add support for per-node memory bindings in private APIs

[Michal Privoznik]

On 08.07.2014 13:50, Martin Kletzander wrote:
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
>   src/conf/numatune_conf.c | 111 ++++++++++++++++++++++++++++++++++++++++-------
>   src/conf/numatune_conf.h |  14 ++++--
>   src/libvirt_private.syms |   1 +
>   src/lxc/lxc_cgroup.c     |   3 +-
>   src/qemu/qemu_cgroup.c   |   2 +-
>   src/qemu/qemu_driver.c   |  10 ++---
>   src/util/virnuma.c       |   6 +--
>   7 files changed, 117 insertions(+), 30 deletions(-)
>
> diff --git a/src/conf/numatune_conf.c b/src/conf/numatune_conf.c
> index 67fc799..60b6867 100644
> --- a/src/conf/numatune_conf.c
> +++ b/src/conf/numatune_conf.c
> @@ -63,6 +63,18 @@ struct _virDomainNumatune {
>   };
>
>
> +static inline bool
> +numa_cell_specified(virDomainNumatunePtr numatune,

Whoa, when I met this function call I thought to myself that this is 
some libnuma function. Please name it differently to match our 
virSomethingShiny pattern.

> +                    int cellid)
> +{
> +    if (numatune &&
> +        cellid >= 0 &&
> +        cellid < numatune->nmem_nodes)
> +        return numatune->mem_nodes[cellid].nodeset;
> +
> +    return false;
> +}
> +
>   static int
>   virDomainNumatuneNodeParseXML(virDomainDefPtr def,
>                                 xmlXPathContextPtr ctxt)
> @@ -312,6 +324,7 @@ void
>   virDomainNumatuneFree(virDomainNumatunePtr numatune)
>   {
>       size_t i = 0;
> +

This change is spurious. Either move it to 8/16 or drop this one.

>       if (!numatune)
>           return;
>
> @@ -324,26 +337,37 @@ virDomainNumatuneFree(virDomainNumatunePtr numatune)
>   }
>
>   virDomainNumatuneMemMode
> -virDomainNumatuneGetMode(virDomainNumatunePtr numatune)
> +virDomainNumatuneGetMode(virDomainNumatunePtr numatune,
> +                         int cellid)
>   {
> -    return (numatune && numatune->memory.specified) ? numatune->memory.mode : 0;
> +    if (!numatune)
> +        return 0;
> +
> +    if (numa_cell_specified(numatune, cellid))
> +        return numatune->mem_nodes[cellid].mode;
> +
> +    if (numatune->memory.specified)
> +        return numatune->memory.mode;
> +
> +    return 0;
>   }
>
>   virBitmapPtr
>   virDomainNumatuneGetNodeset(virDomainNumatunePtr numatune,
> -                            virBitmapPtr auto_nodeset)
> +                            virBitmapPtr auto_nodeset,
> +                            int cellid)
>   {
>       if (!numatune)
>           return NULL;
>
> -    if (numatune->memory.placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO)
> +    if (numatune->memory.specified &&
> +        numatune->memory.placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO)
>           return auto_nodeset;
>
> -    /*
> -     * This weird logic has the same meaning as switch with
> -     * auto/static/default, but can be more readably changed later.
> -     */
> -    if (numatune->memory.placement != VIR_DOMAIN_NUMATUNE_PLACEMENT_STATIC)
> +    if (numa_cell_specified(numatune, cellid))
> +        return numatune->mem_nodes[cellid].nodeset;
> +
> +    if (!numatune->memory.specified)
>           return NULL;
>
>       return numatune->memory.nodeset;
> @@ -351,23 +375,30 @@ virDomainNumatuneGetNodeset(virDomainNumatunePtr numatune,
>
>   char *
>   virDomainNumatuneFormatNodeset(virDomainNumatunePtr numatune,
> -                               virBitmapPtr auto_nodeset)
> +                               virBitmapPtr auto_nodeset,
> +                               int cellid)
>   {
>       return virBitmapFormat(virDomainNumatuneGetNodeset(numatune,
> -                                                       auto_nodeset));
> +                                                       auto_nodeset,
> +                                                       cellid));
>   }
>
>   int
>   virDomainNumatuneMaybeFormatNodeset(virDomainNumatunePtr numatune,
>                                       virBitmapPtr auto_nodeset,
> -                                    char **mask)
> +                                    char **mask,
> +                                    int cellid)
>   {
>       *mask = NULL;
>
>       if (!numatune)
>           return 0;
>
> -    if (numatune->memory.placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO &&
> +    if (!numa_cell_specified(numatune, cellid) && !numatune->memory.specified)
> +        return 0;
> +
> +    if (numatune->memory.specified &&
> +        numatune->memory.placement == VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO &&
>           !auto_nodeset) {
>           virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                          _("Advice from numad is needed in case of "
> @@ -375,7 +406,7 @@ virDomainNumatuneMaybeFormatNodeset(virDomainNumatunePtr numatune,
>           return -1;
>       }
>
> -    *mask = virDomainNumatuneFormatNodeset(numatune, auto_nodeset);
> +    *mask = virDomainNumatuneFormatNodeset(numatune, auto_nodeset, cellid);
>       if (!*mask)
>           return -1;
>
> @@ -469,6 +500,35 @@ virDomainNumatuneSet(virDomainDefPtr def,
>       return ret;
>   }
>
> +static bool
> +virDomainNumatuneNodesEqual(virDomainNumatunePtr n1,
> +                            virDomainNumatunePtr n2)
> +{
> +    size_t i = 0;
> +
> +    if (n1->nmem_nodes != n2->nmem_nodes)
> +        return false;
> +
> +    for (i = 0; i < n1->nmem_nodes; i++) {
> +        virDomainNumatuneNodePtr nd1 = &n1->mem_nodes[i];
> +        virDomainNumatuneNodePtr nd2 = &n2->mem_nodes[i];
> +
> +        if (!nd1->nodeset && !nd2->nodeset)
> +            continue;

So if both are missing nodeset, they are considered equal? What if they 
differ in mode?

> +
> +        if (!nd1->nodeset || !nd2->nodeset)
> +            return false;
> +
> +        if (nd1->mode != nd2->mode)
> +            return false;
> +
> +        if (!virBitmapEqual(nd1->nodeset, nd2->nodeset))
> +            return false;
> +    }
> +
> +    return true;
> +}
> +

Michal