Re: [PATCH v4 4/7] tracing: Add support for dynamic strings to synthetic events

[Tom Zanussi <zanussi@kernel.org>]

Hi Masami,

On Thu, 2020-10-08 at 18:22 +0900, Masami Hiramatsu wrote:
> Hi Tom,
> 
> On Sun,  4 Oct 2020 17:14:06 -0500
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> > Currently, sythetic events only support static string fields such
> > as:
> > 
> >   # echo 'test_latency u64 lat; char somename[32]' >
> > /sys/kernel/debug/tracing/synthetic_events
> > 
> > Which is fine, but wastes a lot of space in the event.
> > 
> > It also prevents the most commonly-defined strings in the existing
> > trace events e.g. those defined using __string(), from being passed
> > to
> > synthetic events via the trace() action.
> > 
> > With this change, synthetic events with dynamic fields can be
> > defined:
> > 
> >   # echo 'test_latency u64 lat; char somename[]' >
> > /sys/kernel/debug/tracing/synthetic_events
> > 
> > And the trace() action can be used to generate events using either
> > dynamic or static strings:
> > 
> >   # echo 'hist:keys=name:lat=common_timestamp.usecs-
> > $ts0:onmatch(sys.event).test_latency($lat,name)' >
> > /sys/kernel/debug/tracing/events
> > 
> > The synthetic event dynamic strings are implemented in the same way
> > as
> > the existing __data_loc strings and appear as such in the format
> > file.
> > 
> > [ <rostedt@goodmis.org>: added __set_synth_event_print_fmt()
> > changes:
> > 
> >   I added the following to make it work with trace-cmd. Dynamic
> > strings
> >   must have __get_str() for events in the print_fmt otherwise it
> > can't be
> >   parsed correctly. ]
> 
> I confirmed this works, but have some questions;
> - It seems no error message when we failed to define with wrong
> syntax

Yes, I need to add an error message for this (as well as others in this
file).

> 
> /sys/kernel/debug/tracing # echo 'myevent char name []' >>
> synthetic_events 
> sh: write error: Invalid argument
> /sys/kernel/debug/tracing # cat error_log 
> /sys/kernel/debug/tracing #
> 
> - what we write and what we see in synthetic_events are different
> 
> /sys/kernel/debug/tracing # echo 'myevent char name[]' >>
> synthetic_events 
> /sys/kernel/debug/tracing # cat synthetic_events 
> myevent	__data_loc char[] name
> 
> - And it is not able to re-define with that syntax
> 
> /sys/kernel/debug/tracing # echo > synthetic_events 
> /sys/kernel/debug/tracing # echo 'myevent __data_loc char[] name' >>
> synthetic_events 
> sh: write error: Invalid argument
> /sys/kernel/debug/tracing # 
> 

Yes, only the format needs to have the __data_loc, but the event
description shouldn't.

> - It seems to accept wrong name for variables
> 
> /sys/kernel/debug/tracing # echo 'myevent char name]' >>
> synthetic_events 
> /sys/kernel/debug/tracing # echo 'myevent2 char name;[]' >>
> synthetic_events 
> /sys/kernel/debug/tracing # cat synthetic_events 
> myevent	char name]
> myevent2	__data_loc char[] name;
> 

Yes, I think all these other errors boil down to allowing illegal
names.  Applying your is_good_name() function for probe events to these
fields should get rid of them.

> Some of those issues are not introduced from this series. I think
> we'd better fix those before introducing this series so that
> we can backport it to stable kernels.
> 

I should have patches addressing all of these shortly, tomorrow at the
latest.

Thanks,

Tom

> Thank you,
> 
>