Re: [PATCH v2 2/2] sched/tracing: Add TASK_RTLOCK_WAIT to TASK_REPORT

[Valentin Schneider <valentin.schneider@arm.com>]

On 17/01/22 13:12, Eric W. Biederman wrote:
> Valentin Schneider <valentin.schneider@arm.com> writes:
>> --- a/fs/proc/array.c
>> +++ b/fs/proc/array.c
>> @@ -128,9 +128,10 @@ static const char * const task_state_array[] = {
>>  	"X (dead)",		/* 0x10 */
>>  	"Z (zombie)",		/* 0x20 */
>>  	"P (parked)",		/* 0x40 */
>> +	"L (rt-locked)",        /* 0x80 */
>>  
>>  	/* states beyond TASK_REPORT: */
>> -	"I (idle)",		/* 0x80 */
>> +	"I (idle)",		/* 0x100 */
>>  };
>
> I think this is at least possibly an ABI break.  I have a vague memory
> that userspace is not ready being reported new task states.  Which is
> why we encode some of our states the way we do.
>
> Maybe it was just someone being very conservative.
>
> Still if you are going to add new states to userspace and risk breaking
> them can you do some basic analysis and report what ps and similar
> programs do.
>
> Simply changing userspace without even mentioning that you are changing
> the userspace output of proc looks dangerous indeed.
>

Yeah, you're right.

> Looking in the history commit 74e37200de8e ("proc: cleanup/simplify
> get_task_state/task_state_array") seems to best document the concern
> that userspace does not know how to handle new states.
>

Thanks for the sha1 and for digging around. Now, I read
74e37200de8e ("proc: cleanup/simplify get_task_state/task_state_array")
as "get_task_state() isn't clear vs what value is actually exposed to
userspace" rather than "get_task_state() could expose things userspace
doesn't know what to do with".

> The fact we have had a parked state for quite a few years despite that
> concern seems to argue it is possible to extend the states.  Or perhaps
> it just argues that parked states are rare enough it does not matter.
>
> It is definitely the case that the ps manpage documents the possible
> states and as such they could be a part of anyone's shell scripts.
>

06eb61844d84 ("sched/debug: Add explicit TASK_IDLE printing") for instance
seems to suggest extending the states OK, but you're right that this then
requires updating ps' manpage.

Alternatively, TASK_RTLOCK_WAIT could be masqueraded as
TASK_(UN)INTERRUPTIBLE when reported to userspace - it is actually somewhat
similar, unlike TASK_IDLE vs TASK_UNINTERRUPTIBLE for instance. The
handling in get_task_state() will be fugly, but it might be preferable over
exposing a detail userspace might not need to be made aware of?

> From the ps man page:
>>        Here are the different values that the s, stat and state output
>>        specifiers (header "STAT" or "S") will display to describe the
>>        state of a process:
>> 
>>                D    uninterruptible sleep (usually IO)
>>                I    Idle kernel thread
>>                R    running or runnable (on run queue)
>>                S    interruptible sleep (waiting for an event to complete)
>>                T    stopped by job control signal
>>                t    stopped by debugger during the tracing
>>                W    paging (not valid since the 2.6.xx kernel)
>>                X    dead (should never be seen)
>>                Z    defunct ("zombie") process, terminated but not reaped by its parent
>> 
>
> So it looks like a change that adds to the number of states in the
> kernel should update the ps man page as well.
>
> Eric