Re: Worker threads in D state since c5a94a618e7ac86 (workqueue: Use TASK_IDLE)

From: Markus Trippelsdorf
Date: Fri Sep 22 2017 - 05:35:42 EST


On 2017.09.21 at 16:41 +0200, Markus Trippelsdorf wrote:
> On 2017.09.21 at 14:30 +0200, Peter Zijlstra wrote:
> > On Thu, Sep 21, 2017 at 01:08:42PM +0200, Markus Trippelsdorf wrote:
> > > On 2017.09.11 at 16:21 +0200, Markus Trippelsdorf wrote:
> > > > On 2017.09.11 at 06:11 -0700, Tejun Heo wrote:
> > > > > Hello,
> > > > >
> > > > > On Sun, Sep 10, 2017 at 09:36:53AM +0200, Markus Trippelsdorf wrote:
> > > > > > Since:
> > > > > >
> > > > > > commit c5a94a618e7ac86b20f53d947f68d7cee6a4c6bc
> > > > > > Author: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > > > > > Date: Wed Aug 23 13:58:44 2017 +0200
> > > > > >
> > > > > > workqueue: Use TASK_IDLE
> > > > > >
> > > > > >
> > > > > > all worker threads are in D state. They all show up when using "magic
> > > > > > SysRq w". In htop they all have big fat red 'D' in the state column.
> > > > > > Is this really desirable?
> > > > > >
> > > > > > I have attached the output of "ps aux" after boot and the SysRq-w
> > > > > > output.
> > > > >
> > > > > Hmm.... looks like we better revert until we figure out how this
> > > > > should get presented in debugging facilities / to userspace. Peter?
> > > >
> > > > BTW rcu recently introduced the same issue:
> > > >
> > > > commit d5374226c3e444239e063f005dfb59cae4390db4
> > > > Author: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
> > > > Date: Tue Jun 20 14:45:47 2017 -0700
> > > >
> > > > rcu: Use idle versions of swait to make idle-hack clear
> > >
> > > Ping?
> > > You may call it a cosmetic issue, but still it makes debugging much
> > > harder. Finding "real" blocked tasks is now like finding a needle in a
> > > haystack.
> >
> > Sorry, was out traveling. We can easily fix sysrq-w, not sure we can do
> > much about htop (I've never seen it).
> >
> > I suppose we can try and make the state character not be D, is that
> > really worth the trouble, or would it simply break htop if we were to
> > return a new character?
>
> It seems to work. Simply returning "I (idle)" from get_task_state() in
> fs/proc/array.c when the state is TASK_IDLE does the trick.
> I've tested top, htop and ps.

So perhaps something like this:

diff --git a/fs/proc/array.c b/fs/proc/array.c
index 525157ca25cb..741687be3b0d 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -142,6 +142,9 @@ static inline const char *get_task_state(struct task_struct *tsk)

BUILD_BUG_ON(1 + ilog2(TASK_REPORT) != ARRAY_SIZE(task_state_array)-1);

+ if (tsk->state == TASK_IDLE)
+ return "I (idle)";
+
return task_state_array[fls(state)];
}

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 18a6966567da..83681990d3f9 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5188,7 +5188,8 @@ void show_state_filter(unsigned long state_filter)
*/
touch_nmi_watchdog();
touch_all_softlockup_watchdogs();
- if (!state_filter || (p->state & state_filter))
+ if (!state_filter ||
+ (!(p->state == TASK_IDLE) && p->state & state_filter))
sched_show_task(p);
}

--
Markus