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

Valentin Schneider [off-list ref] writes:

On 17/01/22 13:12, Eric W. Biederman wrote:

quoted

Valentin Schneider [off-list ref] writes:

quoted

--- 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.

quoted

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".

There is also commit abd50b39e783 ("wait: introduce EXIT_TRACE to avoid
the racy EXIT_DEAD->EXIT_ZOMBIE transition").

Which I think is more of what I was remembering.

quoted

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?

Right.

Frequently I have seen people do a cost/benefit analysis.

If the benefit is enough, and tracking down the userspace programs that
need to be verified to work with the change is inexpensive enough the
change is made.  Always keeping in mind that if something was missed and
the change causes a regression the change will need to be reverted.

If there is little benefit or the cost to track down userspace is great
enough the work is put in to hide the change from userspace.  Just
because it is too much trouble to expose it to userspace.

I honestly don't have any kind of sense about how hard it is to verify
that a userspace regression won't result from a change like this.  I
just know that the question needs to be asked.

Eric