From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH 02/15] xen: tracing: avoid checking tb_init_done multiple times. Date: Wed, 7 Jun 2017 17:55:31 +0200 Message-ID: <1496850931.9462.5.camel@citrix.com> References: <149633614204.12814.14390287626133023934.stgit@Solace.fritz.box> <149633842869.12814.8051616219182929257.stgit@Solace.fritz.box> <59382DDF0200007800160770@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3487829382498408916==" Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dIdJZ-0000dt-Ts for xen-devel@lists.xenproject.org; Wed, 07 Jun 2017 15:56:10 +0000 In-Reply-To: <59382DDF0200007800160770@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: George Dunlap , xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org --===============3487829382498408916== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-DKTt9tJ3zYUwyi0yd1qL" --=-DKTt9tJ3zYUwyi0yd1qL Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 2017-06-07 at 08:46 -0600, Jan Beulich wrote: > > > > On 01.06.17 at 19:33, wrote: > >=20 > > In fact, when calling __trace_var() directly, we can > > assume that tb_init_done has been checked to be true, > > and the if is hence redundant. >=20 > The "assume" here worries me: What if there's a single place > somewhere that a grep can't easily find where no check is > present? Is it certain that ... >=20 > > @@ -691,7 +691,8 @@ void __trace_var(u32 event, bool_t cycles, > > unsigned int extra, > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned int extra_word; > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0bool_t started_below_highwater; > > =C2=A0 > > -=C2=A0=C2=A0=C2=A0=C2=A0if( !tb_init_done ) > > +=C2=A0=C2=A0=C2=A0=C2=A0/* If the event is not interesting, bail, as e= arly as possible > > */ > > +=C2=A0=C2=A0=C2=A0=C2=A0if ( (tb_event_mask & event) =3D=3D 0 ) > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return; >=20 > ... this check would always be false then (i.e. tb_event_mask is > always zero) in that case? >=20 As said when replying to Andrew, I originally put an ASSERT() there. That made me realize, though, that the existing if(!tb_init_done) is ineffective and potentially racy (or, if you want to be kind "it's a best effort kind of measure") already. In fact, even right now, without my patches, we don't hold the tracing lock when we execute that if. Nothing prevents tb_init_done to become 0 _right_after_ we saw it being 1 and decide to go ahead. This, to me, looks like an even more compelling reason to remove it. But I think I can improve the commit message so that it explains this thing that I just said above too. 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) --=-DKTt9tJ3zYUwyi0yd1qL 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 iQIcBAABCAAGBQJZOCHzAAoJEBZCeImluHPuu6UQAOpCh2PSf3c2hTNzgCzibL+7 Iv/MyK2FPxYeZFolgN5ynwICLW9adijB6aej7bbJlc/suT5wPPFpLbs2jgQ/1B/O Bb++/SvvgauaNEq9n91wKF+zitZIpL0ZsfZZYcurQ1klcpOIbFcegsipvHrOXkxg Z12Idefz76KLg+6AJpaUuCeordfUCkLmhroIDpj6O2asP5V6sEYLqjxrc0l99enn CZd6Ux7q9SHp0al0tCfWNrzuoQ+hKmGQxgoNOxm7zG//q6ABLMPsSfBHfblPCAM+ BsIDW/aaBatDAB9h8oXhDdyDnj6K9UqSh306t2NXjLJvARFfntneDBYDOqiLbJDR A16olqQmSbGM2LpiVKw7v8FZxICaIs7FbrAe8rhcb8lFZJaesB/Pd3CjLMVm4agz YYrreanPUOuJKuk0v2F2t1iu4OrT5bqRUp4GmOXCXBmRP3qPMVUoR7PF/Fsom662 NckLMTcGbasoGlWMmtj+wkhRxeCzSbxSbPSGo7U+8RXPaNxqWJmk8cjhilgNP9Pg +hjYZvnAoUC0/He31XhDBbchEUX7cnFbWUGSXxnq/2yzkZo6WwJfjszrenxUgI/H Wo+AweKUNZ9lD+sddsmLCUzwehVD8nrbyuSsFIiJq3e2q9gAFtS3g23YpCCh5vOf PjakGKbk/I/UVqOU2ey4 =wV2I -----END PGP SIGNATURE----- --=-DKTt9tJ3zYUwyi0yd1qL-- --===============3487829382498408916== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============3487829382498408916==--