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.

It works for me too, and seems like it should be a good fix for the
problem.  The size and is_signed might be the overkill you're referring
to, but I'd like to spend time tomorrow verifying that and doing some
testing to make sure.

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