From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [RTDS Patch v2 for Xen4.8] xen: rtds: only tickle non-already tickled CPUs Date: Mon, 27 Feb 2017 16:12:00 +0100 Message-ID: <1488208320.5548.82.camel@citrix.com> References: <1487973277-20854-1-git-send-email-naroahlee@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6072948413180304729==" Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1ciMyA-0002ed-8v for xen-devel@lists.xenproject.org; Mon, 27 Feb 2017 15:12:10 +0000 In-Reply-To: <1487973277-20854-1-git-send-email-naroahlee@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Haoran Li , xen-devel@lists.xenproject.org Cc: George Dunlap , mengxu@cis.upenn.edu List-Id: xen-devel@lists.xenproject.org --===============6072948413180304729== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-3wtEloh/T94do97ip+A6" --=-3wtEloh/T94do97ip+A6 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2017-02-24 at 15:54 -0600, Haoran Li wrote: > From: naroahlee >=20 > Bug Analysis: > Just kill this line above. > 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. > Therefore, always make sure that we only tickle PCPUs that have not > been tickled already. > And I'd say to wrap around the lines at a shorter threshold. `git log', for instance, indents the changelogs, and the idea would be for them to look good on 80 characters terminal. > --- a/xen/common/sched_rt.c > +++ b/xen/common/sched_rt.c > @@ -1144,9 +1144,10 @@ rt_vcpu_sleep(const struct scheduler *ops, > struct vcpu *vc) > =C2=A0 * Called by wake() and context_saved() > =C2=A0 * We have a running candidate here, the kick logic is: > =C2=A0 * Among all the cpus that are within the cpu affinity > - * 1) if the new->cpu is idle, kick it. This could benefit cache hit > - * 2) if there are any idle vcpu, kick it. > - * 3) now all pcpus are busy; > + * 1) if there are any idle vcpu, kick it. > + *=C2=A0=C2=A0=C2=A0=C2=A0For cache benefit, we first search new->cpu. > + *=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 > + * 2) now all pcpus are busy; > As Meng said, no blank line here. > =C2=A0 *=C2=A0=C2=A0=C2=A0=C2=A0among all the running vcpus, pick lowest = priority one > =C2=A0 *=C2=A0=C2=A0=C2=A0=C2=A0if snext has higher priority, kick it. > =C2=A0 * > @@ -1174,17 +1175,11 @@ runq_tickle(const struct scheduler *ops, > struct rt_vcpu *new) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cpumask_and(¬_tickled, online, new->vcpu= ->cpu_hard_affinity); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cpumask_andnot(¬_tickled, ¬_tickled, = &prv->tickled); > =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->process= or)) ) > -=C2=A0=C2=A0=C2=A0=C2=A0{ > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0SCHED_STAT_CRANK(tickled= _idle_cpu); > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cpu_to_tickle =3D new->v= cpu->processor; > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0goto out; > -=C2=A0=C2=A0=C2=A0=C2=A0} > - > -=C2=A0=C2=A0=C2=A0=C2=A0/* 2) if there are any idle pcpu, kick it */ > +=C2=A0=C2=A0=C2=A0=C2=A0/* 1) if there are any idle pcpu, kick it */ > While there, do you mind adding a full stop at the end of the sentence? > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* The same loop also find the one with low= est priority */ > -=C2=A0=C2=A0=C2=A0=C2=A0for_each_cpu(cpu, ¬_tickled) > + /* For cache benefit, we search new->cpu first */ > And this looks to me to be misindented. If you fix these things and resend, you can add (together to Meng's one): Reviewed-by: Dario Faggioli And I'm Cc-ing George, so he can also adivse if he wants, as hee is also a scheduler maintainer... not to mention that he will most likely be the one that will commit the change, so please do Cc him yourself as well when you resend the patch (I should have asked to do that before, but did not notice he was not there). 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) --=-3wtEloh/T94do97ip+A6 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 iQIcBAABCAAGBQJYtEHBAAoJEBZCeImluHPukmEQAN31eHN7Rtha3kWu11t6J3XR XuArsg0GIDOHeUnAFJbvQ4wOsR2N50BZnLYaWUQDco/Dyw2zKxpSbWlnHoSB61tA IpwjV8stzbAWu+Qm3LzJiUBCpOn1goJFigo0Dy9hKZfssoyO2hAaenumQO01qE5b GYiY8UMAUc/9yQp4t29hNp7y1ewuU+WiUxDAp1SXRniAPYIActCzASjz8pJv889a YlQsfVNnqIm64aWyDJBmiCz88IG3pD7nHlOjbjCJgzaNSbemq37o2WXWoN/ggJF+ hg6nVDkcUL0n+nJCBnkde+8egOaO8+SvowkNQfbBDJIgq7PeRkNJeCZlomMm7hZv G0IeqqMsyQ/EBWckZdDo4yIrC6tZvqWsgNMUO793tJEpF/JC6Vu4E40k4/3fnIFl 95vLO90yfmXBLE2TRbXSRbfZKjmDwy4ilqxEfoqe/tQgdpcDP9y2y+VWZ89DpwlN p6ngoGdeUDW3bwU3+vsnlUj9uUBrVqyx6WiB5Qa7z1u+EKSsvA4Ddn3e4DdWg5qU I/+YDOIzish5GsnCIxtSRyWenRab1vVqicFu/Iq1fP7tqCbqCyDTmwBwv3VpPa52 sj2sBeI+axOyxJIRspuUg/Ov8YWQ4BF0l1Te+3OIbxE+rIkLRVC5aOVRRPBwctVH 8c/Ae+8DsX3rUGq/U14Q =5EXH -----END PGP SIGNATURE----- --=-3wtEloh/T94do97ip+A6-- --===============6072948413180304729== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============6072948413180304729==--