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

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

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@citrix.com>
> Signed-off-by: Justin T. Weaver <jtweaver@hawaii.edu>
> ---
> Cc: Anshul Makkar <anshul.makkar@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> 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?

 -George

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