From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v2 1/3] xen: RCU: let the RCU idle timer handler run Date: Thu, 28 Sep 2017 16:08:50 +0200 Message-ID: <1506607730.5001.9.camel@citrix.com> References: <150659250903.4057.6425247157210641083.stgit@Solace.fritz.box> <150659375919.4057.11728919580033384187.stgit@Solace.fritz.box> <59CD0E420200007800180815@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3873524540970467297==" 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 1dxZWa-0004m1-Vv for xen-devel@lists.xenproject.org; Thu, 28 Sep 2017 14:10:49 +0000 In-Reply-To: <59CD0E420200007800180815@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: Stefano Stabellini , George Dunlap , Andrew Cooper , Tim Deegan , Julien Grall , xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org --===============3873524540970467297== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-AZ2CMCJ+wwj0Aq/XCf9k" --=-AZ2CMCJ+wwj0Aq/XCf9k Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, 2017-09-28 at 06:59 -0600, Jan Beulich wrote: > > > > On 28.09.17 at 12:15, wrote: > > --- a/xen/common/rcupdate.c > > +++ b/xen/common/rcupdate.c > > @@ -465,7 +465,21 @@ void rcu_idle_timer_stop() > > =C2=A0=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=A0rdp->idle_timer_active =3D false; > > -=C2=A0=C2=A0=C2=A0=C2=A0stop_timer(&rdp->idle_timer); > > + > > +=C2=A0=C2=A0=C2=A0=C2=A0/* > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* In general, as the CPU is becoming act= ive again, we don't > > need the > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* idle timer, and so we want to stop it. > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* However, in case we are here because i= dle_timer has (just) > > fired and > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* has woken up the CPU, we skip stop_tim= er() now. In fact, if > > we stop > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* it, then the TIMER_SOFTIRQ handler wou= ldn't find idle_timer > > among the > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* active timers any longer, and hence wo= n't call > > rcu_idle_timer_handler(). >=20 > I think it would help if you said explicitly that the softirq run > necessarily happens after this code ran. >=20 Ok. > > --- a/xen/common/timer.c > > +++ b/xen/common/timer.c > > @@ -332,6 +332,23 @@ void stop_timer(struct timer *timer) > > =C2=A0} > > =C2=A0 > > =C2=A0 > > +bool timer_is_expired(struct timer *timer, s_time_t now) >=20 > If you call the parameter now, why is it needed? Wouldn't it be > even more accurate if you instead used ... >=20 > > +{ > > +=C2=A0=C2=A0=C2=A0=C2=A0unsigned long flags; > > +=C2=A0=C2=A0=C2=A0=C2=A0bool ret =3D false; > > + > > +=C2=A0=C2=A0=C2=A0=C2=A0if ( !timer_lock_irqsave(timer, flags) ) > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return ret; > > + > > +=C2=A0=C2=A0=C2=A0=C2=A0if ( active_timer(timer) && timer->expires <= =3D now ) >=20 > ... NOW() here? >=20 So, the cases where you happen to need the current time multiple times, and you expect the difference between calling NOW() repeatedly, and using a value sampled at the beginning is small enough, or does not matter (and therefore you decide to save the overhead). foo() { s_time_t now =3D NOW(); ... bar(now); ... bar2(barbar, now); ... if ( timer_is_expired(timer, now) ) { ... } } This is something we do, some of the times. And a function that takes a 'now' parameter, allows both use cases: the (more natural?) one, where you pass it NOW(), and the least-overhead one, where you pass it a cached value of NOW(). But I don't feel like arguing too much about this (especially now that this is patch is the only use case). If the problem is "just" the parameter (or maybe both the parameter's and the function's) name(s), I 'd be happy to change the parameter name to 't', or 'time' (and the function to 'timer_expires_before()'), and this is my preference. But if you strongly prefer it to just use NOW() inside, I'll go for it. 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) --=-AZ2CMCJ+wwj0Aq/XCf9k 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 iQIcBAABCAAGBQJZzQJzAAoJEBZCeImluHPuzp8P/1fsUXPlJJJtaIAftvcvffCM DfMhyFBCdg+qbvySd0EuBVBiRNXXdkXRPt1yAmkuWbmlRpkqjnpoNwYjcKYrKRJN z218LmHOTQBcrd2u7f0p45ZGXGJSmKoeGVpRtNkF2bHhc4bxsUwWZ68+0/N58Cmh NPhLqjY0RIWA3thuPaanaf4nhqYQmVNN51/HJSmnp4btIVn8tLZu7pZNLnpMI6v3 UQkZGdjMSTHUenYqRTadhpz4O4YMMcQI4ZnzyrOkzXYoxRw3irRBe2jB4wOKHIp9 t8l2cBJLzZItCMbBgvoUWUs8M8WbZl2LHYCoKos72BiBbf0dtPhJZIqoG6SkL/5Z GxBbmEr/DcDlMm3UUVeYVpgzAASmegbMjbBd37rfOQ542xdKNXI6opKi8YWXooM8 WxL0885njzkrPvj7qzHjXu0G22iadQBGHQ6Ehl5olhtuIEP8DG6tgttG8qTVkozA eFujfKSPFybzPT43LEXMqyowhPzE/6lisMDbNpmyqvw0GuxoQfwCMf5VASd6D1QK MzhHD+TKyc6IWq8Vvj0pgn8SME1HE7/Bh3Y6tA4khWw3+G3r+/JXn9LkCcdoAnTS 5qNSGkv2OZc3hg/MuEerUPmQjNZzDVQR0SDMX4Rr2hRcJqGYcL1+3uCEmSQmsc/5 ZrWlAcDESqHIOOr19rv7 =RIod -----END PGP SIGNATURE----- --=-AZ2CMCJ+wwj0Aq/XCf9k-- --===============3873524540970467297== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============3873524540970467297==--