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: Fri, 2 Jun 2017 01:08:29 +0200 Message-ID: <1496358509.10189.7.camel@citrix.com> References: <149633614204.12814.14390287626133023934.stgit@Solace.fritz.box> <149633842869.12814.8051616219182929257.stgit@Solace.fritz.box> <4d7d42e7-551b-d6d3-b066-ac204dcebd48@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6773666824405446551==" 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 1dGZCu-00079X-58 for xen-devel@lists.xenproject.org; Thu, 01 Jun 2017 23:08:44 +0000 In-Reply-To: <4d7d42e7-551b-d6d3-b066-ac204dcebd48@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Andrew Cooper , xen-devel@lists.xenproject.org Cc: George Dunlap List-Id: xen-devel@lists.xenproject.org --===============6773666824405446551== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-bQa1ID4Jlfd5NIwEakMB" --=-bQa1ID4Jlfd5NIwEakMB Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, 2017-06-01 at 18:53 +0100, Andrew Cooper wrote: > On 01/06/17 18:33, Dario Faggioli wrote: > > diff --git a/xen/common/trace.c b/xen/common/trace.c > > index 4fedc26..f29cd4c 100644 > > --- a/xen/common/trace.c > > +++ b/xen/common/trace.c > > @@ -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; >=20 > You probably want an ASSERT(tb_init_done) here to keep callers in > check. >=20 Indeed! And in fact, I did have one. But only to discover that it triggers! :-O After looking at this better, I figured out that the existing check (the one that this patch kills) is not just redundant, but only a best effort measure. In fact, we still haven't acquired any locks here. If I put the ASSERT() and, between when I call __trace_var() and when the ASSERT() is reached, someone manages to set tb_init_done=3D0, the ASSERT(), as said, fires. However, that is no better in current code. In fact, it is very well possible that someone manages to set tb_init_done=3D0 *right after*, here in __trace_var(), we check it with the if. Actually, I think having the check there is misleading, because it makes one think that tb_init_done is guaranteed to be and stay 0 below it, while that isn't true at all. And this is therefore just another reason to get rid of it, IMO. But, sadly, I can't replace it with an ASSERT(). :-( > Otherwise, Reviewed-by: Andrew Cooper >=20 Thanks, let me know if this still stands. > > =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; > > =C2=A0 > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* Convert byte count into word count, ro= unding up */ > Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-bQa1ID4Jlfd5NIwEakMB 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 iQIcBAABCAAGBQJZMJ5uAAoJEBZCeImluHPukBwQANFxuwTlMwrdKGTJWOX9sxct 1C0jQJlV3EXbFycO3UMVqgQgBu3OW3CNw/pPxbyDqvDYO89+43gOI09UE8+6Ry+5 5hK2rPoVXkdvAOw/erXwYEN/tn5FSE1JV68XRs6zKnbDLBKvzp0n4DAe8zryNq8D unhWhKzQnx6aXwoLeU3rlWco0UU5ksews4PnTlYqZ6/T2ARNECOghg7Le/YwANal xD6aO7BA3FB9e6+U/+VAkjaqcpkApTmhmNK9REh4mDssMl8aFBrrHZIKhdz0GOQX PIQDgVRwFL4NXXW4NhgpXf6EfQIlThv4LlWaOTdf42vFAz9+nzxt3/dSNJKIW0D9 6XQ0ZIi6iZVU/mNYFHiqgc8ujd6mDe6kBHUovfvridI3TS0MqERXNcwvZb7hVcxA rn20tIMK7g0OJsOhTYg9Q9P3xMbYKh86p5IP+Irf5J04ZKyNDXh42x5OenLhn65J WU/M1OZfN2pm6roryLx2eLkEZUxFg3mDY4M751fwGoNH3SdyKwXIHDwTGK4AGI4I Y8AfugNyS7cYMpKIk5A/WcqA1SbLzn7YDd3meyc+4LmF9bHsOmCVkjIdwSFtvHf0 obuOSJE+zBwJjW5ma5dxi98aytA5vHS9OEp8ur6JRujmiB8NJh49hO+ha8UxLoIA HJSsq8UsfUFWAqFEE7cV =yLmP -----END PGP SIGNATURE----- --=-bQa1ID4Jlfd5NIwEakMB-- --===============6773666824405446551== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============6773666824405446551==--