Re: [PATCH v8 00/37] tracing: Inter-event (e.g. latency) support

[Tom Zanussi]

Hi Steve,

On Mon, 2018-01-15 at 17:43 -0500, Steven Rostedt wrote:
> On Wed, 10 Jan 2018 09:18:30 -0600
> Tom Zanussi  wrote:
> 
> > I'll add these changes to a v9 along with another small nit found by the
> > kbuild bot (parse_atom() can be static) and repost soon.
> 
> Hi Tom,
> 
> Could you post v9 soon? I'd like to start pulling them into my tree.
> 

Sure, coming up in a few minutes...

Tom

> Thanks!
> 
> -- Steve

Re: [PATCH v8 00/37] tracing: Inter-event (e.g. latency) support

[Tom Zanussi]

Hi Steve,

On Mon, 2018-01-15 at 17:43 -0500, Steven Rostedt wrote:
> On Wed, 10 Jan 2018 09:18:30 -0600
> Tom Zanussi  wrote:
> 
> > I'll add these changes to a v9 along with another small nit found by the
> > kbuild bot (parse_atom() can be static) and repost soon.
> 
> Hi Tom,
> 
> Could you post v9 soon? I'd like to start pulling them into my tree.
> 

Sure, coming up in a few minutes...

Tom

> Thanks!
> 
> -- Steve

Re: [PATCH v8 00/37] tracing: Inter-event (e.g. latency) support

[Steven Rostedt]

On Wed, 10 Jan 2018 09:18:30 -0600
Tom Zanussi  wrote:

> I'll add these changes to a v9 along with another small nit found by the
> kbuild bot (parse_atom() can be static) and repost soon.

Hi Tom,

Could you post v9 soon? I'd like to start pulling them into my tree.

Thanks!

-- Steve

Re: [PATCH v8 00/37] tracing: Inter-event (e.g. latency) support

[Steven Rostedt]

On Wed, 10 Jan 2018 09:18:30 -0600
Tom Zanussi  wrote:

> I'll add these changes to a v9 along with another small nit found by the
> kbuild bot (parse_atom() can be static) and repost soon.

Hi Tom,

Could you post v9 soon? I'd like to start pulling them into my tree.

Thanks!

-- Steve

Re: [PATCH v8 00/37] tracing: Inter-event (e.g. latency) support

[Tom Zanussi]

On Wed, 2018-01-10 at 17:16 +0900, Namhyung Kim wrote:
> Hi Steve,
> 
> On Wed, Jan 10, 2018 at 02:20:54AM -0500, Steven Rostedt wrote:
> > On Wed, 10 Jan 2018 14:45:07 +0900
> > Namhyung Kim  wrote:
> > 
> > > On Thu, Dec 21, 2017 at 10:02:22AM -0600, Tom Zanussi wrote:
> > > > Hi,
> > > > 
> > > > This is V8 of the inter-event tracing patchset, addressing input from
> > > > V7.
> > > > 
> > > > These changes address Namhyung's most recent comments (thanks,
> > > > Namhyung!) and move everything to the latest tracing/for-next:
> > > > 
> > > >   - moved a couple hunks switching hist_field_fn_t params from 15/37
> > > > (add variable support) to 20/37 (Pass tracing_map_elt to
> > > > hist_field_accessor)
> > > >   - in hist_trigger_elt_data_alloc(), remove the unnecessary '+1' from
> > > > TASK_COMM_LEN size.
> > > >   - simplified find_var_file() code according to Namhyung's
> > > > suggestions.
> > > >   - fixed bug in print_synth_event() where entry->fields was being
> > > > used instead of the address as it should have been  
> > > 
> > > I only have a nitpick in the patch 24 but otherwise cannot find an
> > > issue anymore, so
> > > 
> > > Reviewed-by: Namhyung Kim 
> > 
> > Thanks for reviewing this Namhyung.
> 
> You're very welcome.
> 

Yes, thanks for the very thorough review, Namhyung.

> > 
> > I'm currently traveling (what else is new?), and I want to start
> > pulling this in. It wont be ready for the next merge window as it's too
> > close, but I want to get it in by the one after that. I need to
> > allocate some time to pull these patches in and review them as well.
> 
> Yeah, I also would like to see it merged in v4.17.  Have a nice travel
> and take care.
> 

I'll add these changes to a v9 along with another small nit found by the
kbuild bot (parse_atom() can be static) and repost soon.

Thanks,

Tom

Re: [PATCH v8 00/37] tracing: Inter-event (e.g. latency) support

[Tom Zanussi]

On Wed, 2018-01-10 at 17:16 +0900, Namhyung Kim wrote:
> Hi Steve,
> 
> On Wed, Jan 10, 2018 at 02:20:54AM -0500, Steven Rostedt wrote:
> > On Wed, 10 Jan 2018 14:45:07 +0900
> > Namhyung Kim  wrote:
> > 
> > > On Thu, Dec 21, 2017 at 10:02:22AM -0600, Tom Zanussi wrote:
> > > > Hi,
> > > > 
> > > > This is V8 of the inter-event tracing patchset, addressing input from
> > > > V7.
> > > > 
> > > > These changes address Namhyung's most recent comments (thanks,
> > > > Namhyung!) and move everything to the latest tracing/for-next:
> > > > 
> > > >   - moved a couple hunks switching hist_field_fn_t params from 15/37
> > > > (add variable support) to 20/37 (Pass tracing_map_elt to
> > > > hist_field_accessor)
> > > >   - in hist_trigger_elt_data_alloc(), remove the unnecessary '+1' from
> > > > TASK_COMM_LEN size.
> > > >   - simplified find_var_file() code according to Namhyung's
> > > > suggestions.
> > > >   - fixed bug in print_synth_event() where entry->fields was being
> > > > used instead of the address as it should have been  
> > > 
> > > I only have a nitpick in the patch 24 but otherwise cannot find an
> > > issue anymore, so
> > > 
> > > Reviewed-by: Namhyung Kim 
> > 
> > Thanks for reviewing this Namhyung.
> 
> You're very welcome.
> 

Yes, thanks for the very thorough review, Namhyung.

> > 
> > I'm currently traveling (what else is new?), and I want to start
> > pulling this in. It wont be ready for the next merge window as it's too
> > close, but I want to get it in by the one after that. I need to
> > allocate some time to pull these patches in and review them as well.
> 
> Yeah, I also would like to see it merged in v4.17.  Have a nice travel
> and take care.
> 

I'll add these changes to a v9 along with another small nit found by the
kbuild bot (parse_atom() can be static) and repost soon.

Thanks,

Tom

Re: [PATCH v8 00/37] tracing: Inter-event (e.g. latency) support

[Namhyung Kim]

Hi Steve,

On Wed, Jan 10, 2018 at 02:20:54AM -0500, Steven Rostedt wrote:
> On Wed, 10 Jan 2018 14:45:07 +0900
> Namhyung Kim  wrote:
> 
> > On Thu, Dec 21, 2017 at 10:02:22AM -0600, Tom Zanussi wrote:
> > > Hi,
> > > 
> > > This is V8 of the inter-event tracing patchset, addressing input from
> > > V7.
> > > 
> > > These changes address Namhyung's most recent comments (thanks,
> > > Namhyung!) and move everything to the latest tracing/for-next:
> > > 
> > >   - moved a couple hunks switching hist_field_fn_t params from 15/37
> > > (add variable support) to 20/37 (Pass tracing_map_elt to
> > > hist_field_accessor)
> > >   - in hist_trigger_elt_data_alloc(), remove the unnecessary '+1' from
> > > TASK_COMM_LEN size.
> > >   - simplified find_var_file() code according to Namhyung's
> > > suggestions.
> > >   - fixed bug in print_synth_event() where entry->fields was being
> > > used instead of the address as it should have been  
> > 
> > I only have a nitpick in the patch 24 but otherwise cannot find an
> > issue anymore, so
> > 
> > Reviewed-by: Namhyung Kim 
> 
> Thanks for reviewing this Namhyung.

You're very welcome.

> 
> I'm currently traveling (what else is new?), and I want to start
> pulling this in. It wont be ready for the next merge window as it's too
> close, but I want to get it in by the one after that. I need to
> allocate some time to pull these patches in and review them as well.

Yeah, I also would like to see it merged in v4.17.  Have a nice travel
and take care.

Thanks,
Namhyung

Re: [PATCH v8 00/37] tracing: Inter-event (e.g. latency) support

[Namhyung Kim]

Hi Steve,

On Wed, Jan 10, 2018 at 02:20:54AM -0500, Steven Rostedt wrote:
> On Wed, 10 Jan 2018 14:45:07 +0900
> Namhyung Kim  wrote:
> 
> > On Thu, Dec 21, 2017 at 10:02:22AM -0600, Tom Zanussi wrote:
> > > Hi,
> > > 
> > > This is V8 of the inter-event tracing patchset, addressing input from
> > > V7.
> > > 
> > > These changes address Namhyung's most recent comments (thanks,
> > > Namhyung!) and move everything to the latest tracing/for-next:
> > > 
> > >   - moved a couple hunks switching hist_field_fn_t params from 15/37
> > > (add variable support) to 20/37 (Pass tracing_map_elt to
> > > hist_field_accessor)
> > >   - in hist_trigger_elt_data_alloc(), remove the unnecessary '+1' from
> > > TASK_COMM_LEN size.
> > >   - simplified find_var_file() code according to Namhyung's
> > > suggestions.
> > >   - fixed bug in print_synth_event() where entry->fields was being
> > > used instead of the address as it should have been  
> > 
> > I only have a nitpick in the patch 24 but otherwise cannot find an
> > issue anymore, so
> > 
> > Reviewed-by: Namhyung Kim 
> 
> Thanks for reviewing this Namhyung.

You're very welcome.

> 
> I'm currently traveling (what else is new?), and I want to start
> pulling this in. It wont be ready for the next merge window as it's too
> close, but I want to get it in by the one after that. I need to
> allocate some time to pull these patches in and review them as well.

Yeah, I also would like to see it merged in v4.17.  Have a nice travel
and take care.

Thanks,
Namhyung

Re: [PATCH v8 00/37] tracing: Inter-event (e.g. latency) support

[Steven Rostedt]

On Wed, 10 Jan 2018 14:45:07 +0900
Namhyung Kim  wrote:

> On Thu, Dec 21, 2017 at 10:02:22AM -0600, Tom Zanussi wrote:
> > Hi,
> > 
> > This is V8 of the inter-event tracing patchset, addressing input from
> > V7.
> > 
> > These changes address Namhyung's most recent comments (thanks,
> > Namhyung!) and move everything to the latest tracing/for-next:
> > 
> >   - moved a couple hunks switching hist_field_fn_t params from 15/37
> > (add variable support) to 20/37 (Pass tracing_map_elt to
> > hist_field_accessor)
> >   - in hist_trigger_elt_data_alloc(), remove the unnecessary '+1' from
> > TASK_COMM_LEN size.
> >   - simplified find_var_file() code according to Namhyung's
> > suggestions.
> >   - fixed bug in print_synth_event() where entry->fields was being
> > used instead of the address as it should have been  
> 
> I only have a nitpick in the patch 24 but otherwise cannot find an
> issue anymore, so
> 
> Reviewed-by: Namhyung Kim 

Thanks for reviewing this Namhyung.

I'm currently traveling (what else is new?), and I want to start
pulling this in. It wont be ready for the next merge window as it's too
close, but I want to get it in by the one after that. I need to
allocate some time to pull these patches in and review them as well.

-- Steve

Re: [PATCH v8 00/37] tracing: Inter-event (e.g. latency) support

[Steven Rostedt]

On Wed, 10 Jan 2018 14:45:07 +0900
Namhyung Kim  wrote:

> On Thu, Dec 21, 2017 at 10:02:22AM -0600, Tom Zanussi wrote:
> > Hi,
> > 
> > This is V8 of the inter-event tracing patchset, addressing input from
> > V7.
> > 
> > These changes address Namhyung's most recent comments (thanks,
> > Namhyung!) and move everything to the latest tracing/for-next:
> > 
> >   - moved a couple hunks switching hist_field_fn_t params from 15/37
> > (add variable support) to 20/37 (Pass tracing_map_elt to
> > hist_field_accessor)
> >   - in hist_trigger_elt_data_alloc(), remove the unnecessary '+1' from
> > TASK_COMM_LEN size.
> >   - simplified find_var_file() code according to Namhyung's
> > suggestions.
> >   - fixed bug in print_synth_event() where entry->fields was being
> > used instead of the address as it should have been  
> 
> I only have a nitpick in the patch 24 but otherwise cannot find an
> issue anymore, so
> 
> Reviewed-by: Namhyung Kim 

Thanks for reviewing this Namhyung.

I'm currently traveling (what else is new?), and I want to start
pulling this in. It wont be ready for the next merge window as it's too
close, but I want to get it in by the one after that. I need to
allocate some time to pull these patches in and review them as well.

-- Steve

Re: [PATCH v8 00/37] tracing: Inter-event (e.g. latency) support

[Namhyung Kim]

On Thu, Dec 21, 2017 at 10:02:22AM -0600, Tom Zanussi wrote:
> Hi,
> 
> This is V8 of the inter-event tracing patchset, addressing input from
> V7.
> 
> These changes address Namhyung's most recent comments (thanks,
> Namhyung!) and move everything to the latest tracing/for-next:
> 
>   - moved a couple hunks switching hist_field_fn_t params from 15/37
> (add variable support) to 20/37 (Pass tracing_map_elt to
> hist_field_accessor)
>   - in hist_trigger_elt_data_alloc(), remove the unnecessary '+1' from
> TASK_COMM_LEN size.
>   - simplified find_var_file() code according to Namhyung's
> suggestions.
>   - fixed bug in print_synth_event() where entry->fields was being
> used instead of the address as it should have been

I only have a nitpick in the patch 24 but otherwise cannot find an
issue anymore, so

Reviewed-by: Namhyung Kim 

Thanks,
Namhyung

Re: [PATCH v8 00/37] tracing: Inter-event (e.g. latency) support

[Namhyung Kim]

On Thu, Dec 21, 2017 at 10:02:22AM -0600, Tom Zanussi wrote:
> Hi,
> 
> This is V8 of the inter-event tracing patchset, addressing input from
> V7.
> 
> These changes address Namhyung's most recent comments (thanks,
> Namhyung!) and move everything to the latest tracing/for-next:
> 
>   - moved a couple hunks switching hist_field_fn_t params from 15/37
> (add variable support) to 20/37 (Pass tracing_map_elt to
> hist_field_accessor)
>   - in hist_trigger_elt_data_alloc(), remove the unnecessary '+1' from
> TASK_COMM_LEN size.
>   - simplified find_var_file() code according to Namhyung's
> suggestions.
>   - fixed bug in print_synth_event() where entry->fields was being
> used instead of the address as it should have been

I only have a nitpick in the patch 24 but otherwise cannot find an
issue anymore, so

Reviewed-by: Namhyung Kim 

Thanks,
Namhyung

[PATCH v8 00/37] tracing: Inter-event (e.g. latency) support

[Tom Zanussi]

Hi,

This is V8 of the inter-event tracing patchset, addressing input from
V7.

These changes address Namhyung's most recent comments (thanks,
Namhyung!) and move everything to the latest tracing/for-next:

  - moved a couple hunks switching hist_field_fn_t params from 15/37
(add variable support) to 20/37 (Pass tracing_map_elt to
hist_field_accessor)
  - in hist_trigger_elt_data_alloc(), remove the unnecessary '+1' from
TASK_COMM_LEN size.
  - simplified find_var_file() code according to Namhyung's
suggestions.
  - fixed bug in print_synth_event() where entry->fields was being
used instead of the address as it should have been

Changes from V6:

This is the set of changes addressing Namhyung's most recent set of
comments (thanks, Namhyung!) in addition to a couple problems found by
Vedang (thanks, Vedang!) when using kasan, as well as fixes for a
couple problems I noted while testing:

  - moved several hunks to more appropriate patches
  - fixed some memory leaks in simple expression support, onmax_parse()
  - added sys/event checks for local_field_var_ref()
  - used synth_field_is_string() in synth_field_fmt()
  - replaced i with n_u64 in trace_event_raw_event_synth()
  - created and use add_or_delete_synth_event()
  - used list_move in release_all_synth_events()
  - replaced event_var and IS_ERR() checks with IS_ERR_OR_NULL()
  - fixed double-free of cmd in create_field_var_hist()
  - fixed bare synthetic event reference increment and related code
  - removed unused event_name and related code in onmax_create()
  - updated config text to point to histogram.txt instead of events.txt
  - changed cpu type to unsigned int
  - changed $common_timestamp to common_timestamp throughout
  - merged Vedang's timestamp kasan fix in hist_unreg_all()
  - fixed hist_register_trigger() kasan double-free reported by Vedang
  - after responding to Namhyung's comment about variable references
in onmax triggers, realized that there actually was a problem with
onmatch params and variable references.  Added code to check these
references when removing triggers, and moved
unregister_field_var_hists() calls into event_hist_trigger_free()
so it happens after the synthetic variable dependency is removed.
  - after fixing the onmatch param variable references problem,
realized that the code added to find_any_var_ref() to check for
self-references was incorrect - if a variable is referenced by
only the trigger that created it, it shouldn't prevent the trigger
from removal.

Changes from V5:

  - fixed parse_var_defs() again, moving the VARS_MAX check up to the
proper place.  Also added a hist_err message to parse_assignment()
and removed the hist_err message from create_var_field(), since
it's no longer necessary given the above.
  - fixed a bogus format string check in synth_field_fmt()
  - merged in a fix from Fengguang Wu for a 0day bot-flagged sizeof
pointer problem (to 24/37, Add support for synthetic events)
  - noticed that duplicate variables on the same trigger weren't being
detected and added a fix for that

Changes from V4:

  - fixed struct noinline typo
  - for the selftests, the following changes were made [from Rajvi Jingar]
- In extended error support test, changed last "&&" to redirect
  error/output to /dev/null instead.
- In synthetic event create remove test, changed to same event
  style in creating and removing and added a case to check adding
  an event with wrong format
- Added -q option in grep since don’t need to show grep results.
- Changes to use newly added exit_pass()/exit_fail().
  - in the variable reference handling patch, changed list_add_rcu to
list_add_tail_rcu when adding hist triggers.  Using list_add_rcu
causes the triggers to be executed in reverse order to how they
were added, which in the case of multiple triggers with variables
on the same event can lead to variables being accessed before they
were set. [thanks to Rajvi for the bug report]
  - noticed and fixed a couple problems with synthetic event refcounting

Changes from V3:

These mainly directly address the input from V3, but there are a
couple new things too - a new set of selftests (thanks to Rajvi Jingar
for those), and a new 'timestamp_mode' ftrace file making it easy to
see exactly what mode the ring buffer timestamping is in at any given
time (delta or absolute).  The detailed list:

  - fixed up 'if' parsing to also handle variables starting with 'if'
  - added lots of comments to the field variable code
  - added code to check and not fail if a field variable was already created
  - fixed various typos,etc in commit messages
  - moved target_hist_data fix from onmatch patch to field var patch
  - fixed var_name leak in parse_var_defs()
  - changed common_timestamp -> $common_timestamp in commit messages
  - discovered and fixed a bug where variable references were not guaranteed 
unique
  - added 

[PATCH v8 00/37] tracing: Inter-event (e.g. latency) support

[Tom Zanussi]

Hi,

This is V8 of the inter-event tracing patchset, addressing input from
V7.

These changes address Namhyung's most recent comments (thanks,
Namhyung!) and move everything to the latest tracing/for-next:

  - moved a couple hunks switching hist_field_fn_t params from 15/37
(add variable support) to 20/37 (Pass tracing_map_elt to
hist_field_accessor)
  - in hist_trigger_elt_data_alloc(), remove the unnecessary '+1' from
TASK_COMM_LEN size.
  - simplified find_var_file() code according to Namhyung's
suggestions.
  - fixed bug in print_synth_event() where entry->fields was being
used instead of the address as it should have been

Changes from V6:

This is the set of changes addressing Namhyung's most recent set of
comments (thanks, Namhyung!) in addition to a couple problems found by
Vedang (thanks, Vedang!) when using kasan, as well as fixes for a
couple problems I noted while testing:

  - moved several hunks to more appropriate patches
  - fixed some memory leaks in simple expression support, onmax_parse()
  - added sys/event checks for local_field_var_ref()
  - used synth_field_is_string() in synth_field_fmt()
  - replaced i with n_u64 in trace_event_raw_event_synth()
  - created and use add_or_delete_synth_event()
  - used list_move in release_all_synth_events()
  - replaced event_var and IS_ERR() checks with IS_ERR_OR_NULL()
  - fixed double-free of cmd in create_field_var_hist()
  - fixed bare synthetic event reference increment and related code
  - removed unused event_name and related code in onmax_create()
  - updated config text to point to histogram.txt instead of events.txt
  - changed cpu type to unsigned int
  - changed $common_timestamp to common_timestamp throughout
  - merged Vedang's timestamp kasan fix in hist_unreg_all()
  - fixed hist_register_trigger() kasan double-free reported by Vedang
  - after responding to Namhyung's comment about variable references
in onmax triggers, realized that there actually was a problem with
onmatch params and variable references.  Added code to check these
references when removing triggers, and moved
unregister_field_var_hists() calls into event_hist_trigger_free()
so it happens after the synthetic variable dependency is removed.
  - after fixing the onmatch param variable references problem,
realized that the code added to find_any_var_ref() to check for
self-references was incorrect - if a variable is referenced by
only the trigger that created it, it shouldn't prevent the trigger
from removal.

Changes from V5:

  - fixed parse_var_defs() again, moving the VARS_MAX check up to the
proper place.  Also added a hist_err message to parse_assignment()
and removed the hist_err message from create_var_field(), since
it's no longer necessary given the above.
  - fixed a bogus format string check in synth_field_fmt()
  - merged in a fix from Fengguang Wu for a 0day bot-flagged sizeof
pointer problem (to 24/37, Add support for synthetic events)
  - noticed that duplicate variables on the same trigger weren't being
detected and added a fix for that

Changes from V4:

  - fixed struct noinline typo
  - for the selftests, the following changes were made [from Rajvi Jingar]
- In extended error support test, changed last "&&" to redirect
  error/output to /dev/null instead.
- In synthetic event create remove test, changed to same event
  style in creating and removing and added a case to check adding
  an event with wrong format
- Added -q option in grep since don’t need to show grep results.
- Changes to use newly added exit_pass()/exit_fail().
  - in the variable reference handling patch, changed list_add_rcu to
list_add_tail_rcu when adding hist triggers.  Using list_add_rcu
causes the triggers to be executed in reverse order to how they
were added, which in the case of multiple triggers with variables
on the same event can lead to variables being accessed before they
were set. [thanks to Rajvi for the bug report]
  - noticed and fixed a couple problems with synthetic event refcounting

Changes from V3:

These mainly directly address the input from V3, but there are a
couple new things too - a new set of selftests (thanks to Rajvi Jingar
for those), and a new 'timestamp_mode' ftrace file making it easy to
see exactly what mode the ring buffer timestamping is in at any given
time (delta or absolute).  The detailed list:

  - fixed up 'if' parsing to also handle variables starting with 'if'
  - added lots of comments to the field variable code
  - added code to check and not fail if a field variable was already created
  - fixed various typos,etc in commit messages
  - moved target_hist_data fix from onmatch patch to field var patch
  - fixed var_name leak in parse_var_defs()
  - changed common_timestamp -> $common_timestamp in commit messages
  - discovered and fixed a bug where variable references were not guaranteed 
unique
  - added