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
Because wait_event() loops are safe vs spurious wakeups we can allow the
occasional sleep -- which ends up being very similar.
Cc: Rusty Russell <[email protected]>
Reported-by: Dave Jones <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
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);
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 <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
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;
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 <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Seems to suppress the warning, and modules still work.
Tested-by: Dave Jones <[email protected]>
Dave Jones <[email protected]> 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 <[email protected]>
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
>
> Seems to suppress the warning, and modules still work.
>
> Tested-by: Dave Jones <[email protected]>
OK, applied.
Thanks!
Rusty.