From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [RTDS Patch for Xen4.8] xen: sched_rt.c Check not_tickled Mask Date: Fri, 24 Feb 2017 17:58:29 +0100 Message-ID: <1487955509.5548.49.camel@citrix.com> References: Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5386992862945647159==" 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 1chJCX-0004Cm-Uc for xen-devel@lists.xenproject.org; Fri, 24 Feb 2017 16:58:38 +0000 In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Meng Xu , Haoran Li Cc: "xen-devel@lists.xenproject.org" List-Id: xen-devel@lists.xenproject.org --===============5386992862945647159== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-jIOcGP8JuB7/UzpW5mVO" --=-jIOcGP8JuB7/UzpW5mVO Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 2017-02-22 at 17:59 -0500, Meng Xu wrote: > Hi Haoran, >=20 > Thank you for sending this patch out quickly! :-) >=20 > The title can be > [PATCH] xen: rtds: only tickle the same cpu once >=20 Or: xen: rtds: only tickle non-already tickled CPUs (Nitpicking, I know, but I don't like how "the same" sounds in there.) > On Wed, Feb 22, 2017 at 5:16 PM, Haoran Li > wrote: > >=20 > > Bug Analysis: > > We need to exclude tickled VCPUs when trying to evaluate > > runq_tickle() case 1 >=20 > Change the description to the following: >=20 > When more than one idle VCPUs that have the same PCPU as their > previous running core invoke runq_tickle(), they will tickle the same > PCPU. The tickled PCPU will only pick at most one VCPU, i.e., the > highest-priority one, to execute. The other VCPUs will not be > scheduled for a period, even when there is an idle core, making these > VCPUs unnecessarily starve for one period. >=20 Agreed. > To fix this issue, we should always tickle the non-tickled PCPU in > the > runq_tickle(). >=20 I'd change this sentence in something like: "Therefore, always make sure that we only tickle PCPUs that have not been tickled already." > > --- a/xen/common/sched_rt.c > > +++ b/xen/common/sched_rt.c > > @@ -1175,7 +1175,8 @@ runq_tickle(const struct scheduler *ops, > > struct rt_vcpu *new) > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cpumask_andnot(¬_tickled, ¬_tickled= , &prv->tickled); > >=20 > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* 1) if new's previous cpu is idle, kick= it for cache benefit > > */ > > -=C2=A0=C2=A0=C2=A0=C2=A0if ( is_idle_vcpu(curr_on_cpu(new->vcpu->proce= ssor)) ) > > +=C2=A0=C2=A0=C2=A0=C2=A0if ( is_idle_vcpu(curr_on_cpu(new->vcpu->proce= ssor)) && > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cpumask_test_cpu= (new->vcpu->processor, ¬_tickled)) >=20 > You should have a space before the last ). >=20 Indeed. But it looks to me that we can take the chance to tweak the code a little bit, get rid of the special casing, and by that making it more compact (and hence easier to read), and maybe a tiny bit more efficient too. I'm thinking about getting rid entirely of the 'if' above, and then transforming the loop into something like this: =C2=A0 =C2=A0 cpu =3D cpumask_test_or_cycle(new->vcpu->processor, =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 ¬_tickled); =C2=A0 =C2=A0 while ( cpu !=3D nr_cpu_ids ) =C2=A0 =C2=A0 { =C2=A0 =C2=A0 =C2=A0 =C2=A0 iter_vc =3D curr_on_cpu(cpu); =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* ... existing loop body ... */ =C2=A0 =C2=A0 =C2=A0 =C2=A0 cpumask_clear_cpu(cpu, ¬_tickle); =C2=A0 =C2=A0 =C2=A0 =C2=A0 cpu =3D cpumask_cycle(cpu, ¬_tickled); =C2=A0 =C2=A0 } (I do thinks this is correct, but please, do double check.) Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-jIOcGP8JuB7/UzpW5mVO 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 iQIcBAABCAAGBQJYsGY1AAoJEBZCeImluHPuldgP/jphA13w9jet/XShzZ9v9J9a S98CoAUAbVQ/E5FtSv10r+9DYQRSS59en7iirugfQ8s0zdmZKKrVJAsU10JHmzcA KJNAAPDuKAJ6MqtcvzDcCBEEr2nS9gPwXb2wobRrlfDK/hgX649HgGyACPEbXSma ykFKe/x0pqDTRiGjigIGMu0vNRyCti9RWgE3cBJQ+ExKKzhWBdmR8ZrhVfOHW0HG r/61xRWpX6xtE6tnVjE47aXFiy5J2b/p5ncwy0BkoB+el+gtjbBwnf47SsRVHBn4 nPs2pgN90pcboAPfIKv19K4ZuapjU0eqRPMYArqxcxBfinWB901QLtLnuygTRYL5 faaVOFDF9omlisdNz4ZsDcfTNCeLCLY9shiq+kH6GnjVElM5lgeVQq8lY9Y7OLdi wJ7rYBcNur3IS2W2AyNNK9MhhdoTCxg/bNqIV4Q/ClVCkkAke1yIzx8Z9IvmpkJX r1JlHN9/5XhHKemRrvhVRud/w9hh4sFqQ7ATywN5fM1uWV+NeSdgWdDdJI8DhEET 4JyUiJxZMWdQ8WmT9e/73575UNbGtlXDN2HlyG4TT5nnEPPQ6zUPkGuVAhcKcJMb Wev7qvFlM45vtBsB7FWAEVnN6LZR0RnOkiaXtaUTBhu35egCMl21WwDGQiK01DPW bW8lvehKBfjLDNoCB4p8 =PGR4 -----END PGP SIGNATURE----- --=-jIOcGP8JuB7/UzpW5mVO-- --===============5386992862945647159== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============5386992862945647159==--