[PATCH 0/3] A few hist trigger fixes

[PATCH 0/3] A few hist trigger fixes

[Tom Zanussi]

Hi Steve,

Here are a few patches that fix some problems found when testing the
hist trigger inter-event (e.g. latency) support patches.

The first fixes a problem I noticed when printing flags.

The remaining two add some hist_err() calls for fields, the first
reported by Masami, the second something I noticed when fixing the
first.

Thanks,

Tom

The following changes since commit 0566e40ce7c493d39006cdd7edf17bfdc52eb2ac:

  tracing: initcall: Ordered comparison of function pointers (2018-04-26 15:02:46 -0400)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-trace.git ftrace/hist-fixes-1

Tom Zanussi (3):
  tracing: Restore proper field flag printing when displaying triggers
  tracing: Add field parsing hist error for hist triggers
  tracing: Add field modifier parsing hist error for hist triggers

 kernel/trace/trace_events_hist.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

-- 
1.9.3

[PATCH 1/3] tracing: Restore proper field flag printing when displaying triggers

[Tom Zanussi]

The flag-printing code used when displaying hist triggers somehow got
dropped during refactoring of the inter-event patchset.  This restores
it.

Below are a couple examples - in the first case, .usecs wasn't being
displayed properly for common_timestamps and the second illustrates
the same for other flags such as .execname.

Before:

  # echo 'hist:key=common_pid.execname:val=count:sort=count' > /sys/kernel/debug/tracing/events/syscalls/sys_enter_read/trigger
  # cat /sys/kernel/debug/tracing/events/syscalls/sys_enter_read/trigger
  hist:keys=common_pid:vals=hitcount,count:sort=count:size=2048 [active]

  # echo 'hist:keys=pid:ts0=common_timestamp.usecs if comm=="cyclictest"' >> /sys/kernel/debug/tracing/events/sched/sched_wakeup/trigger
  # cat /sys/kernel/debug/tracing/events/sched/sched_wakeup/trigger
  hist:keys=pid:vals=hitcount:ts0=common_timestamp:sort=hitcount:size=2048:clock=global if comm=="cyclictest" [active]

After:

  # echo 'hist:key=common_pid.execname:val=count:sort=count' > /sys/kernel/debug/tracing/events/syscalls/sys_enter_read/trigger
  # cat /sys/kernel/debug/tracing/events/syscalls/sys_enter_read/trigger
  hist:keys=common_pid.execname:vals=hitcount,count:sort=count:size=2048 [active]

  # echo 'hist:keys=pid:ts0=common_timestamp.usecs if comm=="cyclictest"' >> /sys/kernel/debug/tracing/events/sched/sched_wakeup/trigger
  # cat /sys/kernel/debug/tracing/events/sched/sched_wakeup/trigger
  hist:keys=pid:vals=hitcount:ts0=common_timestamp.usecs:sort=hitcount:size=2048:clock=global if comm=="cyclictest" [active]

Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
---
 kernel/trace/trace_events_hist.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 0d7b3ff..66c87be 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -4913,6 +4913,16 @@ static void hist_field_print(struct seq_file *m, struct hist_field *hist_field)
 		seq_printf(m, "%s", field_name);
 	} else if (hist_field->flags & HIST_FIELD_FL_TIMESTAMP)
 		seq_puts(m, "common_timestamp");
+
+	if (hist_field->flags) {
+		if (!(hist_field->flags & HIST_FIELD_FL_VAR_REF) &&
+		    !(hist_field->flags & HIST_FIELD_FL_EXPR)) {
+			const char *flags = get_hist_field_flags(hist_field);
+
+			if (flags)
+				seq_printf(m, ".%s", flags);
+		}
+	}
 }
 
 static int event_hist_trigger_print(struct seq_file *m,
-- 
1.9.3

[PATCH 2/3] tracing: Add field parsing hist error for hist triggers

[Tom Zanussi]

If the user specifies a nonexistent field for a hist trigger, the
current code correctly flags that as an error, but doesn't tell the
user what happened.

Fix this by invoking hist_err() with an appropriate message when
nonexistent fields are specified.

Before:

  # echo 'hist:keys=pid:ts0=common_timestamp.usecs' >> /sys/kernel/debug/tracing/events/sched/sched_switch/trigger
  -su: echo: write error: Invalid argument
  # cat /sys/kernel/debug/tracing/events/sched/sched_switch/hist

After:

  # echo 'hist:keys=pid:ts0=common_timestamp.usecs' >> /sys/kernel/debug/tracing/events/sched/sched_switch/trigger
  -su: echo: write error: Invalid argument
  # cat /sys/kernel/debug/tracing/events/sched/sched_switch/hist
  ERROR: Couldn't find field: pid
    Last command: keys=pid:ts0=common_timestamp.usecs

Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
Reported-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/trace/trace_events_hist.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 66c87be..f231fa2 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -2481,6 +2481,7 @@ static struct hist_field *parse_var_ref(struct hist_trigger_data *hist_data,
 	else {
 		field = trace_find_event_field(file->event_call, field_name);
 		if (!field || !field->size) {
+			hist_err("Couldn't find field: ", field_name);
 			field = ERR_PTR(-EINVAL);
 			goto out;
 		}
-- 
1.9.3

[PATCH 3/3] tracing: Add field modifier parsing hist error for hist triggers

[Tom Zanussi]

If the user specifies an invalid field modifier for a hist trigger,
the current code correctly flags that as an error, but doesn't tell
the user what happened.

Fix this by invoking hist_err() with an appropriate message when
invalid modifiers are specified.

Before:

  # echo 'hist:keys=pid:ts0=common_timestamp.junkusecs' >> /sys/kernel/debug/tracing/events/sched/sched_wakeup/trigger
  -su: echo: write error: Invalid argument
  # cat /sys/kernel/debug/tracing/events/sched/sched_wakeup/hist

After:

  # echo 'hist:keys=pid:ts0=common_timestamp.junkusecs' >> /sys/kernel/debug/tracing/events/sched/sched_wakeup/trigger
  -su: echo: write error: Invalid argument
  # cat /sys/kernel/debug/tracing/events/sched/sched_wakeup/hist
  ERROR: Invalid field modifier: junkusecs
    Last command: keys=pid:ts0=common_timestamp.junkusecs

Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
---
 kernel/trace/trace_events_hist.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index f231fa2..b9061ed 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -2466,6 +2466,7 @@ static struct hist_field *parse_var_ref(struct hist_trigger_data *hist_data,
 		else if (strcmp(modifier, "usecs") == 0)
 			*flags |= HIST_FIELD_FL_TIMESTAMP_USECS;
 		else {
+			hist_err("Invalid field modifier: ", modifier);
 			field = ERR_PTR(-EINVAL);
 			goto out;
 		}
-- 
1.9.3

Re: [PATCH 0/3] A few hist trigger fixes

[Steven Rostedt]

On Thu, 26 Apr 2018 20:04:46 -0500
Tom Zanussi <tom.zanussi@linux.intel.com> wrote:

> Hi Steve,
> 
> Here are a few patches that fix some problems found when testing the
> hist trigger inter-event (e.g. latency) support patches.
> 
> The first fixes a problem I noticed when printing flags.
> 
> The remaining two add some hist_err() calls for fields, the first
> reported by Masami, the second something I noticed when fixing the
> first.
> 

They look good and I queued them up.

Thanks!

-- Steve

[PATCH 0/3] A few hist trigger fixes

[Tom Zanussi]

From: Tom Zanussi <tom.zanussi@linux.intel.com>

Hi Steve,

Here's a fix for a problem I found with the hist trigger snapshot
action related to the refactoring in the later versions of the
patchset ('[PATCH 3/3] tracing: Add a check_val() check before updating
cond_snapshot() track_val').

I also added v2 of 2 other patches that I previously submitted [1] but
realized hadn't gotten picked up. ('[PATCH v2 1/3] tracing: Prevent
hist_field_var_ref() from accessing NULL tracing_map_elts' and '[PATCH
v2 2/3] tracing: Check keys for variable references in expressions
too').

One of them I had to rebase ('[PATCH v2 2/3] tracing: Check keys for
variable references in expressions too') because of the error_log
changes since then, but the original ones still apply to versions
before that).

So in summary, all of these patches apply to current ftrace/core,
while only '[PATCH 3/3] tracing: Add a check_val() check before
updating cond_snapshot() track_val' and the previous 2 original
patches in [1] apply to mainline.

Thanks,

Tom

[1] https://lore.kernel.org/lkml/cover.1541687121.git.tom.zanussi@linux.intel.com/

The following changes since commit c8faaa4c594f55ddf903d61180029a3ea1da5286:

  rcu: validate arguments for rcu tracepoints (2019-04-03 08:26:38 -0400)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-trace.git ftrace/key-ref-fix-v2

Tom Zanussi (3):
  tracing: Prevent hist_field_var_ref() from accessing NULL
    tracing_map_elts
  tracing: Check keys for variable references in expressions too
  tracing: Add a check_val() check before updating cond_snapshot()
    track_val

 kernel/trace/trace_events_hist.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

-- 
2.14.1