From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH 3/5] xen: RCU/x86/ARM: discount CPUs that were idle when grace period started. Date: Wed, 9 Aug 2017 10:48:59 +0200 Message-ID: <1502268539.5719.13.camel@citrix.com> References: <150114201043.22910.12807057883146318803.stgit@Solace> <150114248433.22910.16140726025093688678.stgit@Solace> <5988264402000078001038B6@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6550955133103077457==" 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 1dfMfx-0001gw-CD for xen-devel@lists.xenproject.org; Wed, 09 Aug 2017 08:49:13 +0000 In-Reply-To: <5988264402000078001038B6@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.cooper3@citrix.com, julien.grall@arm.com, sstabellini@kernel.org, xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org --===============6550955133103077457== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-rQCmHKyOiE6xn39mQPw+" --=-rQCmHKyOiE6xn39mQPw+ Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, 2017-08-07 at 02:35 -0600, Jan Beulich wrote: > > > > Dario Faggioli 07/27/17 10:01 AM > > > > >>> > > --- a/xen/arch/x86/acpi/cpu_idle.c > > +++ b/xen/arch/x86/acpi/cpu_idle.c > >=20 > > @@ -433,12 +435,14 @@ static void acpi_idle_do_entry(struct > > acpi_processor_cx *cx) > > because chipsets cannot guarantee that STPCLK# signal > > gets asserted in time to freeze execution properly. */ > > inl(pmtmr_ioport); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return; > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0break; >=20 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0>case ACPI_CSTATE_EM_HALT: >=20 > Wiuld be nice if you also added blank lines between the break-s and > the > subsequent case labels. >=20 Ok. > > @@ -787,6 +792,8 @@ static void mwait_idle(void) > > irq_traced[0], irq_traced[1], irq_traced[2], irq_traced[3]); >=20 > =C2=A0> > > /* Now back in C0. */ > > +=C2=A0=C2=A0=C2=A0=C2=A0rcu_idle_exit(cpu); > > + > > update_idle_stats(power, cx, before, after); > > local_irq_enable(); >=20 > =C2=A0 > Compared to the ACPI code, the window is quite a bit larger here. Any > reason > you can't put these just around the mwait_idle_with_hints() > invocation, which > would match what you do for the ACPI-based driver? >=20 No, no reasons not to do that. I'll do it. > > @@ -248,7 +249,14 @@ static void rcu_start_batch(struct rcu_ctrlblk > > *rcp) > > smp_wmb(); > > rcp->cur++; >=20 > =C2=A0> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cpumask_copy(&rcp->cpu= mask, &cpu_online_map); > > +=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* Accessing idle_cpuma= sk before incrementing rcp->cur > > needs a > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* Barrier=C2=A0=C2=A0O= therwise it can cause tickless idle CPUs to be > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* included in rcp->cpu= mask, which will extend graceperiods > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* unnecessarily. > > +=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=A0smp_mb(); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cpumask_andnot(&rcp->c= pumask, &cpu_online_map, &rcp- > > >idle_cpumask); >=20 > I have some trouble with understanding the comment: You don't access > ->idle_cpumask before you increment ->cur. > It comes verbatim from the Linux commit. You're not the first one that finds it unclear, and I don't like it either. So, this is the Linux patch: if (rcp->next_pending && rcp->completed =3D=3D rcp->cur) { - /* Can't change, since spin lock held. */ - cpus_andnot(rsp->cpumask, cpu_online_map, nohz_cpu_mask); - rcp->next_pending =3D 0; - /* next_pending =3D=3D 0 must be visible in __rcu_process_c= allbacks() - * before it can see new value of cur. + /* + * next_pending =3D=3D 0 must be visible in + * __rcu_process_callbacks() before it can see new value of= cur. */ smp_wmb(); rcp->cur++; + + /* + * Accessing nohz_cpu_mask before incrementing rcp->cur nee= ds a + * Barrier Otherwise it can cause tickless idle CPUs to be + * included in rsp->cpumask, which will extend graceperiods + * unnecessarily. + */ + smp_mb(); + cpus_andnot(rsp->cpumask, cpu_online_map, nohz_cpu_mask); + _I_think_ what the original author meant was something along the line of <cur is unsafe. Therefore, let's access it afterwords, and put a barrier in between.>> But yeah, as said, I don't like it myself. In fact, it's the same exact wording used in the changelog of the patch (Linux commit c3f5902325d3053986e7359f706581d8f032e72f), but while it is fine there, here is completely misleading, as it does not comment/describe the final look of the code. I'm going to change it. > (Also, as a general remark, > please go through patch description and comments again to correct > spelling and alike issues.) >=20 Sure. Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-rQCmHKyOiE6xn39mQPw+ 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 iQIcBAABCAAGBQJZisx9AAoJEBZCeImluHPuxK0P/0NTDBJUmG4gtIcAZSf17vSq zU/oSD8UuG5b9rBjc95+1z+JYu29Ukior0SAHnKmRIkclF2IrG9LbscohdrGZyRJ tpncR2CKJFM6qLAyzu+6ZXxOtLxjePMYh9N8/E1u4CuoJzeiy+7SLZle+/woQjYb Wr4aOUF8uZgZrTEZaTagI/xWsanlLkiB0vMPEnmAfLRJNuei8RkkzakYJgWOpgK5 22MGSyVsdT1wCFB0HgNdcL5wi2SWw45nDrKx78o1dA+qsBSnz1vfiTu3ZakYvVMH kRPK6UzsP4XA+jjg56z3KDHJgIPFEP/u1Oj4ZUjababuCOCrRBsQU+8jyoezOzo9 MKsPZWrCFtP+efyltiIh26/3jfZHpPlDPLmBoVDsZT9uWoyU7BCP1cS/E5QW3LVf 5sHCnKXigTjOQJEMkFICq7tGMG3ixz6owX66aJGBRLIml+21Kn4zBpBvuqjR9UA4 YTdPWHSWs3XUbmFyqJU72Wy84pwWjw5fs6DZb7yQQq0btil1cbwVdtG3J6ZjKhV2 e6zfX6LjDGX8cfbIxjGlH8ZYPawYs4REpu8Cb+WecYpYLeY6eySve2746gQdGg3m 2I3T8WnAmDrvYyVB1TdaFAy/vtqNv1GazMwjCUnPKGx6Gp31h6jSHfB+h1m47Mcp Jg+FzJYU5NOvdXxAUM5q =7E3M -----END PGP SIGNATURE----- --=-rQCmHKyOiE6xn39mQPw+-- --===============6550955133103077457== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============6550955133103077457==--