* [PATCH 0/4] tracing: Updates to dynamic event API @ 2020-01-31 21:55 Tom Zanussi 2020-01-31 21:55 ` [PATCH 1/4] tracing: Consolidate some synth_event_trace code Tom Zanussi ` (3 more replies) 0 siblings, 4 replies; 12+ messages in thread From: Tom Zanussi @ 2020-01-31 21:55 UTC (permalink / raw) To: rostedt; +Cc: artem.bityutskiy, mhiramat, linux-kernel, linux-rt-users Hi Steve, This patchset adds some updates to the 'tracing: Add support for in-kernel dynamic event API', on top of ftrace/core, addressing the comments from v4 of that: - Consolidate the very similar functions synth_event_add_val/next_val() and inline find_synth_field(), as suggested by Steve - Replace the command-building code in dynevent_cmd with equivalent seq_buf functionality, as suggested by Steve - Move the check_arg callbacks from the arg objects and explicitly pass them to the add functions, as suggested by Masami - Get rid of a useless bit of code in dynevent_add_arg_pair() as suggested by Masami With these changes the dynamic event test modules work fine and the trigger selftests all pass. Thanks, Tom The following changes since commit d380dcde9a07ca5de4805dee11f58a98ec0ad6ff: tracing: Fix now invalid var_ref_vals assumption in trace action (2020-01-31 12:59:26 -0500) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-trace.git ftrace/synth-event-gen-updates-v1 Tom Zanussi (4): tracing: Consolidate some synth_event_trace code tracing: Remove check_arg() callbacks from dynevent args tracing: Remove useless code in dynevent_arg_pair_add() tracing: Use seq_buf for building dynevent_cmd string include/linux/trace_events.h | 4 +- kernel/trace/trace_dynevent.c | 110 ++++++++++----------------- kernel/trace/trace_dynevent.h | 11 ++- kernel/trace/trace_events_hist.c | 157 ++++++++++++++++----------------------- kernel/trace/trace_kprobe.c | 12 +-- 5 files changed, 118 insertions(+), 176 deletions(-) -- 2.14.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/4] tracing: Consolidate some synth_event_trace code 2020-01-31 21:55 [PATCH 0/4] tracing: Updates to dynamic event API Tom Zanussi @ 2020-01-31 21:55 ` Tom Zanussi 2020-01-31 22:43 ` Steven Rostedt 2020-01-31 21:55 ` [PATCH 2/4] tracing: Remove check_arg() callbacks from dynevent args Tom Zanussi ` (2 subsequent siblings) 3 siblings, 1 reply; 12+ messages in thread From: Tom Zanussi @ 2020-01-31 21:55 UTC (permalink / raw) To: rostedt; +Cc: artem.bityutskiy, mhiramat, linux-kernel, linux-rt-users The synth_event trace code contains some almost identical functions and some small functions that are called only once - consolidate the common code into single functions and fold in the small functions to simplify the code overall. Signed-off-by: Tom Zanussi <zanussi@kernel.org> --- kernel/trace/trace_events_hist.c | 139 ++++++++++++++++----------------------- 1 file changed, 56 insertions(+), 83 deletions(-) diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index 5b4e04780411..772bd3d7c29f 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -2053,24 +2053,72 @@ int synth_event_trace_start(struct trace_event_file *file, } EXPORT_SYMBOL_GPL(synth_event_trace_start); -static int save_synth_val(struct synth_field *field, u64 val, +int __synth_event_add_val(const char *field_name, u64 val, struct synth_event_trace_state *trace_state) { - struct synth_trace_event *entry = trace_state->entry; + struct synth_field *field = NULL; + struct synth_trace_event *entry; + struct synth_event *event; + int i, ret = 0; + + if (!trace_state) { + ret = -EINVAL; + goto out; + } + + /* can't mix add_next_synth_val() with add_synth_val() */ + if (field_name) { + if (trace_state->add_next) { + ret = -EINVAL; + goto out; + } + trace_state->add_name = true; + } else { + if (trace_state->add_name) { + ret = -EINVAL; + goto out; + } + trace_state->add_next = true; + } + + if (!trace_state->enabled) + goto out; + + event = trace_state->event; + if (trace_state->add_name) { + for (i = 0; i < event->n_fields; i++) { + field = event->fields[i]; + if (strcmp(field->name, field_name) == 0) + break; + } + if (!field) { + ret = -EINVAL; + goto out; + } + } else { + if (trace_state->cur_field >= event->n_fields) { + ret = -EINVAL; + goto out; + } + field = event->fields[trace_state->cur_field++]; + } + entry = trace_state->entry; if (field->is_string) { char *str_val = (char *)(long)val; char *str_field; - if (!str_val) - return -EINVAL; + if (!str_val) { + ret = -EINVAL; + goto out; + } str_field = (char *)&entry->fields[field->offset]; strscpy(str_field, str_val, STR_VAR_LEN_MAX); } else entry->fields[field->offset] = val; - - return 0; + out: + return ret; } /** @@ -2104,54 +2152,10 @@ static int save_synth_val(struct synth_field *field, u64 val, int synth_event_add_next_val(u64 val, struct synth_event_trace_state *trace_state) { - struct synth_field *field; - struct synth_event *event; - int ret = 0; - - if (!trace_state) { - ret = -EINVAL; - goto out; - } - - /* can't mix add_next_synth_val() with add_synth_val() */ - if (trace_state->add_name) { - ret = -EINVAL; - goto out; - } - trace_state->add_next = true; - - if (!trace_state->enabled) - goto out; - - event = trace_state->event; - - if (trace_state->cur_field >= event->n_fields) { - ret = -EINVAL; - goto out; - } - - field = event->fields[trace_state->cur_field++]; - ret = save_synth_val(field, val, trace_state); - out: - return ret; + return __synth_event_add_val(NULL, val, trace_state); } EXPORT_SYMBOL_GPL(synth_event_add_next_val); -static struct synth_field *find_synth_field(struct synth_event *event, - const char *field_name) -{ - struct synth_field *field = NULL; - unsigned int i; - - for (i = 0; i < event->n_fields; i++) { - field = event->fields[i]; - if (strcmp(field->name, field_name) == 0) - return field; - } - - return NULL; -} - /** * synth_event_add_val - Add a named field's value to an open synth trace * @field_name: The name of the synthetic event field value to set @@ -2183,38 +2187,7 @@ static struct synth_field *find_synth_field(struct synth_event *event, int synth_event_add_val(const char *field_name, u64 val, struct synth_event_trace_state *trace_state) { - struct synth_trace_event *entry; - struct synth_event *event; - struct synth_field *field; - int ret = 0; - - if (!trace_state) { - ret = -EINVAL; - goto out; - } - - /* can't mix add_next_synth_val() with add_synth_val() */ - if (trace_state->add_next) { - ret = -EINVAL; - goto out; - } - trace_state->add_name = true; - - if (!trace_state->enabled) - goto out; - - event = trace_state->event; - entry = trace_state->entry; - - field = find_synth_field(event, field_name); - if (!field) { - ret = -EINVAL; - goto out; - } - - ret = save_synth_val(field, val, trace_state); - out: - return ret; + return __synth_event_add_val(field_name, val, trace_state); } EXPORT_SYMBOL_GPL(synth_event_add_val); -- 2.14.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4] tracing: Consolidate some synth_event_trace code 2020-01-31 21:55 ` [PATCH 1/4] tracing: Consolidate some synth_event_trace code Tom Zanussi @ 2020-01-31 22:43 ` Steven Rostedt 2020-01-31 22:47 ` Tom Zanussi 2020-01-31 23:00 ` Tom Zanussi 0 siblings, 2 replies; 12+ messages in thread From: Steven Rostedt @ 2020-01-31 22:43 UTC (permalink / raw) To: Tom Zanussi; +Cc: artem.bityutskiy, mhiramat, linux-kernel, linux-rt-users On Fri, 31 Jan 2020 15:55:31 -0600 Tom Zanussi <zanussi@kernel.org> wrote: > +++ b/kernel/trace/trace_events_hist.c > @@ -2053,24 +2053,72 @@ int synth_event_trace_start(struct trace_event_file *file, > } > EXPORT_SYMBOL_GPL(synth_event_trace_start); > > -static int save_synth_val(struct synth_field *field, u64 val, > +int __synth_event_add_val(const char *field_name, u64 val, Hmm, shouldn't __synth_event_add_val() still be static? -- Steve > struct synth_event_trace_state *trace_state) > { > - struct synth_trace_event *entry = trace_state->entry; > + struct synth_field *field = NULL; > + struct synth_trace_event *entry; > + struct synth_event *event; > + int i, ret = 0; > + ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4] tracing: Consolidate some synth_event_trace code 2020-01-31 22:43 ` Steven Rostedt @ 2020-01-31 22:47 ` Tom Zanussi 2020-01-31 23:00 ` Tom Zanussi 1 sibling, 0 replies; 12+ messages in thread From: Tom Zanussi @ 2020-01-31 22:47 UTC (permalink / raw) To: Steven Rostedt; +Cc: artem.bityutskiy, mhiramat, linux-kernel, linux-rt-users Hi Steve, On Fri, 2020-01-31 at 17:43 -0500, Steven Rostedt wrote: > On Fri, 31 Jan 2020 15:55:31 -0600 > Tom Zanussi <zanussi@kernel.org> wrote: > > > +++ b/kernel/trace/trace_events_hist.c > > @@ -2053,24 +2053,72 @@ int synth_event_trace_start(struct > > trace_event_file *file, > > } > > EXPORT_SYMBOL_GPL(synth_event_trace_start); > > > > -static int save_synth_val(struct synth_field *field, u64 val, > > +int __synth_event_add_val(const char *field_name, u64 val, > > Hmm, shouldn't __synth_event_add_val() still be static? > It's a new function, but yeah, it should be static. Tom > -- Steve > > > struct synth_event_trace_state > > *trace_state) > > { > > - struct synth_trace_event *entry = trace_state->entry; > > + struct synth_field *field = NULL; > > + struct synth_trace_event *entry; > > + struct synth_event *event; > > + int i, ret = 0; > > + ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4] tracing: Consolidate some synth_event_trace code 2020-01-31 22:43 ` Steven Rostedt 2020-01-31 22:47 ` Tom Zanussi @ 2020-01-31 23:00 ` Tom Zanussi 2020-01-31 23:33 ` Steven Rostedt 1 sibling, 1 reply; 12+ messages in thread From: Tom Zanussi @ 2020-01-31 23:00 UTC (permalink / raw) To: Steven Rostedt; +Cc: artem.bityutskiy, mhiramat, linux-kernel, linux-rt-users On Fri, 2020-01-31 at 17:43 -0500, Steven Rostedt wrote: > On Fri, 31 Jan 2020 15:55:31 -0600 > Tom Zanussi <zanussi@kernel.org> wrote: > > > +++ b/kernel/trace/trace_events_hist.c > > @@ -2053,24 +2053,72 @@ int synth_event_trace_start(struct > > trace_event_file *file, > > } > > EXPORT_SYMBOL_GPL(synth_event_trace_start); > > > > -static int save_synth_val(struct synth_field *field, u64 val, > > +int __synth_event_add_val(const char *field_name, u64 val, > > Hmm, shouldn't __synth_event_add_val() still be static? > > -- Steve > > > struct synth_event_trace_state > > *trace_state) > > { > > - struct synth_trace_event *entry = trace_state->entry; > > + struct synth_field *field = NULL; > > + struct synth_trace_event *entry; > > + struct synth_event *event; > > + int i, ret = 0; > > + Here's the patch with __synth_event_add_val() made static. Let me know if you want me to just do a v2 of this series.. Thanks, Tom [PATCH] tracing: Consolidate some synth_event_trace code The synth_event trace code contains some almost identical functions and some small functions that are called only once - consolidate the common code into single functions and fold in the small functions to simplify the code overall. Signed-off-by: Tom Zanussi <zanussi@kernel.org> --- kernel/trace/trace_events_hist.c | 141 ++++++++++++++++----------------------- 1 file changed, 57 insertions(+), 84 deletions(-) diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index 5b4e04780411..42058a1b5146 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -2053,24 +2053,72 @@ int synth_event_trace_start(struct trace_event_file *file, } EXPORT_SYMBOL_GPL(synth_event_trace_start); -static int save_synth_val(struct synth_field *field, u64 val, - struct synth_event_trace_state *trace_state) +static int __synth_event_add_val(const char *field_name, u64 val, + struct synth_event_trace_state *trace_state) { - struct synth_trace_event *entry = trace_state->entry; + struct synth_field *field = NULL; + struct synth_trace_event *entry; + struct synth_event *event; + int i, ret = 0; + + if (!trace_state) { + ret = -EINVAL; + goto out; + } + + /* can't mix add_next_synth_val() with add_synth_val() */ + if (field_name) { + if (trace_state->add_next) { + ret = -EINVAL; + goto out; + } + trace_state->add_name = true; + } else { + if (trace_state->add_name) { + ret = -EINVAL; + goto out; + } + trace_state->add_next = true; + } + + if (!trace_state->enabled) + goto out; + + event = trace_state->event; + if (trace_state->add_name) { + for (i = 0; i < event->n_fields; i++) { + field = event->fields[i]; + if (strcmp(field->name, field_name) == 0) + break; + } + if (!field) { + ret = -EINVAL; + goto out; + } + } else { + if (trace_state->cur_field >= event->n_fields) { + ret = -EINVAL; + goto out; + } + field = event->fields[trace_state->cur_field++]; + } + entry = trace_state->entry; if (field->is_string) { char *str_val = (char *)(long)val; char *str_field; - if (!str_val) - return -EINVAL; + if (!str_val) { + ret = -EINVAL; + goto out; + } str_field = (char *)&entry->fields[field->offset]; strscpy(str_field, str_val, STR_VAR_LEN_MAX); } else entry->fields[field->offset] = val; - - return 0; + out: + return ret; } /** @@ -2104,54 +2152,10 @@ static int save_synth_val(struct synth_field *field, u64 val, int synth_event_add_next_val(u64 val, struct synth_event_trace_state *trace_state) { - struct synth_field *field; - struct synth_event *event; - int ret = 0; - - if (!trace_state) { - ret = -EINVAL; - goto out; - } - - /* can't mix add_next_synth_val() with add_synth_val() */ - if (trace_state->add_name) { - ret = -EINVAL; - goto out; - } - trace_state->add_next = true; - - if (!trace_state->enabled) - goto out; - - event = trace_state->event; - - if (trace_state->cur_field >= event->n_fields) { - ret = -EINVAL; - goto out; - } - - field = event->fields[trace_state->cur_field++]; - ret = save_synth_val(field, val, trace_state); - out: - return ret; + return __synth_event_add_val(NULL, val, trace_state); } EXPORT_SYMBOL_GPL(synth_event_add_next_val); -static struct synth_field *find_synth_field(struct synth_event *event, - const char *field_name) -{ - struct synth_field *field = NULL; - unsigned int i; - - for (i = 0; i < event->n_fields; i++) { - field = event->fields[i]; - if (strcmp(field->name, field_name) == 0) - return field; - } - - return NULL; -} - /** * synth_event_add_val - Add a named field's value to an open synth trace * @field_name: The name of the synthetic event field value to set @@ -2183,38 +2187,7 @@ static struct synth_field *find_synth_field(struct synth_event *event, int synth_event_add_val(const char *field_name, u64 val, struct synth_event_trace_state *trace_state) { - struct synth_trace_event *entry; - struct synth_event *event; - struct synth_field *field; - int ret = 0; - - if (!trace_state) { - ret = -EINVAL; - goto out; - } - - /* can't mix add_next_synth_val() with add_synth_val() */ - if (trace_state->add_next) { - ret = -EINVAL; - goto out; - } - trace_state->add_name = true; - - if (!trace_state->enabled) - goto out; - - event = trace_state->event; - entry = trace_state->entry; - - field = find_synth_field(event, field_name); - if (!field) { - ret = -EINVAL; - goto out; - } - - ret = save_synth_val(field, val, trace_state); - out: - return ret; + return __synth_event_add_val(field_name, val, trace_state); } EXPORT_SYMBOL_GPL(synth_event_add_val); -- 2.14.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4] tracing: Consolidate some synth_event_trace code 2020-01-31 23:00 ` Tom Zanussi @ 2020-01-31 23:33 ` Steven Rostedt 0 siblings, 0 replies; 12+ messages in thread From: Steven Rostedt @ 2020-01-31 23:33 UTC (permalink / raw) To: Tom Zanussi; +Cc: artem.bityutskiy, mhiramat, linux-kernel, linux-rt-users On Fri, 31 Jan 2020 17:00:32 -0600 Tom Zanussi <zanussi@kernel.org> wrote: > > Here's the patch with __synth_event_add_val() made static. Let me know > if you want me to just do a v2 of this series.. > No, I'll just pull this in over the other one and start testing it. Thanks a lot for getting this done today! -- Steve ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/4] tracing: Remove check_arg() callbacks from dynevent args 2020-01-31 21:55 [PATCH 0/4] tracing: Updates to dynamic event API Tom Zanussi 2020-01-31 21:55 ` [PATCH 1/4] tracing: Consolidate some synth_event_trace code Tom Zanussi @ 2020-01-31 21:55 ` Tom Zanussi 2020-02-01 7:28 ` Masami Hiramatsu 2020-01-31 21:55 ` [PATCH 3/4] tracing: Remove useless code in dynevent_arg_pair_add() Tom Zanussi 2020-01-31 21:55 ` [PATCH 4/4] tracing: Use seq_buf for building dynevent_cmd string Tom Zanussi 3 siblings, 1 reply; 12+ messages in thread From: Tom Zanussi @ 2020-01-31 21:55 UTC (permalink / raw) To: rostedt; +Cc: artem.bityutskiy, mhiramat, linux-kernel, linux-rt-users It's kind of strange to have check_arg() callbacks as part of the arg objects themselves; it makes more sense to just pass these in when the args are added instead. Remove the check_arg() callbacks from those objects which also means removing the check_arg() args from the init functions, adding them to the add functions and fixing up existing callers. Signed-off-by: Tom Zanussi <zanussi@kernel.org> --- kernel/trace/trace_dynevent.c | 62 ++++++++++++++++++---------------------- kernel/trace/trace_dynevent.h | 11 ++++--- kernel/trace/trace_events_hist.c | 16 +++++------ kernel/trace/trace_kprobe.c | 10 +++---- 4 files changed, 46 insertions(+), 53 deletions(-) diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c index 6ffdbc4fda53..f9cfcdc9d1f3 100644 --- a/kernel/trace/trace_dynevent.c +++ b/kernel/trace/trace_dynevent.c @@ -228,27 +228,30 @@ fs_initcall(init_dynamic_event); * dynevent_arg_add - Add an arg to a dynevent_cmd * @cmd: A pointer to the dynevent_cmd struct representing the new event cmd * @arg: The argument to append to the current cmd + * @check_arg: An (optional) pointer to a function checking arg sanity * * Append an argument to a dynevent_cmd. The argument string will be * appended to the current cmd string, followed by a separator, if - * applicable. Before the argument is added, the check_arg() - * function, if defined, is called. + * applicable. Before the argument is added, the @check_arg function, + * if present, will be used to check the sanity of the current arg + * string. * - * The cmd string, separator, and check_arg() function should be set - * using the dynevent_arg_init() before any arguments are added using - * this function. + * The cmd string and separator should be set using the + * dynevent_arg_init() before any arguments are added using this + * function. * * Return: 0 if successful, error otherwise. */ int dynevent_arg_add(struct dynevent_cmd *cmd, - struct dynevent_arg *arg) + struct dynevent_arg *arg, + dynevent_check_arg_fn_t check_arg) { int ret = 0; int delta; char *q; - if (arg->check_arg) { - ret = arg->check_arg(arg); + if (check_arg) { + ret = check_arg(arg); if (ret) return ret; } @@ -269,6 +272,7 @@ int dynevent_arg_add(struct dynevent_cmd *cmd, * dynevent_arg_pair_add - Add an arg pair to a dynevent_cmd * @cmd: A pointer to the dynevent_cmd struct representing the new event cmd * @arg_pair: The argument pair to append to the current cmd + * @check_arg: An (optional) pointer to a function checking arg sanity * * Append an argument pair to a dynevent_cmd. An argument pair * consists of a left-hand-side argument and a right-hand-side @@ -278,24 +282,26 @@ int dynevent_arg_add(struct dynevent_cmd *cmd, * * The lhs argument string will be appended to the current cmd string, * followed by an operator, if applicable, followd by the rhs string, - * followed finally by a separator, if applicable. Before anything is - * added, the check_arg() function, if defined, is called. + * followed finally by a separator, if applicable. Before the + * argument is added, the @check_arg function, if present, will be + * used to check the sanity of the current arg strings. * - * The cmd strings, operator, separator, and check_arg() function - * should be set using the dynevent_arg_pair_init() before any arguments - * are added using this function. + * The cmd strings, operator, and separator should be set using the + * dynevent_arg_pair_init() before any arguments are added using this + * function. * * Return: 0 if successful, error otherwise. */ int dynevent_arg_pair_add(struct dynevent_cmd *cmd, - struct dynevent_arg_pair *arg_pair) + struct dynevent_arg_pair *arg_pair, + dynevent_check_arg_fn_t check_arg) { int ret = 0; int delta; char *q; - if (arg_pair->check_arg) { - ret = arg_pair->check_arg(arg_pair); + if (check_arg) { + ret = check_arg(arg_pair); if (ret) return ret; } @@ -385,20 +391,16 @@ void dynevent_cmd_init(struct dynevent_cmd *cmd, char *buf, int maxlen, /** * dynevent_arg_init - Initialize a dynevent_arg object * @arg: A pointer to the dynevent_arg struct representing the arg - * @check_arg: An (optional) pointer to a function checking arg sanity * @separator: An (optional) separator, appended after adding the arg * * Initialize a dynevent_arg object. A dynevent_arg represents an * object used to append single arguments to the current command - * string. The @check_arg function, if present, will be used to check - * the sanity of the current arg string (which is directly set by the - * caller). After the arg string is successfully appended to the + * string. After the arg string is successfully appended to the * command string, the optional @separator is appended. If no * separator was specified when initializing the arg, a space will be * appended. */ void dynevent_arg_init(struct dynevent_arg *arg, - dynevent_check_arg_fn_t check_arg, char separator) { memset(arg, '\0', sizeof(*arg)); @@ -406,14 +408,11 @@ void dynevent_arg_init(struct dynevent_arg *arg, if (!separator) separator = ' '; arg->separator = separator; - - arg->check_arg = check_arg; } /** * dynevent_arg_pair_init - Initialize a dynevent_arg_pair object * @arg_pair: A pointer to the dynevent_arg_pair struct representing the arg - * @check_arg: An (optional) pointer to a function checking arg sanity * @operator: An (optional) operator, appended after adding the first arg * @separator: An (optional) separator, appended after adding the second arg * @@ -422,16 +421,13 @@ void dynevent_arg_init(struct dynevent_arg *arg, * variable_name;' or 'x+y' to the current command string. An * argument pair consists of a left-hand-side argument and a * right-hand-side argument separated by an operator, which can be - * whitespace, all followed by a separator, if applicable. The - * @check_arg function, if present, will be used to check the sanity - * of the current arg strings (which is directly set by the caller). - * After the first arg string is successfully appended to the command - * string, the optional @operator is appended, followed by the second - * arg and and optional @separator. If no separator was specified - * when initializing the arg, a space will be appended. + * whitespace, all followed by a separator, if applicable. After the + * first arg string is successfully appended to the command string, + * the optional @operator is appended, followed by the second arg and + * and optional @separator. If no separator was specified when + * initializing the arg, a space will be appended. */ void dynevent_arg_pair_init(struct dynevent_arg_pair *arg_pair, - dynevent_check_arg_fn_t check_arg, char operator, char separator) { memset(arg_pair, '\0', sizeof(*arg_pair)); @@ -443,8 +439,6 @@ void dynevent_arg_pair_init(struct dynevent_arg_pair *arg_pair, if (!separator) separator = ' '; arg_pair->separator = separator; - - arg_pair->check_arg = check_arg; } /** diff --git a/kernel/trace/trace_dynevent.h b/kernel/trace/trace_dynevent.h index b593fc34c5b1..d6857a254ede 100644 --- a/kernel/trace/trace_dynevent.h +++ b/kernel/trace/trace_dynevent.h @@ -126,28 +126,27 @@ typedef int (*dynevent_check_arg_fn_t)(void *data); struct dynevent_arg { const char *str; char separator; /* e.g. ';', ',', or nothing */ - dynevent_check_arg_fn_t check_arg; }; extern void dynevent_arg_init(struct dynevent_arg *arg, - dynevent_check_arg_fn_t check_arg, char separator); extern int dynevent_arg_add(struct dynevent_cmd *cmd, - struct dynevent_arg *arg); + struct dynevent_arg *arg, + dynevent_check_arg_fn_t check_arg); struct dynevent_arg_pair { const char *lhs; const char *rhs; char operator; /* e.g. '=' or nothing */ char separator; /* e.g. ';', ',', or nothing */ - dynevent_check_arg_fn_t check_arg; }; extern void dynevent_arg_pair_init(struct dynevent_arg_pair *arg_pair, - dynevent_check_arg_fn_t check_arg, char operator, char separator); + extern int dynevent_arg_pair_add(struct dynevent_cmd *cmd, - struct dynevent_arg_pair *arg_pair); + struct dynevent_arg_pair *arg_pair, + dynevent_check_arg_fn_t check_arg); extern int dynevent_str_add(struct dynevent_cmd *cmd, const char *str); #endif diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index 772bd3d7c29f..a566f5d290c1 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -1334,12 +1334,12 @@ int synth_event_add_field(struct dynevent_cmd *cmd, const char *type, if (!type || !name) return -EINVAL; - dynevent_arg_pair_init(&arg_pair, synth_event_check_arg_fn, 0, ';'); + dynevent_arg_pair_init(&arg_pair, 0, ';'); arg_pair.lhs = type; arg_pair.rhs = name; - ret = dynevent_arg_pair_add(cmd, &arg_pair); + ret = dynevent_arg_pair_add(cmd, &arg_pair, synth_event_check_arg_fn); if (ret) return ret; @@ -1377,11 +1377,11 @@ int synth_event_add_field_str(struct dynevent_cmd *cmd, const char *type_name) if (!type_name) return -EINVAL; - dynevent_arg_init(&arg, NULL, ';'); + dynevent_arg_init(&arg, ';'); arg.str = type_name; - ret = dynevent_arg_add(cmd, &arg); + ret = dynevent_arg_add(cmd, &arg, NULL); if (ret) return ret; @@ -1472,9 +1472,9 @@ int __synth_event_gen_cmd_start(struct dynevent_cmd *cmd, const char *name, if (cmd->type != DYNEVENT_TYPE_SYNTH) return -EINVAL; - dynevent_arg_init(&arg, NULL, 0); + dynevent_arg_init(&arg, 0); arg.str = name; - ret = dynevent_arg_add(cmd, &arg); + ret = dynevent_arg_add(cmd, &arg, NULL); if (ret) return ret; @@ -1546,9 +1546,9 @@ int synth_event_gen_cmd_array_start(struct dynevent_cmd *cmd, const char *name, if (n_fields > SYNTH_FIELDS_MAX) return -EINVAL; - dynevent_arg_init(&arg, NULL, 0); + dynevent_arg_init(&arg, 0); arg.str = name; - ret = dynevent_arg_add(cmd, &arg); + ret = dynevent_arg_add(cmd, &arg, NULL); if (ret) return ret; diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index 307abb724a71..fe183d4045d2 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -962,9 +962,9 @@ int __kprobe_event_gen_cmd_start(struct dynevent_cmd *cmd, bool kretprobe, if (ret) return ret; - dynevent_arg_init(&arg, NULL, 0); + dynevent_arg_init(&arg, 0); arg.str = loc; - ret = dynevent_arg_add(cmd, &arg); + ret = dynevent_arg_add(cmd, &arg, NULL); if (ret) return ret; @@ -982,7 +982,7 @@ int __kprobe_event_gen_cmd_start(struct dynevent_cmd *cmd, bool kretprobe, } arg.str = field; - ret = dynevent_arg_add(cmd, &arg); + ret = dynevent_arg_add(cmd, &arg, NULL); if (ret) break; } @@ -1017,7 +1017,7 @@ int __kprobe_event_add_fields(struct dynevent_cmd *cmd, ...) if (cmd->type != DYNEVENT_TYPE_KPROBE) return -EINVAL; - dynevent_arg_init(&arg, NULL, 0); + dynevent_arg_init(&arg, 0); va_start(args, cmd); for (;;) { @@ -1033,7 +1033,7 @@ int __kprobe_event_add_fields(struct dynevent_cmd *cmd, ...) } arg.str = field; - ret = dynevent_arg_add(cmd, &arg); + ret = dynevent_arg_add(cmd, &arg, NULL); if (ret) break; } -- 2.14.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/4] tracing: Remove check_arg() callbacks from dynevent args 2020-01-31 21:55 ` [PATCH 2/4] tracing: Remove check_arg() callbacks from dynevent args Tom Zanussi @ 2020-02-01 7:28 ` Masami Hiramatsu 0 siblings, 0 replies; 12+ messages in thread From: Masami Hiramatsu @ 2020-02-01 7:28 UTC (permalink / raw) To: Tom Zanussi Cc: rostedt, artem.bityutskiy, mhiramat, linux-kernel, linux-rt-users On Fri, 31 Jan 2020 15:55:32 -0600 Tom Zanussi <zanussi@kernel.org> wrote: > It's kind of strange to have check_arg() callbacks as part of the arg > objects themselves; it makes more sense to just pass these in when the > args are added instead. > > Remove the check_arg() callbacks from those objects which also means > removing the check_arg() args from the init functions, adding them to > the add functions and fixing up existing callers. Looks good to me. Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> Thank you, > > Signed-off-by: Tom Zanussi <zanussi@kernel.org> > --- > kernel/trace/trace_dynevent.c | 62 ++++++++++++++++++---------------------- > kernel/trace/trace_dynevent.h | 11 ++++--- > kernel/trace/trace_events_hist.c | 16 +++++------ > kernel/trace/trace_kprobe.c | 10 +++---- > 4 files changed, 46 insertions(+), 53 deletions(-) > > diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c > index 6ffdbc4fda53..f9cfcdc9d1f3 100644 > --- a/kernel/trace/trace_dynevent.c > +++ b/kernel/trace/trace_dynevent.c > @@ -228,27 +228,30 @@ fs_initcall(init_dynamic_event); > * dynevent_arg_add - Add an arg to a dynevent_cmd > * @cmd: A pointer to the dynevent_cmd struct representing the new event cmd > * @arg: The argument to append to the current cmd > + * @check_arg: An (optional) pointer to a function checking arg sanity > * > * Append an argument to a dynevent_cmd. The argument string will be > * appended to the current cmd string, followed by a separator, if > - * applicable. Before the argument is added, the check_arg() > - * function, if defined, is called. > + * applicable. Before the argument is added, the @check_arg function, > + * if present, will be used to check the sanity of the current arg > + * string. > * > - * The cmd string, separator, and check_arg() function should be set > - * using the dynevent_arg_init() before any arguments are added using > - * this function. > + * The cmd string and separator should be set using the > + * dynevent_arg_init() before any arguments are added using this > + * function. > * > * Return: 0 if successful, error otherwise. > */ > int dynevent_arg_add(struct dynevent_cmd *cmd, > - struct dynevent_arg *arg) > + struct dynevent_arg *arg, > + dynevent_check_arg_fn_t check_arg) > { > int ret = 0; > int delta; > char *q; > > - if (arg->check_arg) { > - ret = arg->check_arg(arg); > + if (check_arg) { > + ret = check_arg(arg); > if (ret) > return ret; > } > @@ -269,6 +272,7 @@ int dynevent_arg_add(struct dynevent_cmd *cmd, > * dynevent_arg_pair_add - Add an arg pair to a dynevent_cmd > * @cmd: A pointer to the dynevent_cmd struct representing the new event cmd > * @arg_pair: The argument pair to append to the current cmd > + * @check_arg: An (optional) pointer to a function checking arg sanity > * > * Append an argument pair to a dynevent_cmd. An argument pair > * consists of a left-hand-side argument and a right-hand-side > @@ -278,24 +282,26 @@ int dynevent_arg_add(struct dynevent_cmd *cmd, > * > * The lhs argument string will be appended to the current cmd string, > * followed by an operator, if applicable, followd by the rhs string, > - * followed finally by a separator, if applicable. Before anything is > - * added, the check_arg() function, if defined, is called. > + * followed finally by a separator, if applicable. Before the > + * argument is added, the @check_arg function, if present, will be > + * used to check the sanity of the current arg strings. > * > - * The cmd strings, operator, separator, and check_arg() function > - * should be set using the dynevent_arg_pair_init() before any arguments > - * are added using this function. > + * The cmd strings, operator, and separator should be set using the > + * dynevent_arg_pair_init() before any arguments are added using this > + * function. > * > * Return: 0 if successful, error otherwise. > */ > int dynevent_arg_pair_add(struct dynevent_cmd *cmd, > - struct dynevent_arg_pair *arg_pair) > + struct dynevent_arg_pair *arg_pair, > + dynevent_check_arg_fn_t check_arg) > { > int ret = 0; > int delta; > char *q; > > - if (arg_pair->check_arg) { > - ret = arg_pair->check_arg(arg_pair); > + if (check_arg) { > + ret = check_arg(arg_pair); > if (ret) > return ret; > } > @@ -385,20 +391,16 @@ void dynevent_cmd_init(struct dynevent_cmd *cmd, char *buf, int maxlen, > /** > * dynevent_arg_init - Initialize a dynevent_arg object > * @arg: A pointer to the dynevent_arg struct representing the arg > - * @check_arg: An (optional) pointer to a function checking arg sanity > * @separator: An (optional) separator, appended after adding the arg > * > * Initialize a dynevent_arg object. A dynevent_arg represents an > * object used to append single arguments to the current command > - * string. The @check_arg function, if present, will be used to check > - * the sanity of the current arg string (which is directly set by the > - * caller). After the arg string is successfully appended to the > + * string. After the arg string is successfully appended to the > * command string, the optional @separator is appended. If no > * separator was specified when initializing the arg, a space will be > * appended. > */ > void dynevent_arg_init(struct dynevent_arg *arg, > - dynevent_check_arg_fn_t check_arg, > char separator) > { > memset(arg, '\0', sizeof(*arg)); > @@ -406,14 +408,11 @@ void dynevent_arg_init(struct dynevent_arg *arg, > if (!separator) > separator = ' '; > arg->separator = separator; > - > - arg->check_arg = check_arg; > } > > /** > * dynevent_arg_pair_init - Initialize a dynevent_arg_pair object > * @arg_pair: A pointer to the dynevent_arg_pair struct representing the arg > - * @check_arg: An (optional) pointer to a function checking arg sanity > * @operator: An (optional) operator, appended after adding the first arg > * @separator: An (optional) separator, appended after adding the second arg > * > @@ -422,16 +421,13 @@ void dynevent_arg_init(struct dynevent_arg *arg, > * variable_name;' or 'x+y' to the current command string. An > * argument pair consists of a left-hand-side argument and a > * right-hand-side argument separated by an operator, which can be > - * whitespace, all followed by a separator, if applicable. The > - * @check_arg function, if present, will be used to check the sanity > - * of the current arg strings (which is directly set by the caller). > - * After the first arg string is successfully appended to the command > - * string, the optional @operator is appended, followed by the second > - * arg and and optional @separator. If no separator was specified > - * when initializing the arg, a space will be appended. > + * whitespace, all followed by a separator, if applicable. After the > + * first arg string is successfully appended to the command string, > + * the optional @operator is appended, followed by the second arg and > + * and optional @separator. If no separator was specified when > + * initializing the arg, a space will be appended. > */ > void dynevent_arg_pair_init(struct dynevent_arg_pair *arg_pair, > - dynevent_check_arg_fn_t check_arg, > char operator, char separator) > { > memset(arg_pair, '\0', sizeof(*arg_pair)); > @@ -443,8 +439,6 @@ void dynevent_arg_pair_init(struct dynevent_arg_pair *arg_pair, > if (!separator) > separator = ' '; > arg_pair->separator = separator; > - > - arg_pair->check_arg = check_arg; > } > > /** > diff --git a/kernel/trace/trace_dynevent.h b/kernel/trace/trace_dynevent.h > index b593fc34c5b1..d6857a254ede 100644 > --- a/kernel/trace/trace_dynevent.h > +++ b/kernel/trace/trace_dynevent.h > @@ -126,28 +126,27 @@ typedef int (*dynevent_check_arg_fn_t)(void *data); > struct dynevent_arg { > const char *str; > char separator; /* e.g. ';', ',', or nothing */ > - dynevent_check_arg_fn_t check_arg; > }; > > extern void dynevent_arg_init(struct dynevent_arg *arg, > - dynevent_check_arg_fn_t check_arg, > char separator); > extern int dynevent_arg_add(struct dynevent_cmd *cmd, > - struct dynevent_arg *arg); > + struct dynevent_arg *arg, > + dynevent_check_arg_fn_t check_arg); > > struct dynevent_arg_pair { > const char *lhs; > const char *rhs; > char operator; /* e.g. '=' or nothing */ > char separator; /* e.g. ';', ',', or nothing */ > - dynevent_check_arg_fn_t check_arg; > }; > > extern void dynevent_arg_pair_init(struct dynevent_arg_pair *arg_pair, > - dynevent_check_arg_fn_t check_arg, > char operator, char separator); > + > extern int dynevent_arg_pair_add(struct dynevent_cmd *cmd, > - struct dynevent_arg_pair *arg_pair); > + struct dynevent_arg_pair *arg_pair, > + dynevent_check_arg_fn_t check_arg); > extern int dynevent_str_add(struct dynevent_cmd *cmd, const char *str); > > #endif > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > index 772bd3d7c29f..a566f5d290c1 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -1334,12 +1334,12 @@ int synth_event_add_field(struct dynevent_cmd *cmd, const char *type, > if (!type || !name) > return -EINVAL; > > - dynevent_arg_pair_init(&arg_pair, synth_event_check_arg_fn, 0, ';'); > + dynevent_arg_pair_init(&arg_pair, 0, ';'); > > arg_pair.lhs = type; > arg_pair.rhs = name; > > - ret = dynevent_arg_pair_add(cmd, &arg_pair); > + ret = dynevent_arg_pair_add(cmd, &arg_pair, synth_event_check_arg_fn); > if (ret) > return ret; > > @@ -1377,11 +1377,11 @@ int synth_event_add_field_str(struct dynevent_cmd *cmd, const char *type_name) > if (!type_name) > return -EINVAL; > > - dynevent_arg_init(&arg, NULL, ';'); > + dynevent_arg_init(&arg, ';'); > > arg.str = type_name; > > - ret = dynevent_arg_add(cmd, &arg); > + ret = dynevent_arg_add(cmd, &arg, NULL); > if (ret) > return ret; > > @@ -1472,9 +1472,9 @@ int __synth_event_gen_cmd_start(struct dynevent_cmd *cmd, const char *name, > if (cmd->type != DYNEVENT_TYPE_SYNTH) > return -EINVAL; > > - dynevent_arg_init(&arg, NULL, 0); > + dynevent_arg_init(&arg, 0); > arg.str = name; > - ret = dynevent_arg_add(cmd, &arg); > + ret = dynevent_arg_add(cmd, &arg, NULL); > if (ret) > return ret; > > @@ -1546,9 +1546,9 @@ int synth_event_gen_cmd_array_start(struct dynevent_cmd *cmd, const char *name, > if (n_fields > SYNTH_FIELDS_MAX) > return -EINVAL; > > - dynevent_arg_init(&arg, NULL, 0); > + dynevent_arg_init(&arg, 0); > arg.str = name; > - ret = dynevent_arg_add(cmd, &arg); > + ret = dynevent_arg_add(cmd, &arg, NULL); > if (ret) > return ret; > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index 307abb724a71..fe183d4045d2 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -962,9 +962,9 @@ int __kprobe_event_gen_cmd_start(struct dynevent_cmd *cmd, bool kretprobe, > if (ret) > return ret; > > - dynevent_arg_init(&arg, NULL, 0); > + dynevent_arg_init(&arg, 0); > arg.str = loc; > - ret = dynevent_arg_add(cmd, &arg); > + ret = dynevent_arg_add(cmd, &arg, NULL); > if (ret) > return ret; > > @@ -982,7 +982,7 @@ int __kprobe_event_gen_cmd_start(struct dynevent_cmd *cmd, bool kretprobe, > } > > arg.str = field; > - ret = dynevent_arg_add(cmd, &arg); > + ret = dynevent_arg_add(cmd, &arg, NULL); > if (ret) > break; > } > @@ -1017,7 +1017,7 @@ int __kprobe_event_add_fields(struct dynevent_cmd *cmd, ...) > if (cmd->type != DYNEVENT_TYPE_KPROBE) > return -EINVAL; > > - dynevent_arg_init(&arg, NULL, 0); > + dynevent_arg_init(&arg, 0); > > va_start(args, cmd); > for (;;) { > @@ -1033,7 +1033,7 @@ int __kprobe_event_add_fields(struct dynevent_cmd *cmd, ...) > } > > arg.str = field; > - ret = dynevent_arg_add(cmd, &arg); > + ret = dynevent_arg_add(cmd, &arg, NULL); > if (ret) > break; > } > -- > 2.14.1 > -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/4] tracing: Remove useless code in dynevent_arg_pair_add() 2020-01-31 21:55 [PATCH 0/4] tracing: Updates to dynamic event API Tom Zanussi 2020-01-31 21:55 ` [PATCH 1/4] tracing: Consolidate some synth_event_trace code Tom Zanussi 2020-01-31 21:55 ` [PATCH 2/4] tracing: Remove check_arg() callbacks from dynevent args Tom Zanussi @ 2020-01-31 21:55 ` Tom Zanussi 2020-02-01 7:28 ` Masami Hiramatsu 2020-01-31 21:55 ` [PATCH 4/4] tracing: Use seq_buf for building dynevent_cmd string Tom Zanussi 3 siblings, 1 reply; 12+ messages in thread From: Tom Zanussi @ 2020-01-31 21:55 UTC (permalink / raw) To: rostedt; +Cc: artem.bityutskiy, mhiramat, linux-kernel, linux-rt-users The final addition to q is unnecessary, since q isn't ever used afterwards. Signed-off-by: Tom Zanussi <zanussi@kernel.org> --- kernel/trace/trace_dynevent.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c index f9cfcdc9d1f3..204275ec8d71 100644 --- a/kernel/trace/trace_dynevent.c +++ b/kernel/trace/trace_dynevent.c @@ -322,7 +322,7 @@ int dynevent_arg_pair_add(struct dynevent_cmd *cmd, pr_err("field string is too long: %s\n", arg_pair->rhs); return -E2BIG; } - cmd->remaining -= delta; q += delta; + cmd->remaining -= delta; return ret; } -- 2.14.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/4] tracing: Remove useless code in dynevent_arg_pair_add() 2020-01-31 21:55 ` [PATCH 3/4] tracing: Remove useless code in dynevent_arg_pair_add() Tom Zanussi @ 2020-02-01 7:28 ` Masami Hiramatsu 0 siblings, 0 replies; 12+ messages in thread From: Masami Hiramatsu @ 2020-02-01 7:28 UTC (permalink / raw) To: Tom Zanussi Cc: rostedt, artem.bityutskiy, mhiramat, linux-kernel, linux-rt-users On Fri, 31 Jan 2020 15:55:33 -0600 Tom Zanussi <zanussi@kernel.org> wrote: > The final addition to q is unnecessary, since q isn't ever used > afterwards. > Yeah, thanks for updating :) Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> Thank you, > Signed-off-by: Tom Zanussi <zanussi@kernel.org> > --- > kernel/trace/trace_dynevent.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c > index f9cfcdc9d1f3..204275ec8d71 100644 > --- a/kernel/trace/trace_dynevent.c > +++ b/kernel/trace/trace_dynevent.c > @@ -322,7 +322,7 @@ int dynevent_arg_pair_add(struct dynevent_cmd *cmd, > pr_err("field string is too long: %s\n", arg_pair->rhs); > return -E2BIG; > } > - cmd->remaining -= delta; q += delta; > + cmd->remaining -= delta; > > return ret; > } > -- > 2.14.1 > -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 4/4] tracing: Use seq_buf for building dynevent_cmd string 2020-01-31 21:55 [PATCH 0/4] tracing: Updates to dynamic event API Tom Zanussi ` (2 preceding siblings ...) 2020-01-31 21:55 ` [PATCH 3/4] tracing: Remove useless code in dynevent_arg_pair_add() Tom Zanussi @ 2020-01-31 21:55 ` Tom Zanussi 2020-02-01 7:48 ` Masami Hiramatsu 3 siblings, 1 reply; 12+ messages in thread From: Tom Zanussi @ 2020-01-31 21:55 UTC (permalink / raw) To: rostedt; +Cc: artem.bityutskiy, mhiramat, linux-kernel, linux-rt-users The dynevent_cmd commands that build up the command string don't need to do that themselves - there's a seq_buf facility that does pretty much the same thing those command are doing manually, so use it instead. Signed-off-by: Tom Zanussi <zanussi@kernel.org> --- include/linux/trace_events.h | 4 +--- kernel/trace/trace_dynevent.c | 48 +++++++++++----------------------------- kernel/trace/trace_events_hist.c | 2 +- kernel/trace/trace_kprobe.c | 2 +- 4 files changed, 16 insertions(+), 40 deletions(-) diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h index 7c307a7c9c6a..67f528ecb9e5 100644 --- a/include/linux/trace_events.h +++ b/include/linux/trace_events.h @@ -367,10 +367,8 @@ struct dynevent_cmd; typedef int (*dynevent_create_fn_t)(struct dynevent_cmd *cmd); struct dynevent_cmd { - char *buf; + struct seq_buf seq; const char *event_name; - int maxlen; - int remaining; unsigned int n_fields; enum dynevent_type type; dynevent_create_fn_t run_command; diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c index 204275ec8d71..9f2e8520b748 100644 --- a/kernel/trace/trace_dynevent.c +++ b/kernel/trace/trace_dynevent.c @@ -247,8 +247,6 @@ int dynevent_arg_add(struct dynevent_cmd *cmd, dynevent_check_arg_fn_t check_arg) { int ret = 0; - int delta; - char *q; if (check_arg) { ret = check_arg(arg); @@ -256,14 +254,11 @@ int dynevent_arg_add(struct dynevent_cmd *cmd, return ret; } - q = cmd->buf + (cmd->maxlen - cmd->remaining); - - delta = snprintf(q, cmd->remaining, " %s%c", arg->str, arg->separator); - if (delta >= cmd->remaining) { - pr_err("String is too long: %s\n", arg->str); + ret = seq_buf_printf(&cmd->seq, " %s%c", arg->str, arg->separator); + if (ret) { + pr_err("String is too long: %s%c\n", arg->str, arg->separator); return -E2BIG; } - cmd->remaining -= delta; return ret; } @@ -297,8 +292,6 @@ int dynevent_arg_pair_add(struct dynevent_cmd *cmd, dynevent_check_arg_fn_t check_arg) { int ret = 0; - int delta; - char *q; if (check_arg) { ret = check_arg(arg_pair); @@ -306,23 +299,15 @@ int dynevent_arg_pair_add(struct dynevent_cmd *cmd, return ret; } - q = cmd->buf + (cmd->maxlen - cmd->remaining); - - delta = snprintf(q, cmd->remaining, " %s%c", arg_pair->lhs, - arg_pair->operator); - if (delta >= cmd->remaining) { - pr_err("field string is too long: %s\n", arg_pair->lhs); - return -E2BIG; - } - cmd->remaining -= delta; q += delta; - - delta = snprintf(q, cmd->remaining, "%s%c", arg_pair->rhs, - arg_pair->separator); - if (delta >= cmd->remaining) { - pr_err("field string is too long: %s\n", arg_pair->rhs); + ret = seq_buf_printf(&cmd->seq, " %s%c%s%c", arg_pair->lhs, + arg_pair->operator, arg_pair->rhs, + arg_pair->separator); + if (ret) { + pr_err("field string is too long: %s%c%s%c\n", arg_pair->lhs, + arg_pair->operator, arg_pair->rhs, + arg_pair->separator); return -E2BIG; } - cmd->remaining -= delta; return ret; } @@ -340,17 +325,12 @@ int dynevent_arg_pair_add(struct dynevent_cmd *cmd, int dynevent_str_add(struct dynevent_cmd *cmd, const char *str) { int ret = 0; - int delta; - char *q; - - q = cmd->buf + (cmd->maxlen - cmd->remaining); - delta = snprintf(q, cmd->remaining, "%s", str); - if (delta >= cmd->remaining) { + ret = seq_buf_puts(&cmd->seq, str); + if (ret) { pr_err("String is too long: %s\n", str); return -E2BIG; } - cmd->remaining -= delta; return ret; } @@ -381,9 +361,7 @@ void dynevent_cmd_init(struct dynevent_cmd *cmd, char *buf, int maxlen, { memset(cmd, '\0', sizeof(*cmd)); - cmd->buf = buf; - cmd->maxlen = maxlen; - cmd->remaining = cmd->maxlen; + seq_buf_init(&cmd->seq, buf, maxlen); cmd->type = type; cmd->run_command = run_command; } diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index a566f5d290c1..6dc3078a6d02 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -1762,7 +1762,7 @@ static int synth_event_run_command(struct dynevent_cmd *cmd) struct synth_event *se; int ret; - ret = trace_run_command(cmd->buf, create_or_delete_synth_event); + ret = trace_run_command(cmd->seq.buffer, create_or_delete_synth_event); if (ret) return ret; diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index fe183d4045d2..51efc790aea8 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -903,7 +903,7 @@ static int create_or_delete_trace_kprobe(int argc, char **argv) static int trace_kprobe_run_command(struct dynevent_cmd *cmd) { - return trace_run_command(cmd->buf, create_or_delete_trace_kprobe); + return trace_run_command(cmd->seq.buffer, create_or_delete_trace_kprobe); } /** -- 2.14.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 4/4] tracing: Use seq_buf for building dynevent_cmd string 2020-01-31 21:55 ` [PATCH 4/4] tracing: Use seq_buf for building dynevent_cmd string Tom Zanussi @ 2020-02-01 7:48 ` Masami Hiramatsu 0 siblings, 0 replies; 12+ messages in thread From: Masami Hiramatsu @ 2020-02-01 7:48 UTC (permalink / raw) To: Tom Zanussi Cc: rostedt, artem.bityutskiy, mhiramat, linux-kernel, linux-rt-users On Fri, 31 Jan 2020 15:55:34 -0600 Tom Zanussi <zanussi@kernel.org> wrote: > The dynevent_cmd commands that build up the command string don't need > to do that themselves - there's a seq_buf facility that does pretty > much the same thing those command are doing manually, so use it > instead. Looks so neat :) Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> Thank you, > > Signed-off-by: Tom Zanussi <zanussi@kernel.org> > --- > include/linux/trace_events.h | 4 +--- > kernel/trace/trace_dynevent.c | 48 +++++++++++----------------------------- > kernel/trace/trace_events_hist.c | 2 +- > kernel/trace/trace_kprobe.c | 2 +- > 4 files changed, 16 insertions(+), 40 deletions(-) > > diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h > index 7c307a7c9c6a..67f528ecb9e5 100644 > --- a/include/linux/trace_events.h > +++ b/include/linux/trace_events.h > @@ -367,10 +367,8 @@ struct dynevent_cmd; > typedef int (*dynevent_create_fn_t)(struct dynevent_cmd *cmd); > > struct dynevent_cmd { > - char *buf; > + struct seq_buf seq; > const char *event_name; > - int maxlen; > - int remaining; > unsigned int n_fields; > enum dynevent_type type; > dynevent_create_fn_t run_command; > diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c > index 204275ec8d71..9f2e8520b748 100644 > --- a/kernel/trace/trace_dynevent.c > +++ b/kernel/trace/trace_dynevent.c > @@ -247,8 +247,6 @@ int dynevent_arg_add(struct dynevent_cmd *cmd, > dynevent_check_arg_fn_t check_arg) > { > int ret = 0; > - int delta; > - char *q; > > if (check_arg) { > ret = check_arg(arg); > @@ -256,14 +254,11 @@ int dynevent_arg_add(struct dynevent_cmd *cmd, > return ret; > } > > - q = cmd->buf + (cmd->maxlen - cmd->remaining); > - > - delta = snprintf(q, cmd->remaining, " %s%c", arg->str, arg->separator); > - if (delta >= cmd->remaining) { > - pr_err("String is too long: %s\n", arg->str); > + ret = seq_buf_printf(&cmd->seq, " %s%c", arg->str, arg->separator); > + if (ret) { > + pr_err("String is too long: %s%c\n", arg->str, arg->separator); > return -E2BIG; > } > - cmd->remaining -= delta; > > return ret; > } > @@ -297,8 +292,6 @@ int dynevent_arg_pair_add(struct dynevent_cmd *cmd, > dynevent_check_arg_fn_t check_arg) > { > int ret = 0; > - int delta; > - char *q; > > if (check_arg) { > ret = check_arg(arg_pair); > @@ -306,23 +299,15 @@ int dynevent_arg_pair_add(struct dynevent_cmd *cmd, > return ret; > } > > - q = cmd->buf + (cmd->maxlen - cmd->remaining); > - > - delta = snprintf(q, cmd->remaining, " %s%c", arg_pair->lhs, > - arg_pair->operator); > - if (delta >= cmd->remaining) { > - pr_err("field string is too long: %s\n", arg_pair->lhs); > - return -E2BIG; > - } > - cmd->remaining -= delta; q += delta; > - > - delta = snprintf(q, cmd->remaining, "%s%c", arg_pair->rhs, > - arg_pair->separator); > - if (delta >= cmd->remaining) { > - pr_err("field string is too long: %s\n", arg_pair->rhs); > + ret = seq_buf_printf(&cmd->seq, " %s%c%s%c", arg_pair->lhs, > + arg_pair->operator, arg_pair->rhs, > + arg_pair->separator); > + if (ret) { > + pr_err("field string is too long: %s%c%s%c\n", arg_pair->lhs, > + arg_pair->operator, arg_pair->rhs, > + arg_pair->separator); > return -E2BIG; > } > - cmd->remaining -= delta; > > return ret; > } > @@ -340,17 +325,12 @@ int dynevent_arg_pair_add(struct dynevent_cmd *cmd, > int dynevent_str_add(struct dynevent_cmd *cmd, const char *str) > { > int ret = 0; > - int delta; > - char *q; > - > - q = cmd->buf + (cmd->maxlen - cmd->remaining); > > - delta = snprintf(q, cmd->remaining, "%s", str); > - if (delta >= cmd->remaining) { > + ret = seq_buf_puts(&cmd->seq, str); > + if (ret) { > pr_err("String is too long: %s\n", str); > return -E2BIG; > } > - cmd->remaining -= delta; > > return ret; > } > @@ -381,9 +361,7 @@ void dynevent_cmd_init(struct dynevent_cmd *cmd, char *buf, int maxlen, > { > memset(cmd, '\0', sizeof(*cmd)); > > - cmd->buf = buf; > - cmd->maxlen = maxlen; > - cmd->remaining = cmd->maxlen; > + seq_buf_init(&cmd->seq, buf, maxlen); > cmd->type = type; > cmd->run_command = run_command; > } > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > index a566f5d290c1..6dc3078a6d02 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -1762,7 +1762,7 @@ static int synth_event_run_command(struct dynevent_cmd *cmd) > struct synth_event *se; > int ret; > > - ret = trace_run_command(cmd->buf, create_or_delete_synth_event); > + ret = trace_run_command(cmd->seq.buffer, create_or_delete_synth_event); > if (ret) > return ret; > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index fe183d4045d2..51efc790aea8 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -903,7 +903,7 @@ static int create_or_delete_trace_kprobe(int argc, char **argv) > > static int trace_kprobe_run_command(struct dynevent_cmd *cmd) > { > - return trace_run_command(cmd->buf, create_or_delete_trace_kprobe); > + return trace_run_command(cmd->seq.buffer, create_or_delete_trace_kprobe); > } > > /** > -- > 2.14.1 > -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-02-01 7:48 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-31 21:55 [PATCH 0/4] tracing: Updates to dynamic event API Tom Zanussi 2020-01-31 21:55 ` [PATCH 1/4] tracing: Consolidate some synth_event_trace code Tom Zanussi 2020-01-31 22:43 ` Steven Rostedt 2020-01-31 22:47 ` Tom Zanussi 2020-01-31 23:00 ` Tom Zanussi 2020-01-31 23:33 ` Steven Rostedt 2020-01-31 21:55 ` [PATCH 2/4] tracing: Remove check_arg() callbacks from dynevent args Tom Zanussi 2020-02-01 7:28 ` Masami Hiramatsu 2020-01-31 21:55 ` [PATCH 3/4] tracing: Remove useless code in dynevent_arg_pair_add() Tom Zanussi 2020-02-01 7:28 ` Masami Hiramatsu 2020-01-31 21:55 ` [PATCH 4/4] tracing: Use seq_buf for building dynevent_cmd string Tom Zanussi 2020-02-01 7:48 ` Masami Hiramatsu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).