[PATCH 0/3] tracing: Synthetic event fixes and updates

[PATCH 0/3] tracing: Synthetic event fixes and updates

[Tom Zanussi]

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

[PATCH 1/3] tracing: Add missing nest end to synth_event_trace_start() error case

[Tom Zanussi]

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

[PATCH 2/3] tracing: Don't return -EINVAL when tracing soft disabled synth events

[Tom Zanussi]

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

[PATCH 3/3] tracing: Consolidate trace() functions

[Tom Zanussi]

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

Re: [PATCH 1/3] tracing: Add missing nest end to synth_event_trace_start() error case

[Masami Hiramatsu]

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>

Re: [PATCH 2/3] tracing: Don't return -EINVAL when tracing soft disabled synth events

[Masami Hiramatsu]

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>

Re: [PATCH 2/3] tracing: Don't return -EINVAL when tracing soft disabled synth events

[Steven Rostedt]

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

Re: [PATCH 3/3] tracing: Consolidate trace() functions

[Masami Hiramatsu]

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>

Re: [PATCH 2/3] tracing: Don't return -EINVAL when tracing soft disabled synth events

[Masami Hiramatsu]

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>