tracing: Fix synthetic event generation API and test

PATCH In review Masami Hiramatsu 3 participants 7 messages Feb 17, 2020 – Feb 21, 2020 Tracing

lore

Share & tools

lore

Masami Hiramatsu

author maintainer

2020-02-17 09:52 UTC #0 lore raw

cover tracing: Fix synthetic event generation API and test

Hi,

Here is a couple of patches to fix 2 issues LKP and I found on synthetic event generation test.

[1/2] is for fixing warnings on smp_processor_id() without disabling preemption, and [2/2] is for fixing a bug on the API itself which LKP reported.

Thank you,

---

Masami Hiramatsu (2):
tracing: Fix synth event test to avoid using smp_processor_id()
tracing: Clear trace_state when starting trace

 kernel/trace/synth_event_gen_test.c |   23 +++++++++++++++++------
 kernel/trace/trace_events_hist.c    |    4 ++--
 2 files changed, 19 insertions(+), 8 deletions(-)

--
Masami Hiramatsu (Linaro) <[email protected]>

Masami Hiramatsu

author maintainer

2020-02-17 09:52 UTC #1 lore raw

PATCH 1/2 tracing: Fix synth event test to avoid using smp_processor_id()

Since smp_processor_id() requires irq-disabled or preempt-disabled, synth event generation test module made some warnings. To prevent that, use get_cpu()/put_cpu() instead of smp_processor_id().

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

---
 kernel/trace/synth_event_gen_test.c |   23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

kernel/trace/synth_event_gen_test.c +17 -6

index 4aefe003cb7c..b7775fd6baf5 100644
@@ -114,12 +114,13 @@ static int __init test_gen_synth_cmd(void)
 	vals[1] = (u64)"hula hoops";	/* next_comm_field */
 	vals[2] = 1000000;		/* ts_ns */
 	vals[3] = 1000;			/* ts_ms */
-	vals[4] = smp_processor_id();	/* cpu */
+	vals[4] = get_cpu();	/* cpu */
 	vals[5] = (u64)"thneed";	/* my_string_field */
 	vals[6] = 598;			/* my_int_field */
 
 	/* Now generate a gen_synth_test event */
 	ret = synth_event_trace_array(gen_synth_test, vals, ARRAY_SIZE(vals));
+	put_cpu();
  out:
 	return ret;
  delete:

@@ -221,12 +222,13 @@ static int __init test_empty_synth_event(void)
 	vals[1] = (u64)"tiddlywinks";	/* next_comm_field */
 	vals[2] = 1000000;		/* ts_ns */
 	vals[3] = 1000;			/* ts_ms */
-	vals[4] = smp_processor_id();	/* cpu */
+	vals[4] = get_cpu();		/* cpu */
 	vals[5] = (u64)"thneed_2.0";	/* my_string_field */
 	vals[6] = 399;			/* my_int_field */
 
 	/* Now trace an empty_synth_test event */
 	ret = synth_event_trace_array(empty_synth_test, vals, ARRAY_SIZE(vals));
+	put_cpu();
  out:
 	return ret;
  delete:

@@ -293,12 +295,13 @@ static int __init test_create_synth_event(void)
 	vals[1] = (u64)"tiddlywinks";	/* next_comm_field */
 	vals[2] = 1000000;		/* ts_ns */
 	vals[3] = 1000;			/* ts_ms */
-	vals[4] = smp_processor_id();	/* cpu */
+	vals[4] = get_cpu();		/* cpu */
 	vals[5] = (u64)"thneed";	/* my_string_field */
 	vals[6] = 398;			/* my_int_field */
 
 	/* Now generate a create_synth_test event */
 	ret = synth_event_trace_array(create_synth_test, vals, ARRAY_SIZE(vals));
+	put_cpu();
  out:
 	return ret;
  delete:

@@ -315,6 +318,7 @@ static int __init test_create_synth_event(void)
 static int __init test_add_next_synth_val(void)
 {
 	struct synth_event_trace_state trace_state;
+	unsigned int cpu;
 	int ret;
 
 	/* Start by reserving space in the trace buffer */

@@ -322,6 +326,8 @@ static int __init test_add_next_synth_val(void)
 	if (ret)
 		return ret;
 
+	cpu = get_cpu();
+
 	/* Write some bogus values into the trace buffer, one after another */
 
 	/* next_pid_field */

@@ -345,7 +351,7 @@ static int __init test_add_next_synth_val(void)
 		goto out;
 
 	/* cpu */
-	ret = synth_event_add_next_val(smp_processor_id(), &trace_state);
+	ret = synth_event_add_next_val(cpu, &trace_state);
 	if (ret)
 		goto out;

@@ -357,6 +363,7 @@ static int __init test_add_next_synth_val(void)
 	/* my_int_field */
 	ret = synth_event_add_next_val(395, &trace_state);
  out:
+	put_cpu();
 	/* Finally, commit the event */
 	ret = synth_event_trace_end(&trace_state);

@@ -371,6 +378,7 @@ static int __init test_add_next_synth_val(void)
 static int __init test_add_synth_val(void)
 {
 	struct synth_event_trace_state trace_state;
+	unsigned int cpu;
 	int ret;
 
 	/* Start by reserving space in the trace buffer */

@@ -378,6 +386,7 @@ static int __init test_add_synth_val(void)
 	if (ret)
 		return ret;
 
+	cpu = get_cpu();
 	/* Write some bogus values into the trace buffer, using field names */
 
 	ret = synth_event_add_val("ts_ns", 1000000, &trace_state);

@@ -388,7 +397,7 @@ static int __init test_add_synth_val(void)
 	if (ret)
 		goto out;
 
-	ret = synth_event_add_val("cpu", smp_processor_id(), &trace_state);
+	ret = synth_event_add_val("cpu", cpu, &trace_state);
 	if (ret)
 		goto out;

@@ -408,6 +417,7 @@ static int __init test_add_synth_val(void)
 
 	ret = synth_event_add_val("my_int_field", 3999, &trace_state);
  out:
+	put_cpu();
 	/* Finally, commit the event */
 	ret = synth_event_trace_end(&trace_state);

@@ -427,9 +437,10 @@ static int __init test_trace_synth_event(void)
 				(u64)"clackers",	/* next_comm_field */
 				1000000,		/* ts_ns */
 				1000,			/* ts_ms */
-				smp_processor_id(),	/* cpu */
+				get_cpu(),		/* cpu */
 				(u64)"Thneed",		/* my_string_field */
 				999);			/* my_int_field */
+	put_cpu();
 	return ret;
 }

Steven Rostedt

maintainer

2020-02-20 22:48 UTC #2 lore raw

Re: [PATCH 1/2] tracing: Fix synth event test to avoid using smp_processor_id()

Quoted 9 lines · On Mon, 17 Feb 2020 18:52:29 +0900 Masami Hiramatsu <[email protected]> wrote:

Since smp_processor_id() requires irq-disabled or preempt-disabled, synth event generation test module made some warnings. To prevent that, use get_cpu()/put_cpu() instead of smp_processor_id().
Signed-off-by Masami Hiramatsu <[email protected]>
---
 kernel/trace/synth_event_gen_test.c |   23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

I just noticed this patch, after applying my version that just uses the raw_smp_processor_id(). We don't really care what CPU it is do we?

I didn't want a test to muck with preemption disabling and all that fun.

-- Steve

Tom Zanussi

2020-02-20 22:56 UTC #3 lore raw

Re: [PATCH 1/2] tracing: Fix synth event test to avoid using smp_processor_id()

Hi Steve,

Quoted 13 lines · Steven Rostedt

Quoted 9 lines · On Mon, 17 Feb 2020 18:52:29 +0900 Masami Hiramatsu <[email protected]> wrote:
Since smp_processor_id() requires irq-disabled or preempt-disabled, synth event generation test module made some warnings. To prevent that, use get_cpu()/put_cpu() instead of smp_processor_id().
Signed-off-by Masami Hiramatsu <[email protected]>
---
 kernel/trace/synth_event_gen_test.c |   23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

Quoted 7 lines

I just noticed this patch, after applying my version that just uses the
raw_smp_processor_id(). We don't really care what CPU it is do we?
I didn't want a test to muck with preemption disabling and all that fun.

Right, we don't really care, it's just to test the trace API - for this it could even be a constant. I just happened to pick that as an example, and wasn't expecting unrelated complications.

Tom

Quoted 1 line

-- Steve

Masami Hiramatsu

author maintainer

2020-02-21 10:24 UTC #4 lore raw

Re: [PATCH 1/2] tracing: Fix synth event test to avoid using smp_processor_id()

In reply to Steven Rostedt

Quoted 13 lines · On Thu, 20 Feb 2020 17:48:01 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 9 lines · On Mon, 17 Feb 2020 18:52:29 +0900 Masami Hiramatsu <[email protected]> wrote:
Since smp_processor_id() requires irq-disabled or preempt-disabled, synth event generation test module made some warnings. To prevent that, use get_cpu()/put_cpu() instead of smp_processor_id().
Signed-off-by Masami Hiramatsu <[email protected]>
---
 kernel/trace/synth_event_gen_test.c |   23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

Quoted 4 lines

I just noticed this patch, after applying my version that just uses the raw_smp_processor_id(). We don't really care what CPU it is do we?
I didn't want a test to muck with preemption disabling and all that fun.

OK, I confirmed that the ring_buffer_nest_start() ensures the preempt disabled. So just using raw_smp_processor_id() is good to me.

Thank you,

Signature · 1 line

Masami Hiramatsu <[email protected]>

Masami Hiramatsu

author maintainer

2020-02-17 09:52 UTC #5 lore raw

PATCH 2/2 tracing: Clear trace_state when starting trace

In reply to Masami Hiramatsu

Clear trace_state data structure when starting trace in __synth_event_trace_start() internal function.

Currently trace_state is initialized only in the synth_event_trace_start() API, but the trace_state in synth_event_trace() and synth_event_trace_array() are on the stack without initialization.
This means those APIs will see wrong parameters and wil skip closing process in __synth_event_trace_end() because trace_state->disabled may be !0.

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

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

kernel/trace/trace_events_hist.c +2 -2

index 7f0e5cdf17ae..9ec0a7551d62 100644
@@ -1798,6 +1798,8 @@ __synth_event_trace_start(struct trace_event_file *file,
 	int entry_size, fields_size = 0;
 	int ret = 0;
 
+	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

@@ -1993,8 +1995,6 @@ int synth_event_trace_start(struct trace_event_file *file,
 	if (!trace_state)
 		return -EINVAL;
 
-	memset(trace_state, '\0', sizeof(*trace_state));
-
 	ret = __synth_event_trace_start(file, trace_state);
 	if (ret == -ENOENT)
 		ret = 0; /* just disabled, not really an error */

Tom Zanussi

Reviewed-by on the v1 series

2020-02-18 15:18 UTC #6 lore raw

In reply to Masami Hiramatsu

Hi Masami,

Quoted 10 lines · Masami Hiramatsu

Hi,
Here is a couple of patches to fix 2 issues LKP and I found on synthetic event generation test.
[1/2] is for fixing warnings on smp_processor_id() without disabling preemption, and [2/2] is for fixing a bug on the API itself which LKP reported.
Thank you,

Thank you for fixing these.

For both,

Reviewed-by Tom Zanussi <[email protected]>

Tom

Quoted 14 lines

---
Masami Hiramatsu (2):
tracing: Fix synth event test to avoid using smp_processor_id()
tracing: Clear trace_state when starting trace
 kernel/trace/synth_event_gen_test.c |   23 +++++++++++++++++------
 kernel/trace/trace_events_hist.c    |    4 ++--
 2 files changed, 19 insertions(+), 8 deletions(-)
--
Masami Hiramatsu (Linaro) <[email protected]>