[PATCH 0/2] tracing: Fix synthetic event generation API and test

[PATCH 0/2] tracing: Fix synthetic event generation API and test

[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,

---

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) <mhiramat@kernel.org>

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

[Masami Hiramatsu]

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 <mhiramat@kernel.org>
---
 kernel/trace/synth_event_gen_test.c |   23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/synth_event_gen_test.c b/kernel/trace/synth_event_gen_test.c
index 4aefe003cb7c..b7775fd6baf5 100644
--- a/kernel/trace/synth_event_gen_test.c
+++ b/kernel/trace/synth_event_gen_test.c
@@ -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;
 }
 

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

[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 <mhiramat@kernel.org>
---
 kernel/trace/trace_events_hist.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 7f0e5cdf17ae..9ec0a7551d62 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -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 */

Re: [PATCH 0/2] tracing: Fix synthetic event generation API and test

[Tom Zanussi]

Hi Masami,

On Mon, 2020-02-17 at 18:52 +0900, Masami Hiramatsu wrote:
> 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 <zanussi@kernel.org>


Tom

> 
> ---
> 
> 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) <mhiramat@kernel.org>

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

[Steven Rostedt]

On Mon, 17 Feb 2020 18:52:29 +0900
Masami Hiramatsu <mhiramat@kernel.org> 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 <mhiramat@kernel.org>
> ---
>  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

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

[Tom Zanussi]

Hi Steve,

On Thu, 2020-02-20 at 17:48 -0500, Steven Rostedt wrote:
> On Mon, 17 Feb 2020 18:52:29 +0900
> Masami Hiramatsu <mhiramat@kernel.org> 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 <mhiramat@kernel.org>
> > ---
> >  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.
> 

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 

> -- Steve

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

[Masami Hiramatsu]

On Thu, 20 Feb 2020 17:48:01 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 17 Feb 2020 18:52:29 +0900
> Masami Hiramatsu <mhiramat@kernel.org> 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 <mhiramat@kernel.org>
> > ---
> >  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.

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,

-- 
Masami Hiramatsu <mhiramat@kernel.org>