From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH] xen: idle_loop: either deal with tasklets or go idle Date: Fri, 16 Jun 2017 12:44:01 +0200 Message-ID: <1497609841.30417.5.camel@citrix.com> References: <149745892779.20244.4770433880444010417.stgit@Solace.fritz.box> <149745919711.20244.17843343131079129783.stgit@Solace.fritz.box> <5943B8F202000078001635CC@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5642683510594127095==" 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 1dLojj-0003NX-31 for xen-devel@lists.xenproject.org; Fri, 16 Jun 2017 10:44:19 +0000 In-Reply-To: <5943B8F202000078001635CC@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Jan Beulich Cc: Andrew Cooper , Julien Grall , Stefano Stabellini , Boris Ostrovsky , xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org --===============5642683510594127095== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-+jJEH+00o+QgEI/87Fwa" --=-+jJEH+00o+QgEI/87Fwa Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2017-06-16 at 02:54 -0600, Jan Beulich wrote: > > > > On 14.06.17 at 18:53, wrote: > >=20 > > --- a/xen/arch/x86/domain.c > > +++ b/xen/arch/x86/domain.c > > @@ -112,12 +112,18 @@ static void play_dead(void) > > =C2=A0 > > =C2=A0static void idle_loop(void) > > =C2=A0{ > > +=C2=A0=C2=A0=C2=A0=C2=A0unsigned int cpu =3D smp_processor_id(); > > + > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0for ( ; ; ) > > =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 ( cpu_is_offline(sm= p_processor_id()) ) > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( cpu_is_offline(cp= u) ) > > =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=A0play_dead(); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0(*pm_idle)(); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0do_tasklet(); > > + > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* Are we here for run= ning vcpu context tasklets, or for > > idling? */ > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( unlikely(tasklet_= work_to_do(cpu)) ) >=20 > I'm not really sure about the "unlikely()" here. >=20 It's basically already there, without this patch, at the very beginning of do_tasklet(): =C2=A0=C2=A0=C2=A0=C2=A0if ( likely(*work_to_do !=3D (TASKLET_enqueued|TASK= LET_scheduled)) ) =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return; Which is right the check that I moved in tasklet_work_to_do(), and as you can see, it has the likely. So, I fundamentally kept it for consistency with old code. I actually think it does make sense, but I don't have a too strong opinion about this. > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0do_tasklet(cpu); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0else > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0(*pm_idle)(); >=20 > Please take the opportunity and drop the pointless parentheses > and indirection. >=20 Ok. > > --- a/xen/common/tasklet.c > > +++ b/xen/common/tasklet.c > > @@ -104,19 +104,11 @@ static void do_tasklet_work(unsigned int cpu, > > struct list_head *list) > > =C2=A0} > > =C2=A0 > > =C2=A0/* VCPU context work */ > > -void do_tasklet(void) > > +void do_tasklet(unsigned int cpu) > > =C2=A0{ > > -=C2=A0=C2=A0=C2=A0=C2=A0unsigned int cpu =3D smp_processor_id(); >=20 > I'm not convinced it is a good idea to have the caller pass in the > CPU > number.=20 > Yes, I know. I couldn't make up my mind about it either. I guess I get get rid of this aspect of the patch. > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned long *work_to_do =3D &per_cpu(ta= sklet_work_to_do, cpu); > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct list_head *list =3D &per_cpu(taskl= et_list, cpu); > > =C2=A0 > > -=C2=A0=C2=A0=C2=A0=C2=A0/* > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* Work must be enqueued *and* scheduled.= Otherwise there is > > no work to > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* do, and/or scheduler needs to run to u= pdate idle vcpu > > priority. > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ > > -=C2=A0=C2=A0=C2=A0=C2=A0if ( likely(*work_to_do !=3D > > (TASKLET_enqueued|TASKLET_scheduled)) ) > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return; >=20 > Perhaps it also wouldn't hurt to convert this to an ASSERT() too. >=20 Nope, I can't. It's a best effort check, and *work_to_do (which is per_cpu(tasklet_work_to_do,cpu)) can change, and the assert may fail. The code is, of course, safe, because, if we think that there's work but there's not, the list of pending tasklets will be empty (and we check that after taking the tasklet lock). > > --- a/xen/include/xen/tasklet.h > > +++ b/xen/include/xen/tasklet.h > > @@ -40,9 +40,19 @@ DECLARE_PER_CPU(unsigned long, > > tasklet_work_to_do); > > =C2=A0#define TASKLET_enqueued=C2=A0=C2=A0=C2=A0(1ul << _TASKLET_enqueu= ed) > > =C2=A0#define TASKLET_scheduled=C2=A0=C2=A0(1ul << _TASKLET_scheduled) > > =C2=A0 > > +static inline bool tasklet_work_to_do(unsigned int cpu) > > +{ > > +=C2=A0=C2=A0=C2=A0=C2=A0/* > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* Work must be enqueued *and* scheduled.= Otherwise there is > > no work to > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* do, and/or scheduler needs to run to u= pdate idle vcpu > > priority. > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ > > +=C2=A0=C2=A0=C2=A0=C2=A0return per_cpu(tasklet_work_to_do, cpu) =3D=3D= (TASKLET_enqueued| > > +=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=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0TASKLE= T_scheduled) > > ; > > +} >=20 > Wouldn't cpu_is_haltable() then also better use this new function? >=20 Mmm... Perhaps. It's certainly less code chrun. ARM code would then contain two invocations of cpu_is_haltable() (the first happens with IRQ enabled, so a second one with IRQ disabled is necessary). But that is *exactly* the same thing we do on x86 (they're just in different functions in that case). So, I reworked the patch according to these suggestions, and you can look at it below. If you like it better, I'm ok re-submitting it properly in this shape. Other thoughts anyone else? Thanks and Regards, Dario --- NOTE that, since we call do_tasklet() after having checked cpu_is_haltable(), the if in there is not likely any longer. --- diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 76310ed..86cd612 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -41,20 +41,28 @@ DEFINE_PER_CPU(struct vcpu *, curr_vcpu); =20 void idle_loop(void) { + unsigned int cpu =3D smp_processor_id(); + for ( ; ; ) { - if ( cpu_is_offline(smp_processor_id()) ) + if ( cpu_is_offline(cpu) ) stop_cpu(); =20 - local_irq_disable(); - if ( cpu_is_haltable(smp_processor_id()) ) + /* Are we here for running vcpu context tasklets, or for idling? *= / + if ( cpu_is_haltable(cpu) ) { - dsb(sy); - wfi(); + local_irq_disable(); + /* We need to check again, with IRQ disabled */ + if ( cpu_is_haltable(cpu) ) + { + dsb(sy); + wfi(); + } + local_irq_enable(); } - local_irq_enable(); + else + do_tasklet(); =20 - do_tasklet(); do_softirq(); /* * We MUST be last (or before dsb, wfi). Otherwise after we get th= e diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index 49388f4..c520fdd 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -112,12 +112,18 @@ static void play_dead(void) =20 static void idle_loop(void) { + unsigned int cpu =3D smp_processor_id(); + for ( ; ; ) { - if ( cpu_is_offline(smp_processor_id()) ) + if ( cpu_is_offline(cpu) ) play_dead(); - (*pm_idle)(); - do_tasklet(); + + /* Are we here for idling, or for running vcpu context tasklets? *= / + if ( cpu_is_haltable(cpu) ) + pm_idle(); + else + do_tasklet(); do_softirq(); /* * We MUST be last (or before pm_idle). Otherwise after we get the diff --git a/xen/common/tasklet.c b/xen/common/tasklet.c index 365a777..ebdce12 100644 --- a/xen/common/tasklet.c +++ b/xen/common/tasklet.c @@ -114,7 +114,7 @@ void do_tasklet(void) * Work must be enqueued *and* scheduled. Otherwise there is no work t= o * do, and/or scheduler needs to run to update idle vcpu priority. */ - if ( likely(*work_to_do !=3D (TASKLET_enqueued|TASKLET_scheduled)) ) + if ( *work_to_do !=3D (TASKLET_enqueued|TASKLET_scheduled) ) return; =20 spin_lock_irq(&tasklet_lock); --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-+jJEH+00o+QgEI/87Fwa 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 iQIcBAABCAAGBQJZQ7Z0AAoJEBZCeImluHPuJqIP/2MMPVeHKlwSugNrrgBEbbTT zmDICX97doe9/o5fnqhSYegTr2/7/541tHVV2hdZFUqJVGrKLuKLw+lBZm2p2GWg e3EM/e7T4LQ9yLFJ//cyxXxSE3X+FxaQvCxaIsUyf3LIG/z9oNvkQdnTmy8UCWov RHPKm3TVe1jtcL9yuMC/2rd9bcUoopfDsxow5RsNhLZeADm6D0HBqgVmk/ezkjFS M0RbWfv892iON4U3hs1rSKqXNM49l+aYLYJEkyJnW9mnnABIq+5qGcRsUf9dImgW 4atnk9CH0GWY5xzoXULCgCTvCtiUH+MOPjhcda3X1PqETCt63OVHY1Du3oG8HFZw T+vl5ItZhinPwYxDF+wP0iaE9u4cd/Yj6r/NdSCxOUTcs+j4YPMUPMkhbHGsLV4d yEVEgGxp5FuIb8RvEs6FGPJWSgDTnewPFrh6EMQCltwEgVt0L3CaVo0xgph7BMWq dO4me8m0h96sJeCfIjmWObCBsFJpBLgF2sylhHdSanU4kQWr5OIQHktwfzbTV2bK fj6Sah7rSxzuZI5C5OPEXHROP+jKyXA0Hrl2NoLTNQe+OFTqWRkdJf4QqBMpuJd0 PFRX/4AgOqA/nfkdParJ+9FuSHFAYRRTFUWNznLP5YFZ0MwjS4a/BOiTMt4rV3fW Jal6RAoCXJyjW3Yz7yOo =rePq -----END PGP SIGNATURE----- --=-+jJEH+00o+QgEI/87Fwa-- --===============5642683510594127095== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============5642683510594127095==--