Re: Unresolved reference for histogram variable

[Tom Zanussi <zanussi@kernel.org>]

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) {