Re: [PATCH RESEND 01/12] xen: numa-sched: leave node-affinity alone if not in "auto" mode

[Keir Fraser <keir.xen@gmail.com>]

On 12/11/2013 08:11, "Jan Beulich" <JBeulich@suse.com> wrote:

> Hi Keir,
> 
> below the one remaining patch mentioned yesterday.
> 
> Jan
> 
>>>> On 05.11.13 at 15:34, Dario Faggioli <dario.faggioli@citrix.com> wrote:
>> If the domain's NUMA node-affinity is being specified by the
>> user/toolstack (instead of being automatically computed by Xen),
>> we really should stick to that. This means domain_update_node_affinity()
>> is wrong when it filters out some stuff from there even in "!auto"
>> mode.
>> 
>> This commit fixes that. Of course, this does not mean node-affinity
>> is always honoured (e.g., a vcpu won't run on a pcpu of a different
>> cpupool) but the necessary logic for taking into account all the
>> possible situations lives in the scheduler code, where it belongs.
>> 
>> What could happen without this change is that, under certain
>> circumstances, the node-affinity of a domain may change when the
>> user modifies the vcpu-affinity of the domain's vcpus. This, even
>> if probably not a real bug, is at least something the user does
>> not expect, so let's avoid it.
>> 
>> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
>> Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>

Acked-by: Keir Fraser <keir@xen.org>

>> ---
>> This has been submitted already as a single patch on its own.
>> Since this series needs the change done here, just include it
>> in here, instead of pinging the original submission and deferring
>> posting this series.
>> ---
>>  xen/common/domain.c |   28 +++++++++-------------------
>>  1 file changed, 9 insertions(+), 19 deletions(-)
>> 
>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>> index 5999779..af31ab4 100644
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -352,7 +352,6 @@ void domain_update_node_affinity(struct domain *d)
>>      cpumask_var_t cpumask;
>>      cpumask_var_t online_affinity;
>>      const cpumask_t *online;
>> -    nodemask_t nodemask = NODE_MASK_NONE;
>>      struct vcpu *v;
>>      unsigned int node;
>>  
>> @@ -374,28 +373,19 @@ void domain_update_node_affinity(struct domain *d)
>>          cpumask_or(cpumask, cpumask, online_affinity);
>>      }
>>  
>> +    /*
>> +     * If d->auto_node_affinity is true, the domain's node-affinity mask
>> +     * (d->node_affinity) is automaically computed from all the domain's
>> +     * vcpus' vcpu-affinity masks (the union of which we have just built
>> +     * above in cpumask). OTOH, if d->auto_node_affinity is false, we
>> +     * must leave the node-affinity of the domain alone.
>> +     */
>>      if ( d->auto_node_affinity )
>>      {
>> -        /* Node-affinity is automaically computed from all vcpu-affinities
>> */
>> +        nodes_clear(d->node_affinity);
>>          for_each_online_node ( node )
>>              if ( cpumask_intersects(&node_to_cpumask(node), cpumask) )
>> -                node_set(node, nodemask);
>> -
>> -        d->node_affinity = nodemask;
>> -    }
>> -    else
>> -    {
>> -        /* Node-affinity is provided by someone else, just filter out cpus
>> -         * that are either offline or not in the affinity of any vcpus. */
>> -        nodemask = d->node_affinity;
>> -        for_each_node_mask ( node, d->node_affinity )
>> -            if ( !cpumask_intersects(&node_to_cpumask(node), cpumask) )
>> -                node_clear(node, nodemask);//d->node_affinity);
>> -
>> -        /* Avoid loosing track of node-affinity because of a bad
>> -         * vcpu-affinity has been specified. */
>> -        if ( !nodes_empty(nodemask) )
>> -            d->node_affinity = nodemask;
>> +                node_set(node, d->node_affinity);
>>      }
>>  
>>      sched_set_node_affinity(d, &d->node_affinity);
> 
> 
>