[PATCH 0/4] [GIT PULL] tracing: updates

[PATCH 0/4] [GIT PULL] tracing: updates

[Steven Rostedt]

Ingo,

This is based off of the last pull request I sent.

Please pull the latest tip/tracing/core-1 tree, which can be found at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
tip/tracing/core-1


Carsten Emde (1):
      tracing/sched: Fix task states in sched switch event

Li Zefan (1):
      tracing: Fix function declarations if !CONFIG_STACKTRACE

Steven Rostedt (2):
      tracing: Allow mmio tracer to display trace_printk() and other events
      tracing: Update branch trace to new event API

----
 fs/proc/array.c                |   25 +++++++++++++------------
 include/linux/sched.h          |   38 ++++++++++++++++++++++++++++++++++++--
 include/trace/events/sched.h   |   12 +++++++++---
 kernel/trace/trace.h           |    4 ++--
 kernel/trace/trace_branch.c    |    8 ++++++--
 kernel/trace/trace_mmiotrace.c |    3 ++-
 6 files changed, 68 insertions(+), 22 deletions(-)

[PATCH 1/4] tracing: Fix function declarations if !CONFIG_STACKTRACE

[Steven Rostedt]

[-- Attachment #1: 0001-tracing-Fix-function-declarations-if-CONFIG_STACKTRA.patch --]
[-- Type: text/plain, Size: 1192 bytes --]

From: Li Zefan <lizf@cn.fujitsu.com>

ftrace_trace_stack() and frace_trace_userstacke() take a
struct ring_buffer argument, not struct trace_array. Commit
e77405ad("tracing: pass around ring buffer instead of tracer")
made this change.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
LKML-Reference: <4BE77C14.5010806@cn.fujitsu.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 6356259..40cd171 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -419,12 +419,12 @@ void ftrace_trace_userstack(struct ring_buffer *buffer, unsigned long flags,
 void __trace_stack(struct trace_array *tr, unsigned long flags, int skip,
 		   int pc);
 #else
-static inline void ftrace_trace_stack(struct trace_array *tr,
+static inline void ftrace_trace_stack(struct ring_buffer *buffer,
 				      unsigned long flags, int skip, int pc)
 {
 }
 
-static inline void ftrace_trace_userstack(struct trace_array *tr,
+static inline void ftrace_trace_userstack(struct ring_buffer *buffer,
 					  unsigned long flags, int pc)
 {
 }
-- 
1.7.0

[PATCH 2/4] tracing/sched: Fix task states in sched switch event

[Steven Rostedt]

[-- Attachment #1: 0002-tracing-sched-Fix-task-states-in-sched-switch-event.patch --]
[-- Type: text/plain, Size: 5038 bytes --]

From: Carsten Emde <C.Emde@osadl.org>

The sched_switch trace event displays erroneous character codes of task
states, after a new task state was added in the scheduler code but
omitted to add in the trace event code.

Define character codes of task states individually. In addition, define
task state descriptions needed in /proc at the same place. This will
help to keep the task state bits, characters and descriptions in sync
should they ever need to be changed again.

Signed-off-by: Carsten Emde <C.Emde@osadl.org>
LKML-Reference: <20100311122822.678489605@osadl.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 fs/proc/array.c              |   25 +++++++++++++------------
 include/linux/sched.h        |   38 ++++++++++++++++++++++++++++++++++++--
 include/trace/events/sched.h |   12 +++++++++---
 3 files changed, 58 insertions(+), 17 deletions(-)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index e51f2ec..6ececdd 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -128,21 +128,22 @@ static inline void task_name(struct seq_file *m, struct task_struct *p)
 
 /*
  * The task state array is a strange "bitmap" of
- * reasons to sleep. Thus "running" is zero, and
- * you can test for combinations of others with
+ * reasons to sleep. Thus, the first element is zero,
+ * and you can test for combinations of others with
  * simple bit tests.
  */
+#define TASK_STATE_X(num) TASK_STATE_##num " (" DESCR_TASK_STATE_##num ")"
 static const char *task_state_array[] = {
-	"R (running)",		/*   0 */
-	"S (sleeping)",		/*   1 */
-	"D (disk sleep)",	/*   2 */
-	"T (stopped)",		/*   4 */
-	"t (tracing stop)",	/*   8 */
-	"Z (zombie)",		/*  16 */
-	"X (dead)",		/*  32 */
-	"x (dead)",		/*  64 */
-	"K (wakekill)",		/* 128 */
-	"W (waking)",		/* 256 */
+	TASK_STATE_X(0),
+	TASK_STATE_X(1),
+	TASK_STATE_X(2),
+	TASK_STATE_X(4),
+	TASK_STATE_X(8),
+	TASK_STATE_X(16),
+	TASK_STATE_X(32),
+	TASK_STATE_X(64),
+	TASK_STATE_X(128),
+	TASK_STATE_X(256)
 };
 
 static inline const char *get_task_state(struct task_struct *tsk)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index dad7f66..1d0b0ab 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -172,7 +172,9 @@ print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq *cfs_rq)
 
 /*
  * Task state bitmask. NOTE! These bits are also
- * encoded in fs/proc/array.c: get_task_state().
+ * used in fs/proc/array.c: get_task_state() and
+ * in include/trace/events/sched.h in the
+ * sched_switch trace event.
  *
  * We have two separate sets of flags: task->state
  * is about runnability, while task->exit_state are
@@ -181,20 +183,52 @@ print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq *cfs_rq)
  * mistake.
  */
 #define TASK_RUNNING		0
+#define TASK_STATE_0		"R"
+#define DESCR_TASK_STATE_0	"running"
+
 #define TASK_INTERRUPTIBLE	1
+#define TASK_STATE_1		"S"
+#define DESCR_TASK_STATE_1	"sleeping"
+
 #define TASK_UNINTERRUPTIBLE	2
+#define TASK_STATE_2		"D"
+#define DESCR_TASK_STATE_2	"disk sleep"
+
 #define __TASK_STOPPED		4
+#define TASK_STATE_4		"T"
+#define DESCR_TASK_STATE_4	"stopped"
+
 #define __TASK_TRACED		8
+#define TASK_STATE_8		"t"
+#define DESCR_TASK_STATE_8	"tracing stop"
+
 /* in tsk->exit_state */
 #define EXIT_ZOMBIE		16
+#define TASK_STATE_16		"Z"
+#define DESCR_TASK_STATE_16	"zombie"
+
 #define EXIT_DEAD		32
+#define TASK_STATE_32		"X"
+#define DESCR_TASK_STATE_32	"dead"
+
 /* in tsk->state again */
 #define TASK_DEAD		64
+#define TASK_STATE_64		"x"
+#define DESCR_TASK_STATE_64	"dead"
+
 #define TASK_WAKEKILL		128
+#define TASK_STATE_128		"K"
+#define DESCR_TASK_STATE_128	"wakekill"
+
 #define TASK_WAKING		256
+#define TASK_STATE_256		"W"
+#define DESCR_TASK_STATE_256	"waking"
+
 #define TASK_STATE_MAX		512
 
-#define TASK_STATE_TO_CHAR_STR "RSDTtZXxKW"
+#define TASK_STATE_TO_CHAR_STR \
+  TASK_STATE_0 TASK_STATE_1 TASK_STATE_2 TASK_STATE_4 TASK_STATE_8 \
+  TASK_STATE_16 TASK_STATE_32 TASK_STATE_64 TASK_STATE_128 TASK_STATE_256
 
 extern char ___assert_task_state[1 - 2*!!(
 		sizeof(TASK_STATE_TO_CHAR_STR)-1 != ilog2(TASK_STATE_MAX)+1)];
diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
index cfceb0b..d073711 100644
--- a/include/trace/events/sched.h
+++ b/include/trace/events/sched.h
@@ -161,11 +161,17 @@ TRACE_EVENT(sched_switch,
 		__entry->prev_comm, __entry->prev_pid, __entry->prev_prio,
 		__entry->prev_state ?
 		  __print_flags(__entry->prev_state, "|",
-				{ 1, "S"} , { 2, "D" }, { 4, "T" }, { 8, "t" },
-				{ 16, "Z" }, { 32, "X" }, { 64, "x" },
-				{ 128, "W" }) : "R",
+			{ 1, TASK_STATE_1} , { 2, TASK_STATE_2 },
+			{ 4, TASK_STATE_4 }, { 8, TASK_STATE_8 },
+			{ 16, TASK_STATE_16 }, { 32, TASK_STATE_32 },
+			{ 64, TASK_STATE_64 }, { 128, TASK_STATE_128 },
+			{ 256, TASK_STATE_256 }
+			) : TASK_STATE_0,
 		__entry->next_comm, __entry->next_pid, __entry->next_prio)
 );
+#if TASK_STATE_MAX != 512
+#error "Please add new task state array in __print_flags() above."
+#endif
 
 /*
  * Tracepoint for a task being migrated:
-- 
1.7.0

[PATCH 3/4] tracing: Allow mmio tracer to display trace_printk() and other events

[Steven Rostedt]

[-- Attachment #1: 0003-tracing-Allow-mmio-tracer-to-display-trace_printk-an.patch --]
[-- Type: text/plain, Size: 1267 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

The mmio tracer has its own function to handle reading of events.
But if it encounters an event that it does not understand it ignores
it instead of telling the calling function that it is not processing
it.

If someone adds trace_printk() or enables events along with the mmio
tracer, then these events will not be displayed in the trace output.

Simple solution is to just have the mmio print return UNHANDLED to
let the caller know that it did not processes the event and the
caller can process the event further.

Reported-by: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Pekka Paalanen <pq@iki.fi>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace_mmiotrace.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace_mmiotrace.c b/kernel/trace/trace_mmiotrace.c
index 017fa37..592c00f 100644
--- a/kernel/trace/trace_mmiotrace.c
+++ b/kernel/trace/trace_mmiotrace.c
@@ -282,7 +282,8 @@ static enum print_line_t mmio_print_line(struct trace_iterator *iter)
 	case TRACE_PRINT:
 		return mmio_print_mark(iter);
 	default:
-		return TRACE_TYPE_HANDLED; /* ignore unknown entries */
+		/* Not our event */
+		return TRACE_TYPE_UNHANDLED;
 	}
 }
 
-- 
1.7.0

[PATCH 4/4] tracing: Update branch trace to new event API

[Steven Rostedt]

[-- Attachment #1: 0004-tracing-Update-branch-trace-to-new-event-API.patch --]
[-- Type: text/plain, Size: 1208 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

The branch tracer was not updated with the updates to shrink
TRACE_EVENT(). Although the branch tracer does not use TRACE_EVENT()
some of the API changes caused it to fail to compile.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace_branch.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_branch.c b/kernel/trace/trace_branch.c
index b9bc4d4..8d3538b 100644
--- a/kernel/trace/trace_branch.c
+++ b/kernel/trace/trace_branch.c
@@ -143,7 +143,7 @@ static void branch_trace_reset(struct trace_array *tr)
 }
 
 static enum print_line_t trace_branch_print(struct trace_iterator *iter,
-					    int flags)
+					    int flags, struct trace_event *event)
 {
 	struct trace_branch *field;
 
@@ -167,9 +167,13 @@ static void branch_print_header(struct seq_file *s)
 		"    |\n");
 }
 
+static struct trace_event_functions trace_branch_funcs = {
+	.trace		= trace_branch_print,
+};
+
 static struct trace_event trace_branch_event = {
 	.type		= TRACE_BRANCH,
-	.trace		= trace_branch_print,
+	.funcs		= &trace_branch_funcs,
 };
 
 static struct tracer branch_trace __read_mostly =
-- 
1.7.0

Re: [PATCH 2/4] tracing/sched: Fix task states in sched switch event

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> +#define TASK_STATE_X(num) TASK_STATE_##num " (" DESCR_TASK_STATE_##num ")"
>  static const char *task_state_array[] = {
> -	"R (running)",		/*   0 */
> -	"S (sleeping)",		/*   1 */
> -	"D (disk sleep)",	/*   2 */
> -	"T (stopped)",		/*   4 */
> -	"t (tracing stop)",	/*   8 */
> -	"Z (zombie)",		/*  16 */
> -	"X (dead)",		/*  32 */
> -	"x (dead)",		/*  64 */
> -	"K (wakekill)",		/* 128 */
> -	"W (waking)",		/* 256 */
> +	TASK_STATE_X(0),
> +	TASK_STATE_X(1),
> +	TASK_STATE_X(2),
> +	TASK_STATE_X(4),
> +	TASK_STATE_X(8),
> +	TASK_STATE_X(16),
> +	TASK_STATE_X(32),
> +	TASK_STATE_X(64),
> +	TASK_STATE_X(128),
> +	TASK_STATE_X(256)

Hm, this is totally unreadable. What does 'TASK_STATE_X' mean??

	Ingo

Re: [PATCH 3/4] tracing: Allow mmio tracer to display trace_printk() and other events

[Pekka Paalanen]

On Wed, 12 May 2010 21:21:13 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Steven Rostedt <srostedt@redhat.com>
> 
> The mmio tracer has its own function to handle reading of events.
> But if it encounters an event that it does not understand it
> ignores it instead of telling the calling function that it is not
> processing it.
> 
> If someone adds trace_printk() or enables events along with the
> mmio tracer, then these events will not be displayed in the trace
> output.
> 
> Simple solution is to just have the mmio print return UNHANDLED to
> let the caller know that it did not processes the event and the
> caller can process the event further.

Does this not mean that the mmiotrace output may contain
foreign lines? If it does, it will break the user space.
The dump format is specified in
Documentation/trace/mmiotrace.txt.

If you want to handle arbitrary messages, format them as
MARK events, please.

If I understood correctly, then NAK for this patch. Otherwise,
could you explain how this does not break the mmiotrace dump
format?

Is the tracing infrastructure now supporting several
active tracers at the same time? If yes, and if mmiotrace
should be able to co-operate, we need a new revision of the
dump format, or a tool that extracts the mmiotrace
events in the current format.

Thanks.

> Reported-by: Larry Finger <Larry.Finger@lwfinger.net>
> Cc: Pekka Paalanen <pq@iki.fi>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  kernel/trace/trace_mmiotrace.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/trace/trace_mmiotrace.c
> b/kernel/trace/trace_mmiotrace.c index 017fa37..592c00f 100644
> --- a/kernel/trace/trace_mmiotrace.c
> +++ b/kernel/trace/trace_mmiotrace.c
> @@ -282,7 +282,8 @@ static enum print_line_t
> mmio_print_line(struct trace_iterator *iter) case TRACE_PRINT:
>  		return mmio_print_mark(iter);
>  	default:
> -		return TRACE_TYPE_HANDLED; /* ignore unknown
> entries */
> +		/* Not our event */
> +		return TRACE_TYPE_UNHANDLED;
>  	}
>  }
>  
> -- 
> 1.7.0

-- 
Pekka Paalanen
http://www.iki.fi/pq/

Re: [PATCH 2/4] tracing/sched: Fix task states in sched switch event

[Carsten Emde]

Hi Ingo,

> Hm, this is totally unreadable. What does 'TASK_STATE_X' mean??
Would this be better?
+#define MAKE_TASK_STATE_STRING(num) TASK_STATE_##num " (" DESCR_TASK_STATE_##num ")"
 static const char *task_state_array[] = {
-	"R (running)",		/*   0 */
-	"S (sleeping)",		/*   1 */
-	"D (disk sleep)",	/*   2 */
-	"T (stopped)",		/*   4 */
-	"t (tracing stop)",	/*   8 */
-	"Z (zombie)",		/*  16 */
-	"X (dead)",		/*  32 */
-	"x (dead)",		/*  64 */
-	"K (wakekill)",		/* 128 */
-	"W (waking)",		/* 256 */
+	MAKE_TASK_STATE_STRING(0),
+	MAKE_TASK_STATE_STRING(1),
+	MAKE_TASK_STATE_STRING(2),
+	MAKE_TASK_STATE_STRING(4),
+	MAKE_TASK_STATE_STRING(8),
+	MAKE_TASK_STATE_STRING(16),
+	MAKE_TASK_STATE_STRING(32),
+	MAKE_TASK_STATE_STRING(64),
+	MAKE_TASK_STATE_STRING(128),
+	MAKE_TASK_STATE_STRING(256)

	Carsten.

Re: [PATCH 3/4] tracing: Allow mmio tracer to display trace_printk() and other events

[Steven Rostedt]

On Thu, 2010-05-13 at 11:54 +0300, Pekka Paalanen wrote:
> On Wed, 12 May 2010 21:21:13 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > From: Steven Rostedt <srostedt@redhat.com>
> > 
> > The mmio tracer has its own function to handle reading of events.
> > But if it encounters an event that it does not understand it
> > ignores it instead of telling the calling function that it is not
> > processing it.
> > 
> > If someone adds trace_printk() or enables events along with the
> > mmio tracer, then these events will not be displayed in the trace
> > output.
> > 
> > Simple solution is to just have the mmio print return UNHANDLED to
> > let the caller know that it did not processes the event and the
> > caller can process the event further.
> 
> Does this not mean that the mmiotrace output may contain
> foreign lines? If it does, it will break the user space.
> The dump format is specified in
> Documentation/trace/mmiotrace.txt.
> 
> If you want to handle arbitrary messages, format them as
> MARK events, please.
> 
> If I understood correctly, then NAK for this patch. Otherwise,
> could you explain how this does not break the mmiotrace dump
> format?
> 
> Is the tracing infrastructure now supporting several
> active tracers at the same time? If yes, and if mmiotrace
> should be able to co-operate, we need a new revision of the
> dump format, or a tool that extracts the mmiotrace
> events in the current format.
> 

It only displays other events if the user enabled those events.

But that said, I don't want to break existing userspace tools. I can add
a mmiotrace option "mmiotrace_all_events", if the user wants to see all
events within the mmiotracer then they can just enable that option,
otherwise, the mmiotracer will act like it currently does.

How does that sound?

-- Steve

> Thanks.
> 
> > Reported-by: Larry Finger <Larry.Finger@lwfinger.net>
> > Cc: Pekka Paalanen <pq@iki.fi>
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > ---
> >  kernel/trace/trace_mmiotrace.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_mmiotrace.c
> > b/kernel/trace/trace_mmiotrace.c index 017fa37..592c00f 100644
> > --- a/kernel/trace/trace_mmiotrace.c
> > +++ b/kernel/trace/trace_mmiotrace.c
> > @@ -282,7 +282,8 @@ static enum print_line_t
> > mmio_print_line(struct trace_iterator *iter) case TRACE_PRINT:
> >  		return mmio_print_mark(iter);
> >  	default:
> > -		return TRACE_TYPE_HANDLED; /* ignore unknown
> > entries */
> > +		/* Not our event */
> > +		return TRACE_TYPE_UNHANDLED;
> >  	}
> >  }
> >  
> > -- 
> > 1.7.0
> 

Re: [PATCH 3/4] tracing: Allow mmio tracer to display trace_printk() and other events

[Pekka Paalanen]

On Thu, 13 May 2010 08:15:09 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 2010-05-13 at 11:54 +0300, Pekka Paalanen wrote:
> > On Wed, 12 May 2010 21:21:13 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > From: Steven Rostedt <srostedt@redhat.com>
> > > 
> > > The mmio tracer has its own function to handle reading of
> > > events. But if it encounters an event that it does not
> > > understand it ignores it instead of telling the calling
> > > function that it is not processing it.
> > > 
> > > If someone adds trace_printk() or enables events along with
> > > the mmio tracer, then these events will not be displayed in
> > > the trace output.
> > > 
> > > Simple solution is to just have the mmio print return
> > > UNHANDLED to let the caller know that it did not processes
> > > the event and the caller can process the event further.
> > 
> > Does this not mean that the mmiotrace output may contain
> > foreign lines? If it does, it will break the user space.
> > The dump format is specified in
> > Documentation/trace/mmiotrace.txt.
> > 
> > If you want to handle arbitrary messages, format them as
> > MARK events, please.
> > 
> > If I understood correctly, then NAK for this patch. Otherwise,
> > could you explain how this does not break the mmiotrace dump
> > format?
> > 
> > Is the tracing infrastructure now supporting several
> > active tracers at the same time? If yes, and if mmiotrace
> > should be able to co-operate, we need a new revision of the
> > dump format, or a tool that extracts the mmiotrace
> > events in the current format.
> > 
> 
> It only displays other events if the user enabled those events.
> 
> But that said, I don't want to break existing userspace tools. I
> can add a mmiotrace option "mmiotrace_all_events", if the user
> wants to see all events within the mmiotracer then they can just
> enable that option, otherwise, the mmiotracer will act like it
> currently does.
> 
> How does that sound?

That would be fine. Is it not redundant with what you said in
your first sentence?

-- 
Pekka Paalanen
http://www.iki.fi/pq/

Re: [PATCH 2/4] tracing/sched: Fix task states in sched switch event

[Ingo Molnar]

* Carsten Emde <C.Emde@osadl.org> wrote:

> Hi Ingo,
> 
> > Hm, this is totally unreadable. What does 'TASK_STATE_X' mean??
> Would this be better?
> +#define MAKE_TASK_STATE_STRING(num) TASK_STATE_##num " (" DESCR_TASK_STATE_##num ")"
>  static const char *task_state_array[] = {
> -	"R (running)",		/*   0 */
> -	"S (sleeping)",		/*   1 */
> -	"D (disk sleep)",	/*   2 */
> -	"T (stopped)",		/*   4 */
> -	"t (tracing stop)",	/*   8 */
> -	"Z (zombie)",		/*  16 */
> -	"X (dead)",		/*  32 */
> -	"x (dead)",		/*  64 */
> -	"K (wakekill)",		/* 128 */
> -	"W (waking)",		/* 256 */
> +	MAKE_TASK_STATE_STRING(0),
> +	MAKE_TASK_STATE_STRING(1),
> +	MAKE_TASK_STATE_STRING(2),
> +	MAKE_TASK_STATE_STRING(4),
> +	MAKE_TASK_STATE_STRING(8),
> +	MAKE_TASK_STATE_STRING(16),
> +	MAKE_TASK_STATE_STRING(32),
> +	MAKE_TASK_STATE_STRING(64),
> +	MAKE_TASK_STATE_STRING(128),
> +	MAKE_TASK_STATE_STRING(256)

The whole enumeration there is pointless in that .c file - it tells nothing to 
the code reader.

If it cannot be expressed in a meaningful way then introduce 
TASK_STATE_STRINGS_INIT construct that is defined next to the strings (in a .h 
file or so) - that way it's a coherent whole.

Thanks,

	Ingo

Re: [PATCH 3/4] tracing: Allow mmio tracer to display trace_printk() and other events

[Steven Rostedt]

On Thu, 2010-05-13 at 15:29 +0300, Pekka Paalanen wrote:
> On Thu, 13 May 2010 08:15:09 -0400

> > It only displays other events if the user enabled those events.
> > 
> > But that said, I don't want to break existing userspace tools. I
> > can add a mmiotrace option "mmiotrace_all_events", if the user
> > wants to see all events within the mmiotracer then they can just
> > enable that option, otherwise, the mmiotracer will act like it
> > currently does.
> > 
> > How does that sound?
> 
> That would be fine. Is it not redundant with what you said in
> your first sentence?
> 

Right now with this patch as is. When you enable the mmiotracer it
clears the ring buffer. But if someone previously enabled an event (like
sched_switch for example) then that event will appear in the output of
the tracer.

The user will need to disable that event and restart the mmiotracer so
the output will not break the userspace tools. Is this OK?

If not, then my suggestion is to have an mmiotracer option that keeps it
from printing out any event except for the ones it knows about.

The reason I added this patch in the first place was because Larry
Finger was using the mmiotrace with trace_printk() and the current code
does not print out the trace_printk() when mmiotracer is active.

-- Steve

Re: [PATCH 2/4] tracing/sched: Fix task states in sched switch event

[Steven Rostedt]

On Thu, 2010-05-13 at 15:35 +0200, Ingo Molnar wrote:
> * Carsten Emde <C.Emde@osadl.org> wrote:
> 
> > Hi Ingo,
> > 
> > > Hm, this is totally unreadable. What does 'TASK_STATE_X' mean??
> > Would this be better?
> > +#define MAKE_TASK_STATE_STRING(num) TASK_STATE_##num " (" DESCR_TASK_STATE_##num ")"
> >  static const char *task_state_array[] = {
> > -	"R (running)",		/*   0 */
> > -	"S (sleeping)",		/*   1 */
> > -	"D (disk sleep)",	/*   2 */
> > -	"T (stopped)",		/*   4 */
> > -	"t (tracing stop)",	/*   8 */
> > -	"Z (zombie)",		/*  16 */
> > -	"X (dead)",		/*  32 */
> > -	"x (dead)",		/*  64 */
> > -	"K (wakekill)",		/* 128 */
> > -	"W (waking)",		/* 256 */
> > +	MAKE_TASK_STATE_STRING(0),
> > +	MAKE_TASK_STATE_STRING(1),
> > +	MAKE_TASK_STATE_STRING(2),
> > +	MAKE_TASK_STATE_STRING(4),
> > +	MAKE_TASK_STATE_STRING(8),
> > +	MAKE_TASK_STATE_STRING(16),
> > +	MAKE_TASK_STATE_STRING(32),
> > +	MAKE_TASK_STATE_STRING(64),
> > +	MAKE_TASK_STATE_STRING(128),
> > +	MAKE_TASK_STATE_STRING(256)
> 
> The whole enumeration there is pointless in that .c file - it tells nothing to 
> the code reader.
> 
> If it cannot be expressed in a meaningful way then introduce 
> TASK_STATE_STRINGS_INIT construct that is defined next to the strings (in a .h 
> file or so) - that way it's a coherent whole.

I'm rebasing both pull requests. I'll leave this one out until there is
an agreement.

Thanks,

-- Steve

Re: [PATCH 3/4] tracing: Allow mmio tracer to display trace_printk() and other events

[Steven Rostedt]

On Thu, 2010-05-13 at 11:11 -0400, Steven Rostedt wrote:

> > > How does that sound?
> > 
> > That would be fine. Is it not redundant with what you said in
> > your first sentence?
> > 
> 
> Right now with this patch as is. When you enable the mmiotracer it
> clears the ring buffer. But if someone previously enabled an event (like
> sched_switch for example) then that event will appear in the output of
> the tracer.
> 
> The user will need to disable that event and restart the mmiotracer so
> the output will not break the userspace tools. Is this OK?
> 
> If not, then my suggestion is to have an mmiotracer option that keeps it
> from printing out any event except for the ones it knows about.
> 
> The reason I added this patch in the first place was because Larry
> Finger was using the mmiotrace with trace_printk() and the current code
> does not print out the trace_printk() when mmiotracer is active.

Pekka,

Are you OK with this version of the patch or do you want me to add the
option as described above?

-- Steve

Re: [PATCH 3/4] tracing: Allow mmio tracer to display trace_printk() and other events

[Pekka Paalanen]

Sorry, I'm not at my email every day. Real life...

On Thu, 13 May 2010 11:11:23 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 2010-05-13 at 15:29 +0300, Pekka Paalanen wrote:
> > On Thu, 13 May 2010 08:15:09 -0400
> 
> > > It only displays other events if the user enabled those
> > > events.
> > > 
> > > But that said, I don't want to break existing userspace
> > > tools. I can add a mmiotrace option "mmiotrace_all_events",
> > > if the user wants to see all events within the mmiotracer
> > > then they can just enable that option, otherwise, the
> > > mmiotracer will act like it currently does.
> > > 
> > > How does that sound?
> > 
> > That would be fine. Is it not redundant with what you said in
> > your first sentence?
> > 
> 
> Right now with this patch as is. When you enable the mmiotracer it
> clears the ring buffer. But if someone previously enabled an
> event (like sched_switch for example) then that event will appear
> in the output of the tracer.
> 
> The user will need to disable that event and restart the
> mmiotracer so the output will not break the userspace tools. Is
> this OK?

I think it is ok. No non-mmiotrace events are enabled automatically,
right? Except perhaps trace_printk()?

If a user enables other events while mmiotracing, I would
assume he knows what he is doing. End users doing dumps per
request never even know about other kinds of tracing than
mmiotrace.

> If not, then my suggestion is to have an mmiotracer option that
> keeps it from printing out any event except for the ones it knows
> about.
> 
> The reason I added this patch in the first place was because Larry
> Finger was using the mmiotrace with trace_printk() and the
> current code does not print out the trace_printk() when
> mmiotracer is active.

If this is *only* about trace_printk(), why not make a handler
for it to emit MARK lines? Actually, I somehow assumed that
would have been the case, but apparently the event type is
different. I do not recall these things too well anymore.


Thanks for keeping me in the loop.

-- 
Pekka Paalanen
http://www.iki.fi/pq/

Re: [PATCH 3/4] tracing: Allow mmio tracer to display trace_printk() and other events

[Steven Rostedt]

On Sat, 2010-05-15 at 10:46 +0300, Pekka Paalanen wrote:
> Sorry, I'm not at my email every day. Real life...

heh, no problem. I'm in no hurry here.

> 
> On Thu, 13 May 2010 11:11:23 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Thu, 2010-05-13 at 15:29 +0300, Pekka Paalanen wrote:
> > > On Thu, 13 May 2010 08:15:09 -0400
> > 
> > > > It only displays other events if the user enabled those
> > > > events.
> > > > 
> > > > But that said, I don't want to break existing userspace
> > > > tools. I can add a mmiotrace option "mmiotrace_all_events",
> > > > if the user wants to see all events within the mmiotracer
> > > > then they can just enable that option, otherwise, the
> > > > mmiotracer will act like it currently does.
> > > > 
> > > > How does that sound?
> > > 
> > > That would be fine. Is it not redundant with what you said in
> > > your first sentence?
> > > 
> > 
> > Right now with this patch as is. When you enable the mmiotracer it
> > clears the ring buffer. But if someone previously enabled an
> > event (like sched_switch for example) then that event will appear
> > in the output of the tracer.
> > 
> > The user will need to disable that event and restart the
> > mmiotracer so the output will not break the userspace tools. Is
> > this OK?
> 
> I think it is ok. No non-mmiotrace events are enabled automatically,
> right? Except perhaps trace_printk()?

Yep, trace events do not show up in a trace unless a user enabled them
themselves. trace_printk() is only for developers modifying their kernel
and needs works the opposite: You need to disable it from writing.

But trace_printk() only exists in the kernel if a developer added it and
recompiled their kernel. End users will never see it.

> 
> If a user enables other events while mmiotracing, I would
> assume he knows what he is doing. End users doing dumps per
> request never even know about other kinds of tracing than
> mmiotrace.
> 
> > If not, then my suggestion is to have an mmiotracer option that
> > keeps it from printing out any event except for the ones it knows
> > about.
> > 
> > The reason I added this patch in the first place was because Larry
> > Finger was using the mmiotrace with trace_printk() and the
> > current code does not print out the trace_printk() when
> > mmiotracer is active.
> 
> If this is *only* about trace_printk(), why not make a handler
> for it to emit MARK lines? Actually, I somehow assumed that
> would have been the case, but apparently the event type is
> different. I do not recall these things too well anymore.

Well, I can imagine that another user may want events too. But still,
I'm thinking the original patch to write everything should work. You
would only see events if you specifically enable them.

> 
> 
> Thanks for keeping me in the loop.
> 

No problem, it is your code we are dealing with.

-- Steve

Re: [PATCH 2/4] tracing/sched: Fix task states in sched switch event

[Carsten Emde]

Hi Ingo,

>>> Hm, this is totally unreadable. What does 'TASK_STATE_X' mean??
>> Would this be better?
>> +#define MAKE_TASK_STATE_STRING(num) TASK_STATE_##num " (" DESCR_TASK_STATE_##num ")"
>>   static const char *task_state_array[] = {
>> -	"R (running)",		/*   0 */
>> -	"S (sleeping)",		/*   1 */
>> -	"D (disk sleep)",	/*   2 */
>> -	"T (stopped)",		/*   4 */
>> -	"t (tracing stop)",	/*   8 */
>> -	"Z (zombie)",		/*  16 */
>> -	"X (dead)",		/*  32 */
>> -	"x (dead)",		/*  64 */
>> -	"K (wakekill)",		/* 128 */
>> -	"W (waking)",		/* 256 */
>> +	MAKE_TASK_STATE_STRING(0),
>> +	MAKE_TASK_STATE_STRING(1),
>> +	MAKE_TASK_STATE_STRING(2),
>> +	MAKE_TASK_STATE_STRING(4),
>> +	MAKE_TASK_STATE_STRING(8),
>> +	MAKE_TASK_STATE_STRING(16),
>> +	MAKE_TASK_STATE_STRING(32),
>> +	MAKE_TASK_STATE_STRING(64),
>> +	MAKE_TASK_STATE_STRING(128),
>> +	MAKE_TASK_STATE_STRING(256)
>
> The whole enumeration there is pointless in that .c file - it tells nothing to
> the code reader.
>
> If it cannot be expressed in a meaningful way then introduce
> TASK_STATE_STRINGS_INIT construct that is defined next to the strings (in a .h
> file or so) - that way it's a coherent whole.
This is what I did and submitted some time ago. Is there anything else
you want me to change?

Thanks,

	Carsten.