[PATCH 1/2] tracing/sysprof: add missing tracing_{start,stop}_record_cmdline()

[PATCH 1/2] tracing/sysprof: add missing tracing_{start,stop}_record_cmdline()

[Frederic Weisbecker]

Add the missing pair tracing_{start,stop}_record_cmdline() to record well
the cmdline associated with pid.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/trace/trace_sysprof.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/kernel/trace/trace_sysprof.c b/kernel/trace/trace_sysprof.c
index 84ca9d8..9902c15 100644
--- a/kernel/trace/trace_sysprof.c
+++ b/kernel/trace/trace_sysprof.c
@@ -238,6 +238,8 @@ static int stack_trace_init(struct trace_array *tr)
 {
 	sysprof_trace = tr;
 
+	tracing_start_cmdline_record();
+
 	mutex_lock(&sample_timer_lock);
 	start_stack_timers();
 	tracer_enabled = 1;
@@ -247,6 +249,7 @@ static int stack_trace_init(struct trace_array *tr)
 
 static void stack_trace_reset(struct trace_array *tr)
 {
+	tracing_stop_cmdline_record();
 	stop_stack_trace(tr);
 }
 
-- 
1.6.1

Re: [PATCH 1/2] tracing/sysprof: add missing tracing_{start,stop}_record_cmdline()

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Add the missing pair tracing_{start,stop}_record_cmdline() to record well
> the cmdline associated with pid.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> ---
>  kernel/trace/trace_sysprof.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)

applied to tip/tracing/sysprof, thanks Frederic!

	Ingo

Re: [PATCH 1/2] tracing/sysprof: add missing tracing_{start,stop}_record_cmdline()

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > Add the missing pair tracing_{start,stop}_record_cmdline() to record well
> > the cmdline associated with pid.
> > 
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > ---
> >  kernel/trace/trace_sysprof.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> applied to tip/tracing/sysprof, thanks Frederic!

hm, causes a build failure:

kernel/built-in.o: In function `stack_trace_reset':
trace_sysprof.c:(.text+0x3e91a): undefined reference to `tracing_stop_cmdline_record'
kernel/built-in.o: In function `stack_trace_init':
trace_sysprof.c:(.text+0x3e953): undefined reference to `tracing_start_cmdline_record'

	Ingo

Re: [PATCH 1/2] tracing/sysprof: add missing tracing_{start,stop}_record_cmdline()

[Frederic Weisbecker]

On Tue, Feb 10, 2009 at 01:31:45PM +0100, Ingo Molnar wrote:
> 
> * Ingo Molnar <mingo@elte.hu> wrote:
> 
> > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > 
> > > Add the missing pair tracing_{start,stop}_record_cmdline() to record well
> > > the cmdline associated with pid.
> > > 
> > > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > > ---
> > >  kernel/trace/trace_sysprof.c |    3 +++
> > >  1 files changed, 3 insertions(+), 0 deletions(-)
> > 
> > applied to tip/tracing/sysprof, thanks Frederic!
> 
> hm, causes a build failure:
> 
> kernel/built-in.o: In function `stack_trace_reset':
> trace_sysprof.c:(.text+0x3e91a): undefined reference to `tracing_stop_cmdline_record'
> kernel/built-in.o: In function `stack_trace_init':
> trace_sysprof.c:(.text+0x3e953): undefined reference to `tracing_start_cmdline_record'
> 
> 	Ingo

Sorry, this fixlet solves the problem:

--
>From 97113f668e9aef7ba101fbb3399e5ebb27aeec4c Mon Sep 17 00:00:00 2001
From: Frederic Weisbecker <fweisbec@gmail.com>
Date: Tue, 10 Feb 2009 15:07:22 +0100
Subject: [PATCH] tracing/sysprof: fix a build error on sysprof tracer

Fix the following build error:

kernel/built-in.o: In function `stack_trace_reset':
trace_sysprof.c:(.text+0x3e91a): undefined reference to `tracing_stop_cmdline_record'
kernel/built-in.o: In function `stack_trace_init':
trace_sysprof.c:(.text+0x3e953): undefined reference to `tracing_start_cmdline_record'

Reported-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/trace/Kconfig |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 79be773..620f9cd 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -134,6 +134,7 @@ config SYSPROF_TRACER
 	bool "Sysprof Tracer"
 	depends on X86
 	select TRACING
+	select CONTEXT_SWITCH_TRACER
 	help
 	  This tracer provides the trace needed by the 'Sysprof' userspace
 	  tool.
-- 
1.6.1

Re: [PATCH 1/2] tracing/sysprof: add missing tracing_{start,stop}_record_cmdline()

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Tue, Feb 10, 2009 at 01:31:45PM +0100, Ingo Molnar wrote:
> > 
> > * Ingo Molnar <mingo@elte.hu> wrote:
> > 
> > > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > > 
> > > > Add the missing pair tracing_{start,stop}_record_cmdline() to record well
> > > > the cmdline associated with pid.
> > > > 
> > > > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > > > ---
> > > >  kernel/trace/trace_sysprof.c |    3 +++
> > > >  1 files changed, 3 insertions(+), 0 deletions(-)
> > > 
> > > applied to tip/tracing/sysprof, thanks Frederic!
> > 
> > hm, causes a build failure:
> > 
> > kernel/built-in.o: In function `stack_trace_reset':
> > trace_sysprof.c:(.text+0x3e91a): undefined reference to `tracing_stop_cmdline_record'
> > kernel/built-in.o: In function `stack_trace_init':
> > trace_sysprof.c:(.text+0x3e953): undefined reference to `tracing_start_cmdline_record'
> > 
> > 	Ingo
> 
> Sorry, this fixlet solves the problem:

Could you please send one combo patch? The patch was so small and was at
the tail of tracing/sysprof that i didnt apply it after all.

	Ingo

Re: [PATCH 1/2] tracing/sysprof: add missing tracing_{start,stop}_record_cmdline()

[Frederic Weisbecker]

On Tue, Feb 10, 2009 at 03:25:43PM +0100, Ingo Molnar wrote:
> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > On Tue, Feb 10, 2009 at 01:31:45PM +0100, Ingo Molnar wrote:
> > > 
> > > * Ingo Molnar <mingo@elte.hu> wrote:
> > > 
> > > > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > > > 
> > > > > Add the missing pair tracing_{start,stop}_record_cmdline() to record well
> > > > > the cmdline associated with pid.
> > > > > 
> > > > > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > > > > ---
> > > > >  kernel/trace/trace_sysprof.c |    3 +++
> > > > >  1 files changed, 3 insertions(+), 0 deletions(-)
> > > > 
> > > > applied to tip/tracing/sysprof, thanks Frederic!
> > > 
> > > hm, causes a build failure:
> > > 
> > > kernel/built-in.o: In function `stack_trace_reset':
> > > trace_sysprof.c:(.text+0x3e91a): undefined reference to `tracing_stop_cmdline_record'
> > > kernel/built-in.o: In function `stack_trace_init':
> > > trace_sysprof.c:(.text+0x3e953): undefined reference to `tracing_start_cmdline_record'
> > > 
> > > 	Ingo
> > 
> > Sorry, this fixlet solves the problem:
> 
> Could you please send one combo patch? The patch was so small and was at
> the tail of tracing/sysprof that i didnt apply it after all.
> 
> 	Ingo


Ok.
I've dropped the build error example in the changelog, keeping only the reason
of the error, as this is a V2.
Tell me if you are fine with it.

--
>From 1656ef9b22049a0d93c7c3946e28774876df535b Mon Sep 17 00:00:00 2001
From: Frederic Weisbecker <fweisbec@gmail.com>
Date: Tue, 10 Feb 2009 15:33:43 +0100
Subject: [PATCH v2] tracing/sysprof: add missing tracing_{start,stop}_record_cmdline()

Add the missing pair tracing_{start,stop}_record_cmdline() to record well
the cmdline associated with pid.

Changes in v2:

_ fix a build error, the sched_switch tracer is needed to record the
cmdline.

Reported-by: Ingo Molnar <mingo@elte.hu>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/trace/Kconfig         |    1 +
 kernel/trace/trace_sysprof.c |    3 +++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 79be773..620f9cd 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -134,6 +134,7 @@ config SYSPROF_TRACER
 	bool "Sysprof Tracer"
 	depends on X86
 	select TRACING
+	select CONTEXT_SWITCH_TRACER
 	help
 	  This tracer provides the trace needed by the 'Sysprof' userspace
 	  tool.
diff --git a/kernel/trace/trace_sysprof.c b/kernel/trace/trace_sysprof.c
index 84ca9d8..9902c15 100644
--- a/kernel/trace/trace_sysprof.c
+++ b/kernel/trace/trace_sysprof.c
@@ -238,6 +238,8 @@ static int stack_trace_init(struct trace_array *tr)
 {
 	sysprof_trace = tr;
 
+	tracing_start_cmdline_record();
+
 	mutex_lock(&sample_timer_lock);
 	start_stack_timers();
 	tracer_enabled = 1;
@@ -247,6 +249,7 @@ static int stack_trace_init(struct trace_array *tr)
 
 static void stack_trace_reset(struct trace_array *tr)
 {
+	tracing_stop_cmdline_record();
 	stop_stack_trace(tr);
 }
 
-- 
1.6.1

Re: [PATCH 1/2] tracing/sysprof: add missing tracing_{start,stop}_record_cmdline()

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> 
> Ok.
> I've dropped the build error example in the changelog, keeping only the reason
> of the error, as this is a V2.
> Tell me if you are fine with it.

Looks good - applied to tip:tracing/sysprof, thanks Frederic!

	Ingo

Re: [PATCH 1/2] tracing/sysprof: add missing tracing_{start,stop}_record_cmdline()

[Ingo Molnar]

hm, still no good:

kernel/built-in.o: In function `boot_trace_init':
trace_boot.c:(.text+0x7751a): undefined reference to 
`tracing_sched_switch_assign_trace'
kernel/built-in.o: In function `disable_boot_trace':
(.text+0x7762c): undefined reference to `tracing_stop_sched_switch_record'

	Ingo

Re: [PATCH 1/2] tracing/sysprof: add missing tracing_{start,stop}_record_cmdline()

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> 
> hm, still no good:
> 
> kernel/built-in.o: In function `boot_trace_init':
> trace_boot.c:(.text+0x7751a): undefined reference to 
> `tracing_sched_switch_assign_trace'
> kernel/built-in.o: In function `disable_boot_trace':
> (.text+0x7762c): undefined reference to `tracing_stop_sched_switch_record'

actually, that's Arnaldo's bug, caused by:

 9182702: tracing: use a single trace_enabled variable

	Ingo

Re: [PATCH 1/2] tracing/sysprof: add missing tracing_{start,stop}_record_cmdline()

[Steven Rostedt]

On Tue, 10 Feb 2009, Frederic Weisbecker wrote:

Hi Frederic!

Just a quick comment about the change log. I'm still learning how to
write them too, but I'll give my 2 cents anyway ;-)

When looking a logs, I sometimes like to know the reason for the change.
So below I would have also included...

> 
> --
> >From 97113f668e9aef7ba101fbb3399e5ebb27aeec4c Mon Sep 17 00:00:00 2001
> From: Frederic Weisbecker <fweisbec@gmail.com>
> Date: Tue, 10 Feb 2009 15:07:22 +0100
> Subject: [PATCH] tracing/sysprof: fix a build error on sysprof tracer
> 

The sysprof tracer gets the command line data from the context switch 
tracer. We must select it otherwise we get 

>  the following build error:
> 
> kernel/built-in.o: In function `stack_trace_reset':
> trace_sysprof.c:(.text+0x3e91a): undefined reference to `tracing_stop_cmdline_record'
> kernel/built-in.o: In function `stack_trace_init':
> trace_sysprof.c:(.text+0x3e953): undefined reference to `tracing_start_cmdline_record'
> 
> Reported-by: Ingo Molnar <mingo@elte.hu>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

Just makes reading the logs a bit easier to see what is happening.

Thanks!

Acked-by: Steven Rostedt <srostedt@redhat.com>

-- Steve

Re: [PATCH 1/2] tracing/sysprof: add missing tracing_{start,stop}_record_cmdline()

[Frederic Weisbecker]

On Tue, Feb 10, 2009 at 09:26:11AM -0500, Steven Rostedt wrote:
> 
> On Tue, 10 Feb 2009, Frederic Weisbecker wrote:
> 
> Hi Frederic!
> 
> Just a quick comment about the change log. I'm still learning how to
> write them too, but I'll give my 2 cents anyway ;-)
> 
> When looking a logs, I sometimes like to know the reason for the change.
> So below I would have also included...
> 
> > 
> > --
> > >From 97113f668e9aef7ba101fbb3399e5ebb27aeec4c Mon Sep 17 00:00:00 2001
> > From: Frederic Weisbecker <fweisbec@gmail.com>
> > Date: Tue, 10 Feb 2009 15:07:22 +0100
> > Subject: [PATCH] tracing/sysprof: fix a build error on sysprof tracer
> > 
> 
> The sysprof tracer gets the command line data from the context switch 
> tracer. We must select it otherwise we get 
> 
> >  the following build error:
> > 
> > kernel/built-in.o: In function `stack_trace_reset':
> > trace_sysprof.c:(.text+0x3e91a): undefined reference to `tracing_stop_cmdline_record'
> > kernel/built-in.o: In function `stack_trace_init':
> > trace_sysprof.c:(.text+0x3e953): undefined reference to `tracing_start_cmdline_record'
> > 
> > Reported-by: Ingo Molnar <mingo@elte.hu>
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> 
> Just makes reading the logs a bit easier to see what is happening.


Exact, you're right :-)
Anyway, I've unified it in a combo patch...

 
> Thanks!
> 
> Acked-by: Steven Rostedt <srostedt@redhat.com>
> 
> -- Steve
> 

Re: [PATCH 1/2] tracing/sysprof: add missing tracing_{start,stop}_record_cmdline()

[Arnaldo Carvalho de Melo]

Em Tue, Feb 10, 2009 at 04:43:30AM +0100, Frederic Weisbecker escreveu:
> Add the missing pair tracing_{start,stop}_record_cmdline() to record well
> the cmdline associated with pid.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

This is another thing that I think begs for a TRACE_INIT_CMDLINE in
struct trace->flags so that just before calling ->init we call
tracing_start_cmdline_record? no?

Another thing I noticed is that even we having a ->stop most of the time
this semantic action is done thru ->reset :-\

- Arnaldo

>  kernel/trace/trace_sysprof.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/trace/trace_sysprof.c b/kernel/trace/trace_sysprof.c
> index 84ca9d8..9902c15 100644
> --- a/kernel/trace/trace_sysprof.c
> +++ b/kernel/trace/trace_sysprof.c
> @@ -238,6 +238,8 @@ static int stack_trace_init(struct trace_array *tr)
>  {
>  	sysprof_trace = tr;
>  
> +	tracing_start_cmdline_record();
> +
>  	mutex_lock(&sample_timer_lock);
>  	start_stack_timers();
>  	tracer_enabled = 1;
> @@ -247,6 +249,7 @@ static int stack_trace_init(struct trace_array *tr)
>  
>  static void stack_trace_reset(struct trace_array *tr)
>  {
> +	tracing_stop_cmdline_record();
>  	stop_stack_trace(tr);
>  }

Re: [PATCH 1/2] tracing/sysprof: add missing tracing_{start,stop}_record_cmdline()

[Frederic Weisbecker]

On Tue, Feb 10, 2009 at 10:54:24AM -0200, Arnaldo Carvalho de Melo wrote:
> Em Tue, Feb 10, 2009 at 04:43:30AM +0100, Frederic Weisbecker escreveu:
> > Add the missing pair tracing_{start,stop}_record_cmdline() to record well
> > the cmdline associated with pid.
> > 
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> 
> This is another thing that I think begs for a TRACE_INIT_CMDLINE in
> struct trace->flags so that just before calling ->init we call
> tracing_start_cmdline_record? no?


Right!
But the flags on struct tracer are more likely for private use, they have
a meaning only for the concerned tracer.

I guess it will be something for the global flags, when they will be
per-tracer.

 
> Another thing I noticed is that even we having a ->stop most of the time
> this semantic action is done thru ->reset :-\


Yes, perhaps the tracing of cmdline could be even performed on tracing_stop,
unless... perhaps it's not needed since the sched_switch tracer will be stopped too
anyway.

> 
> - Arnaldo
> 
> >  kernel/trace/trace_sysprof.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_sysprof.c b/kernel/trace/trace_sysprof.c
> > index 84ca9d8..9902c15 100644
> > --- a/kernel/trace/trace_sysprof.c
> > +++ b/kernel/trace/trace_sysprof.c
> > @@ -238,6 +238,8 @@ static int stack_trace_init(struct trace_array *tr)
> >  {
> >  	sysprof_trace = tr;
> >  
> > +	tracing_start_cmdline_record();
> > +
> >  	mutex_lock(&sample_timer_lock);
> >  	start_stack_timers();
> >  	tracer_enabled = 1;
> > @@ -247,6 +249,7 @@ static int stack_trace_init(struct trace_array *tr)
> >  
> >  static void stack_trace_reset(struct trace_array *tr)
> >  {
> > +	tracing_stop_cmdline_record();
> >  	stop_stack_trace(tr);
> >  }