Re: [PATCH v11 24/28] tracing: Add support for multiple hist triggers per event

[Tom Zanussi]

On Tue, 2015-11-03 at 09:34 +0900, Namhyung Kim wrote:
> On Thu, Oct 22, 2015 at 01:14:28PM -0500, Tom Zanussi wrote:
> > Allow users to define any number of hist triggers per trace event.
> > Any number of hist triggers may be added for a given event, which may
> > differ by key, value, or filter.
> > 
> > Reading the event's 'hist' file will display the output of all the
> > hist triggers defined on an event concatenated in the order they were
> > defined.
> > 
> > Signed-off-by: Tom Zanussi <tom.zanussi@xxxxxxxxxxxxxxx>
> > Tested-by: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>
> > ---
> >  Documentation/trace/events.txt   | 151 +++++++++++++++++++++++++++++++++++++--
> >  kernel/trace/trace.c             |   8 ++-
> >  kernel/trace/trace_events_hist.c | 138 ++++++++++++++++++++++++++---------
> >  3 files changed, 256 insertions(+), 41 deletions(-)
> > 
> > diff --git a/Documentation/trace/events.txt b/Documentation/trace/events.txt
> > index b3aa47e..6c64cb7 100644
> > --- a/Documentation/trace/events.txt
> > +++ b/Documentation/trace/events.txt
> > @@ -532,12 +532,14 @@ The following commands are supported:
> >  
> >    'hist' triggers add a 'hist' file to each event's subdirectory.
> >    Reading the 'hist' file for the event will dump the hash table in
> > -  its entirety to stdout.  Each printed hash table entry is a simple
> > -  list of the keys and values comprising the entry; keys are printed
> > -  first and are delineated by curly braces, and are followed by the
> > -  set of value fields for the entry.  By default, numeric fields are
> > -  displayed as base-10 integers.  This can be modified by appending
> > -  any of the following modifiers to the field name:
> > +  its entirety to stdout.  If there are multiple hist triggers
> > +  attached to an event, there will be a table for each trigger in the
> > +  output.  Each printed hash table entry is a simple list of the keys
> > +  and values comprising the entry; keys are printed first and are
> > +  delineated by curly braces, and are followed by the set of value
> > +  fields for the entry.  By default, numeric fields are displayed as
> > +  base-10 integers.  This can be modified by appending any of the
> > +  following modifiers to the field name:
> >  
> >          .hex        display a number as a hex value
> >  	.sym        display an address as a symbol
> > @@ -1629,3 +1631,140 @@ The following commands are supported:
> >      .
> >      .
> >      .
> > +
> > +  The following example demonstrates how multiple hist triggers can be
> > +  attached to a given event.  This capability can be useful for
> > +  creating a set of different summaries derived from the same set of
> > +  events, or for comparing the effects of different filters, among
> > +  other things.
> > +
> > +    # echo 'hist:keys=skbaddr.hex:vals=len if len < 0' > \
> > +           /sys/kernel/debug/tracing/events/net/netif_receive_skb/trigger
> > +    # echo 'hist:keys=skbaddr.hex:vals=len if len > 4096' > \
> > +           /sys/kernel/debug/tracing/events/net/netif_receive_skb/trigger
> > +    # echo 'hist:keys=skbaddr.hex:vals=len if len == 256' > \
> > +           /sys/kernel/debug/tracing/events/net/netif_receive_skb/trigger
> > +    # echo 'hist:keys=skbaddr.hex:vals=len' > \
> > +           /sys/kernel/debug/tracing/events/net/netif_receive_skb/trigger
> > +    # echo 'hist:keys=len:vals=common_preempt_count' > \
> > +           /sys/kernel/debug/tracing/events/net/netif_receive_skb/trigger
> 
> AFAIK other tracefs files honor the truncation flag so open for
> writing would destroy other hist triggers.  What do you think?
> 

Yeah, I think you're right - adding multiple triggers should use >> (and
> should truncate as mentioned).

> 
> > +
> > +  The above set of commands create four triggers differing only in
> > +  their filters, along with a completely different though fairly
> > +  nonsensical trigger.
> 
> [SNIP]
> > @@ -1289,22 +1368,18 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
> >  
> >  	trigger_data->private_data = hist_data;
> >  
> > +	if (param) { /* if param is non-empty, it's supposed to be a filter */
> > +		ret = cmd_ops->set_filter(param, trigger_data, file);
> 
> Maybe you want to check ->set_filter being NULL first. :)

Yep, thanks.

Tom

> Thanks,
> Namhyung
> 
> 
> > +		if (ret < 0)
> > +			goto out_free;
> > +	}
> > +
> >  	if (glob[0] == '!') {
> >  		cmd_ops->unreg(glob+1, trigger_ops, trigger_data, file);
> >  		ret = 0;
> >  		goto out_free;
> >  	}
> >  
> > -	if (!param) /* if param is non-empty, it's supposed to be a filter */
> > -		goto out_reg;
> > -
> > -	if (!cmd_ops->set_filter)
> > -		goto out_reg;
> > -
> > -	ret = cmd_ops->set_filter(param, trigger_data, file);
> > -	if (ret < 0)
> > -		goto out_free;
> > - out_reg:
> >  	ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
> >  	/*
> >  	 * The above returns on success the # of triggers registered,
> > @@ -1337,7 +1412,7 @@ static struct event_command trigger_hist_cmd = {
> >  	.needs_rec		= true,
> >  	.func			= event_hist_trigger_func,
> >  	.reg			= hist_register_trigger,
> > -	.unreg			= unregister_trigger,
> > +	.unreg			= hist_unregister_trigger,
> >  	.get_trigger_ops	= event_hist_get_trigger_ops,
> >  	.set_filter		= set_trigger_filter,
> >  };
> > @@ -1364,7 +1439,6 @@ hist_enable_trigger(struct event_trigger_data *data, void *rec)
> >  				test->paused = false;
> >  			else
> >  				test->paused = true;
> > -			break;
> >  		}
> >  	}
> >  }
> > -- 
> > 1.9.3
> > 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/