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

From: Masami Hiramatsu
Date: Thu Jul 22 2021 - 00:02:10 EST


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

> From: "Steven Rostedt (VMware)" <rostedt@xxxxxxxxxxx>
>
> 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@xxxxxxxxxx>

Thank you,

> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 8b7622bf94a44 ("tracing: Add cpu field for hist triggers")
> Signed-off-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>