Re: [PATCH v6 01/15] tracing: Refactor hist trigger action code

From: Tom Zanussi
Date: Tue Oct 23 2018 - 17:18:12 EST

Hi Masami,

On Tue, 2018-10-23 at 14:09 +0900, Masami Hiramatsu wrote:
> Hi Tom,
>
> On Thu, 11 Oct 2018 16:01:58 -0500
> Tom Zanussi <zanussi@xxxxxxxxxx> wrote:
>
> > From: Tom Zanussi <tom.zanussi@xxxxxxxxxxxxxxx>
> >
> > 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().
>

Later patches change things so it makes more sense i.e. onmax_print is
renamed to track_data_print() which handles not just ONMAX but ONCHANGE
as well, and then this check is needed. But I can move this check to
those patches.

> > + 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.

Same here, this changes to track_data_create() in later patches, where
it makes sense. But, will move it there.

>
> > + 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.


Makes sense, will change.

>
> > + 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,
> > - syst
> > em,
> > - even
> > t_name,
> > - para
> > m);
> > + 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?
>

Yep, makes sense, will change.

> > + 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.
>

Yeah, I'll change them all to just break, and have them all fall into
the final ret.

Thanks,

Tom

> > + break;
> > }
> > }
> >
>
> Thank you,
>
>