|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 3/7] xen: credit2: soft-affinity awareness in fallback_cpu()
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:
signature.asc _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |