Re: [PATCH v2] tracing/histogram: Rename "cpu" to "common_cpu"

[Masami Hiramatsu <mhiramat@kernel.org>]

On Tue, 20 Jul 2021 23:33:36 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> Currently the histogram logic allows the user to write "cpu" in as an
> event field, and it will record the CPU that the event happened on.
> 
> The problem with this is that there's a lot of events that have "cpu"
> as a real field, and using "cpu" as the CPU it ran on, makes it
> impossible to run histograms on the "cpu" field of events.
> 
> For example, if I want to have a histogram on the count of the
> workqueue_queue_work event on its cpu field, running:
> 
>  ># echo 'hist:keys=cpu' > events/workqueue/workqueue_queue_work/trigger
> 
> Gives a misleading and wrong result.
> 
> Change the command to "common_cpu" as no event should have "common_*"
> fields as that's a reserved name for fields used by all events. And
> this makes sense here as common_cpu would be a field used by all events.
> 
> Now we can even do:
> 
>  ># echo 'hist:keys=common_cpu,cpu if cpu < 100' > events/workqueue/workqueue_queue_work/trigger
>  ># cat events/workqueue/workqueue_queue_work/hist
>  # event histogram
>  #
>  # trigger info: hist:keys=common_cpu,cpu:vals=hitcount:sort=hitcount:size=2048 if cpu < 100 [active]
>  #
> 
>  { common_cpu:          0, cpu:          2 } hitcount:          1
>  { common_cpu:          0, cpu:          4 } hitcount:          1
>  { common_cpu:          7, cpu:          7 } hitcount:          1
>  { common_cpu:          0, cpu:          7 } hitcount:          1
>  { common_cpu:          0, cpu:          1 } hitcount:          1
>  { common_cpu:          0, cpu:          6 } hitcount:          2
>  { common_cpu:          0, cpu:          5 } hitcount:          2
>  { common_cpu:          1, cpu:          1 } hitcount:          4
>  { common_cpu:          6, cpu:          6 } hitcount:          4
>  { common_cpu:          5, cpu:          5 } hitcount:         14
>  { common_cpu:          4, cpu:          4 } hitcount:         26
>  { common_cpu:          0, cpu:          0 } hitcount:         39
>  { common_cpu:          2, cpu:          2 } hitcount:        184
> 
> Now for backward compatibility, I added a trick. If "cpu" is used, and
> the field is not found, it will fall back to "common_cpu" and work as
> it did before. This way, it will still work for old programs that use
> "cpu" to get the actual CPU, but if the event has a "cpu" as a field, it
> will get that event's "cpu" field, which is probably what it wants
> anyway.
> 
> I updated the tracefs/README to include documentation about both the
> common_timestamp and the common_cpu. This way, if that text is present in
> the README, then an application can know that common_cpu is supported over
> just plain "cpu".
> 

This looks good to me.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you,

> Cc: stable@vger.kernel.org
> Fixes: 8b7622bf94a44 ("tracing: Add cpu field for hist triggers")
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> Changes since v1:
>    Added the README text about common_cpu and common_timestamp so
>    that user space can know that it is supported.
> 
>  Documentation/trace/histogram.rst |  2 +-
>  kernel/trace/trace.c              |  4 ++++
>  kernel/trace/trace_events_hist.c  | 22 ++++++++++++++++------
>  3 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/trace/histogram.rst b/Documentation/trace/histogram.rst
> index b71e09f745c3..f99be8062bc8 100644
> --- a/Documentation/trace/histogram.rst
> +++ b/Documentation/trace/histogram.rst
> @@ -191,7 +191,7 @@ Documentation written by Tom Zanussi
>                                  with the event, in nanoseconds.  May be
>  			        modified by .usecs to have timestamps
>  			        interpreted as microseconds.
> -    cpu                    int  the cpu on which the event occurred.
> +    common_cpu             int  the cpu on which the event occurred.
>      ====================== ==== =======================================
>  
>  Extended error information
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 14f56e9fa001..af77452135ff 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -5619,6 +5619,10 @@ static const char readme_msg[] =
>  	"\t            [:name=histname1]\n"
>  	"\t            [:<handler>.<action>]\n"
>  	"\t            [if <filter>]\n\n"
> +	"\t    Note, special fields can be used as well:
> +	"\t            common_timestamp - to record current timestamp\n"
> +	"\t            common_cpu - to record the CPU the event happened on\n"
> +	"\n"
>  	"\t    When a matching event is hit, an entry is added to a hash\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"
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 16a9dfc9fffc..34325f41ebc0 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -1111,7 +1111,7 @@ static const char *hist_field_name(struct hist_field *field,
>  		 field->flags & HIST_FIELD_FL_ALIAS)
>  		field_name = hist_field_name(field->operands[0], ++level);
>  	else if (field->flags & HIST_FIELD_FL_CPU)
> -		field_name = "cpu";
> +		field_name = "common_cpu";
>  	else if (field->flags & HIST_FIELD_FL_EXPR ||
>  		 field->flags & HIST_FIELD_FL_VAR_REF) {
>  		if (field->system) {
> @@ -1991,14 +1991,24 @@ parse_field(struct hist_trigger_data *hist_data, struct trace_event_file *file,
>  		hist_data->enable_timestamps = true;
>  		if (*flags & HIST_FIELD_FL_TIMESTAMP_USECS)
>  			hist_data->attrs->ts_in_usecs = true;
> -	} else if (strcmp(field_name, "cpu") == 0)
> +	} else if (strcmp(field_name, "common_cpu") == 0)
>  		*flags |= HIST_FIELD_FL_CPU;
>  	else {
>  		field = trace_find_event_field(file->event_call, field_name);
>  		if (!field || !field->size) {
> -			hist_err(tr, HIST_ERR_FIELD_NOT_FOUND, errpos(field_name));
> -			field = ERR_PTR(-EINVAL);
> -			goto out;
> +			/*
> +			 * For backward compatibility, if field_name
> +			 * was "cpu", then we treat this the same as
> +			 * common_cpu.
> +			 */
> +			if (strcmp(field_name, "cpu") == 0) {
> +				*flags |= HIST_FIELD_FL_CPU;
> +			} else {
> +				hist_err(tr, HIST_ERR_FIELD_NOT_FOUND,
> +					 errpos(field_name));
> +				field = ERR_PTR(-EINVAL);
> +				goto out;
> +			}
>  		}
>  	}
>   out:
> @@ -5085,7 +5095,7 @@ static void hist_field_print(struct seq_file *m, struct hist_field *hist_field)
>  		seq_printf(m, "%s=", hist_field->var.name);
>  
>  	if (hist_field->flags & HIST_FIELD_FL_CPU)
> -		seq_puts(m, "cpu");
> +		seq_puts(m, "common_cpu");
>  	else if (field_name) {
>  		if (hist_field->flags & HIST_FIELD_FL_VAR_REF ||
>  		    hist_field->flags & HIST_FIELD_FL_ALIAS)
> -- 
> 2.31.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>