3.19 scheduler might_sleep triggered from module loading during boot.

3.19 scheduler might_sleep triggered from module loading during boot.

[Dave Jones]

I don't recall seeing this one until now..

	Dave

[    3.559609] WARNING: CPU: 0 PID: 181 at kernel/sched/core.c:7326 __might_sleep+0xd0/0xf0()
[    3.559701] do not call blocking ops when !TASK_RUNNING; state=1 set at [<ffffffff950e1db3>] prepare_to_wait_event+0x93/0x1f0
[    3.559800] CPU: 0 PID: 181 Comm: modprobe Not tainted 3.19.0+ #5
[    3.559932]  ffffffff950b75b0 ffff88020da3fba8 ffffffff9573726a ffffffff95101ec3
[    3.560174]  ffff88020da3fbf8 ffff88020da3fbe8 ffffffff950742f2 ffffffff964c1fa0
[    3.561927]  ffffffff95ad86e8 000000000000026d 0000000000000000 ffff88020da9aac0
[    3.562159] Call Trace:
[    3.562218]  [<ffffffff950b75b0>] ? __might_sleep+0xd0/0xf0
[    3.562288]  [<ffffffff9573726a>] dump_stack+0x84/0xb9
[    3.562359]  [<ffffffff95101ec3>] ? console_unlock+0x373/0x690
[    3.562431]  [<ffffffff950742f2>] warn_slowpath_common+0xd2/0x140
[    3.562495]  [<ffffffff95074457>] warn_slowpath_fmt+0x57/0x70
[    3.562569]  [<ffffffff950e1db3>] ? prepare_to_wait_event+0x93/0x1f0
[    3.562641]  [<ffffffff950e1db3>] ? prepare_to_wait_event+0x93/0x1f0
[    3.562728]  [<ffffffff950b75b0>] __might_sleep+0xd0/0xf0
[    3.562799]  [<ffffffff9573b9c1>] mutex_lock_nested+0x41/0x760
[    3.562870]  [<ffffffff950cb16d>] ? local_clock+0x4d/0x60
[    3.562933]  [<ffffffff957439ef>] ? _raw_spin_unlock_irqrestore+0x5f/0xc0
[    3.563009]  [<ffffffff9514e888>] resolve_symbol.isra.35+0x48/0x150
[    3.563081]  [<ffffffff951521b4>] load_module+0x1134/0x20b0
[    3.563149]  [<ffffffff953ed85e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[    3.563221]  [<ffffffff950e1570>] ? wait_woken+0x110/0x110
[    3.563290]  [<ffffffff95744ba0>] ? retint_restore_args+0xe/0xe
[    3.563359]  [<ffffffff95153262>] SyS_init_module+0x132/0x190
[    3.563433]  [<ffffffff95743f52>] system_call_fastpath+0x12/0x17

[PATCH] module: Annotate nested sleep in resolve_symbol()

[Peter Zijlstra]

Because wait_event() loops are safe vs spurious wakeups we can allow the
occasional sleep -- which ends up being very similar.

Cc: Rusty Russell <rusty@rustcorp.com.au>
Reported-by: Dave Jones <davej@codemonkey.org.uk>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/module.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/module.c b/kernel/module.c
index d856e96a3cce..11d3bc18157e 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1225,6 +1225,12 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
 	const unsigned long *crc;
 	int err;
 
+	/*
+	 * The module_mutex should not be a heavily contended lock;
+	 * if we get the occasional sleep here, we'll go an extra iteration
+	 * in the wait_event_interruptible(), which is harmless.
+	 */
+	sched_annotate_sleep();
 	mutex_lock(&module_mutex);
 	sym = find_symbol(name, &owner, &crc,
 			  !(mod->taints & (1 << TAINT_PROPRIETARY_MODULE)), true);

Re: [PATCH] module: Annotate nested sleep in resolve_symbol()

[Peter Zijlstra]

On which, we should probably do this.

---
Subject: module: Replace over-engineered nested sleep 

Since the introduction of the nested sleep warning; we've established
that the occasional sleep inside a wait_event() is fine.

wait_event() loops are invariant wrt. spurious wakeups, and the
occasional sleep has a similar effect on them. As long as its occasional
its harmless.

Therefore replace the 'correct' but verbose wait_woken() thing with
a simple annotation to shut up the warning.

Cc: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/module.c | 36 ++++++++----------------------------
 1 file changed, 8 insertions(+), 28 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index d856e96a3cce..98231bf59b6e 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2978,6 +2978,12 @@ static bool finished_loading(const char *name)
 	struct module *mod;
 	bool ret;
 
+	/*
+	 * The module_mutex should not be a heavily contended lock;
+	 * if we get the occasional sleep here, we'll go an extra iteration
+	 * in the wait_event_interruptible(), which is harmless.
+	 */
+	sched_annotate_sleep();
 	mutex_lock(&module_mutex);
 	mod = find_module_all(name, strlen(name), true);
 	ret = !mod || mod->state == MODULE_STATE_LIVE
@@ -3120,32 +3126,6 @@ static int may_init_module(void)
 }
 
 /*
- * Can't use wait_event_interruptible() because our condition
- * 'finished_loading()' contains a blocking primitive itself (mutex_lock).
- */
-static int wait_finished_loading(struct module *mod)
-{
-	DEFINE_WAIT_FUNC(wait, woken_wake_function);
-	int ret = 0;
-
-	add_wait_queue(&module_wq, &wait);
-	for (;;) {
-		if (finished_loading(mod->name))
-			break;
-
-		if (signal_pending(current)) {
-			ret = -ERESTARTSYS;
-			break;
-		}
-
-		wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
-	}
-	remove_wait_queue(&module_wq, &wait);
-
-	return ret;
-}
-
-/*
  * We try to place it in the list now to make sure it's unique before
  * we dedicate too many resources.  In particular, temporary percpu
  * memory exhaustion.
@@ -3165,8 +3145,8 @@ static int add_unformed_module(struct module *mod)
 		    || old->state == MODULE_STATE_UNFORMED) {
 			/* Wait in case it fails to load. */
 			mutex_unlock(&module_mutex);
-
-			err = wait_finished_loading(mod);
+			err = wait_event_interruptible(module_wq,
+					       finished_loading(mod->name));
 			if (err)
 				goto out_unlocked;
 			goto again;

Re: [PATCH] module: Annotate nested sleep in resolve_symbol()

[Dave Jones]

On Tue, Feb 10, 2015 at 06:12:55PM +0100, Peter Zijlstra wrote:
 > On which, we should probably do this.
 > 
 > ---
 > Subject: module: Replace over-engineered nested sleep 
 > 
 > Since the introduction of the nested sleep warning; we've established
 > that the occasional sleep inside a wait_event() is fine.
 > 
 > wait_event() loops are invariant wrt. spurious wakeups, and the
 > occasional sleep has a similar effect on them. As long as its occasional
 > its harmless.
 > 
 > Therefore replace the 'correct' but verbose wait_woken() thing with
 > a simple annotation to shut up the warning.
 > 
 > Cc: Rusty Russell <rusty@rustcorp.com.au>
 > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Seems to suppress the warning, and modules still work.

Tested-by: Dave Jones <davej@codemonkey.org.uk>

Re: [PATCH] module: Annotate nested sleep in resolve_symbol()

[Rusty Russell]

Dave Jones <davej@codemonkey.org.uk> writes:
> On Tue, Feb 10, 2015 at 06:12:55PM +0100, Peter Zijlstra wrote:
>  > On which, we should probably do this.
>  > 
>  > ---
>  > Subject: module: Replace over-engineered nested sleep 
>  > 
>  > Since the introduction of the nested sleep warning; we've established
>  > that the occasional sleep inside a wait_event() is fine.
>  > 
>  > wait_event() loops are invariant wrt. spurious wakeups, and the
>  > occasional sleep has a similar effect on them. As long as its occasional
>  > its harmless.
>  > 
>  > Therefore replace the 'correct' but verbose wait_woken() thing with
>  > a simple annotation to shut up the warning.
>  > 
>  > Cc: Rusty Russell <rusty@rustcorp.com.au>
>  > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>
> Seems to suppress the warning, and modules still work.
>
> Tested-by: Dave Jones <davej@codemonkey.org.uk>

OK, applied.

Thanks!
Rusty.