* [PATCH v2] tracing/histogram: Rename "cpu" to "common_cpu"
@ 2021-07-21 3:33 Steven Rostedt
2021-07-22 0:40 ` Masami Hiramatsu
0 siblings, 1 reply; 2+ messages in thread
From: Steven Rostedt @ 2021-07-21 3:33 UTC (permalink / raw)
To: LKML, Tom Zanussi
Cc: Masami Hiramatsu, Namhyung Kim, Ingo Molnar, Andrew Morton
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".
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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v2] tracing/histogram: Rename "cpu" to "common_cpu"
2021-07-21 3:33 [PATCH v2] tracing/histogram: Rename "cpu" to "common_cpu" Steven Rostedt
@ 2021-07-22 0:40 ` Masami Hiramatsu
0 siblings, 0 replies; 2+ messages in thread
From: Masami Hiramatsu @ 2021-07-22 0:40 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Tom Zanussi, Masami Hiramatsu, Namhyung Kim, Ingo Molnar,
Andrew Morton
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>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-07-22 0:40 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-21 3:33 [PATCH v2] tracing/histogram: Rename "cpu" to "common_cpu" Steven Rostedt
2021-07-22 0:40 ` Masami Hiramatsu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.