Hi Tom, On Thu, 11 Oct 2018 16:01:58 -0500 Tom Zanussi <[email protected]> wrote:
> From: Tom Zanussi <[email protected]> > > 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(¶ms, ","); > 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 <[email protected]>
