* [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
* [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
* [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 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
* 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 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
* 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
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).