Re: [PATCH 3/7] tracing: Use var_refs[] for hist trigger reference checking

[Masami Hiramatsu <mhiramat@kernel.org>]

Hi Tom,

On Tue, 18 Dec 2018 14:33:22 -0600
Tom Zanussi <zanussi@kernel.org> wrote:

> From: Tom Zanussi <tom.zanussi@linux.intel.com>
> 
> Since all the variable reference hist_fields are collected into
> hist_data->var_refs[] array, there's no need to go through all the
> fields looking for them, or in separate arrays like synth_var_refs[],
> which will be going away soon anyway.
> 
> This also allows us to get rid of some unnecessary code and functions
> currently used for the same purpose.

I just have a nitpick. 

> 
> Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
> ---
>  kernel/trace/trace_events_hist.c | 57 +++++-----------------------------------
>  1 file changed, 7 insertions(+), 50 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 25d06b3ae1f6..ee839c52bd3f 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -1299,45 +1299,11 @@ check_field_for_var_ref(struct hist_field *hist_field,
>  {
>  	struct hist_field *found = NULL;
>  
> -	if (hist_field && hist_field->flags & HIST_FIELD_FL_VAR_REF) {
> -		if (hist_field->var.idx == var_idx &&
> -		    hist_field->var.hist_data == var_data) {
> -			found = hist_field;
> -		}
> -	}
> -
> -	return found;
> -}
> -
> -static struct hist_field *
> -check_field_for_var_refs(struct hist_trigger_data *hist_data,
> -			 struct hist_field *hist_field,
> -			 struct hist_trigger_data *var_data,
> -			 unsigned int var_idx,
> -			 unsigned int level)
> -{
> -	struct hist_field *found = NULL;
> -	unsigned int i;
> -
> -	if (level > 3)
> -		return found;
> -
> -	if (!hist_field)
> -		return found;
> +	WARN_ON(!(hist_field && hist_field->flags & HIST_FIELD_FL_VAR_REF));
>  
> -	found = check_field_for_var_ref(hist_field, var_data, var_idx);
> -	if (found)
> -		return found;
> -
> -	for (i = 0; i < HIST_FIELD_OPERANDS_MAX; i++) {
> -		struct hist_field *operand;
> -
> -		operand = hist_field->operands[i];
> -		found = check_field_for_var_refs(hist_data, operand, var_data,
> -						 var_idx, level + 1);
> -		if (found)
> -			return found;
> -	}
> +	if (hist_field && hist_field->var.idx == var_idx &&
> +	    hist_field->var.hist_data == var_data)
> +		found = hist_field;
>  
>  	return found;

It seems we don't need "found" var here. Just return hist_field or NULL.

>  }
> @@ -1349,18 +1315,9 @@ static struct hist_field *find_var_ref(struct hist_trigger_data *hist_data,
>  	struct hist_field *hist_field, *found = NULL;
>  	unsigned int i;
>  
> -	for_each_hist_field(i, hist_data) {
> -		hist_field = hist_data->fields[i];
> -		found = check_field_for_var_refs(hist_data, hist_field,
> -						 var_data, var_idx, 0);
> -		if (found)
> -			return found;
> -	}
> -
> -	for (i = 0; i < hist_data->n_synth_var_refs; i++) {
> -		hist_field = hist_data->synth_var_refs[i];
> -		found = check_field_for_var_refs(hist_data, hist_field,
> -						 var_data, var_idx, 0);
> +	for (i = 0; i < hist_data->n_var_refs; i++) {
> +		hist_field = hist_data->var_refs[i];
> +		found = check_field_for_var_ref(hist_field, var_data, var_idx);
>  		if (found)
>  			return found;
>  	}

Here too. If you return soon after found it, you don't need to assign it.

Except for that, it looks good to me.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you,

> -- 
> 2.14.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>