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

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7153 bytes --]

On Tue, 2017-07-25 at 11:19 +0100, George Dunlap wrote:
> On 06/16/2017 03:13 PM, Dario Faggioli wrote:
> > 
> > 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?"
>
Exactly. If we are in this function, it means we failed to take the
locks we needed, for making a choice based on load, idleness, etc, but
we need a CPU, so we pick whatever is valid.

For choosing among all the valid ones, we act how it is explained in
the comment.

>  So on a properly configured system, we should never take the second
> iteration of the loop?
> 
Mmm.. I think you're right. In fact, in a properly configured system,
we'll never go past step 3 (from the comment at the top).

Which is not ideal, or at least not what I had in mind. In fact, I
think it's better to check step 4 (svc->vcpu->processor in hard-
affinity) and step 5 (a CPU from svc's runqueue in hard affinity), as
that would mean avoiding a runqueue migration.

What about I basically kill step 3, i.e., if we reach this point during
the soft-affinity step, I just continue to the hard-affinity one?

> > +        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?
> 
ASSERT_UNREACHABLE() is indeed much better. What do you mean with
"something random"? The value to be assigned to cpu?

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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