Date:   Tue, 23 Oct 2018 14:09:23 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Tom Zanussi <zanussi@...nel.org>
Cc:     rostedt@...dmis.org, tglx@...utronix.de, mhiramat@...nel.org,
        namhyung@...nel.org, vedang.patel@...el.com, bigeasy@...utronix.de,
        joel@...lfernandes.org, mathieu.desnoyers@...icios.com,
        julia@...com, linux-kernel@...r.kernel.org,
        linux-rt-users@...r.kernel.org
Subject: Re: [PATCH v6 01/15] tracing: Refactor hist trigger action code

Hi Tom,

On Thu, 11 Oct 2018 16:01:58 -0500
Tom Zanussi <zanussi@...nel.org> wrote:

> From: Tom Zanussi <tom.zanussi@...ux.intel.com>
> 
> The hist trigger action code currently implements two essentially
> hard-coded pairs of 'actions' - onmax(), which tracks a variable and
> saves some event fields when a max is hit, and onmatch(), which is
> hard-coded to generate a synthetic event.
> 
> These hardcoded pairs (track max/save fields and detect match/generate
> synthetic event) should really be decoupled into separate components
> that can then be arbitrarily combined.  The first component of each
> pair (track max/detect match) is called a 'handler' in the new code,
> while the second component (save fields/generate synthetic event) is
> called an 'action' in this scheme.
> 
> This change refactors the action code to reflect this split by adding
> two handlers, HANDLER_ONMATCH and HANDLER_ONMAX, along with two
> actions, ACTION_SAVE and ACTION_TRACE.
> 
> The new code combines them to produce the existing ONMATCH/TRACE and
> ONMAX/SAVE functionality, but doesn't implement the other combinations
> now possible.  Future patches will expand these to further useful
> cases, such as ONMAX/TRACE, as well as add additional handlers and
> actions such as ONCHANGE and SNAPSHOT.

This patch looks good, but I have some comment below;

[...]
> @@ -3248,11 +3260,12 @@ static void onmax_print(struct seq_file *m,
>  {
>  	unsigned int i, save_var_idx, max_idx = data->onmax.max_var->var.idx;
>  
> -	seq_printf(m, "\n\tmax: %10llu", tracing_map_read_var(elt, max_idx));
> +	if (data->handler == HANDLER_ONMAX)

It seems a bit odd that checks HANDLER_ONMAX or not in onmax_X function().

> +		seq_printf(m, "\n\tmax: %10llu", tracing_map_read_var(elt, max_idx));
>  
> -	for (i = 0; i < hist_data->n_max_vars; i++) {
> -		struct hist_field *save_val = hist_data->max_vars[i]->val;
> -		struct hist_field *save_var = hist_data->max_vars[i]->var;
> +	for (i = 0; i < hist_data->n_save_vars; i++) {
> +		struct hist_field *save_val = hist_data->save_vars[i]->val;
> +		struct hist_field *save_var = hist_data->save_vars[i]->var;
>  		u64 val;
>  
>  		save_var_idx = save_var->var.idx;
> @@ -3296,7 +3309,7 @@ static void onmax_destroy(struct action_data *data)
>  	destroy_hist_field(data->onmax.var, 0);
>  
>  	kfree(data->onmax.var_str);
> -	kfree(data->onmax.fn_name);
> +	kfree(data->action_name);
>  
>  	for (i = 0; i < data->n_params; i++)
>  		kfree(data->params[i]);
> @@ -3304,16 +3317,17 @@ static void onmax_destroy(struct action_data *data)
>  	kfree(data);
>  }
>  
> +static int action_create(struct hist_trigger_data *hist_data,
> +			 struct action_data *data);
> +
>  static int onmax_create(struct hist_trigger_data *hist_data,
>  			struct action_data *data)
>  {
> +	struct hist_field *var_field, *ref_field, *max_var = NULL;
>  	struct trace_event_file *file = hist_data->event_file;
> -	struct hist_field *var_field, *ref_field, *max_var;
>  	unsigned int var_ref_idx = hist_data->n_var_refs;
> -	struct field_var *field_var;
> -	char *onmax_var_str, *param;
> +	char *onmax_var_str;
>  	unsigned long flags;
> -	unsigned int i;
>  	int ret = 0;
>  
>  	onmax_var_str = data->onmax.var_str;
> @@ -3343,9 +3357,10 @@ static int onmax_create(struct hist_trigger_data *hist_data,
>  	ref_field->var_ref_idx = hist_data->n_var_refs++;
>  	data->onmax.var = ref_field;
>  
> -	data->fn = onmax_save;
>  	data->onmax.max_var_ref_idx = var_ref_idx;
> -	max_var = create_var(hist_data, file, "max", sizeof(u64), "u64");
> +
> +	if (data->handler == HANDLER_ONMAX)

Ditto.

> +		max_var = create_var(hist_data, file, "max", sizeof(u64), "u64");
>  	if (IS_ERR(max_var)) {
>  		hist_err("onmax: Couldn't create onmax variable: ", "max");
>  		ret = PTR_ERR(max_var);
> @@ -3353,27 +3368,7 @@ static int onmax_create(struct hist_trigger_data *hist_data,
>  	}
>  	data->onmax.max_var = max_var;
>  
> -	for (i = 0; i < data->n_params; i++) {
> -		param = kstrdup(data->params[i], GFP_KERNEL);
> -		if (!param) {
> -			ret = -ENOMEM;
> -			goto out;
> -		}
> -
> -		field_var = create_target_field_var(hist_data, NULL, NULL, param);
> -		if (IS_ERR(field_var)) {
> -			hist_err("onmax: Couldn't create field variable: ", param);
> -			ret = PTR_ERR(field_var);
> -			kfree(param);
> -			goto out;
> -		}
> -
> -		hist_data->max_vars[hist_data->n_max_vars++] = field_var;
> -		if (field_var->val->flags & HIST_FIELD_FL_STRING)
> -			hist_data->n_max_var_str++;
> -
> -		kfree(param);
> -	}
> +	ret = action_create(hist_data, data);
>   out:
>  	return ret;
>  }
> @@ -3384,11 +3379,14 @@ static int parse_action_params(char *params, struct action_data *data)
>  	int ret = 0;
>  
>  	while (params) {
> -		if (data->n_params >= SYNTH_FIELDS_MAX)
> +		if (data->n_params >= SYNTH_FIELDS_MAX) {
> +			hist_err("Too many action params", "");
>  			goto out;
> +		}
>  
>  		param = strsep(&params, ",");
>  		if (!param) {
> +			hist_err("No action param found", "");
>  			ret = -EINVAL;
>  			goto out;
>  		}
> @@ -3412,10 +3410,69 @@ static int parse_action_params(char *params, struct action_data *data)
>  	return ret;
>  }
>  
> -static struct action_data *onmax_parse(char *str)
> +static int action_parse(char *str, struct action_data *data,
> +			enum handler_id handler)
> +{
> +	char *action_name;
> +	int ret = 0;
> +
> +	strsep(&str, ".");
> +	if (!str) {
> +		hist_err("action parsing: No action found", "");
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	action_name = strsep(&str, "(");
> +	if (!action_name || !str) {
> +		hist_err("action parsing: No action found", "");
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (strncmp(action_name, "save", strlen("save")) == 0) {
> +		char *params = strsep(&str, ")");
> +
> +		if (!params) {
> +			hist_err("action parsing: No params found for %s", "save");
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		ret = parse_action_params(params, data);
> +		if (ret)
> +			goto out;
> +
> +		if (handler == HANDLER_ONMAX)
> +			data->fn = onmax_save;
> +
> +		data->action = ACTION_SAVE;
> +	} else {
> +		char *params = strsep(&str, ")");
> +
> +		if (params) {
> +			ret = parse_action_params(params, data);
> +			if (ret)
> +				goto out;
> +		}
> +
> +		data->fn = action_trace;
> +		data->action = ACTION_TRACE;
> +	}
> +
> +	data->action_name = kstrdup(action_name, GFP_KERNEL);
> +	if (!data->action_name) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}

Since we know "handler", it is natural that data->handler is assigned here.

> + out:
> +	return ret;
> +}
> +
> +static struct action_data *onmax_parse(char *str, enum handler_id handler)
>  {
> -	char *onmax_fn_name, *onmax_var_str;
>  	struct action_data *data;
> +	char *onmax_var_str;
>  	int ret = -EINVAL;
>  
>  	data = kzalloc(sizeof(*data), GFP_KERNEL);
> @@ -3434,33 +3491,11 @@ static struct action_data *onmax_parse(char *str)
>  		goto free;
>  	}
>  
> -	strsep(&str, ".");
> -	if (!str)
> -		goto free;
> -
> -	onmax_fn_name = strsep(&str, "(");
> -	if (!onmax_fn_name || !str)
> -		goto free;
> -
> -	if (strncmp(onmax_fn_name, "save", strlen("save")) == 0) {
> -		char *params = strsep(&str, ")");
> -
> -		if (!params) {
> -			ret = -EINVAL;
> -			goto free;
> -		}
> -
> -		ret = parse_action_params(params, data);
> -		if (ret)
> -			goto free;
> -	} else
> +	ret = action_parse(str, data, handler);
> +	if (ret)
>  		goto free;
>  
> -	data->onmax.fn_name = kstrdup(onmax_fn_name, GFP_KERNEL);
> -	if (!data->onmax.fn_name) {
> -		ret = -ENOMEM;
> -		goto free;
> -	}
> +	data->handler = handler;
>   out:
>  	return data;
>   free:
> @@ -3477,7 +3512,7 @@ static void onmatch_destroy(struct action_data *data)
>  
>  	kfree(data->onmatch.match_event);
>  	kfree(data->onmatch.match_event_system);
> -	kfree(data->onmatch.synth_event_name);
> +	kfree(data->action_name);
>  
>  	for (i = 0; i < data->n_params; i++)
>  		kfree(data->params[i]);
> @@ -3554,8 +3589,9 @@ static int check_synth_field(struct synth_event *event,
>  }
>  
>  static struct hist_field *
> -onmatch_find_var(struct hist_trigger_data *hist_data, struct action_data *data,
> -		 char *system, char *event, char *var)
> +trace_action_find_var(struct hist_trigger_data *hist_data,
> +		      struct action_data *data,
> +		      char *system, char *event, char *var)
>  {
>  	struct hist_field *hist_field;
>  
> @@ -3563,7 +3599,7 @@ onmatch_find_var(struct hist_trigger_data *hist_data, struct action_data *data,
>  
>  	hist_field = find_target_event_var(hist_data, system, event, var);
>  	if (!hist_field) {
> -		if (!system) {
> +		if (!system && data->handler == HANDLER_ONMATCH) {
>  			system = data->onmatch.match_event_system;
>  			event = data->onmatch.match_event;
>  		}
> @@ -3572,15 +3608,15 @@ onmatch_find_var(struct hist_trigger_data *hist_data, struct action_data *data,
>  	}
>  
>  	if (!hist_field)
> -		hist_err_event("onmatch: Couldn't find onmatch param: $", system, event, var);
> +		hist_err_event("trace action: Couldn't find param: $", system, event, var);
>  
>  	return hist_field;
>  }
>  
>  static struct hist_field *
> -onmatch_create_field_var(struct hist_trigger_data *hist_data,
> -			 struct action_data *data, char *system,
> -			 char *event, char *var)
> +trace_action_create_field_var(struct hist_trigger_data *hist_data,
> +			      struct action_data *data, char *system,
> +			      char *event, char *var)
>  {
>  	struct hist_field *hist_field = NULL;
>  	struct field_var *field_var;
> @@ -3603,7 +3639,7 @@ onmatch_create_field_var(struct hist_trigger_data *hist_data,
>  		 * looking for fields on the onmatch(system.event.xxx)
>  		 * event.
>  		 */
> -		if (!system) {
> +		if (!system && data->handler == HANDLER_ONMATCH) {
>  			system = data->onmatch.match_event_system;
>  			event = data->onmatch.match_event;
>  		}
> @@ -3627,9 +3663,8 @@ onmatch_create_field_var(struct hist_trigger_data *hist_data,
>  	goto out;
>  }
>  
> -static int onmatch_create(struct hist_trigger_data *hist_data,
> -			  struct trace_event_file *file,
> -			  struct action_data *data)
> +static int trace_action_create(struct hist_trigger_data *hist_data,
> +			       struct action_data *data)
>  {
>  	char *event_name, *param, *system = NULL;
>  	struct hist_field *hist_field, *var_ref;
> @@ -3639,12 +3674,14 @@ static int onmatch_create(struct hist_trigger_data *hist_data,
>  	int ret = 0;
>  
>  	mutex_lock(&synth_event_mutex);
> -	event = find_synth_event(data->onmatch.synth_event_name);
> +
> +	event = find_synth_event(data->action_name);
>  	if (!event) {
> -		hist_err("onmatch: Couldn't find synthetic event: ", data->onmatch.synth_event_name);
> +		hist_err("trace action: Couldn't find synthetic event: ", data->action_name);
>  		mutex_unlock(&synth_event_mutex);
>  		return -EINVAL;
>  	}
> +
>  	event->ref++;
>  	mutex_unlock(&synth_event_mutex);
>  
> @@ -3673,13 +3710,15 @@ static int onmatch_create(struct hist_trigger_data *hist_data,
>  		}
>  
>  		if (param[0] == '$')
> -			hist_field = onmatch_find_var(hist_data, data, system,
> -						      event_name, param);
> +			hist_field = trace_action_find_var(hist_data, data,
> +							   system, event_name,
> +							   param);
>  		else
> -			hist_field = onmatch_create_field_var(hist_data, data,
> -							      system,
> -							      event_name,
> -							      param);
> +			hist_field = trace_action_create_field_var(hist_data,
> +								   data,
> +								   system,
> +								   event_name,
> +								   param);
>  
>  		if (!hist_field) {
>  			kfree(p);
> @@ -3701,7 +3740,7 @@ static int onmatch_create(struct hist_trigger_data *hist_data,
>  			continue;
>  		}
>  
> -		hist_err_event("onmatch: Param type doesn't match synthetic event field type: ",
> +		hist_err_event("trace action: Param type doesn't match synthetic event field type: ",
>  			       system, event_name, param);
>  		kfree(p);
>  		ret = -EINVAL;
> @@ -3709,12 +3748,11 @@ static int onmatch_create(struct hist_trigger_data *hist_data,
>  	}
>  
>  	if (field_pos != event->n_fields) {
> -		hist_err("onmatch: Param count doesn't match synthetic event field count: ", event->name);
> +		hist_err("trace action: Param count doesn't match synthetic event field count: ", event->name);
>  		ret = -EINVAL;
>  		goto err;
>  	}
>  
> -	data->fn = action_trace;
>  	data->onmatch.synth_event = event;
>  	data->onmatch.var_ref_idx = var_ref_idx;
>   out:
> @@ -3727,10 +3765,60 @@ static int onmatch_create(struct hist_trigger_data *hist_data,
>  	goto out;
>  }
>  
> +static int action_create(struct hist_trigger_data *hist_data,
> +			 struct action_data *data)
> +{
> +	struct field_var *field_var;
> +	unsigned int i;
> +	char *param;
> +	int ret = 0;
> +
> +	if (data->action == ACTION_TRACE) {
> +		ret = trace_action_create(hist_data, data);

This can return soon without "goto".

> +		goto out;
> +	}
> +
> +	if (data->action == ACTION_SAVE) {
> +		if (hist_data->n_save_vars) {
> +			ret = -EINVAL;

Maybe -EEXIST?

> +			hist_err("save action: Can't have more than one save() action per hist", "");
> +			goto out;
> +		}
> +
> +		for (i = 0; i < data->n_params; i++) {
> +			param = kstrdup(data->params[i], GFP_KERNEL);
> +			if (!param) {
> +				ret = -ENOMEM;
> +				goto out;
> +			}
> +
> +			field_var = create_target_field_var(hist_data, NULL, NULL, param);
> +			if (IS_ERR(field_var)) {
> +				hist_err("save action: Couldn't create field variable: ", param);
> +				ret = PTR_ERR(field_var);
> +				kfree(param);
> +				goto out;
> +			}
> +
> +			hist_data->save_vars[hist_data->n_save_vars++] = field_var;
> +			if (field_var->val->flags & HIST_FIELD_FL_STRING)
> +				hist_data->n_save_var_str++;
> +			kfree(param);
> +		}
> +	}
> + out:
> +	return ret;
> +}
> +
> +static int onmatch_create(struct hist_trigger_data *hist_data,
> +			  struct action_data *data)
> +{
> +	return action_create(hist_data, data);
> +}
> +
>  static struct action_data *onmatch_parse(struct trace_array *tr, char *str)
>  {
>  	char *match_event, *match_event_system;
> -	char *synth_event_name, *params;
>  	struct action_data *data;
>  	int ret = -EINVAL;
>  
> @@ -3768,33 +3856,11 @@ static struct action_data *onmatch_parse(struct trace_array *tr, char *str)
>  		goto free;
>  	}
>  
> -	strsep(&str, ".");
> -	if (!str) {
> -		hist_err("onmatch: Missing . after onmatch(): ", str);
> -		goto free;
> -	}
> -
> -	synth_event_name = strsep(&str, "(");
> -	if (!synth_event_name || !str) {
> -		hist_err("onmatch: Missing opening paramlist paren: ", synth_event_name);
> -		goto free;
> -	}
> -
> -	data->onmatch.synth_event_name = kstrdup(synth_event_name, GFP_KERNEL);
> -	if (!data->onmatch.synth_event_name) {
> -		ret = -ENOMEM;
> -		goto free;
> -	}
> -
> -	params = strsep(&str, ")");
> -	if (!params || !str || (str && strlen(str))) {
> -		hist_err("onmatch: Missing closing paramlist paren: ", params);
> -		goto free;
> -	}
> -
> -	ret = parse_action_params(params, data);
> +	ret = action_parse(str, data, HANDLER_ONMATCH);
>  	if (ret)
>  		goto free;
> +
> +	data->handler = HANDLER_ONMATCH;

This assignment seems to be done in action_parse().

>   out:
>  	return data;
>   free:
> @@ -4232,9 +4298,9 @@ static void destroy_actions(struct hist_trigger_data *hist_data)
>  	for (i = 0; i < hist_data->n_actions; i++) {
>  		struct action_data *data = hist_data->actions[i];
>  
> -		if (data->fn == action_trace)
> +		if (data->handler == HANDLER_ONMATCH)
>  			onmatch_destroy(data);
> -		else if (data->fn == onmax_save)
> +		else if (data->handler == HANDLER_ONMAX)
>  			onmax_destroy(data);
>  		else
>  			kfree(data);
> @@ -4260,16 +4326,14 @@ static int parse_actions(struct hist_trigger_data *hist_data)
>  				ret = PTR_ERR(data);
>  				break;
>  			}
> -			data->fn = action_trace;
>  		} else if (strncmp(str, "onmax(", strlen("onmax(")) == 0) {
>  			char *action_str = str + strlen("onmax(");
>  
> -			data = onmax_parse(action_str);
> +			data = onmax_parse(action_str, HANDLER_ONMAX);
>  			if (IS_ERR(data)) {
>  				ret = PTR_ERR(data);
>  				break;
>  			}
> -			data->fn = onmax_save;
>  		} else {
>  			ret = -EINVAL;
>  			break;
> @@ -4281,8 +4345,7 @@ static int parse_actions(struct hist_trigger_data *hist_data)
>  	return ret;
>  }
>  
> -static int create_actions(struct hist_trigger_data *hist_data,
> -			  struct trace_event_file *file)
> +static int create_actions(struct hist_trigger_data *hist_data)
>  {
>  	struct action_data *data;
>  	unsigned int i;
> @@ -4291,14 +4354,17 @@ static int create_actions(struct hist_trigger_data *hist_data,
>  	for (i = 0; i < hist_data->attrs->n_actions; i++) {
>  		data = hist_data->actions[i];
>  
> -		if (data->fn == action_trace) {
> -			ret = onmatch_create(hist_data, file, data);
> +		if (data->handler == HANDLER_ONMATCH) {
> +			ret = onmatch_create(hist_data, data);
>  			if (ret)
>  				return ret;
> -		} else if (data->fn == onmax_save) {
> +		} else if (data->handler == HANDLER_ONMAX) {
>  			ret = onmax_create(hist_data, data);
>  			if (ret)
>  				return ret;
> +		} else {
> +			ret = -EINVAL;

This is a bit odd (or just mixtured), return -EINVAL will be enough here.
Or above "if (ret) return ret;" shoud be "if (ret) break;" since this function
returns ret soon after this loop.

> +			break;
>  		}
>  	}
>  

Thank you,


-- 
Masami Hiramatsu <mhiramat@...nel.org>

Powered by blists - more mailing lists