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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.06.17 at 17:55, <dario.faggioli@xxxxxxxxxx> wrote:
> On Wed, 2017-06-07 at 08:46 -0600, Jan Beulich wrote:
>> > > > On 01.06.17 at 19:33, <dario.faggioli@xxxxxxxxxx> wrote:
>> > 
>> > 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.
>> 
>> 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 ...
>> 
>> > @@ -691,7 +691,8 @@ void __trace_var(u32 event, bool_t cycles,
>> > unsigned int extra,
>> >      unsigned int extra_word;
>> >      bool_t started_below_highwater;
>> >  
>> > -    if( !tb_init_done )
>> > +    /* If the event is not interesting, bail, as early as possible
>> > */
>> > +    if ( (tb_event_mask & event) == 0 )
>> >          return;
>> 
>> ... this check would always be false then (i.e. tb_event_mask is
>> always zero) in that case?
>> 
> 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.

Well, my question wasn't about a possible race (as the code would
need to be able to deal with that no matter what change you do
here), but about the case where tb_init_done has never been set.
Would tb_event_mask be reliably zero in that case, no matter
what other operations may have been performed?

Jan


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