Re: [PATCH v11 10/15] tracing: Add alternative synthetic event trace action syntax

[Namhyung Kim]

Hi Tom,

On Fri, Jan 11, 2019 at 10:25:40AM -0600, Tom Zanussi wrote:
> Hi Namhyung,
> 
> On Fri, 2019-01-11 at 15:07 +0900, Namhyung Kim wrote:
> > On Wed, Jan 09, 2019 at 01:49:17PM -0600, Tom Zanussi wrote:
> > > From: Tom Zanussi <tom.zanussi@xxxxxxxxxxxxxxx>
> > > 
> > > Add a 'trace(synthetic_event_name, params)' alternative to
> > > synthetic_event_name(params).
> > > 
> > > Currently, the syntax used for generating synthetic events is to
> > > invoke synthetic_event_name(params) i.e. use the synthetic event
> > > name
> > > as a function call.
> > > 
> > > Users requested a new form that more explicitly shows that the
> > > synthetic event is in effect being traced.  In this version, a new
> > > 'trace()' keyword is used, and the synthetic event name is passed
> > > in
> > > as the first argument.
> > > 
> > > Signed-off-by: Tom Zanussi <tom.zanussi@xxxxxxxxxxxxxxx>
> > > ---
> > >  Documentation/trace/histogram.rst | 21 ++++++++++++++++++++
> > >  kernel/trace/trace.c              |  1 +
> > >  kernel/trace/trace_events_hist.c  | 42
> > > +++++++++++++++++++++++++++++++++++----
> > >  3 files changed, 60 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/Documentation/trace/histogram.rst
> > > b/Documentation/trace/histogram.rst
> > > index 79476c906b1a..4939bad1c1cd 100644
> > > --- a/Documentation/trace/histogram.rst
> > > +++ b/Documentation/trace/histogram.rst
> > > @@ -1874,6 +1874,7 @@ The available handlers are:
> > >  The available actions are:
> > >  
> > >    - <synthetic_event_name>(param list)         - generate
> > > synthetic event
> > > +  - trace(<synthetic_event_name>,(param list)) - generate
> > > synthetic event
> > 
> > Shouldn't it be
> > 
> > 	"trace(<synthetic_event_name>,param list)"
> > 
> > ?  Otherwise it looks like we need two parentheses.
> 
> Good point, I'll remove the params.
> 
> > 
> > IMHO, it seems better for consistency using this new syntax only.
> > Of course it should support the old syntax as well for compatibility
> > (and maybe make it undocumented?).  But I won't insist strongly..
> > 
> 
> OK, yeah, I really hate when things are undocumented, so I think
> removing the documentation would be a step backward, but maybe changing
> the emphasis to the trace() form would suffice.  How about the below?:

Looks good to me!

Thanks,
Namhyung


> 
> 
> diff --git a/Documentation/trace/histogram.rst b/Documentation/trace/histogram.rst
> index e5bcef360997..0ea59d45aef1 100644
> --- a/Documentation/trace/histogram.rst
> +++ b/Documentation/trace/histogram.rst
> @@ -1873,46 +1873,45 @@ The available handlers are:
>  
>  The available actions are:
>  
> -  - <synthetic_event_name>(param list)         - generate synthetic event
>    - trace(<synthetic_event_name>,param list)   - generate synthetic event
>    - save(field,...)                            - save current event fields
>    - snapshot()                                 - snapshot the trace buffer
>  
>  The following commonly-used handler.action pairs are available:
>  
> -  - onmatch(matching.event).<synthetic_event_name>(param list)
> -
> -    or
> -
>    - onmatch(matching.event).trace(<synthetic_event_name>,param list)
>  
> -    The 'onmatch(matching.event).<synthetic_event_name>(params)' hist
> -    trigger action is invoked whenever an event matches and the
> -    histogram entry would be added or updated.  It causes the named
> -    synthetic event to be generated with the values given in the
> +    The 'onmatch(matching.event).trace(<synthetic_event_name>,param
> +    list)' hist trigger action is invoked whenever an event matches
> +    and the histogram entry would be added or updated.  It causes the
> +    named synthetic event to be generated with the values given in the
>      'param list'.  The result is the generation of a synthetic event
>      that consists of the values contained in those variables at the
> -    time the invoking event was hit.
> -
> -    There are two equivalent forms available for generating synthetic
> -    events.  In the first form, the synthetic event name is used as if
> -    it were a function name.  For example, if the synthetic event name
> -    is 'wakeup_latency', the wakeup_latency event would be generated
> -    by invoking it as if it were a function call, with the event field
> -    values passed in as arguments: wakeup_latency(arg1,arg2).  The
> -    second form simply uses the 'trace' keyword as the function name
> -    and passes in the synthetic event name as the first argument,
> -    followed by the field values: trace(wakeup_latency,arg1,arg2).
> -
> -    The 'param list' consists of one or more parameters which may be
> -    either variables or fields defined on either the 'matching.event'
> -    or the target event.  The variables or fields specified in the
> -    param list may be either fully-qualified or unqualified.  If a
> -    variable is specified as unqualified, it must be unique between
> -    the two events.  A field name used as a param can be unqualified
> -    if it refers to the target event, but must be fully qualified if
> -    it refers to the matching event.  A fully-qualified name is of the
> -    form 'system.event_name.$var_name' or 'system.event_name.field'.
> +    time the invoking event was hit.  For example, if the synthetic
> +    event name is 'wakeup_latency', a wakeup_latency event is
> +    generated using onmatch(event).trace(wakeup_latency,arg1,arg2).
> +
> +    There is also an equivalent alternative form available for
> +    generating synthetic events.  In this form, the synthetic event
> +    name is used as if it were a function name.  For example, using
> +    the 'wakeup_latency' synthetic event name again, the
> +    wakeup_latency event would be generated by invoking it as if it
> +    were a function call, with the event field values passed in as
> +    arguments: onmatch(event).wakeup_latency(arg1,arg2).  The syntax
> +    for this form is:
> +
> +      onmatch(matching.event).<synthetic_event_name>(param list)
> +
> +    In either case, the 'param list' consists of one or more
> +    parameters which may be either variables or fields defined on
> +    either the 'matching.event' or the target event.  The variables or
> +    fields specified in the param list may be either fully-qualified
> +    or unqualified.  If a variable is specified as unqualified, it
> +    must be unique between the two events.  A field name used as a
> +    param can be unqualified if it refers to the target event, but
> +    must be fully qualified if it refers to the matching event.  A
> +    fully-qualified name is of the form 'system.event_name.$var_name'
> +    or 'system.event_name.field'.
>  
>      The 'matching.event' specification is simply the fully qualified
>      event name of the event that matches the target event for the
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 84981b61d45f..8c220d97c214 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -4899,7 +4899,6 @@ static const char readme_msg[] =
>  	"\t        onmax(var)               - invoke if var exceeds current max\n"
>  	"\t        onchange(var)            - invoke action if var changes\n\n"
>  	"\t    The available actions are:\n\n"
> -	"\t        <synthetic_event>(param list)        - generate synthetic event\n"
>  	"\t        trace(<synthetic_event>,param list)  - generate synthetic event\n"
>  	"\t        save(field,...)                      - save current event fields\n"
>  	"\t        snapshot()                           - snapshot the trace buffer\n"