Messages in this thread Patch in this message | | | Subject | Re: Unresolved reference for histogram variable | From | Tom Zanussi <> | Date | Fri, 17 Jan 2020 16:04:50 -0600 |
| |
Hi Steve,
On Thu, 2020-01-16 at 16:56 -0500, Steven Rostedt wrote: > On Thu, 16 Jan 2020 15:42:16 -0500 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > in parse_expr(): > > > > operand1->read_once = true; > > operand2->read_once = true; > > > > Why is that? > > > > This means that any variable used in an expression can not be use > > later > > on. > > > > Or should the variable be detected that it is used multiple times > > in > > the expression, and have the parser detect this, and just reuse the > > same variable multiple times? > > This patch seems to fix the problem, and lets us reuse the same > variable multiple times. >
I tested this patch after removing the checks for size and is_signed, which aren't needed because just the var.idx and hist_data is enough to know you're referencing the same variable.
So removing these lines:
ref_field->size == var_field->size && ref_field->is_signed == var_field->is_signed) {
passed selftests and the specific failure you found.
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.
Anyway, here's my reviewed and tested-by:
Reviewed-by: Tom Zanuss <zanussi@kernel.org> Tested-by: Tom Zanussi <zanussi@kernel.org>
Thanks,
Tom
> -- Steve > > diff --git a/kernel/trace/trace_events_hist.c > b/kernel/trace/trace_events_hist.c > index 117a1202a6b9..b7f944735a4a 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; > @@ -2432,8 +2433,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); > @@ -2475,6 +2484,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) > @@ -2670,6 +2681,19 @@ 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; > + > + for (i = 0; i < hist_data->n_var_refs; i++) { > + ref_field = hist_data->var_refs[i]; > + /* Maybe this is overkill? */ > + if (ref_field->var.idx == var_field->var.idx && > + ref_field->var.hist_data == var_field->hist_data > && > + ref_field->size == var_field->size && > + ref_field->is_signed == var_field->is_signed) { > + get_hist_field(ref_field); > + return ref_field; > + } > + } > > ref_field = create_hist_field(var_field->hist_data, NULL, > flags, NULL); > if (ref_field) {
| |