Re: [PATCH v2] credit: generalize __vcpu_has_soft_affinity()

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

On 03/06/2015 10:16 AM, Jan Beulich wrote:
>>>> On 06.03.15 at 10:53, <george.dunlap@eu.citrix.com> wrote:
>> On 03/06/2015 07:36 AM, Jan Beulich wrote:
>>> As pointed out in the discussion of the patch at
>>> http://lists.xenproject.org/archives/html/xen-devel/2015-02/msg03256.html 
>>> generalizing the conditions here means code elsewhere doesn't need to
>>> take into consideration internals of how load balancing in the credit
>>> scheduler works.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> v2: Use VCPU2ONLINE(vc) (or really an open coded variant thereof)
>>>     instead of cpu_online_map (suggested by Dario).
>>>
>>> --- a/xen/common/sched_credit.c
>>> +++ b/xen/common/sched_credit.c
>>> @@ -292,11 +292,10 @@ __runq_remove(struct csched_vcpu *svc)
>>>  static inline int __vcpu_has_soft_affinity(const struct vcpu *vc,
>>>                                             const cpumask_t *mask)
>>>  {
>>> -    if ( cpumask_full(vc->cpu_soft_affinity)
>>> -         || !cpumask_intersects(vc->cpu_soft_affinity, mask) )
>>> -        return 0;
>>> -
>>> -    return 1;
>>> +    return !cpumask_subset(cpupool_online_cpumask(vc->domain->cpupool),
>>> +                           vc->cpu_soft_affinity) &&
>>> +           !cpumask_subset(vc->cpu_soft_affinity, vc->cpu_hard_affinity) &&
>>> +           cpumask_intersects(vc->cpu_soft_affinity, mask);
>>
>> It looks like the comment above this line could use changing too; perhaps:
>>
>> ---
>> Hard affinity balancing is always necessary and must never be skipped.
>> But soft affinity need only be considered when it has a functionally
>> different effect than other constraints (such as hard affinity, cpus
>> online, or cpupools).
>>
>> Soft affinity only needs to be considered if:
>> * The cpus in the cpupool are not a subset of soft affinity
>> * The hard affinity is not a subset of soft affinity
> 
> "hard" and "soft" appear to be swapped here. I corrected this,
> please let me know if you disagree (in which case the patch would
> need changing too).

Uum -- I think my comment is right.  If the soft affinity is a subset of
hard affinity, then there are some cpus in the hard affinity which are
"preferred" (soft affine) and some that are "not preferred"
(non-soft-affine).  Whereas, if hard affinity is a subset of soft
affinity, then all cpus in the hard affinity are "preffered" (soft
affine), and so there's no sense in doing the soft affinity step.

In which case, yes, I think the patch needs to be adjusted.

Dario, am I crazy?

 -George