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

[George Dunlap <george.dunlap@citrix.com>]

On 07/06/17 17:06, Jan Beulich wrote:
>>>> On 07.06.17 at 17:55, <dario.faggioli@citrix.com> wrote:
>> On Wed, 2017-06-07 at 08:46 -0600, Jan Beulich wrote:
>>>>>> On 01.06.17 at 19:33, <dario.faggioli@citrix.com> 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?

Looks like tb_event_mask can be set even if opt_tbuf_size == 0; so yes,
if someone enabled the event mask before allocating any buffers, *and*
there was a bug where __trace_var() was called without first checking
tb_init_done, then we might get past this point.

But it looks like that's still OK -- we'll then bail when our bit in
tb_cpu_mask isn't set, or finally bail when we find t_bufs to be zero.

 -George



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