trace: fix task state printout

trace: fix task state printout

[Thomas Gleixner]

Impact: tracer task state decoding is wrong, size check is buggy

The tracing code has interesting varieties of printing out task state.
Unfortunalely only one of the instances is correct as it copies the
code from sched.c:sched_show_task(). The others are plain wrong as
they treatthe bitfield as an integer offset into the character
array. Also the size check of the character array is wrong as it
includes the trailing \0.

Use a common state decoder inline which does the Right Thing.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/trace/trace.c |   39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

Index: linux-2.6/kernel/trace/trace.c
===================================================================
--- linux-2.6.orig/kernel/trace/trace.c
+++ linux-2.6/kernel/trace/trace.c
@@ -1301,6 +1301,13 @@ lat_print_timestamp(struct trace_seq *s,
 
 static const char state_to_char[] = TASK_STATE_TO_CHAR_STR;
 
+static int task_state_char(unsigned long state)
+{
+	int bit = state ? __ffs(state) + 1 : 0;
+
+	return bit < sizeof(state_to_char) - 1 ? state_to_char[bit] : '?';
+}
+
 /*
  * The message is supposed to contain an ending newline.
  * If the printing stops prematurely, try to add a newline of our own.
@@ -1395,12 +1402,8 @@ print_lat_fmt(struct trace_iterator *ite
 
 		trace_assign_type(field, entry);
 
-		T = field->next_state < sizeof(state_to_char) ?
-			state_to_char[field->next_state] : 'X';
-
-		state = field->prev_state ?
-			__ffs(field->prev_state) + 1 : 0;
-		S = state < sizeof(state_to_char) - 1 ? state_to_char[state] : 'X';
+		T = task_state_char(field->next_state);
+		S = task_state_char(field->prev_state);
 		comm = trace_find_cmdline(field->next_pid);
 		trace_seq_printf(s, " %5d:%3d:%c %s [%03d] %5d:%3d:%c %s\n",
 				 field->prev_pid,
@@ -1518,10 +1521,8 @@ static enum print_line_t print_trace_fmt
 
 		trace_assign_type(field, entry);
 
-		S = field->prev_state < sizeof(state_to_char) ?
-			state_to_char[field->prev_state] : 'X';
-		T = field->next_state < sizeof(state_to_char) ?
-			state_to_char[field->next_state] : 'X';
+		T = task_state_char(field->next_state);
+		S = task_state_char(field->prev_state);
 		ret = trace_seq_printf(s, " %5d:%3d:%c %s [%03d] %5d:%3d:%c\n",
 				       field->prev_pid,
 				       field->prev_prio,
@@ -1620,12 +1621,9 @@ static enum print_line_t print_raw_fmt(s
 
 		trace_assign_type(field, entry);
 
-		S = field->prev_state < sizeof(state_to_char) ?
-			state_to_char[field->prev_state] : 'X';
-		T = field->next_state < sizeof(state_to_char) ?
-			state_to_char[field->next_state] : 'X';
-		if (entry->type == TRACE_WAKE)
-			S = '+';
+		T = task_state_char(field->next_state);
+		S = entry->type == TRACE_WAKE ? '+' :
+			task_state_char(field->prev_state);
 		ret = trace_seq_printf(s, "%d %d %c %d %d %d %c\n",
 				       field->prev_pid,
 				       field->prev_prio,
@@ -1711,12 +1709,9 @@ static enum print_line_t print_hex_fmt(s
 
 		trace_assign_type(field, entry);
 
-		S = field->prev_state < sizeof(state_to_char) ?
-			state_to_char[field->prev_state] : 'X';
-		T = field->next_state < sizeof(state_to_char) ?
-			state_to_char[field->next_state] : 'X';
-		if (entry->type == TRACE_WAKE)
-			S = '+';
+		T = task_state_char(field->next_state);
+		S = entry->type == TRACE_WAKE ? '+' :
+			task_state_char(field->prev_state);
 		SEQ_PUT_HEX_FIELD_RET(s, field->prev_pid);
 		SEQ_PUT_HEX_FIELD_RET(s, field->prev_prio);
 		SEQ_PUT_HEX_FIELD_RET(s, S);

Re: trace: fix task state printout

[Steven Rostedt]

On Wed, 17 Dec 2008, Thomas Gleixner wrote:

> Impact: tracer task state decoding is wrong, size check is buggy

The Imact line seems more of a subject line. The real Impact line should 
be something like:

Impact: fix wrong decoding of task state int tracer

Keyword is 'fix'

> 
> The tracing code has interesting varieties of printing out task state.
> Unfortunalely only one of the instances is correct as it copies the
> code from sched.c:sched_show_task(). The others are plain wrong as
> they treatthe bitfield as an integer offset into the character
> array. Also the size check of the character array is wrong as it
> includes the trailing \0.
> 
> Use a common state decoder inline which does the Right Thing.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

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


Ingo, this is a low risk fix that probably should be pushed towards Linus 
for inclusion into 2.6.28.

-- Steve

Re: trace: fix task state printout

[Ingo Molnar]

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

> 
> On Wed, 17 Dec 2008, Thomas Gleixner wrote:
> 
> > Impact: tracer task state decoding is wrong, size check is buggy
> 
> The Imact line seems more of a subject line. The real Impact line should 
> be something like:
> 
> Impact: fix wrong decoding of task state int tracer
> 
> Keyword is 'fix'
> 
> > 
> > The tracing code has interesting varieties of printing out task state. 
> > Unfortunalely only one of the instances is correct as it copies the 
> > code from sched.c:sched_show_task(). The others are plain wrong as 
> > they treatthe bitfield as an integer offset into the character array. 
> > Also the size check of the character array is wrong as it includes the 
> > trailing \0.
> > 
> > Use a common state decoder inline which does the Right Thing.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 
> Acked-by: Steven Rostedt <srostedt@redhat.com>
> 
> Ingo, this is a low risk fix that probably should be pushed towards 
> Linus for inclusion into 2.6.28.

ok, we can do that after a few days of testing.

Btw., note that when capturing traces via trace_pipe, i still see frequent 
mistakes of PIDs being matched up to the wrong 'comm' string

 loop-1sec-2895 ...
      Xorg-2895 ...

the Xorg-2895 is bogus. I have been seeing these for a long time and 
thought we fixed it, but i noticed them yesterday once more, while i was 
tracing some code. Any ideas about what's wrong there?

	Ingo