[PATCH] RFC: siox: don't create a thread without starting it

[PATCH] RFC: siox: don't create a thread without starting it

[Uwe Kleine-König]

On a machine with a siox device I see:

	[  241.130465] INFO: task siox-0:626 blocked for more than 120 seconds.
	[  241.136996]       Not tainted 4.17.0-20180520-1-g6248d1b64190-dirty #8
	[  241.146831] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
	[  241.154868] siox-0          D    0   626      2 0x00000000
	[  241.163241] [<c05f8430>] (__schedule) from [<c05f88ec>] (schedule+0x58/0xc8)
	[  241.172142] [<c05f88ec>] (schedule) from [<c0039098>] (kthread+0xc4/0x140)
	[  241.180899] [<c0039098>] (kthread) from [<c00090e0>] (ret_from_fork+0x14/0x34)
	[  241.188315] Exception stack(0xc72a7fb0 to 0xc72a7ff8)
	[  241.196327] 7fa0:                                     00000000 00000000 00000000 00000000
	[  241.204721] 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
	[  241.215861] 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000

when I just boot without any other siox-related action. So the kthread (created
in drivers/siox/siox-core.c:siox_master_register()) is never started.

While you could argue that there is little reason to not start the
thread there also is little reason to actually do it.

peterz in #kernelnewbies said "[...] kernel/kthread.c:kthread() should
really be using __set_current_state(TASK_IDLE), I suppose". This however
seems to interfere with problems fixed in a076e4bca2fd ("freezer: fix
kthread_create vs freezer theoretical race").

So I wonder where the real problem is and how it can be fixed.

Explicitly-not-signed-off-for-discussion-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/siox/siox-core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/siox/siox-core.c b/drivers/siox/siox-core.c
index 16590dfaafa4..cef307c0399c 100644
--- a/drivers/siox/siox-core.c
+++ b/drivers/siox/siox-core.c
@@ -715,17 +715,17 @@ int siox_master_register(struct siox_master *smaster)
 
 	dev_set_name(&smaster->dev, "siox-%d", smaster->busno);
 
+	mutex_init(&smaster->lock);
+	INIT_LIST_HEAD(&smaster->devices);
+
 	smaster->last_poll = jiffies;
-	smaster->poll_thread = kthread_create(siox_poll_thread, smaster,
-					      "siox-%d", smaster->busno);
+	smaster->poll_thread = kthread_run(siox_poll_thread, smaster,
+					   "siox-%d", smaster->busno);
 	if (IS_ERR(smaster->poll_thread)) {
 		smaster->active = 0;
 		return PTR_ERR(smaster->poll_thread);
 	}
 
-	mutex_init(&smaster->lock);
-	INIT_LIST_HEAD(&smaster->devices);
-
 	ret = device_add(&smaster->dev);
 	if (ret)
 		kthread_stop(smaster->poll_thread);
-- 
2.17.1

Re: [PATCH] RFC: siox: don't create a thread without starting it

[Peter Zijlstra]

On Mon, Jun 25, 2018 at 12:20:56PM +0200, Uwe Kleine-König wrote:
> when I just boot without any other siox-related action. So the kthread (created
> in drivers/siox/siox-core.c:siox_master_register()) is never started.
> 
> While you could argue that there is little reason to not start the
> thread there also is little reason to actually do it.

Well, you really _should_ wake up the thread. That first wakeup really
is part of the whole 'create/setup' kthread pattern.

> peterz in #kernelnewbies said "[...] kernel/kthread.c:kthread() should
> really be using __set_current_state(TASK_IDLE), I suppose". This however
> seems to interfere with problems fixed in a076e4bca2fd ("freezer: fix
> kthread_create vs freezer theoretical race").

I don't think so, that patch has an issue with INTERRUPTIBLE, but IDLE
very much doesn't allow signals like INTERRUPTIBLE does.

> So I wonder where the real problem is and how it can be fixed.

Without the first wakeup, the kthread will not run the provided function
and we can therefore argue the creation is incomplete. I really feel you
should just wake the thing up to land in your own wait-condition-loop.

That said, irrespective of the whole UNINTERRUPTIBLE/IDLE thing, I find
this construct fairly fragile. We rely on not getting any spurious
wakeups without a 'special' state. The only reason this doesn't normally
happen is because it's a new task, but since it is already hashed, it
might well be possible to trick someone into sending a wakeup.

Re: [PATCH] RFC: siox: don't create a thread without starting it

[Uwe Kleine-König]

Hello Peter,

On Mon, Jun 25, 2018 at 02:51:05PM +0200, Peter Zijlstra wrote:
> On Mon, Jun 25, 2018 at 12:20:56PM +0200, Uwe Kleine-König wrote:
> > when I just boot without any other siox-related action. So the kthread (created
> > in drivers/siox/siox-core.c:siox_master_register()) is never started.
> > 
> > While you could argue that there is little reason to not start the
> > thread there also is little reason to actually do it.
> 
> Well, you really _should_ wake up the thread. That first wakeup really
> is part of the whole 'create/setup' kthread pattern.

ok

> > peterz in #kernelnewbies said "[...] kernel/kthread.c:kthread() should
> > really be using __set_current_state(TASK_IDLE), I suppose". This however
> > seems to interfere with problems fixed in a076e4bca2fd ("freezer: fix
> > kthread_create vs freezer theoretical race").
> 
> I don't think so, that patch has an issue with INTERRUPTIBLE, but IDLE
> very much doesn't allow signals like INTERRUPTIBLE does.

I don't think I can provide a good commit log for
s/TASK_UNINTERRUPTIBLE/TASK_IDLE/ in kernel/kthread.c:kthread(). But I
can confirm that this patch makes the warning go away, so if you want to
address this, you can add my Tested-by:.
 
> > So I wonder where the real problem is and how it can be fixed.
> 
> Without the first wakeup, the kthread will not run the provided function
> and we can therefore argue the creation is incomplete. I really feel you
> should just wake the thing up to land in your own wait-condition-loop.
> 
> That said, irrespective of the whole UNINTERRUPTIBLE/IDLE thing, I find
> this construct fairly fragile. We rely on not getting any spurious
> wakeups without a 'special' state.

Well, if the thread is woken up unintentionally nothing happens (apart
from the lock and the list that I moved in my patch that might not be
initialized yet; this should be fixed for sure). It just enters the
thread's own wait-condition-loop. So you could argue that the wakeup
call is just overhead that isn't necessary until the thread is really
needed. (I won't argue, I can accept your opinion and do the wakeup.)

> The only reason this doesn't normally happen is because it's a new
> task, but since it is already hashed, it might well be possible to
> trick someone into sending a wakeup.

Is this an ack for the RFC patch you replied to?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] RFC: siox: don't create a thread without starting it

[Peter Zijlstra]

On Mon, Jun 25, 2018 at 09:21:21PM +0200, Uwe Kleine-König wrote:
> > > So I wonder where the real problem is and how it can be fixed.
> > 
> > Without the first wakeup, the kthread will not run the provided function
> > and we can therefore argue the creation is incomplete. I really feel you
> > should just wake the thing up to land in your own wait-condition-loop.

> Is this an ack for the RFC patch you replied to?

Yes,

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

[PATCH] siox: don't create a thread without starting it

[Uwe Kleine-König]

When a siox master device is registered a kthread is created that is
only started when triggered by userspace. So this thread might be in
TASK_UNINTERRUPTIBLE state for long and trigger a warning

	[  241.130465] INFO: task siox-0:626 blocked for more than 120 seconds.

with the respective debug settings enabled. It might be right to put an
unstarted thread to TASK_IDLE (in kernel/kthread.c:kthread()) instead,
but independant of this discussion it is cleaner for
siox_master_register() to start the thread immediately. The effect is
that it enters its own waiting state and then stays in state TASK_IDLE
which doesn't trigger the above warning.

As siox_poll_thread() uses some variables of the device the
initialisation of these is moved before thread creation.

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

this is the same patch as before with an updated commit log. I added
Peter's ack anyhow assuming my text isn't too bad to make him withdraw
it.

I think this issue isn't critical as the machine works fine otherwise,
so applying it for 4.18-rc isn't necessary.
(OTOH it's an SMP machine so the tests might not have noticed one blocked
CPU, didn't test this.)

Greg, you applied the initial patches creating drivers/siox. I assume
you will continue to apply siox patches and tell if I should search a
different path for these.

Best regards
Uwe

 drivers/siox/siox-core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/siox/siox-core.c b/drivers/siox/siox-core.c
index 16590dfaafa4..cef307c0399c 100644
--- a/drivers/siox/siox-core.c
+++ b/drivers/siox/siox-core.c
@@ -715,17 +715,17 @@ int siox_master_register(struct siox_master *smaster)
 
 	dev_set_name(&smaster->dev, "siox-%d", smaster->busno);
 
+	mutex_init(&smaster->lock);
+	INIT_LIST_HEAD(&smaster->devices);
+
 	smaster->last_poll = jiffies;
-	smaster->poll_thread = kthread_create(siox_poll_thread, smaster,
-					      "siox-%d", smaster->busno);
+	smaster->poll_thread = kthread_run(siox_poll_thread, smaster,
+					   "siox-%d", smaster->busno);
 	if (IS_ERR(smaster->poll_thread)) {
 		smaster->active = 0;
 		return PTR_ERR(smaster->poll_thread);
 	}
 
-	mutex_init(&smaster->lock);
-	INIT_LIST_HEAD(&smaster->devices);
-
 	ret = device_add(&smaster->dev);
 	if (ret)
 		kthread_stop(smaster->poll_thread);
-- 
2.18.0

Re: [PATCH] siox: don't create a thread without starting it

[Uwe Kleine-König]

Hello,

I sent the patch from a different machine than usual and there had an
old email address from Greg in my aliases. I will bounce the original to
the right address, please make sure to correct his address to
@linuxfoundation.org when replying.

Thanks and sorry,
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] siox: don't create a thread without starting it

[Schenk, Gavin]

On Thu, 2018-06-28 at 09:57 +0200, Uwe Kleine-König wrote:
> When a siox master device is registered a kthread is created that is
> only started when triggered by userspace. So this thread might be in
> TASK_UNINTERRUPTIBLE state for long and trigger a warning
> 
> 	[  241.130465] INFO: task siox-0:626 blocked for more than 120 seconds.
> 
> with the respective debug settings enabled. It might be right to put an
> unstarted thread to TASK_IDLE (in kernel/kthread.c:kthread()) instead,
> but independant of this discussion it is cleaner for
> siox_master_register() to start the thread immediately. The effect is
> that it enters its own waiting state and then stays in state TASK_IDLE
> which doesn't trigger the above warning.
> 
> As siox_poll_thread() uses some variables of the device the
> initialisation of these is moved before thread creation.
> 
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
Acked-by: Gavin Schenk <g.schenk@eckelmann.de

I tested that this warning is gone with this patch.

Regards
Gavin Schenk

Re: [PATCH] siox: don't create a thread without starting it

[Uwe Kleine-König]

Hello Greg,

Cc -= Peter Zijlstra
Cc += Stephen Rothwell

On Thu, Jun 28, 2018 at 09:57:42AM +0200, Uwe Kleine-König wrote:
> Greg, you applied the initial patches creating drivers/siox. I assume
> you will continue to apply siox patches and tell if I should search a
> different path for these.

I put the two patches that I'd like to get in during the next merge
window into a branch in my repository:

	https://git.pengutronix.de/git/ukl/linux siox/next

I assume you wouldn't object to add this branch to linux-next?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] siox: don't create a thread without starting it

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1582 bytes --]

Hi Uwe,

On Fri, 29 Jun 2018 09:38:58 +0200 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
>
> On Thu, Jun 28, 2018 at 09:57:42AM +0200, Uwe Kleine-König wrote:
> > Greg, you applied the initial patches creating drivers/siox. I assume
> > you will continue to apply siox patches and tell if I should search a
> > different path for these.  
> 
> I put the two patches that I'd like to get in during the next merge
> window into a branch in my repository:
> 
> 	https://git.pengutronix.de/git/ukl/linux siox/next
> 
> I assume you wouldn't object to add this branch to linux-next?

Added from today.  Is this a continuing effort, or should I just remove
the tree once Greg has merged it?

Thanks for adding your subsystem tree as a participant of linux-next.  As
you may know, this is not a judgement of your code.  The purpose of
linux-next is for integration testing and to lower the impact of
conflicts between subsystems in the next merge window. 

You will need to ensure that the patches/commits in your tree/series have
been:
     * submitted under GPL v2 (or later) and include the Contributor's
        Signed-off-by,
     * posted to the relevant mailing list,
     * reviewed by you (or another maintainer of your subsystem tree),
     * successfully unit tested, and 
     * destined for the current or next Linux merge window.

Basically, this should be just what you would send to Linus (or ask him
to fetch).  It is allowed to be rebased if you deem it necessary.

-- 
Cheers,
Stephen Rothwell 
sfr@canb.auug.org.au

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] siox: don't create a thread without starting it

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1781 bytes --]

Hi all,

Just cc'ing a more recent address for Greg ...

On Mon, 2 Jul 2018 09:34:04 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> Hi Uwe,
> 
> On Fri, 29 Jun 2018 09:38:58 +0200 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> >
> > On Thu, Jun 28, 2018 at 09:57:42AM +0200, Uwe Kleine-König wrote:  
> > > Greg, you applied the initial patches creating drivers/siox. I assume
> > > you will continue to apply siox patches and tell if I should search a
> > > different path for these.    
> > 
> > I put the two patches that I'd like to get in during the next merge
> > window into a branch in my repository:
> > 
> > 	https://git.pengutronix.de/git/ukl/linux siox/next
> > 
> > I assume you wouldn't object to add this branch to linux-next?  
> 
> Added from today.  Is this a continuing effort, or should I just remove
> the tree once Greg has merged it?
> 
> Thanks for adding your subsystem tree as a participant of linux-next.  As
> you may know, this is not a judgement of your code.  The purpose of
> linux-next is for integration testing and to lower the impact of
> conflicts between subsystems in the next merge window. 
> 
> You will need to ensure that the patches/commits in your tree/series have
> been:
>      * submitted under GPL v2 (or later) and include the Contributor's
>         Signed-off-by,
>      * posted to the relevant mailing list,
>      * reviewed by you (or another maintainer of your subsystem tree),
>      * successfully unit tested, and 
>      * destined for the current or next Linux merge window.
> 
> Basically, this should be just what you would send to Linus (or ask him
> to fetch).  It is allowed to be rebased if you deem it necessary.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] siox: don't create a thread without starting it

[Uwe Kleine-König]

Hello,

On Mon, Jul 02, 2018 at 09:34:04AM +1000, Stephen Rothwell wrote:
> On Fri, 29 Jun 2018 09:38:58 +0200 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> >
> > On Thu, Jun 28, 2018 at 09:57:42AM +0200, Uwe Kleine-König wrote:
> > > Greg, you applied the initial patches creating drivers/siox. I assume
> > > you will continue to apply siox patches and tell if I should search a
> > > different path for these.  
> > 
> > I put the two patches that I'd like to get in during the next merge
> > window into a branch in my repository:
> > 
> > 	https://git.pengutronix.de/git/ukl/linux siox/next
> > 
> > I assume you wouldn't object to add this branch to linux-next?
> 
> Added from today. 

Ok, great. 

> Is this a continuing effort, or should I just remove the tree once
> Greg has merged it?

I expect the patch count to be zero for most future releases because
there are probably no users apart from Eckelmann. On the other hand if
we still need a patch it would continue to be good to have the exposure
in next.

So I'd say, if it reduces your burden feel free to drop it and if there
is something relevant later, it seems to be easy enough to readd it
then.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] RFC: siox: don't create a thread without starting it

[Peter Zijlstra]

On Mon, Jun 25, 2018 at 09:21:21PM +0200, Uwe Kleine-König wrote:
> > That said, irrespective of the whole UNINTERRUPTIBLE/IDLE thing, I find
> > this construct fairly fragile. We rely on not getting any spurious
> > wakeups without a 'special' state.
> 
> Well, if the thread is woken up unintentionally nothing happens (apart

In your case sure. But people rely on the task being 'idle/blocked'
after kthread_create() to call things like kthread_bind() on it.

If it were to be woken early because dodgy games, the kthread_bind()
would fail, which in turn can lead to all sorts of 'fun' problems.

That is the whole point of this intermediate blocked state, it allows
you to muck with the kthread before it starts running custom code.
Arguably one could do it differently, but this is what it is.

Re: [PATCH] RFC: siox: don't create a thread without starting it

[Peter Zijlstra]

On Mon, Jun 25, 2018 at 09:21:21PM +0200, Uwe Kleine-König wrote:
> > I don't think so, that patch has an issue with INTERRUPTIBLE, but IDLE
> > very much doesn't allow signals like INTERRUPTIBLE does.
> 
> I don't think I can provide a good commit log for
> s/TASK_UNINTERRUPTIBLE/TASK_IDLE/ in kernel/kthread.c:kthread(). But I
> can confirm that this patch makes the warning go away, so if you want to
> address this, you can add my Tested-by:.

Yeah, it's also a little bit more involved, I'd also have to change all
the kthread_bind() code and audit all kthread users to see if anybody
else actually relies on TASK_UNINTERRUPTIBLE.

So I think I'll leave it as is for now, maybe another day ... :-)

Re: [PATCH] RFC: siox: don't create a thread without starting it

[Uwe Kleine-König]

Hello Peter,

On Tue, Jun 26, 2018 at 09:38:41AM +0200, Peter Zijlstra wrote:
> On Mon, Jun 25, 2018 at 09:21:21PM +0200, Uwe Kleine-König wrote:
> > > I don't think so, that patch has an issue with INTERRUPTIBLE, but IDLE
> > > very much doesn't allow signals like INTERRUPTIBLE does.
> > 
> > I don't think I can provide a good commit log for
> > s/TASK_UNINTERRUPTIBLE/TASK_IDLE/ in kernel/kthread.c:kthread(). But I
> > can confirm that this patch makes the warning go away, so if you want to
> > address this, you can add my Tested-by:.
> 
> Yeah, it's also a little bit more involved, I'd also have to change all
> the kthread_bind() code and audit all kthread users to see if anybody
> else actually relies on TASK_UNINTERRUPTIBLE.
> 
> So I think I'll leave it as is for now, maybe another day ... :-)

Should we add a reminder to kthread() à la:

--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -227,7 +227,14 @@ static int kthread(void *_create)
        init_completion(&self->parked);
        current->vfork_done = &self->exited;
 
-       /* OK, tell user we're spawned, wait for stop or wakeup */
+       /*
+        * OK, tell user we're spawned, wait for stop or wakeup.
+        * It might be possible to use TASK_IDLE here to not trigger the
+        * hung-task-check if the creator doesn't run the thread immediately.
+        * Changing this would however need some research first as this has an
+        * effect on e.g. kthread_bind() or the caller might rely on
+        * TASK_UNINTERRUPTIBLE.
+        */
        __set_current_state(TASK_UNINTERRUPTIBLE);
        create->result = current;
        complete(done);

?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |