| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 22 Jul 2021 09:40:41 +0900
From: Masami Hiramatsu <mhiramat@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
Tom Zanussi <zanussi@...nel.org>,
Masami Hiramatsu <mhiramat@...nel.org>,
Namhyung Kim <namhyung@...nel.org>,
Ingo Molnar <mingo@...nel.org>,
Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v2] tracing/histogram: Rename "cpu" to "common_cpu"
On Tue, 20 Jul 2021 23:33:36 -0400
Steven Rostedt <rostedt@...dmis.org> wrote:
> From: "Steven Rostedt (VMware)" <rostedt@...dmis.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@...nel.org>
Thank you,
> Cc: stable@...r.kernel.org
> Fixes: 8b7622bf94a44 ("tracing: Add cpu field for hist triggers")
> Signed-off-by: Steven Rostedt (VMware) <rostedt@...dmis.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@...nel.org>
Powered by blists - more mailing lists