From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH 2/6] xen: credit: (micro) optimize csched_runq_steal(). Date: Fri, 3 Mar 2017 14:53:51 +0100 Message-ID: <1488549231.5548.216.camel@citrix.com> References: <148844531279.23452.17528540110704914171.stgit@Solace.fritz.box> <148845109247.23452.8451353979591593272.stgit@Solace.fritz.box> <0b5c692e-6d72-7183-96d6-5f51adfd7cfd@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7446320118408661756==" 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 1cjneh-0002nw-LN for xen-devel@lists.xenproject.org; Fri, 03 Mar 2017 13:53:59 +0000 In-Reply-To: <0b5c692e-6d72-7183-96d6-5f51adfd7cfd@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: anshul makkar , xen-devel@lists.xenproject.org Cc: George Dunlap List-Id: xen-devel@lists.xenproject.org --===============7446320118408661756== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-/Y976ztlJkfoZF3Jz/EQ" --=-/Y976ztlJkfoZF3Jz/EQ Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2017-03-03 at 09:48 +0000, anshul makkar wrote: > On 02/03/17 10:38, Dario Faggioli wrote: > > --- a/xen/common/sched_credit.c > > +++ b/xen/common/sched_credit.c > > @@ -708,12 +708,10 @@ static inline int > > =C2=A0 __csched_vcpu_is_migrateable(struct vcpu *vc, int dest_cpu, > > cpumask_t *mask) > > =C2=A0 { > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* Don't pick up work that's in the peer'= s scheduling tail or > > hot on > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* peer PCPU. Only pick up work that pref= ers and/or is allowed > > to run > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* on our CPU. > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* Don't pick up work that's or hot on pe= er PCPU, or that > > can't (or > Not clear. > Well, there's actually a typo (redundant 'or'). Good catch. :-) > >=20 > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* would prefer not to) run on cpu. > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ > > -=C2=A0=C2=A0=C2=A0=C2=A0return !vc->is_running && > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0!__c= sched_vcpu_is_cache_hot(vc) && > > +=C2=A0=C2=A0=C2=A0=C2=A0return !__csched_vcpu_is_cache_hot(vc) && > > =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_test_cpu(dest_cpu, mask); > !vc->is_running doesn't ease the complexity and doesn't save much on > cpu=C2=A0 > cycles. Infact, I think (!vc->is_running) makes the intention to > check=C2=A0 > for migration much more clear to understand. >=20 But the point is not saving the overhead of a !vc->is_running check here, it is actually to pull it out from within this function and check that before. And that's ok because the value won't change, and is good thing because what we save is a call to =C2=A0=C2=A0__vcpu_has_soft_affinity() and, potentially, to =C2=A0=C2=A0csched_balance_cpumask() i.e., more specifically... =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * > > @@ -1633,8 +1633,9 @@ csched_runq_steal(int peer_cpu, int cpu, int > > pri, int balance_step) > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* vCP= Us with useful soft affinities in some sort of > > bitmap > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* or = counter. > > =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=A0if ( balance_step =3D= =3D CSCHED_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&& !__vcpu_has_soft_affinity(vc, vc- > > >cpu_hard_affinity) ) > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( vc->is_running || > > +=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(balance_step =3D=3D CSCHED_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=C2=A0&& !__vcpu_has_soft_affinity(vc, vc- > > >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= =A0=C2=A0=C2=A0continue; > > =C2=A0=C2=A0 > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0csched_bala= nce_cpumask(vc, balance_step, > > cpumask_scratch); > >=20 ...these ones here. I agree that the check was a good fit for that function, but --with the updated comments-- I don't think it's too terrible to have it outside. Or were you suggesting to have it in _both_ places? If that's the case, no... I agree it's cheap, but that would look confusing to me (I totally see myself, in 3 months, sending a patch to remove the redundant is_running check! :-P). 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) --=-/Y976ztlJkfoZF3Jz/EQ 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 iQIcBAABCAAGBQJYuXVvAAoJEBZCeImluHPu2d4QAOeV25lfioXVg+GC4CtRNU8D 0ZO1rTdqpmex8IqOsQM493k1mKjy9n1i4mHKFjVivQK7PeLzE9Xjnu1SIS8ZmZzf e74JPfu3w+qMI+C8jXW9ci6igJ3wLWHBRsVJrtU+ePjZeD1BlM+KcjZ2QfH2XFrc zreB54Fg7/mvhmTl5sfbLyKJIuBRfkJJXfG44B5ZmaZyaLga4VmH+9FMpCYsQm1x G2DGX5r/0n9mj3MlbHAXjNpNAXByVst2QlCdvdnzEZrHRkdVar5K3wN2v5FaNzIk UpCP8PLwPqLW4TLdBChGoMIfjDA8nQk3q0OMW+6rryWh8uIzZ2SLwVtsWjBHBqzW srhX2XyMoi6mfWrqUXNxOzYONPANa7Sg5zOjOwZYdJuhsFG4pYrLjb+SwIVLRWEs fKvzbyIuWJnBvmWOW0/h20pSAKI7yxVFKxwO0jlCV1b7G+ceYat8OXSz0/mdwW6n SUzJ8PZ0psW3RiFB2XnsnfQ40kKUBPW49MZAXdbltx7mCT3rzc6KBHTT28kziDMo knOmQecuJF2GblDK1P/Aog+1WT/DxYYEvnujBkziD8+ocxGda0qLCGU056TPascm 4TAAWq+I/bIaiJNWY1l7p/3vwEROsUel5r9mW6NekcGU0notiF7/XNgg+eye8NvA 5W9ebzpUCf5zJH9yDT31 =y4cJ -----END PGP SIGNATURE----- --=-/Y976ztlJkfoZF3Jz/EQ-- --===============7446320118408661756== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============7446320118408661756==--