| Message ID | 149633842869.12814.8051616219182929257.stgit@Solace.fritz.box (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/06/17 18:33, Dario Faggioli 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. > > While there, also: > - still in __trace_var(), move the check that the event > is actually being traced up a little bit (to bail as > soon as possible, if it is not); > - make it explicit that tb_init_done is likely 0 in > trace_will_trace_event(), as it is almost everywhere > in the code. > > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> > --- > Cc: George Dunlap <george.dunlap@eu.citrix.com> > --- > xen/common/trace.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > 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 > @@ -311,7 +311,7 @@ static int tb_set_size(unsigned int pages) > > int trace_will_trace_event(u32 event) > { > - if ( !tb_init_done ) > + if ( likely(!tb_init_done) ) > return 0; > > /* > @@ -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. Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > - 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 */ > @@ -705,9 +706,6 @@ void __trace_var(u32 event, bool_t cycles, unsigned int extra, > /* Round size up to nearest word */ > extra = extra_word * sizeof(u32); > > - if ( (tb_event_mask & event) == 0 ) > - return; > - > /* match class */ > if ( ((tb_event_mask >> TRC_CLS_SHIFT) & (event >> TRC_CLS_SHIFT)) == 0 ) > return; > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
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@citrix.com> > 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
>>> 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? Jan
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. Thanks and Regards, Dario
>>> 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? Jan
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
On 01/06/17 18:33, Dario Faggioli 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. You probably want to adjust the comment before the smp_rmb() that mentions tb_init_done as well. Other than that, looks good. -George
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 @@ -311,7 +311,7 @@ static int tb_set_size(unsigned int pages) int trace_will_trace_event(u32 event) { - if ( !tb_init_done ) + if ( likely(!tb_init_done) ) return 0; /* @@ -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; /* Convert byte count into word count, rounding up */ @@ -705,9 +706,6 @@ void __trace_var(u32 event, bool_t cycles, unsigned int extra, /* Round size up to nearest word */ extra = extra_word * sizeof(u32); - if ( (tb_event_mask & event) == 0 ) - return; - /* match class */ if ( ((tb_event_mask >> TRC_CLS_SHIFT) & (event >> TRC_CLS_SHIFT)) == 0 ) return;
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. While there, also: - still in __trace_var(), move the check that the event is actually being traced up a little bit (to bail as soon as possible, if it is not); - make it explicit that tb_init_done is likely 0 in trace_will_trace_event(), as it is almost everywhere in the code. Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> --- Cc: George Dunlap <george.dunlap@eu.citrix.com> --- xen/common/trace.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)