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

[Carsten Emde <C.Emde@osadl.org>]

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.