From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH 3/7] xen: credit2: soft-affinity awareness in fallback_cpu() Date: Tue, 25 Jul 2017 18:00:59 +0200 Message-ID: <1500998459.26429.4.camel@citrix.com> References: <149762114626.11899.6393770850121347748.stgit@Solace.fritz.box> <149762243723.11899.13163340131516329714.stgit@Solace.fritz.box> <45a42cae-9683-9fd7-690f-6f5424e24ea9@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1555010911485322094==" Return-path: Received: from mail6.bemta6.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1da2Gy-0001At-76 for xen-devel@lists.xenproject.org; Tue, 25 Jul 2017 16:01:24 +0000 In-Reply-To: <45a42cae-9683-9fd7-690f-6f5424e24ea9@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: George Dunlap , xen-devel@lists.xenproject.org Cc: George Dunlap , Anshul Makkar , "Justin T. Weaver" List-Id: xen-devel@lists.xenproject.org --===============1555010911485322094== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-sdDf+mlZOjHucPv6BX2b" --=-sdDf+mlZOjHucPv6BX2b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2017-07-25 at 11:19 +0100, George Dunlap wrote: > On 06/16/2017 03:13 PM, Dario Faggioli wrote: > >=20 > > 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) > > =C2=A0} > > =C2=A0 > > =C2=A0/* > > - * 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: > > - *=C2=A0=C2=A0- svc's current pcpu; > > - *=C2=A0=C2=A0- another pcpu from svc's current runq; > > - *=C2=A0=C2=A0- 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: > > + *=C2=A0=C2=A01) svc's current pcpu, if it is part of svc's soft affin= ity; > > + *=C2=A0=C2=A02) a pcpu in svc's current runqueue that is also in svc'= s soft > > affinity; > > + *=C2=A0=C2=A03) just one valid pcpu from svc's soft affinity; > > + *=C2=A0=C2=A04) svc's current pcpu, if it is part of svc's hard affin= ity; > > + *=C2=A0=C2=A05) a pcpu in svc's current runqueue that is also in svc'= s hard > > affinity; > > + *=C2=A0=C2=A06) 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. > > =C2=A0 */ > > =C2=A0static int get_fallback_cpu(struct csched2_vcpu *svc) > > =C2=A0{ > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct vcpu *v =3D svc->vcpu; > > -=C2=A0=C2=A0=C2=A0=C2=A0int cpu =3D v->processor; > > +=C2=A0=C2=A0=C2=A0=C2=A0unsigned int bs; > > =C2=A0 > > -=C2=A0=C2=A0=C2=A0=C2=A0cpumask_and(cpumask_scratch_cpu(cpu), v->cpu_h= ard_affinity, > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0cpupool_domain_cpumask(v->domain)); > > +=C2=A0=C2=A0=C2=A0=C2=A0for_each_affinity_balance_step( bs ) > > +=C2=A0=C2=A0=C2=A0=C2=A0{ > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int cpu =3D v->process= or; > > + > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( bs =3D=3D BALANCE= _SOFT_AFFINITY && > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0!has_soft_affinity(v, v->cpu_hard_affinity) ) > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0continue; > > =C2=A0 > > -=C2=A0=C2=A0=C2=A0=C2=A0if ( likely(cpumask_test_cpu(cpu, cpumask_scra= tch_cpu(cpu))) ) > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return cpu; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0affinity_balance_cpuma= sk(v, bs, cpumask_scratch_cpu(cpu)); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cpumask_and(cpumask_sc= ratch_cpu(cpu), > > cpumask_scratch_cpu(cpu), > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cpupool_domain_cpumask(v= ->domain)); > > =C2=A0 > > -=C2=A0=C2=A0=C2=A0=C2=A0if ( likely(cpumask_intersects(cpumask_scratch= _cpu(cpu), > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0&svc->rqd= ->active)) ) > > -=C2=A0=C2=A0=C2=A0=C2=A0{ > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cpumask_and(cpumask_sc= ratch_cpu(cpu), &svc->rqd->active, > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cpumask_scratch_cpu(cpu)= ); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return cpumask_first(c= pumask_scratch_cpu(cpu)); > > -=C2=A0=C2=A0=C2=A0=C2=A0} > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* This is cases = 1 or 4 (depending on bs): if v->processor=20 > > is (still) > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* in our affinit= y, go for it, for cache betterness. > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( likely(cpumask_te= st_cpu(cpu, > > cpumask_scratch_cpu(cpu))) ) > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0return cpu; > > =C2=A0 > > -=C2=A0=C2=A0=C2=A0=C2=A0ASSERT(!cpumask_empty(cpumask_scratch_cpu(cpu)= )); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* This is cases = 2 or 5 (depending on bs): v->processor > > isn't there > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* any longer, ch= eck if we at least can stay in our > > current runq. > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( likely(cpumask_in= tersects(cpumask_scratch_cpu(cpu), > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0&svc->rqd->active)) ) > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0{ > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0cpumask_and(cpumask_scratch_cpu(cpu), > > cpumask_scratch_cpu(cpu), > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= &svc->rqd->active); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0return cpumask_first(cpumask_scratch_cpu(cpu)); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > > =C2=A0 > > -=C2=A0=C2=A0=C2=A0=C2=A0return cpumask_first(cpumask_scratch_cpu(cpu))= ; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* This is cases = 3 or 6 (depending on bs): last stand, > > just one valid > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* pcpu from our = soft affinity, if we have one and if > > there's any. In > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* fact, if we ar= e doing soft-affinity, it is possible > > that we fail, > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* which means we= stay in the loop and look for hard > > affinity. OTOH, > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* if we are at t= he hard-affinity balancing step, it's > > guaranteed that > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* there is at le= ast one valid cpu, and therefore we are > > sure that we > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* return it, and= never really exit the loop. > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ASSERT(!cpumask_empty(= cpumask_scratch_cpu(cpu)) || > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0bs =3D=3D BALANCE_SOFT_AFFINITY); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cpu =3D cpumask_first(= cpumask_scratch_cpu(cpu)); >=20 > 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. > =C2=A0So on a properly configured system, we should never take the second > iteration of the loop? >=20 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? > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( likely(cpu < nr_c= pu_ids) ) > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0return cpu; > > +=C2=A0=C2=A0=C2=A0=C2=A0} > > +=C2=A0=C2=A0=C2=A0=C2=A0BUG_ON(1); >=20 > Do we want to BUG() here?=C2=A0=C2=A0I don't think this constitutes an > unrecoverable error; an ASSERT_UNREACHABLE() plus something random > would > be better, wouldn't it? >=20 ASSERT_UNREACHABLE() is indeed much better. What do you mean with "something random"? The value to be assigned to cpu? Thanks and Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-sdDf+mlZOjHucPv6BX2b Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJZd2s8AAoJEBZCeImluHPuepIQAI60x/p8MQl9tDQnvMXq9ALl nDveYaK+3DXqZQylrp9cbCIkcz7TeMDUSGVHwkiAMyJfGoYCFMrlqabFkimkVF/W thb5qgI6vLvPoKqeRxD5LZfKRzU6DCwTLYn1jnbIrf/mj65FmZ0KNfnPCSiVJiuX WK+BODjcuIp5yubbU3za4hdCqRPPiIQ0Mg4zdUL97nERX4oHhXchf4u3cVWDHXmc yIHvrClzgLaycriJ7oX+5/ag6cC3gujC+tD5tBL0cS3cUuxt5jttEu09J+70DPw/ qawSKRniozzB5YlijtOSxuHzwoYti3Q4eO1VqS7SDM5xJIhHW8mUSZT/snAnTKms f8OXmCy5ics5DaUz/AkosQ0zZoVxmbaxICbS0x5YOeBtUmJ/wlS0xI00H3RiOvcR drDg/oJRTWF1osmg0ifcQzYsYkf+gF1Sm/j8rNsaVoOqzO69pzCSzr/uY0oEhV/p 5z5AabzaOFjjBQt2iDUmAAol+gu839ktiENb+HdN6F/14sq/sOzv+JoRfrHw33UK 6WJVwcJSu9NVhO+IJyFShAVaUQ40Ax9khlDjMEmeA2pRMaqTvqZiruSNuSPHjriS urfRszPr33lVti8Opgc8kO8pmjq0Zk3tWrcf8vTF12wn5fMT9kdsfN6uRYJaFOFP 7SjtEFnd3h1mchsxmjg+ =yKWy -----END PGP SIGNATURE----- --=-sdDf+mlZOjHucPv6BX2b-- --===============1555010911485322094== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============1555010911485322094==--