Re: [Xen-devel] [PATCH 3/7] xen: credit2: soft-affinity awareness in fallback_cpu()

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 07/25/2017 11:19 AM, George Dunlap wrote:
> On 06/16/2017 03:13 PM, Dario Faggioli wrote:
>> By, basically, moving all the logic of the function
>> inside the usual two steps (soft-affinity step and
>> hard-affinity step) loop.
>>
>> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
>> Signed-off-by: Justin T. Weaver <jtweaver@xxxxxxxxxx>
>> ---
>> Cc: Anshul Makkar <anshul.makkar@xxxxxxxxxx>
>> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
>> ---
>> George, you gave your Reviewed-by to:
>>  https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg02201.html
>>
>> which was adding soft-affinity awareness to both fallback_cpu and 
>> cpu_pick(). See here:
>>  https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg03259.html
>>
>> I changed the cpu_pick() part a lot, and that's why I decided to split the
>> patch in two.  As far as fallback_cpu(), though, what's done in this patch is
>> exactly the same that was being done in the original one.
>>
>> So, of course I'm dropping the Rev-by, but I thought it could have been 
>> useful
>> to mention this. :-)
>> ---
>>  xen/common/sched_credit2.c |   77 
>> ++++++++++++++++++++++++++++++++------------
>>  1 file changed, 56 insertions(+), 21 deletions(-)
>>
>> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
>> index c749d4e..54f6e21 100644
>> --- a/xen/common/sched_credit2.c
>> +++ b/xen/common/sched_credit2.c
>> @@ -537,36 +537,71 @@ void smt_idle_mask_clear(unsigned int cpu, cpumask_t 
>> *mask)
>>  }
>>  
>>  /*
>> - * When a hard affinity change occurs, we may not be able to check some
>> - * (any!) of the other runqueues, when looking for the best new processor
>> - * for svc (as trylock-s in csched2_cpu_pick() can fail). If that happens, 
>> we
>> - * pick, in order of decreasing preference:
>> - *  - svc's current pcpu;
>> - *  - another pcpu from svc's current runq;
>> - *  - any cpu.
>> + * In csched2_cpu_pick(), it may not be possible to actually look at remote
>> + * runqueues (the trylock-s on their spinlocks can fail!). If that happens,
>> + * we pick, in order of decreasing preference:
>> + *  1) svc's current pcpu, if it is part of svc's soft affinity;
>> + *  2) a pcpu in svc's current runqueue that is also in svc's soft affinity;
>> + *  3) just one valid pcpu from svc's soft affinity;
>> + *  4) svc's current pcpu, if it is part of svc's hard affinity;
>> + *  5) a pcpu in svc's current runqueue that is also in svc's hard affinity;
>> + *  6) just one valid pcpu from svc's hard affinity
>> + *
>> + * Of course, 1, 2 and 3 makes sense only if svc has a soft affinity. Also
>> + * note that at least 6 is guaranteed to _always_ return at least one pcpu.
>>   */
>>  static int get_fallback_cpu(struct csched2_vcpu *svc)
>>  {
>>      struct vcpu *v = svc->vcpu;
>> -    int cpu = v->processor;
>> +    unsigned int bs;
>>  
>> -    cpumask_and(cpumask_scratch_cpu(cpu), v->cpu_hard_affinity,
>> -                cpupool_domain_cpumask(v->domain));
>> +    for_each_affinity_balance_step( bs )
>> +    {
>> +        int cpu = v->processor;
>> +
>> +        if ( bs == BALANCE_SOFT_AFFINITY &&
>> +             !has_soft_affinity(v, v->cpu_hard_affinity) )
>> +            continue;
>>  
>> -    if ( likely(cpumask_test_cpu(cpu, cpumask_scratch_cpu(cpu))) )
>> -        return cpu;
>> +        affinity_balance_cpumask(v, bs, cpumask_scratch_cpu(cpu));
>> +        cpumask_and(cpumask_scratch_cpu(cpu), cpumask_scratch_cpu(cpu),
>> +                    cpupool_domain_cpumask(v->domain));
>>  
>> -    if ( likely(cpumask_intersects(cpumask_scratch_cpu(cpu),
>> -                                   &svc->rqd->active)) )
>> -    {
>> -        cpumask_and(cpumask_scratch_cpu(cpu), &svc->rqd->active,
>> -                    cpumask_scratch_cpu(cpu));
>> -        return cpumask_first(cpumask_scratch_cpu(cpu));
>> -    }
>> +        /*
>> +         * This is cases 1 or 4 (depending on bs): if v->processor is 
>> (still)
>> +         * in our affinity, go for it, for cache betterness.
>> +         */
>> +        if ( likely(cpumask_test_cpu(cpu, cpumask_scratch_cpu(cpu))) )
>> +            return cpu;
>>  
>> -    ASSERT(!cpumask_empty(cpumask_scratch_cpu(cpu)));
>> +        /*
>> +         * This is cases 2 or 5 (depending on bs): v->processor isn't there
>> +         * any longer, check if we at least can stay in our current runq.
>> +         */
>> +        if ( likely(cpumask_intersects(cpumask_scratch_cpu(cpu),
>> +                                       &svc->rqd->active)) )
>> +        {
>> +            cpumask_and(cpumask_scratch_cpu(cpu), cpumask_scratch_cpu(cpu),
>> +                        &svc->rqd->active);
>> +            return cpumask_first(cpumask_scratch_cpu(cpu));
>> +        }
>>  
>> -    return cpumask_first(cpumask_scratch_cpu(cpu));
>> +        /*
>> +         * This is cases 3 or 6 (depending on bs): last stand, just one 
>> valid
>> +         * pcpu from our soft affinity, if we have one and if there's any. 
>> In
>> +         * fact, if we are doing soft-affinity, it is possible that we fail,
>> +         * which means we stay in the loop and look for hard affinity. OTOH,
>> +         * if we are at the hard-affinity balancing step, it's guaranteed 
>> that
>> +         * there is at least one valid cpu, and therefore we are sure that 
>> we
>> +         * return it, and never really exit the loop.
>> +         */
>> +        ASSERT(!cpumask_empty(cpumask_scratch_cpu(cpu)) ||
>> +               bs == BALANCE_SOFT_AFFINITY);
>> +        cpu = cpumask_first(cpumask_scratch_cpu(cpu));
> 
> So just checking my understanding here... at this point we're not taking
> into consideration load or idleness or anything else -- we're just
> saying, "Is there a cpu in my soft affinity it is *possible* to run on?"
>  So on a properly configured system, we should never take the second
> iteration of the loop?
> 
>> +        if ( likely(cpu < nr_cpu_ids) )
>> +            return cpu;
>> +    }
>> +    BUG_ON(1);
> 
> Do we want to BUG() here?  I don't think this constitutes an
> unrecoverable error; an ASSERT_UNREACHABLE() plus something random would
> be better, wouldn't it?

Oh, should have said, everything else looks good; but apparently I said
that before. :-)

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel