Hi Steve,
For 4.19, this patch applies if these patches are applied first:
commit 656fe2ba85e81d00e4447bf77b8da2be3c47acb2
Author: Tom Zanussi <tom.zanussi@xxxxxxxxxxxxxxx>
Date: Tue Dec 18 14:33:24 2018 -0600
tracing: Use hist trigger's var_ref array to destroy var_refs
commit de40f033d4e84e843d6a12266e3869015ea9097c
Author: Tom Zanussi <tom.zanussi@xxxxxxxxxxxxxxx>
Date: Tue Dec 18 14:33:23 2018 -0600
tracing: Remove open-coding of hist trigger var_ref management
After applying all 3 to 4.19, I built and ran the ftrace/trigger
selftests and didn't see any problems.
Tom
On Mon, 2020-01-27 at 16:37 +0100, gregkh@xxxxxxxxxxxxxxxxxxx wrote:
> The patch below does not apply to the 4.19-stable tree.
> If someone wants it applied there, or to any other stable or longterm
> tree, then please email the backport, including the original git
> commit
> id to <stable@xxxxxxxxxxxxxxx>.
>
> thanks,
>
> greg k-h
>
> ------------------ original commit in Linus's tree ------------------
>
> From 8bcebc77e85f3d7536f96845a0fe94b1dddb6af0 Mon Sep 17 00:00:00
> 2001
> From: "Steven Rostedt (VMware)" <rostedt@xxxxxxxxxxx>
> Date: Mon, 20 Jan 2020 13:07:31 -0500
> Subject: [PATCH] tracing: Fix histogram code when expression has same
> var as
> value
>
> While working on a tool to convert SQL syntex into the histogram
> language of
> the kernel, I discovered the following bug:
>
> # echo 'first u64 start_time u64 end_time pid_t pid u64 delta' >>
> synthetic_events
> # echo 'hist:keys=pid:start=common_timestamp' >
> events/sched/sched_waking/trigger
> # echo 'hist:keys=next_pid:delta=common_timestamp-
> $start,start2=$start:onmatch(sched.sched_waking).trace(first,$start2,
> common_timestamp,next_pid,$delta)' >
> events/sched/sched_switch/trigger
>
> Would not display any histograms in the sched_switch histogram side.
>
> But if I were to swap the location of
>
> "delta=common_timestamp-$start" with "start2=$start"
>
> Such that the last line had:
>
> # echo 'hist:keys=next_pid:start2=$start,delta=common_timestamp-
> $start:onmatch(sched.sched_waking).trace(first,$start2,common_timesta
> mp,next_pid,$delta)' > events/sched/sched_switch/trigger
>
> The histogram works as expected.
>
> What I found out is that the expressions clear out the value once it
> is
> resolved. As the variables are resolved in the order listed, when
> processing:
>
> delta=common_timestamp-$start
>
> The $start is cleared. When it gets to "start2=$start", it errors out
> with
> "unresolved symbol" (which is silent as this happens at the location
> of the
> trace), and the histogram is dropped.
>
> When processing the histogram for variable references, instead of
> adding a
> new reference for a variable used twice, use the same reference. That
> way,
> not only is it more efficient, but the order will no longer matter in
> processing of the variables.
>
> From Tom Zanussi:
>
> "Just to clarify some more about what the problem was is that
> without
> your patch, we would have two separate references to the same
> variable,
> and during resolve_var_refs(), they'd both want to be resolved
> separately, so in this case, since the first reference to start
> wasn't
> part of an expression, it wouldn't get the read-once flag set, so
> would
> be read normally, and then the second reference would do the read-
> once
> read and also be read but using read-once. So everything worked
> and
> you didn't see a problem:
>
> from: start2=$start,delta=common_timestamp-$start
>
> In the second case, when you switched them around, the first
> reference
> would be resolved by doing the read-once, and following that the
> second
> reference would try to resolve and see that the variable had
> already
> been read, so failed as unset, which caused it to short-circuit out
> and
> not do the trigger action to generate the synthetic event:
>
> to: delta=common_timestamp-$start,start2=$start
>
> With your patch, we only have the single resolution which happens
> correctly the one time it's resolved, so this can't happen."
>
> Link: https://lore.kernel.org/r/20200116154216.58ca08eb@gandalf.local
> .home
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 067fe038e70f6 ("tracing: Add variable reference handling to
> hist triggers")
> Reviewed-by: Tom Zanuss <zanussi@xxxxxxxxxx>
> Tested-by: Tom Zanussi <zanussi@xxxxxxxxxx>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>
>
> diff --git a/kernel/trace/trace_events_hist.c
> b/kernel/trace/trace_events_hist.c
> index d33b046f985a..6ac35b9e195d 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -116,6 +116,7 @@ struct hist_field {
> struct ftrace_event_field *field;
> unsigned long flags;
> hist_field_fn_t fn;
> + unsigned int ref;
> unsigned int size;
> unsigned int offset;
> unsigned int is_signed;
> @@ -2427,8 +2428,16 @@ static int contains_operator(char *str)
> return field_op;
> }
>
> +static void get_hist_field(struct hist_field *hist_field)
> +{
> + hist_field->ref++;
> +}
> +
> static void __destroy_hist_field(struct hist_field *hist_field)
> {
> + if (--hist_field->ref > 1)
> + return;
> +
> kfree(hist_field->var.name);
> kfree(hist_field->name);
> kfree(hist_field->type);
> @@ -2470,6 +2479,8 @@ static struct hist_field
> *create_hist_field(struct hist_trigger_data *hist_data,
> if (!hist_field)
> return NULL;
>
> + hist_field->ref = 1;
> +
> hist_field->hist_data = hist_data;
>
> if (flags & HIST_FIELD_FL_EXPR || flags &
> HIST_FIELD_FL_ALIAS)
> @@ -2665,6 +2676,17 @@ static struct hist_field
> *create_var_ref(struct hist_trigger_data *hist_data,
> {
> unsigned long flags = HIST_FIELD_FL_VAR_REF;
> struct hist_field *ref_field;
> + int i;
> +
> + /* Check if the variable already exists */
> + for (i = 0; i < hist_data->n_var_refs; i++) {
> + ref_field = hist_data->var_refs[i];
> + if (ref_field->var.idx == var_field->var.idx &&
> + ref_field->var.hist_data == var_field-
> >hist_data) {
> + get_hist_field(ref_field);
> + return ref_field;
> + }
> + }
>
> ref_field = create_hist_field(var_field->hist_data, NULL,
> flags, NULL);
> if (ref_field) {
>
