[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 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |