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

[Carsten Emde]

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)
 );