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

[Namhyung Kim <namhyung@kernel.org>]

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@linux.intel.com>
> Tested-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> ---
>  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?


> +
> +  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. :)

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
>