Re: [PATCH v2 08/16] xen: derive NUMA node affinity from hard and soft CPU affinity

[George Dunlap <george.dunlap@eu.citrix.com>]

On 13/11/13 19:12, Dario Faggioli wrote:
> if a domain's NUMA node-affinity (which is what controls
> memory allocations) is provided by the user/toolstack, it
> just is not touched. However, if the user does not say
> anything, leaving it all to Xen, let's compute it in the
> following way:
>
>   1. cpupool's cpus & hard-affinity & soft-affinity
>   2. if (1) is empty: cpupool's cpus & hard-affinity
>
> This guarantees memory to be allocated from the narrowest
> possible set of NUMA nodes, ad makes it relatively easy to
> set up NUMA-aware scheduling on top of soft affinity.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> ---
>   xen/common/domain.c |   22 +++++++++++++++++++++-
>   1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 2916490..4b8fca8 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -353,7 +353,7 @@ struct domain *domain_create(
>   
>   void domain_update_node_affinity(struct domain *d)
>   {
> -    cpumask_var_t cpumask;
> +    cpumask_var_t cpumask, cpumask_soft;
>       cpumask_var_t online_affinity;
>       const cpumask_t *online;
>       struct vcpu *v;
> @@ -361,9 +361,15 @@ void domain_update_node_affinity(struct domain *d)
>   
>       if ( !zalloc_cpumask_var(&cpumask) )
>           return;
> +    if ( !zalloc_cpumask_var(&cpumask_soft) )
> +    {
> +        free_cpumask_var(cpumask);
> +        return;
> +    }
>       if ( !alloc_cpumask_var(&online_affinity) )
>       {
>           free_cpumask_var(cpumask);
> +        free_cpumask_var(cpumask_soft);
>           return;
>       }
>   
> @@ -373,8 +379,12 @@ void domain_update_node_affinity(struct domain *d)
>   
>       for_each_vcpu ( d, v )
>       {
> +        /* Build up the mask of online pcpus we have hard affinity with */
>           cpumask_and(online_affinity, v->cpu_hard_affinity, online);
>           cpumask_or(cpumask, cpumask, online_affinity);
> +
> +        /* As well as the mask of all pcpus we have soft affinity with */
> +        cpumask_or(cpumask_soft, cpumask_soft, v->cpu_soft_affinity);

Is this really the most efficient way to do this?  I would have thought 
or'ing cpumask and cpumask_soft, and then if it's not empty, then use 
it; maybe use a pointer so you don't have to copy one into the other one?

Also, all of the above computation happens unconditionally, but is only 
used if d->auto_node_affinity is true.  It seems like it would be better 
to move this calculation inside the conditional.

Leaving the allocation outside might make sense to reduce the code 
covered by the lock -- not sure about that; I don't think this is a 
heavily-contended lock, is it?

>       }
>   
>       /*
> @@ -386,6 +396,15 @@ void domain_update_node_affinity(struct domain *d)
>        */
>       if ( d->auto_node_affinity )
>       {
> +        /*
> +         * We're looking for the narower possible set of nodes. So, if
> +         * possible (i.e., if not empty!) let's use the intersection
> +         * between online, hard and soft affinity. If not, just fall back
> +         * to online & hard affinity.
> +         */
> +        if ( cpumask_intersects(cpumask, cpumask_soft) )
> +            cpumask_and(cpumask, cpumask, cpumask_soft);
> +
>           nodes_clear(d->node_affinity);
>           for_each_online_node ( node )
>               if ( cpumask_intersects(&node_to_cpumask(node), cpumask) )
> @@ -397,6 +416,7 @@ void domain_update_node_affinity(struct domain *d)
>       spin_unlock(&d->node_affinity_lock);



>   
>       free_cpumask_var(online_affinity);
> +    free_cpumask_var(cpumask_soft);
>       free_cpumask_var(cpumask);
>   }
>   
>