Re: [PATCH] kthread: Use TASK_IDLE state for newly created kernel threads

[Greg Kroah-Hartman]

On Thu, May 21, 2020 at 03:21:25PM +0530, Pavan Kondeti wrote:
> On Thu, May 21, 2020 at 07:56:39AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, May 21, 2020 at 07:05:44AM +0530, Pavan Kondeti wrote:
> > > On Wed, May 20, 2020 at 08:18:58PM +0200, Greg Kroah-Hartman wrote:
> > > > On Wed, May 20, 2020 at 05:25:09PM +0530, Pavankumar Kondeti wrote:
> > > > > When kernel threads are created for later use, they will be in
> > > > > TASK_UNINTERRUPTIBLE state until they are woken up. This results
> > > > > in increased loadavg and false hung task reports. To fix this,
> > > > > use TASK_IDLE state instead of TASK_UNINTERRUPTIBLE when
> > > > > a kernel thread schedules out for the first time.
> > > > > 
> > > > > Signed-off-by: Pavankumar Kondeti <pkondeti@xxxxxxxxxxxxxx>
> > > > > ---
> > > > >  kernel/kthread.c | 6 +++---
> > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/kernel/kthread.c b/kernel/kthread.c
> > > > > index bfbfa48..b74ed8e 100644
> > > > > --- a/kernel/kthread.c
> > > > > +++ b/kernel/kthread.c
> > > > > @@ -250,7 +250,7 @@ static int kthread(void *_create)
> > > > >  	current->vfork_done = &self->exited;
> > > > >  
> > > > >  	/* OK, tell user we're spawned, wait for stop or wakeup */
> > > > > -	__set_current_state(TASK_UNINTERRUPTIBLE);
> > > > > +	__set_current_state(TASK_IDLE);
> > > > >  	create->result = current;
> > > > >  	/*
> > > > >  	 * Thread is going to call schedule(), do not preempt it,
> > > > > @@ -428,7 +428,7 @@ static void __kthread_bind(struct task_struct *p, unsigned int cpu, long state)
> > > > >  
> > > > >  void kthread_bind_mask(struct task_struct *p, const struct cpumask *mask)
> > > > >  {
> > > > > -	__kthread_bind_mask(p, mask, TASK_UNINTERRUPTIBLE);
> > > > > +	__kthread_bind_mask(p, mask, TASK_IDLE);
> > > > >  }
> > > > >  
> > > > >  /**
> > > > > @@ -442,7 +442,7 @@ void kthread_bind_mask(struct task_struct *p, const struct cpumask *mask)
> > > > >   */
> > > > >  void kthread_bind(struct task_struct *p, unsigned int cpu)
> > > > >  {
> > > > > -	__kthread_bind(p, cpu, TASK_UNINTERRUPTIBLE);
> > > > > +	__kthread_bind(p, cpu, TASK_IDLE);
> > > > >  }
> > > > >  EXPORT_SYMBOL(kthread_bind);
> > > > 
> > > > It's as if people never read mailing lists:
> > > > 	https://lore.kernel.org/r/DM6PR11MB3531D3B164357B2DC476102DDFC90@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> > > > 
> > > > Given that this is an identical resend of the previous patch, why are
> > > > you doing so, and what has changed since that original rejection?
> > > > 
> > > I did not know that it is attempted before. Thanks for pointing to the
> > > previous discussion. 
> > > 
> > > We have seen hung task reports from customers and it is due to a downstream
> > > change which create bunch of kernel threads for later use.
> > 
> > Do you have a pointer to that specific change?
> > 
> Here it is
> https://source.codeaurora.org/quic/la/kernel/msm-4.19/commit/drivers/staging/android/ion/ion_system_heap.c?h=LA.UM.8.12.r1-11300-sm8250.0&id=0734b477f1e77cb9f91f5e5c0d7742d3113f2cd3

Ick, ion system heaps :(

Anyway, why not just use a workqueue for this, why a full kernel thread?
What does that help with?  It also might fix those crazy hacks you have
in there for "cpu affinity" as all of that would be taken care of for
you automatically with a workqueue, right?

good luck!

greg k-h