Re: [PATCH 1/1] fix-task-states-in-sched_switch-event.patch

From: Carsten Emde
Date: Mon May 17 2010 - 11:24:55 EST


Hi Peter,

#define TASK_RUNNING 0
+#define TASK_STATE_0 "R"
+#define TASK_STATE_NAME_0 "running"
+
#define TASK_INTERRUPTIBLE 1
+#define TASK_STATE_1 "S"
+#define TASK_STATE_NAME_1 "sleeping"
+
#define TASK_UNINTERRUPTIBLE 2
+#define TASK_STATE_2 "D"
+#define TASK_STATE_NAME_2 "disk sleep"
+
#define __TASK_STOPPED 4
+#define TASK_STATE_4 "T"
+#define TASK_STATE_NAME_4 "stopped"
+
#define __TASK_TRACED 8
+#define TASK_STATE_8 "t"
+#define TASK_STATE_NAME_8 "tracing stop"
+
/* in tsk->exit_state */
#define EXIT_ZOMBIE 16
+#define TASK_STATE_16 "Z"
+#define TASK_STATE_NAME_16 "zombie"
+
#define EXIT_DEAD 32
+#define TASK_STATE_32 "X"
+#define TASK_STATE_NAME_32 "dead"
+
/* in tsk->state again */
#define TASK_DEAD 64
+#define TASK_STATE_64 "x"
+#define TASK_STATE_NAME_64 "dead"
+
#define TASK_WAKEKILL 128
+#define TASK_STATE_128 "K"
+#define TASK_STATE_NAME_128 "wakekill"
+
#define TASK_WAKING 256
+#define TASK_STATE_256 "W"
+#define TASK_STATE_NAME_256 "waking"

Since we all love vile macro magic, is the below any better?

include/linux/task_states.h

TASK_STATE(RUNNING, "R", "running")
TASK_STATE(INTERRUPTIBLE, "S", "sleeping")
...
Well, yes, this looks very nice and is perfectly readable and
maintainable.

enum {
#define TASK_STATE(tstate, tstate_c, tstate_s) __TASK_##tstate,
#include<linux/task_states.h>
#undef TASK_STATE
};

enum {
#define TASK_STATE(tstate, tstate_c, tstate_s) \
TASK_##tstate = 1<< __TASK_##tstate,
#include<linux/task_states.h>
#undef TASK_STATE
};

const char *task_state_to_char =
#define TASK_STATE(tstate, tstate_c, tstate_s) tstate_c
#include<linux/task_states.h>
#undef TASK_STATE
;

const char *task_state_to_string[] = {
#define TASK_STATE(tstate, tstate_c, tstate_s) tstate_s,
#include<linux/task_states.h>
#undef TASK_STATE
};
I find this section less convincing (although certainly
indistinguishable from magic).

In addition, we need to take care of the various state name prefixes
TASK, __TASK and EXIT and name clashes:
TASK_RUNNING
TASK_INTERRUPTIBLE
TASK_UNINTERRUPTIBLE
__TASK_STOPPED
__TASK_TRACED
EXIT_ZOMBIE
EXIT_DEAD
TASK_DEAD
TASK_WAKEKILL
TASK_WAKING

And we still need to maintain the defines in include/trace/events/
sched.h:
{ 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,

If we could use a general approach for all states, I would immediately
go for your proposal. But since we anyway need to define the states
individually, I would vote for the current version of the patch.

Or would you prefer to simply apply a minimal fix to correct the
erroneous output of the sched_switch event and to leave the rest as an
exercise for the future?


Carsten. ---
include/trace/events/sched.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

Index: head/include/trace/events/sched.h
===================================================================
--- head.orig/include/trace/events/sched.h
+++ head/include/trace/events/sched.h
@@ -151,7 +151,7 @@ TRACE_EVENT(sched_switch,
__print_flags(__entry->prev_state, "|",
{ 1, "S"} , { 2, "D" }, { 4, "T" }, { 8, "t" },
{ 16, "Z" }, { 32, "X" }, { 64, "x" },
- { 128, "W" }) : "R",
+ { 128, "K" }, { 256, "W" }) : "R",
__entry->next_comm, __entry->next_pid, __entry->next_prio)
);