tracing: Skip software disabled event at __synth_event_trace_end()

PATCH In review Masami Hiramatsu 3 participants 6 messages Feb 12, 2020 – Feb 20, 2020 Tracing

Share & tools

Masami Hiramatsu

author maintainer

2020-02-12 05:54 UTC #0 lore raw

PATCH 1/1 tracing: Skip software disabled event at __synth_event_trace_end()

When the synthetic event is software disabled, __synth_event_trace_start() does not allocate an event buffer. In this case __synth_event_trace_end() also should not commit the buffer.

Check the trace_state->disabled at __synth_event_trace_end() and if it is disabled, skip it.

Signed-off-by Masami Hiramatsu <[email protected]>

---
 kernel/trace/trace_events_hist.c |    3 +++
 1 file changed, 3 insertions(+)

kernel/trace/trace_events_hist.c +3 -0

index 483b3fd1094f..781e4b55e117 100644
@@ -1847,6 +1847,9 @@ __synth_event_trace_start(struct trace_event_file *file,
 static inline void
 __synth_event_trace_end(struct synth_event_trace_state *trace_state)
 {
+	if (trace_state->disabled)
+		return;
+
 	trace_event_buffer_commit(&trace_state->fbuffer);
 
 	ring_buffer_nest_end(trace_state->buffer);

Tom Zanussi

Reviewed-by on v1 1/1

2020-02-12 16:03 UTC #1 lore raw

Hi Masami,

You're right, this is a bug, thanks for sending the patch to fix it.

Tom

Reviewed-by Tom Zanussi <[email protected]>

Quoted 30 lines · Masami Hiramatsu

When the synthetic event is software disabled, __synth_event_trace_start() does not allocate an event buffer. In this case __synth_event_trace_end() also should not commit the buffer.
Check the trace_state->disabled at __synth_event_trace_end() and if it is disabled, skip it.
Signed-off-by Masami Hiramatsu <[email protected]>
---
 kernel/trace/trace_events_hist.c |    3 +++
 1 file changed, 3 insertions(+)
kernel/trace/trace_events_hist.c +3 -0
index 483b3fd1094f..781e4b55e117 100644
@@ -1847,6 +1847,9 @@ __synth_event_trace_start(struct
 trace_event_file *file,
 static inline void
 __synth_event_trace_end(struct synth_event_trace_state *trace_state)
 {
+	if (trace_state->disabled)
+		return;
+
 	trace_event_buffer_commit(&trace_state->fbuffer);
 
ring_buffer_nest_end(trace_state->buffer);

Masami Hiramatsu

author maintainer

2020-02-17 09:33 UTC #2 lore raw

In reply to Masami Hiramatsu

Quoted 18 lines · On Wed, 12 Feb 2020 14:54:19 +0900 Masami Hiramatsu <[email protected]> wrote:

When the synthetic event is software disabled, __synth_event_trace_start() does not allocate an event buffer. In this case __synth_event_trace_end() also should not commit the buffer.
Check the trace_state->disabled at __synth_event_trace_end() and if it is disabled, skip it.
Signed-off-by Masami Hiramatsu <[email protected]>
---
 kernel/trace/trace_events_hist.c |    3 +++
 1 file changed, 3 insertions(+)
kernel/trace/trace_events_hist.c +0 -0
index 483b3fd1094f..781e4b55e117 100644

kernel/trace/trace_events_hist.c __synth_event_trace_start(struct trace_event_file *file, · line 1852

Quoted 6 lines

kernel/trace/trace_events_hist.c +3 -0

@@ -1847,6 +1847,9 @@ __synth_event_trace_start(struct trace_event_file *file,
 static inline void
 __synth_event_trace_end(struct synth_event_trace_state *trace_state)
 {
+	if (trace_state->disabled)
+		return;
+

Aah, I assumed that trace_state should be initialized with 0, but in really, it could be just allocated on the stack.
We has to set trace_state->disabled = false in __synth_event_trace_start().

Thank you,

Quoted 4 lines

kernel/trace/trace_events_hist.c +0 -0

 	trace_event_buffer_commit(&trace_state->fbuffer);
 
 	ring_buffer_nest_end(trace_state->buffer);

Signature · 1 line

Masami Hiramatsu <[email protected]>

Steven Rostedt

maintainer

2020-02-20 19:34 UTC #3 lore raw

Quoted 10 lines · On Mon, 17 Feb 2020 18:33:40 +0900 Masami Hiramatsu <[email protected]> wrote:

kernel/trace/trace_events_hist.c __synth_event_trace_start(struct trace_event_file *file, · line 1852
Quoted 6 lines
static inline void
__synth_event_trace_end(struct synth_event_trace_state *trace_state) {
kernel/trace/trace_events_hist.c +3 -0
+	if (trace_state->disabled)
+		return;
+  
Aah, I assumed that trace_state should be initialized with 0, but in really, it could be just allocated on the stack.
We has to set trace_state->disabled = false in __synth_event_trace_start().

Is this patch good enough to take, or is there another one coming?

-- Steve

Tom Zanussi

2020-02-20 19:51 UTC #4 lore raw

Hi Steve,

Quoted 16 lines · Steven Rostedt

Quoted 4 lines · On Mon, 17 Feb 2020 18:33:40 +0900 Masami Hiramatsu <[email protected]> wrote:
Quoted 4 lines
static inline void __synth_event_trace_end(struct synth_event_trace_state
*trace_state)
{
Quoted 8 lines
kernel/trace/trace_events_hist.c __synth_event_trace_start(struct trace_event_file *file, · line 1852
Quoted 3 lines
kernel/trace/trace_events_hist.c +3 -0
+	if (trace_state->disabled)
+		return;
+  
Aah, I assumed that trace_state should be initialized with 0, but in really, it could be just allocated on the stack.
We has to set trace_state->disabled = false in __synth_event_trace_start().

Quoted 2 lines

Is this patch good enough to take, or is there another one coming?

I think this patch is good to take. The fix for setting trace_state-

Quoted 1 line

disabled to false would be covered by this later patch:

[PATCH 2/2] tracing: Clear trace_state when starting trace

https://lore.kernel.org/lkml/158193315899.8868.1781259176894639952.stgit@devnote2/

Tom

Quoted 1 line

-- Steve

Steven Rostedt

maintainer

2020-02-20 19:54 UTC #5 lore raw

Quoted 22 lines · On Thu, 20 Feb 2020 13:51:48 -0600 Tom Zanussi <[email protected]> wrote:

Hi Steve,
Quoted 16 lines · Steven Rostedt
Quoted 4 lines · On Mon, 17 Feb 2020 18:33:40 +0900 Masami Hiramatsu <[email protected]> wrote:
Quoted 4 lines
static inline void __synth_event_trace_end(struct synth_event_trace_state
*trace_state)
{
Quoted 8 lines
kernel/trace/trace_events_hist.c __synth_event_trace_start(struct trace_event_file *file, · line 1852
Quoted 3 lines
kernel/trace/trace_events_hist.c +3 -0
+	if (trace_state->disabled)
+		return;
+    
Aah, I assumed that trace_state should be initialized with 0, but in really, it could be just allocated on the stack.
We has to set trace_state->disabled = false in __synth_event_trace_start().
Quoted 2 lines
Is this patch good enough to take, or is there another one coming?

Quoted 7 lines

I think this patch is good to take. The fix for setting trace_state-
Quoted 1 line
disabled to false would be covered by this later patch:
[PATCH 2/2] tracing: Clear trace_state when starting trace
https://lore.kernel.org/lkml/158193315899.8868.1781259176894639952.stgit@devnote2/

Thanks Tom for the info!

-- Steve