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

[Carsten Emde]

On 05/17/2010 06:54 PM, Peter Zijlstra wrote:
> On Mon, 2010-05-17 at 17:21 +0200, Carsten Emde wrote:
>
> > > 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
>
> We could manually add:
>
> #define EXIT_ZOMBIE TASK_ZOMBIE
> #define EXIT_DEAD TASK_DEAD
>
> But those two __TASK ones are unfortunate indeed.
>
> > 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,
>
> #define TASK_STATE(tstate, tstate_c, tstate_s) \
> 	{ __TASK_##tstate, tstate_c },
> #include<linux/task_state.h>
> #undef TASK_STATE
>
> Should get you mostly there I guess, trick would be making the user deal
> with { 0, "R" }
>
> > 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?
>
> Dunno, I guess we can do with your version, just wanted to mention this
> method.
Yes, thanks, your method is great - much better than using intermediate
scripts and similar. However, this approach works best, if you start
with a new design and you are free to arrange everything to build on
it. At least, we should have this approach in mind when making upgrade
changes in the related code - maybe, your method will fit better one
day.

	Carsten.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/