From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v4] xen: rtds: only tickle non-already tickled CPUs Date: Wed, 2 Aug 2017 18:08:06 +0200 Message-ID: <1501690086.19956.7.camel@citrix.com> References: <1501615476-3059-1-git-send-email-mengxu@cis.upenn.edu> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1888737942665185407==" Return-path: In-Reply-To: <1501615476-3059-1-git-send-email-mengxu@cis.upenn.edu> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Meng Xu , xen-devel@lists.xen.org Cc: george.dunlap@eu.citrix.com, xumengpanda@gmail.com, Haoran Li List-Id: xen-devel@lists.xenproject.org --===============1888737942665185407== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-yqGoXC/X/4w2VjBn7emi" --=-yqGoXC/X/4w2VjBn7emi Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2017-08-01 at 15:24 -0400, Meng Xu wrote: > The initial discussion of this patch can be found at > https://lists.xenproject.org/archives/html/xen-devel/2017-02/msg02857 > .html >=20 > Changes in v4: > 1) Take Dario's suggestions: > =C2=A0=C2=A0=C2=A0Search the new->cpu first for the cpu to tickle. > =C2=A0=C2=A0=C2=A0This get rid of the if statement in previous versions. > 2) Reword the comments and commit messages. > 3) Rebased on staging branch. >=20 > Issues in v2 and v3: > Did not rebase on the latest staging branch. > Did not solve the comments/issues in v1. > Please ignore the v2 and v3. > Ok, thanks for taking care of this. I've only have two more minor comments. > diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c > index 39f6bee..5fec95f 100644 > --- a/xen/common/sched_rt.c > +++ b/xen/common/sched_rt.c > @@ -1147,9 +1147,9 @@ 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. > Either: "if there is an ile CPU, kick it." Or "if there are any idle CPUs, kick one." Feel both more accurate (it's a CPU that is idle, not a vCPU, although, yes, vcpus of the idle domain do exist, I know), and better in English. This applies to both here and below, where this line is repeated. > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0For cache benefit,we first search ne= w->cpu. > + * 2) now all pcpus are busy; > =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 * > @@ -1177,17 +1177,13 @@ 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/* The same loop also find the one with lowest p= riority */ > -=C2=A0=C2=A0=C2=A0=C2=A0for_each_cpu(cpu, ¬_tickled) > +=C2=A0=C2=A0=C2=A0=C2=A0/* > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* 1) If there are any idle vcpu, kick it. > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*=C2=A0=C2=A0=C2=A0=C2=A0For cache benefit= ,we first search new->cpu. > "we check new->cpu for first" (or "as first", I think they both can be used but I'm no native speaker). With these two adjustments, you can have my: Reviewed-by: Dario Faggioli Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-yqGoXC/X/4w2VjBn7emi 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 iQIcBAABCAAGBQJZgfjnAAoJEBZCeImluHPuzWoP/jKBUHAbSy5ql/RwzJN87aHI YdiYntsPO8xV7Wz/sO/NHKQfmmhItETBqlAXufBwVD+dcN58qY0gRb9+jQq8Mhhe RhDSaoA7uJ560YIc1M/EhZsgMggxDxnAybc2/YmzLNJiXwGG036G6Njagt5EHNeu hUT0JmIlMAuOcHeM7hdfxss4VbioUsUkawwZNQAZkiu9z+nIocLlsERS5SmIQeqo vV2L5uVbivh3nJ72N8odpxsO4uR3CfVnAagYct0wRr5KHTgkz61yFIspVOfu3aUU 89LnZT+QF3FU2bWj3Y3qzMH8GHCV+Iwtgy7Wx7b35xPYjlwkhcUo3D09EU6UKawk lhamGgpM8mYLBgyKXHaKCb2gtG2D8YYjz01Pxd2Hv1UinBa9u0WLJljXlUuafeUA x9s9PXOkxfYFM4AnHCX12ikL7YFHnjwzfRxuGMpnGXNnkhVvwLHYlOr+yr0KEZ1V HquyQReI2tmldIYYGcFmlCm9fHLhgjMzfp+puQzPYghEhG3gEq73MnlGcUHcM3Pt vsmE+RPLAobo2B+s21hs0XqR7+j1xFS9mrV5SmDIi19VkTK+4THVNR9cpNgPT7t7 nAiUKyqoSVHE0+Morzxc2FhwG8GlcJS2lRNrKJDRGkqAutSC42QC4JOM1aYLHDF0 I2kFMpuWtmEJeURcbZlq =b4Fe -----END PGP SIGNATURE----- --=-yqGoXC/X/4w2VjBn7emi-- --===============1888737942665185407== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============1888737942665185407==--