Re: [PATCH v11 10/28] tracing: Add hist trigger support for multiple values ('vals=' param)

[Namhyung Kim <namhyung@kernel.org>]

On Thu, Oct 22, 2015 at 01:14:14PM -0500, Tom Zanussi wrote:
> Allow users to specify trace event fields to use in aggregated sums
> via a new 'vals=' keyword.  Before this addition, the only aggregated
> sum supported was the implied value 'hitcount'.  With this addition,
> 'hitcount' is also supported as an explicit value field, as is any
> numeric trace event field.
> 
> This expands the hist trigger syntax from this:
> 
>   # echo hist:keys=xxx [ if filter] > event/trigger
> 
> to this:
> 
>   # echo hist:keys=xxx:vals=yyy [ if filter] > event/trigger
> 
> Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
> Tested-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> ---

Just nipicks..


>  kernel/trace/trace.c             | 12 ++++---
>  kernel/trace/trace_events_hist.c | 75 +++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 81 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 6d5870b..98817d5 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -3799,14 +3799,16 @@ static const char readme_msg[] =
>  #ifdef CONFIG_HIST_TRIGGERS
>  	"      hist trigger\t- If set, event hits are aggregated into a hash table\n"
>  	"\t    Format: hist:keys=<field1>\n"
> +	"\t            [:values=<field1[,field2,...]]\n"

				<field1>[,<field2>,...]
or
				<field1[,field2,...]>


>  	"\t            [:size=#entries]\n"
>  	"\t            [if <filter>]\n\n"
>  	"\t    When a matching event is hit, an entry is added to a hash\n"
> -	"\t    table using the key named, and the value of a sum called\n"
> -	"\t    'hitcount' is incremented.  Keys correspond to fields in the\n"
> -	"\t    event's format description.  Keys can be any field.  The\n"
> -	"\t    'size' parameter can be  used to specify more or fewer than\n"
> -	"\t    the default 2048 entries for the hashtable size.\n\n"
> +	"\t    table using the key(s) and value(s) named, and the value of a\n"
> +	"\t    sum called 'hitcount' is incremented.  Keys and values\n"
> +	"\t    correspond to fields in the event's format description.  Keys\n"
> +	"\t    can be any field.  Values must correspond to numeric fields.\n"
> +	"\t    The 'size' parameter can be  used to specify more or fewer\n"
> +	"\t    than the default 2048 entries for the hashtable size.\n\n"
>  	"\t    Reading the 'hist' file for the event will dump the hash\n"
>  	"\t    table in its entirety to stdout."
>  #endif

[SNIP]
> @@ -534,6 +593,12 @@ hist_trigger_entry_print(struct seq_file *m,
>  	seq_printf(m, " hitcount: %10llu",
>  		   tracing_map_read_sum(elt, HITCOUNT_IDX));
>  
> +	for (i = 1; i < hist_data->n_vals; i++) {
> +		seq_printf(m, "  %s: %10llu",
> +			   hist_data->fields[i]->field->name,
> +			   tracing_map_read_sum(elt, i));
> +	}
> +
>  	seq_puts(m, "\n");
>  }
>  
> @@ -641,7 +706,15 @@ static int event_hist_trigger_print(struct seq_file *m,
>  	}
>  
>  	seq_puts(m, ":vals=");
> -	seq_puts(m, "hitcount");
> +
> +	for (i = 0; i < hist_data->n_vals; i++) {
> +		if (i == 0)

s/0/HITCOUNT_IDX/ ?

Thanks,
Namhyung


> +			seq_puts(m, "hitcount");
> +		else {
> +			seq_puts(m, ",");
> +			hist_field_print(m, hist_data->fields[i]);
> +		}
> +	}
>  
>  	seq_puts(m, ":sort=");
>  	seq_puts(m, "hitcount");
> -- 
> 1.9.3
>