Re: [Xen-devel] [PATCH 02/15] xen: tracing: avoid checking tb_init_done multiple times.

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

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,
> >      unsigned int extra_word;
> >      bool_t started_below_highwater;
> 
> You probably want an ASSERT(tb_init_done) here to keep callers in
> check.
> 
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=0, 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=0 *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 <andrew.cooper3@xxxxxxxxxx>
> 
Thanks, let me know if this still stands.
> >  
> > -    if( !tb_init_done )
> > +    /* If the event is not interesting, bail, as early as possible
> > */
> > +    if ( (tb_event_mask & event) == 0 )
> >          return;
> >  
> >      /* Convert byte count into word count, rounding up */
>
Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel