* [PATCH 0/3] tracing: Synthetic event fixes and updates
@ 2020-02-10 23:06 Tom Zanussi
2020-02-10 23:06 ` [PATCH 1/3] tracing: Add missing nest end to synth_event_trace_start() error case Tom Zanussi
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Tom Zanussi @ 2020-02-10 23:06 UTC (permalink / raw)
To: rostedt; +Cc: artem.bityutskiy, mhiramat, linux-kernel, linux-rt-users
Hi Steve,
I noticed a couple bugs while creating a patch consolidating the
synthetic event trace() functions. Here are the two bugfixes along
with the consolidated trace patch.
The first patch adds a missing ring_buffer_nest_end() in an error
case, and the second removes unnecessary error returns when an event
is soft disabled.
The third patch consolidates the common code in synth_event_trace_array(),
synth_event_trace() and synth_event_trace_start().
Thanks,
Tom
The following changes since commit 7a1f8097178832627261a16e32973b12a0355dad:
bootconfig: Use parse_args() to find bootconfig and '--' (2020-02-07 20:51:08 -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-fixes-v1
Tom Zanussi (3):
tracing: Add missing nest end to synth_event_trace_start() error case
tracing: Don't return -EINVAL when tracing soft disabled synth events
tracing: Consolidate trace() functions
include/linux/trace_events.h | 2 +-
kernel/trace/trace_events_hist.c | 227 +++++++++++++++------------------------
2 files changed, 87 insertions(+), 142 deletions(-)
--
2.14.1
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/3] tracing: Add missing nest end to synth_event_trace_start() error case 2020-02-10 23:06 [PATCH 0/3] tracing: Synthetic event fixes and updates Tom Zanussi @ 2020-02-10 23:06 ` Tom Zanussi 2020-02-12 3:24 ` Masami Hiramatsu 2020-02-10 23:06 ` [PATCH 2/3] tracing: Don't return -EINVAL when tracing soft disabled synth events Tom Zanussi 2020-02-10 23:06 ` [PATCH 3/3] tracing: Consolidate trace() functions Tom Zanussi 2 siblings, 1 reply; 9+ messages in thread From: Tom Zanussi @ 2020-02-10 23:06 UTC (permalink / raw) To: rostedt; +Cc: artem.bityutskiy, mhiramat, linux-kernel, linux-rt-users If the ring_buffer reserve in synth_event_trace_start() fails, the matching ring_buffer_nest_end() should be called in the error code, since nothing else will ever call it in this case. Signed-off-by: Tom Zanussi <zanussi@kernel.org> --- kernel/trace/trace_events_hist.c | 1 + 1 file changed, 1 insertion(+) diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index b3bcfd8c7332..a546ffa14785 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -2043,6 +2043,7 @@ int synth_event_trace_start(struct trace_event_file *file, entry = trace_event_buffer_reserve(&trace_state->fbuffer, file, sizeof(*entry) + fields_size); if (!entry) { + ring_buffer_nest_end(trace_state->buffer); ret = -EINVAL; goto out; } -- 2.14.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] tracing: Add missing nest end to synth_event_trace_start() error case 2020-02-10 23:06 ` [PATCH 1/3] tracing: Add missing nest end to synth_event_trace_start() error case Tom Zanussi @ 2020-02-12 3:24 ` Masami Hiramatsu 0 siblings, 0 replies; 9+ messages in thread From: Masami Hiramatsu @ 2020-02-12 3:24 UTC (permalink / raw) To: Tom Zanussi Cc: rostedt, artem.bityutskiy, mhiramat, linux-kernel, linux-rt-users On Mon, 10 Feb 2020 17:06:48 -0600 Tom Zanussi <zanussi@kernel.org> wrote: > If the ring_buffer reserve in synth_event_trace_start() fails, the > matching ring_buffer_nest_end() should be called in the error code, > since nothing else will ever call it in this case. > Looks good to me. Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> Thanks, > Signed-off-by: Tom Zanussi <zanussi@kernel.org> > --- > kernel/trace/trace_events_hist.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > index b3bcfd8c7332..a546ffa14785 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -2043,6 +2043,7 @@ int synth_event_trace_start(struct trace_event_file *file, > entry = trace_event_buffer_reserve(&trace_state->fbuffer, file, > sizeof(*entry) + fields_size); > if (!entry) { > + ring_buffer_nest_end(trace_state->buffer); > ret = -EINVAL; > goto out; > } > -- > 2.14.1 > -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] tracing: Don't return -EINVAL when tracing soft disabled synth events 2020-02-10 23:06 [PATCH 0/3] tracing: Synthetic event fixes and updates Tom Zanussi 2020-02-10 23:06 ` [PATCH 1/3] tracing: Add missing nest end to synth_event_trace_start() error case Tom Zanussi @ 2020-02-10 23:06 ` Tom Zanussi 2020-02-12 3:24 ` Masami Hiramatsu 2020-02-10 23:06 ` [PATCH 3/3] tracing: Consolidate trace() functions Tom Zanussi 2 siblings, 1 reply; 9+ messages in thread From: Tom Zanussi @ 2020-02-10 23:06 UTC (permalink / raw) To: rostedt; +Cc: artem.bityutskiy, mhiramat, linux-kernel, linux-rt-users There's no reason to return -EINVAL when tracing a synthetic event if it's soft disabled - treat it the same as if it were hard disabled and return normally. Have synth_event_trace() and synth_event_trace_array() just return normally, and have synth_event_trace_start set the trace state to disabled and return. Signed-off-by: Tom Zanussi <zanussi@kernel.org> --- kernel/trace/trace_events_hist.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index a546ffa14785..99a02168599b 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -1828,7 +1828,8 @@ int synth_event_trace(struct trace_event_file *file, unsigned int n_vals, ...) * called directly by the user, we don't have that but we * still need to honor not logging when disabled. */ - if (!(file->flags & EVENT_FILE_FL_ENABLED)) + if (!(file->flags & EVENT_FILE_FL_ENABLED) || + trace_trigger_soft_disabled(file)) return 0; event = file->event_call->data; @@ -1836,9 +1837,6 @@ int synth_event_trace(struct trace_event_file *file, unsigned int n_vals, ...) if (n_vals != event->n_fields) return -EINVAL; - if (trace_trigger_soft_disabled(file)) - return -EINVAL; - fields_size = event->n_u64 * sizeof(u64); /* @@ -1918,7 +1916,8 @@ int synth_event_trace_array(struct trace_event_file *file, u64 *vals, * called directly by the user, we don't have that but we * still need to honor not logging when disabled. */ - if (!(file->flags & EVENT_FILE_FL_ENABLED)) + if (!(file->flags & EVENT_FILE_FL_ENABLED) || + trace_trigger_soft_disabled(file)) return 0; event = file->event_call->data; @@ -1926,9 +1925,6 @@ int synth_event_trace_array(struct trace_event_file *file, u64 *vals, if (n_vals != event->n_fields) return -EINVAL; - if (trace_trigger_soft_disabled(file)) - return -EINVAL; - fields_size = event->n_u64 * sizeof(u64); /* @@ -2017,7 +2013,8 @@ int synth_event_trace_start(struct trace_event_file *file, * trace case, we save the enabed state upon start and just * ignore the following data calls. */ - if (!(file->flags & EVENT_FILE_FL_ENABLED)) { + if (!(file->flags & EVENT_FILE_FL_ENABLED) || + trace_trigger_soft_disabled(file)) { trace_state->enabled = false; goto out; } @@ -2026,11 +2023,6 @@ int synth_event_trace_start(struct trace_event_file *file, trace_state->event = file->event_call->data; - if (trace_trigger_soft_disabled(file)) { - ret = -EINVAL; - goto out; - } - fields_size = trace_state->event->n_u64 * sizeof(u64); /* -- 2.14.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] tracing: Don't return -EINVAL when tracing soft disabled synth events 2020-02-10 23:06 ` [PATCH 2/3] tracing: Don't return -EINVAL when tracing soft disabled synth events Tom Zanussi @ 2020-02-12 3:24 ` Masami Hiramatsu 2020-02-12 3:28 ` Steven Rostedt 0 siblings, 1 reply; 9+ messages in thread From: Masami Hiramatsu @ 2020-02-12 3:24 UTC (permalink / raw) To: Tom Zanussi Cc: rostedt, artem.bityutskiy, mhiramat, linux-kernel, linux-rt-users On Mon, 10 Feb 2020 17:06:49 -0600 Tom Zanussi <zanussi@kernel.org> wrote: > There's no reason to return -EINVAL when tracing a synthetic event if > it's soft disabled - treat it the same as if it were hard disabled and > return normally. > > Have synth_event_trace() and synth_event_trace_array() just return > normally, and have synth_event_trace_start set the trace state to > disabled and return. > Looks good to me. Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> Thanks, > Signed-off-by: Tom Zanussi <zanussi@kernel.org> > --- > kernel/trace/trace_events_hist.c | 20 ++++++-------------- > 1 file changed, 6 insertions(+), 14 deletions(-) > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > index a546ffa14785..99a02168599b 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -1828,7 +1828,8 @@ int synth_event_trace(struct trace_event_file *file, unsigned int n_vals, ...) > * called directly by the user, we don't have that but we > * still need to honor not logging when disabled. > */ > - if (!(file->flags & EVENT_FILE_FL_ENABLED)) > + if (!(file->flags & EVENT_FILE_FL_ENABLED) || > + trace_trigger_soft_disabled(file)) > return 0; > > event = file->event_call->data; > @@ -1836,9 +1837,6 @@ int synth_event_trace(struct trace_event_file *file, unsigned int n_vals, ...) > if (n_vals != event->n_fields) > return -EINVAL; > > - if (trace_trigger_soft_disabled(file)) > - return -EINVAL; > - > fields_size = event->n_u64 * sizeof(u64); > > /* > @@ -1918,7 +1916,8 @@ int synth_event_trace_array(struct trace_event_file *file, u64 *vals, > * called directly by the user, we don't have that but we > * still need to honor not logging when disabled. > */ > - if (!(file->flags & EVENT_FILE_FL_ENABLED)) > + if (!(file->flags & EVENT_FILE_FL_ENABLED) || > + trace_trigger_soft_disabled(file)) > return 0; > > event = file->event_call->data; > @@ -1926,9 +1925,6 @@ int synth_event_trace_array(struct trace_event_file *file, u64 *vals, > if (n_vals != event->n_fields) > return -EINVAL; > > - if (trace_trigger_soft_disabled(file)) > - return -EINVAL; > - > fields_size = event->n_u64 * sizeof(u64); > > /* > @@ -2017,7 +2013,8 @@ int synth_event_trace_start(struct trace_event_file *file, > * trace case, we save the enabed state upon start and just > * ignore the following data calls. > */ > - if (!(file->flags & EVENT_FILE_FL_ENABLED)) { > + if (!(file->flags & EVENT_FILE_FL_ENABLED) || > + trace_trigger_soft_disabled(file)) { > trace_state->enabled = false; > goto out; > } > @@ -2026,11 +2023,6 @@ int synth_event_trace_start(struct trace_event_file *file, > > trace_state->event = file->event_call->data; > > - if (trace_trigger_soft_disabled(file)) { > - ret = -EINVAL; > - goto out; > - } > - > fields_size = trace_state->event->n_u64 * sizeof(u64); > > /* > -- > 2.14.1 > -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] tracing: Don't return -EINVAL when tracing soft disabled synth events 2020-02-12 3:24 ` Masami Hiramatsu @ 2020-02-12 3:28 ` Steven Rostedt 2020-02-12 5:24 ` Masami Hiramatsu 0 siblings, 1 reply; 9+ messages in thread From: Steven Rostedt @ 2020-02-12 3:28 UTC (permalink / raw) To: Masami Hiramatsu Cc: Tom Zanussi, artem.bityutskiy, linux-kernel, linux-rt-users On Wed, 12 Feb 2020 12:24:15 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > On Mon, 10 Feb 2020 17:06:49 -0600 > Tom Zanussi <zanussi@kernel.org> wrote: > > > There's no reason to return -EINVAL when tracing a synthetic event if > > it's soft disabled - treat it the same as if it were hard disabled and > > return normally. > > > > Have synth_event_trace() and synth_event_trace_array() just return > > normally, and have synth_event_trace_start set the trace state to > > disabled and return. > > > > Looks good to me. > > Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> > Thanks for the review Masami, but these patches have already landed in Linus's tree ;-) -- Steve ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] tracing: Don't return -EINVAL when tracing soft disabled synth events 2020-02-12 3:28 ` Steven Rostedt @ 2020-02-12 5:24 ` Masami Hiramatsu 0 siblings, 0 replies; 9+ messages in thread From: Masami Hiramatsu @ 2020-02-12 5:24 UTC (permalink / raw) To: Steven Rostedt Cc: Tom Zanussi, artem.bityutskiy, linux-kernel, linux-rt-users On Tue, 11 Feb 2020 22:28:56 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > On Wed, 12 Feb 2020 12:24:15 +0900 > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > On Mon, 10 Feb 2020 17:06:49 -0600 > > Tom Zanussi <zanussi@kernel.org> wrote: > > > > > There's no reason to return -EINVAL when tracing a synthetic event if > > > it's soft disabled - treat it the same as if it were hard disabled and > > > return normally. > > > > > > Have synth_event_trace() and synth_event_trace_array() just return > > > normally, and have synth_event_trace_start set the trace state to > > > disabled and return. > > > > > > > Looks good to me. > > > > Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> > > > > Thanks for the review Masami, but these patches have already landed in > Linus's tree ;-) Oh, OK. But I think [3/3] still has a real bug (not checking state->disabled in __synth_event_trace_end()). I'll send a fix. Thank you, -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] tracing: Consolidate trace() functions 2020-02-10 23:06 [PATCH 0/3] tracing: Synthetic event fixes and updates Tom Zanussi 2020-02-10 23:06 ` [PATCH 1/3] tracing: Add missing nest end to synth_event_trace_start() error case Tom Zanussi 2020-02-10 23:06 ` [PATCH 2/3] tracing: Don't return -EINVAL when tracing soft disabled synth events Tom Zanussi @ 2020-02-10 23:06 ` Tom Zanussi 2020-02-12 3:36 ` Masami Hiramatsu 2 siblings, 1 reply; 9+ messages in thread From: Tom Zanussi @ 2020-02-10 23:06 UTC (permalink / raw) To: rostedt; +Cc: artem.bityutskiy, mhiramat, linux-kernel, linux-rt-users Move the checking, buffer reserve and buffer commit code in synth_event_trace_start/end() into inline functions __synth_event_trace_start/end() so they can also be used by synth_event_trace() and synth_event_trace_array(), and then have all those functions use them. Also, change synth_event_trace_state.enabled to disabled so it only needs to be set if the event is disabled, which is not normally the case. Signed-off-by: Tom Zanussi <zanussi@kernel.org> --- include/linux/trace_events.h | 2 +- kernel/trace/trace_events_hist.c | 220 +++++++++++++++------------------------ 2 files changed, 87 insertions(+), 135 deletions(-) diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h index 67f528ecb9e5..21098298b49b 100644 --- a/include/linux/trace_events.h +++ b/include/linux/trace_events.h @@ -424,7 +424,7 @@ struct synth_event_trace_state { struct synth_event *event; unsigned int cur_field; unsigned int n_u64; - bool enabled; + bool disabled; bool add_next; bool add_name; }; diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index 99a02168599b..65b54d6a1422 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -1791,6 +1791,60 @@ void synth_event_cmd_init(struct dynevent_cmd *cmd, char *buf, int maxlen) } EXPORT_SYMBOL_GPL(synth_event_cmd_init); +static inline int +__synth_event_trace_start(struct trace_event_file *file, + struct synth_event_trace_state *trace_state) +{ + int entry_size, fields_size = 0; + int ret = 0; + + /* + * Normal event tracing doesn't get called at all unless the + * ENABLED bit is set (which attaches the probe thus allowing + * this code to be called, etc). Because this is called + * directly by the user, we don't have that but we still need + * to honor not logging when disabled. For the the iterated + * trace case, we save the enabed state upon start and just + * ignore the following data calls. + */ + if (!(file->flags & EVENT_FILE_FL_ENABLED) || + trace_trigger_soft_disabled(file)) { + trace_state->disabled = true; + ret = -ENOENT; + goto out; + } + + trace_state->event = file->event_call->data; + + fields_size = trace_state->event->n_u64 * sizeof(u64); + + /* + * Avoid ring buffer recursion detection, as this event + * is being performed within another event. + */ + trace_state->buffer = file->tr->array_buffer.buffer; + ring_buffer_nest_start(trace_state->buffer); + + entry_size = sizeof(*trace_state->entry) + fields_size; + trace_state->entry = trace_event_buffer_reserve(&trace_state->fbuffer, + file, + entry_size); + if (!trace_state->entry) { + ring_buffer_nest_end(trace_state->buffer); + ret = -EINVAL; + } +out: + return ret; +} + +static inline void +__synth_event_trace_end(struct synth_event_trace_state *trace_state) +{ + trace_event_buffer_commit(&trace_state->fbuffer); + + ring_buffer_nest_end(trace_state->buffer); +} + /** * synth_event_trace - Trace a synthetic event * @file: The trace_event_file representing the synthetic event @@ -1812,69 +1866,38 @@ EXPORT_SYMBOL_GPL(synth_event_cmd_init); */ int synth_event_trace(struct trace_event_file *file, unsigned int n_vals, ...) { - struct trace_event_buffer fbuffer; - struct synth_trace_event *entry; - struct trace_buffer *buffer; - struct synth_event *event; + struct synth_event_trace_state state; unsigned int i, n_u64; - int fields_size = 0; va_list args; - int ret = 0; - - /* - * Normal event generation doesn't get called at all unless - * the ENABLED bit is set (which attaches the probe thus - * allowing this code to be called, etc). Because this is - * called directly by the user, we don't have that but we - * still need to honor not logging when disabled. - */ - if (!(file->flags & EVENT_FILE_FL_ENABLED) || - trace_trigger_soft_disabled(file)) - return 0; - - event = file->event_call->data; - - if (n_vals != event->n_fields) - return -EINVAL; - - fields_size = event->n_u64 * sizeof(u64); - - /* - * Avoid ring buffer recursion detection, as this event - * is being performed within another event. - */ - buffer = file->tr->array_buffer.buffer; - ring_buffer_nest_start(buffer); + int ret; - entry = trace_event_buffer_reserve(&fbuffer, file, - sizeof(*entry) + fields_size); - if (!entry) { - ret = -EINVAL; - goto out; + ret = __synth_event_trace_start(file, &state); + if (ret) { + if (ret == -ENOENT) + ret = 0; /* just disabled, not really an error */ + return ret; } va_start(args, n_vals); - for (i = 0, n_u64 = 0; i < event->n_fields; i++) { + for (i = 0, n_u64 = 0; i < state.event->n_fields; i++) { u64 val; val = va_arg(args, u64); - if (event->fields[i]->is_string) { + if (state.event->fields[i]->is_string) { char *str_val = (char *)(long)val; - char *str_field = (char *)&entry->fields[n_u64]; + char *str_field = (char *)&state.entry->fields[n_u64]; strscpy(str_field, str_val, STR_VAR_LEN_MAX); n_u64 += STR_VAR_LEN_MAX / sizeof(u64); } else { - entry->fields[n_u64] = val; + state.entry->fields[n_u64] = val; n_u64++; } } va_end(args); - trace_event_buffer_commit(&fbuffer); -out: - ring_buffer_nest_end(buffer); + __synth_event_trace_end(&state); return ret; } @@ -1901,62 +1924,31 @@ EXPORT_SYMBOL_GPL(synth_event_trace); int synth_event_trace_array(struct trace_event_file *file, u64 *vals, unsigned int n_vals) { - struct trace_event_buffer fbuffer; - struct synth_trace_event *entry; - struct trace_buffer *buffer; - struct synth_event *event; + struct synth_event_trace_state state; unsigned int i, n_u64; - int fields_size = 0; - int ret = 0; - - /* - * Normal event generation doesn't get called at all unless - * the ENABLED bit is set (which attaches the probe thus - * allowing this code to be called, etc). Because this is - * called directly by the user, we don't have that but we - * still need to honor not logging when disabled. - */ - if (!(file->flags & EVENT_FILE_FL_ENABLED) || - trace_trigger_soft_disabled(file)) - return 0; - - event = file->event_call->data; - - if (n_vals != event->n_fields) - return -EINVAL; - - fields_size = event->n_u64 * sizeof(u64); - - /* - * Avoid ring buffer recursion detection, as this event - * is being performed within another event. - */ - buffer = file->tr->array_buffer.buffer; - ring_buffer_nest_start(buffer); + int ret; - entry = trace_event_buffer_reserve(&fbuffer, file, - sizeof(*entry) + fields_size); - if (!entry) { - ret = -EINVAL; - goto out; + ret = __synth_event_trace_start(file, &state); + if (ret) { + if (ret == -ENOENT) + ret = 0; /* just disabled, not really an error */ + return ret; } - for (i = 0, n_u64 = 0; i < event->n_fields; i++) { - if (event->fields[i]->is_string) { + for (i = 0, n_u64 = 0; i < state.event->n_fields; i++) { + if (state.event->fields[i]->is_string) { char *str_val = (char *)(long)vals[i]; - char *str_field = (char *)&entry->fields[n_u64]; + char *str_field = (char *)&state.entry->fields[n_u64]; strscpy(str_field, str_val, STR_VAR_LEN_MAX); n_u64 += STR_VAR_LEN_MAX / sizeof(u64); } else { - entry->fields[n_u64] = vals[i]; + state.entry->fields[n_u64] = vals[i]; n_u64++; } } - trace_event_buffer_commit(&fbuffer); -out: - ring_buffer_nest_end(buffer); + __synth_event_trace_end(&state); return ret; } @@ -1993,55 +1985,17 @@ EXPORT_SYMBOL_GPL(synth_event_trace_array); int synth_event_trace_start(struct trace_event_file *file, struct synth_event_trace_state *trace_state) { - struct synth_trace_event *entry; - int fields_size = 0; - int ret = 0; + int ret; - if (!trace_state) { - ret = -EINVAL; - goto out; - } + if (!trace_state) + return -EINVAL; memset(trace_state, '\0', sizeof(*trace_state)); - /* - * Normal event tracing doesn't get called at all unless the - * ENABLED bit is set (which attaches the probe thus allowing - * this code to be called, etc). Because this is called - * directly by the user, we don't have that but we still need - * to honor not logging when disabled. For the the iterated - * trace case, we save the enabed state upon start and just - * ignore the following data calls. - */ - if (!(file->flags & EVENT_FILE_FL_ENABLED) || - trace_trigger_soft_disabled(file)) { - trace_state->enabled = false; - goto out; - } - - trace_state->enabled = true; + ret = __synth_event_trace_start(file, trace_state); + if (ret == -ENOENT) + ret = 0; /* just disabled, not really an error */ - trace_state->event = file->event_call->data; - - fields_size = trace_state->event->n_u64 * sizeof(u64); - - /* - * Avoid ring buffer recursion detection, as this event - * is being performed within another event. - */ - trace_state->buffer = file->tr->array_buffer.buffer; - ring_buffer_nest_start(trace_state->buffer); - - entry = trace_event_buffer_reserve(&trace_state->fbuffer, file, - sizeof(*entry) + fields_size); - if (!entry) { - ring_buffer_nest_end(trace_state->buffer); - ret = -EINVAL; - goto out; - } - - trace_state->entry = entry; -out: return ret; } EXPORT_SYMBOL_GPL(synth_event_trace_start); @@ -2074,7 +2028,7 @@ static int __synth_event_add_val(const char *field_name, u64 val, trace_state->add_next = true; } - if (!trace_state->enabled) + if (trace_state->disabled) goto out; event = trace_state->event; @@ -2209,9 +2163,7 @@ int synth_event_trace_end(struct synth_event_trace_state *trace_state) if (!trace_state) return -EINVAL; - trace_event_buffer_commit(&trace_state->fbuffer); - - ring_buffer_nest_end(trace_state->buffer); + __synth_event_trace_end(trace_state); return 0; } -- 2.14.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] tracing: Consolidate trace() functions 2020-02-10 23:06 ` [PATCH 3/3] tracing: Consolidate trace() functions Tom Zanussi @ 2020-02-12 3:36 ` Masami Hiramatsu 0 siblings, 0 replies; 9+ messages in thread From: Masami Hiramatsu @ 2020-02-12 3:36 UTC (permalink / raw) To: Tom Zanussi Cc: rostedt, artem.bityutskiy, mhiramat, linux-kernel, linux-rt-users Hi Tom, On Mon, 10 Feb 2020 17:06:50 -0600 Tom Zanussi <zanussi@kernel.org> wrote: > Move the checking, buffer reserve and buffer commit code in > synth_event_trace_start/end() into inline functions > __synth_event_trace_start/end() so they can also be used by > synth_event_trace() and synth_event_trace_array(), and then have all > those functions use them. > > Also, change synth_event_trace_state.enabled to disabled so it only > needs to be set if the event is disabled, which is not normally the > case. I have some comments :) > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > index 99a02168599b..65b54d6a1422 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -1791,6 +1791,60 @@ void synth_event_cmd_init(struct dynevent_cmd *cmd, char *buf, int maxlen) > } > EXPORT_SYMBOL_GPL(synth_event_cmd_init); > > +static inline int > +__synth_event_trace_start(struct trace_event_file *file, > + struct synth_event_trace_state *trace_state) > +{ > + int entry_size, fields_size = 0; > + int ret = 0; > + > + /* > + * Normal event tracing doesn't get called at all unless the > + * ENABLED bit is set (which attaches the probe thus allowing > + * this code to be called, etc). Because this is called > + * directly by the user, we don't have that but we still need > + * to honor not logging when disabled. For the the iterated > + * trace case, we save the enabed state upon start and just > + * ignore the following data calls. > + */ > + if (!(file->flags & EVENT_FILE_FL_ENABLED) || > + trace_trigger_soft_disabled(file)) { > + trace_state->disabled = true; > + ret = -ENOENT; If the state.disabled is set, I think __synth_event_trace_start() might be better to return 0 because all caller change the result. > + goto out; > + } > + > + trace_state->event = file->event_call->data; > + > + fields_size = trace_state->event->n_u64 * sizeof(u64); > + > + /* > + * Avoid ring buffer recursion detection, as this event > + * is being performed within another event. > + */ > + trace_state->buffer = file->tr->array_buffer.buffer; > + ring_buffer_nest_start(trace_state->buffer); > + > + entry_size = sizeof(*trace_state->entry) + fields_size; > + trace_state->entry = trace_event_buffer_reserve(&trace_state->fbuffer, > + file, > + entry_size); > + if (!trace_state->entry) { > + ring_buffer_nest_end(trace_state->buffer); > + ret = -EINVAL; > + } > +out: > + return ret; > +} > + > +static inline void > +__synth_event_trace_end(struct synth_event_trace_state *trace_state) > +{ Should we check trace_state->disabled here too? > + trace_event_buffer_commit(&trace_state->fbuffer); > + > + ring_buffer_nest_end(trace_state->buffer); > +} > + > /** > * synth_event_trace - Trace a synthetic event > * @file: The trace_event_file representing the synthetic event > @@ -1812,69 +1866,38 @@ EXPORT_SYMBOL_GPL(synth_event_cmd_init); > */ > int synth_event_trace(struct trace_event_file *file, unsigned int n_vals, ...) > { > - struct trace_event_buffer fbuffer; > - struct synth_trace_event *entry; > - struct trace_buffer *buffer; > - struct synth_event *event; > + struct synth_event_trace_state state; > unsigned int i, n_u64; > - int fields_size = 0; > va_list args; > - int ret = 0; > - > - /* > - * Normal event generation doesn't get called at all unless > - * the ENABLED bit is set (which attaches the probe thus > - * allowing this code to be called, etc). Because this is > - * called directly by the user, we don't have that but we > - * still need to honor not logging when disabled. > - */ > - if (!(file->flags & EVENT_FILE_FL_ENABLED) || > - trace_trigger_soft_disabled(file)) > - return 0; > - > - event = file->event_call->data; > - > - if (n_vals != event->n_fields) > - return -EINVAL; > - > - fields_size = event->n_u64 * sizeof(u64); > - > - /* > - * Avoid ring buffer recursion detection, as this event > - * is being performed within another event. > - */ > - buffer = file->tr->array_buffer.buffer; > - ring_buffer_nest_start(buffer); > + int ret; > > - entry = trace_event_buffer_reserve(&fbuffer, file, > - sizeof(*entry) + fields_size); > - if (!entry) { > - ret = -EINVAL; > - goto out; > + ret = __synth_event_trace_start(file, &state); > + if (ret) { And here, we can check "if (ret || state.disabled)" then > + if (ret == -ENOENT) > + ret = 0; /* just disabled, not really an error */ We don't need this check. > + return ret; > } > > va_start(args, n_vals); > - for (i = 0, n_u64 = 0; i < event->n_fields; i++) { > + for (i = 0, n_u64 = 0; i < state.event->n_fields; i++) { > u64 val; > > val = va_arg(args, u64); > > - if (event->fields[i]->is_string) { > + if (state.event->fields[i]->is_string) { > char *str_val = (char *)(long)val; > - char *str_field = (char *)&entry->fields[n_u64]; > + char *str_field = (char *)&state.entry->fields[n_u64]; > > strscpy(str_field, str_val, STR_VAR_LEN_MAX); > n_u64 += STR_VAR_LEN_MAX / sizeof(u64); > } else { > - entry->fields[n_u64] = val; > + state.entry->fields[n_u64] = val; > n_u64++; > } > } > va_end(args); > > - trace_event_buffer_commit(&fbuffer); > -out: > - ring_buffer_nest_end(buffer); > + __synth_event_trace_end(&state); > > return ret; > } > @@ -1901,62 +1924,31 @@ EXPORT_SYMBOL_GPL(synth_event_trace); > int synth_event_trace_array(struct trace_event_file *file, u64 *vals, > unsigned int n_vals) > { > - struct trace_event_buffer fbuffer; > - struct synth_trace_event *entry; > - struct trace_buffer *buffer; > - struct synth_event *event; > + struct synth_event_trace_state state; > unsigned int i, n_u64; > - int fields_size = 0; > - int ret = 0; > - > - /* > - * Normal event generation doesn't get called at all unless > - * the ENABLED bit is set (which attaches the probe thus > - * allowing this code to be called, etc). Because this is > - * called directly by the user, we don't have that but we > - * still need to honor not logging when disabled. > - */ > - if (!(file->flags & EVENT_FILE_FL_ENABLED) || > - trace_trigger_soft_disabled(file)) > - return 0; > - > - event = file->event_call->data; > - > - if (n_vals != event->n_fields) > - return -EINVAL; > - > - fields_size = event->n_u64 * sizeof(u64); > - > - /* > - * Avoid ring buffer recursion detection, as this event > - * is being performed within another event. > - */ > - buffer = file->tr->array_buffer.buffer; > - ring_buffer_nest_start(buffer); > + int ret; > > - entry = trace_event_buffer_reserve(&fbuffer, file, > - sizeof(*entry) + fields_size); > - if (!entry) { > - ret = -EINVAL; > - goto out; > + ret = __synth_event_trace_start(file, &state); > + if (ret) { > + if (ret == -ENOENT) > + ret = 0; /* just disabled, not really an error */ Ditto. > + return ret; > } > > - for (i = 0, n_u64 = 0; i < event->n_fields; i++) { > - if (event->fields[i]->is_string) { > + for (i = 0, n_u64 = 0; i < state.event->n_fields; i++) { > + if (state.event->fields[i]->is_string) { > char *str_val = (char *)(long)vals[i]; > - char *str_field = (char *)&entry->fields[n_u64]; > + char *str_field = (char *)&state.entry->fields[n_u64]; > > strscpy(str_field, str_val, STR_VAR_LEN_MAX); > n_u64 += STR_VAR_LEN_MAX / sizeof(u64); > } else { > - entry->fields[n_u64] = vals[i]; > + state.entry->fields[n_u64] = vals[i]; > n_u64++; > } > } > > - trace_event_buffer_commit(&fbuffer); > -out: > - ring_buffer_nest_end(buffer); > + __synth_event_trace_end(&state); > > return ret; > } > @@ -1993,55 +1985,17 @@ EXPORT_SYMBOL_GPL(synth_event_trace_array); > int synth_event_trace_start(struct trace_event_file *file, > struct synth_event_trace_state *trace_state) > { > - struct synth_trace_event *entry; > - int fields_size = 0; > - int ret = 0; > + int ret; > > - if (!trace_state) { > - ret = -EINVAL; > - goto out; > - } > + if (!trace_state) > + return -EINVAL; > > memset(trace_state, '\0', sizeof(*trace_state)); > > - /* > - * Normal event tracing doesn't get called at all unless the > - * ENABLED bit is set (which attaches the probe thus allowing > - * this code to be called, etc). Because this is called > - * directly by the user, we don't have that but we still need > - * to honor not logging when disabled. For the the iterated > - * trace case, we save the enabed state upon start and just > - * ignore the following data calls. > - */ > - if (!(file->flags & EVENT_FILE_FL_ENABLED) || > - trace_trigger_soft_disabled(file)) { > - trace_state->enabled = false; > - goto out; > - } > - > - trace_state->enabled = true; > + ret = __synth_event_trace_start(file, trace_state); > + if (ret == -ENOENT) > + ret = 0; /* just disabled, not really an error */ We can also skip this tweak. Thank you, > > - trace_state->event = file->event_call->data; > - > - fields_size = trace_state->event->n_u64 * sizeof(u64); > - > - /* > - * Avoid ring buffer recursion detection, as this event > - * is being performed within another event. > - */ > - trace_state->buffer = file->tr->array_buffer.buffer; > - ring_buffer_nest_start(trace_state->buffer); > - > - entry = trace_event_buffer_reserve(&trace_state->fbuffer, file, > - sizeof(*entry) + fields_size); > - if (!entry) { > - ring_buffer_nest_end(trace_state->buffer); > - ret = -EINVAL; > - goto out; > - } > - > - trace_state->entry = entry; > -out: > return ret; > } > EXPORT_SYMBOL_GPL(synth_event_trace_start); > @@ -2074,7 +2028,7 @@ static int __synth_event_add_val(const char *field_name, u64 val, > trace_state->add_next = true; > } > > - if (!trace_state->enabled) > + if (trace_state->disabled) > goto out; > > event = trace_state->event; > @@ -2209,9 +2163,7 @@ int synth_event_trace_end(struct synth_event_trace_state *trace_state) > if (!trace_state) > return -EINVAL; > > - trace_event_buffer_commit(&trace_state->fbuffer); > - > - ring_buffer_nest_end(trace_state->buffer); > + __synth_event_trace_end(trace_state); > > return 0; > } > -- > 2.14.1 > -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-02-12 5:24 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-10 23:06 [PATCH 0/3] tracing: Synthetic event fixes and updates Tom Zanussi 2020-02-10 23:06 ` [PATCH 1/3] tracing: Add missing nest end to synth_event_trace_start() error case Tom Zanussi 2020-02-12 3:24 ` Masami Hiramatsu 2020-02-10 23:06 ` [PATCH 2/3] tracing: Don't return -EINVAL when tracing soft disabled synth events Tom Zanussi 2020-02-12 3:24 ` Masami Hiramatsu 2020-02-12 3:28 ` Steven Rostedt 2020-02-12 5:24 ` Masami Hiramatsu 2020-02-10 23:06 ` [PATCH 3/3] tracing: Consolidate trace() functions Tom Zanussi 2020-02-12 3:36 ` 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).