[PATCH] Linux 2.6.0-test8 __might_sleep warnings on boot

[PATCH] Linux 2.6.0-test8 __might_sleep warnings on boot

[Roland Dreier]

I'm seeing the following on boot (and I've seen at least one other
similar report on lkml):

    Detected 930.391 MHz processor.
    Console: colour VGA+ 80x25
    Debug: sleeping function called from invalid context at include/asm/semaphore.h:119
    in_atomic():1, irqs_disabled():1
    Call Trace:
     [<c011a67d>] __might_sleep+0x9d/0xb0
     [<c011cdf2>] acquire_console_sem+0x2a/0x48
     [<c011d05d>] register_console+0x109/0x178
     [<c03b8a70>] con_init+0x1e0/0x1ec
     [<c0105000>] _stext+0x0/0x50
     [<c03b8184>] console_init+0x24/0x34
     [<c03aa575>] start_kernel+0xa5/0x17c
     [<c03aa3c4>] unknown_bootoption+0x0/0xdc

    Memory: 515040k/524032k available (1801k kernel code, 8244k reserved, 912k data, 132k init, 0k highmem)
    Debug: sleeping function called from invalid context at mm/slab.c:1857
    in_atomic():1, irqs_disabled():0
    Call Trace:
     [<c011a67d>] __might_sleep+0x9d/0xb0
     [<c01386b3>] kmem_cache_alloc+0x1f/0x54
     [<c0137a0c>] kmem_cache_create+0x6c/0x438
     [<c0105000>] _stext+0x0/0x50
     [<c03b6076>] kmem_cache_init+0xf6/0x23c
     [<c0105000>] _stext+0x0/0x50
     [<c03aa5c0>] start_kernel+0xf0/0x17c

    Calibrating delay loop... 1839.10 BogoMIPS

These were probably always there but are just getting exposed because
of the fix for printing __might_sleep warnings before the jiffies
wrap (which went in between -test7 and -test8).

I can see what's going on -- in init/main.c, we have:

        console_init();
        profile_init();
        local_irq_enable();

        /* ... deletia ... */

        kmem_cache_init();

        /* ... deletia ... */

        init_idle(current, smp_processor_id());

irqs_disabled() is true until the local_irq_enable() and in_atomic()
is true until init_idle().

I guess the fix is something like the patch below (tested and working
in my setup), which just disables __might_sleep warnings until after
init_idle().

(Another alternative might be to add functions like
"i_know_its_safe_to_call_functions_that_might_sleep()" and
"done_with_functions_that_might_sleep()" and add them everywhere
appropriate, but that seems a lot harder)

 - Roland

--- linux-2.6.0-test8/kernel/sched.c~early_might_sleep	Sat Oct 18 11:54:24 2003
+++ linux-2.6.0-test8/kernel/sched.c	Sat Oct 18 12:16:57 2003
@@ -38,6 +38,10 @@
 #include <linux/cpu.h>
 #include <linux/percpu.h>
 
+/* we need to know when init_idle() is done so that we don't print
+   spurious __might_sleep warnings on boot. */
+static int init_idle_done;
+
 #ifdef CONFIG_NUMA
 #define cpu_to_node_mask(cpu) node_to_cpumask(cpu_to_node(cpu))
 #else
@@ -2538,6 +2542,9 @@ void __init init_idle(task_t *idle, int 
 #else
 	idle->thread_info->preempt_count = 0;
 #endif
+
+	/* __might_sleep checks now make sense */
+	init_idle_done = 1;
 }
 
 #ifdef CONFIG_SMP
@@ -2848,7 +2855,7 @@ void __might_sleep(char *file, int line)
 #if defined(in_atomic)
 	static unsigned long prev_jiffy;	/* ratelimiting */
 
-	if (in_atomic() || irqs_disabled()) {
+	if (init_idle_done && (in_atomic() || irqs_disabled())) {
 		if (time_before(jiffies, prev_jiffy + HZ) && prev_jiffy)
 			return;
 		prev_jiffy = jiffies;

Re: [PATCH] Linux 2.6.0-test8 __might_sleep warnings on boot

[Andrew Morton]

Roland Dreier <roland@digitalvampire.org> wrote:
>
>  +	/* __might_sleep checks now make sense */
>  +	init_idle_done = 1;

We already have the `system_running' flag which is suitable for this.

I've been dithering over whether we should use it, mainly because doing
things like downing a semaphore before we've even called sched_init()
(cpufreq) is a happens-to-be-harmless but fairly dumb thing to do.

Yes, we should probably do something like this, but later.

Re: [PATCH] Linux 2.6.0-test8 __might_sleep warnings on boot

[Roland Dreier]

    Andrew> We already have the `system_running' flag which is
    Andrew> suitable for this.

OK, an alternate patch is below (also tested on my system).  I felt
that my first patch, which added the "init_idle_done" flag, was nice
because the flag gets set exactly when might_sleep() becomes valid,
and everything is contained in kernel/sched.c.  On the other hand, I
can see why not adding yet another flag is nice, too.  It's a matter
of taste, I guess.

    Andrew> I've been dithering over whether we should use it, mainly
    Andrew> because doing things like downing a semaphore before we've
    Andrew> even called sched_init() (cpufreq) is a
    Andrew> happens-to-be-harmless but fairly dumb thing to do.

True, but fixing some of the problems will be pretty ugly.  For
example, to fix the warning from kmem_cache_init:

    Debug: sleeping function called from invalid context at mm/slab.c:1857
    in_atomic():1, irqs_disabled():0
    Call Trace:
     [<c0105000>] _stext+0x0/0x30
     [<c011aea8>] __might_sleep+0x88/0xa0
     [<c013e708>] kmem_cache_alloc+0x1d8/0x1e0
     [<c011d531>] printk+0x121/0x150
     [<c0105000>] _stext+0x0/0x30
     [<c013d059>] kmem_cache_create+0x169/0x6a0
     [<c03019c0>] mem_init+0x1a0/0x210
     [<c0105000>] _stext+0x0/0x30
     [<c030469e>] kmem_cache_init+0x11e/0x2c0
     [<c02fa6d8>] start_kernel+0xb8/0x150
     [<c02fa4e0>] unknown_bootoption+0x0/0x110

We would probably have to change lots of stuff in mm/slab.c, including
kmem_cache_create(), since right now it unconditionally does

        /* Get cache's description obj. */
        cachep = (kmem_cache_t *) kmem_cache_alloc(&cache_cache, SLAB_KERNEL);

which will trigger a might_sleep.  Maybe the least bad way to fix that
would be to do something like

        if (system_running)
                cachep = (kmem_cache_t *) kmem_cache_alloc(&cache_cache, SLAB_KERNEL);
        else
                cachep = (kmem_cache_t *) kmem_cache_alloc(&cache_cache, SLAB_ATOMIC);

which strikes me as pretty ugly.  And there would be other changes in slab.c ...

Similarly, for the register_console() warning, I guess we would change
register_console to do

        if (system_running)
                acquire_console_sem();

        /* ... */

        if (system_running)
                release_console_sem();

in kernel/printk.c.  Let me know if you'd like a patch like that, I
can cook it up.

    Andrew> Yes, we should probably do something like this, but later.

I've already seen several more reports of this problem on lkml.  I
definitely think this should be fixed for -test9 and possibly even
-test8-mm1 -- otherwise you're going to continue to get reports of
this.  Nobody likes seeing tracebacks in their kernel messages.  Your
call though, obviously...

 - Roland

--- linux-2.6.0-test8/kernel/sched.c~early_might_sleep	Sat Oct 18 11:54:24 2003
+++ linux-2.6.0-test8/kernel/sched.c	Sat Oct 18 16:44:14 2003
@@ -2847,8 +2847,12 @@ void __might_sleep(char *file, int line)
 {
 #if defined(in_atomic)
 	static unsigned long prev_jiffy;	/* ratelimiting */
+	extern int system_running;
 
-	if (in_atomic() || irqs_disabled()) {
+	/* Don't print warnings until system_running is set.  This avoids
+	   spurious warnings during boot before local_irq_enable() and
+	   init_idle(). */
+	if (system_running && (in_atomic() || irqs_disabled())) {
 		if (time_before(jiffies, prev_jiffy + HZ) && prev_jiffy)
 			return;
 		prev_jiffy = jiffies;

Re: [PATCH] Linux 2.6.0-test8 __might_sleep warnings on boot

[Andrew Morton]

Roland Dreier <roland@digitalvampire.org> wrote:
>
>         if (system_running)
>                 cachep = (kmem_cache_t *) kmem_cache_alloc(&cache_cache, SLAB_KERNEL);
>         else
>                 cachep = (kmem_cache_t *) kmem_cache_alloc(&cache_cache, SLAB_ATOMIC);
>

I've already done this; didn't like it.

>         if (system_running)
>                 acquire_console_sem();
> 
>         /* ... */
> 
>         if (system_running)
>                 release_console_sem();

Ditto.

> --- linux-2.6.0-test8/kernel/sched.c~early_might_sleep	Sat Oct 18 11:54:24 2003
> +++ linux-2.6.0-test8/kernel/sched.c	Sat Oct 18 16:44:14 2003
> @@ -2847,8 +2847,12 @@ void __might_sleep(char *file, int line)
>  {
>  #if defined(in_atomic)
>  	static unsigned long prev_jiffy;	/* ratelimiting */
> +	extern int system_running;
>  
> -	if (in_atomic() || irqs_disabled()) {
> +	/* Don't print warnings until system_running is set.  This avoids
> +	   spurious warnings during boot before local_irq_enable() and
> +	   init_idle(). */
> +	if (system_running && (in_atomic() || irqs_disabled())) {
>  		if (time_before(jiffies, prev_jiffy + HZ) && prev_jiffy)
>  			return;
>  		prev_jiffy = jiffies;

And ditto.  (Except declarations go in .h files, not in .c files).

Sigh.  We may as well use this approach I guess.

Re: [PATCH] Linux 2.6.0-test8 __might_sleep warnings on boot

[Roland Dreier]

    Andrew> And ditto.  (Except declarations go in .h files, not in .c
    Andrew> files).

    Andrew> Sigh.  We may as well use this approach I guess.

We could go back to waiting 5 minutes after boot before printing
might_sleep warnings ;)

Here's a patch that

- fixes the early might_sleep warnings by not printing them until
  system_running is set.
- adds a declaration of system_running to linux/kernel.h.
- gets rid of declarations of system_running in kernel/kmod.c and
  kernel/sys.c.

 - Roland

diff -Nurp linux-2.6.0-test8.orig/include/linux/kernel.h linux-2.6.0-test8/include/linux/kernel.h
--- linux-2.6.0-test8.orig/include/linux/kernel.h	Fri Oct 17 14:42:52 2003
+++ linux-2.6.0-test8/include/linux/kernel.h	Sat Oct 18 17:56:27 2003
@@ -101,6 +101,7 @@ static inline void console_verbose(void)
 extern void bust_spinlocks(int yes);
 extern int oops_in_progress;		/* If set, an oops, panic(), BUG() or die() is in progress */
 extern int panic_on_oops;
+extern int system_running;
 
 extern int tainted;
 extern const char *print_tainted(void);
diff -Nurp linux-2.6.0-test8.orig/kernel/kmod.c linux-2.6.0-test8/kernel/kmod.c
--- linux-2.6.0-test8.orig/kernel/kmod.c	Fri Oct 17 14:43:00 2003
+++ linux-2.6.0-test8/kernel/kmod.c	Sat Oct 18 17:58:27 2003
@@ -36,7 +36,7 @@
 #include <linux/kernel.h>
 #include <asm/uaccess.h>
 
-extern int max_threads, system_running;
+extern int max_threads;
 
 #ifdef CONFIG_KMOD
 
diff -Nurp linux-2.6.0-test8.orig/kernel/sched.c linux-2.6.0-test8/kernel/sched.c
--- linux-2.6.0-test8.orig/kernel/sched.c	Fri Oct 17 14:43:24 2003
+++ linux-2.6.0-test8/kernel/sched.c	Sat Oct 18 17:54:43 2003
@@ -2847,8 +2847,10 @@ void __might_sleep(char *file, int line)
 {
 #if defined(in_atomic)
 	static unsigned long prev_jiffy;	/* ratelimiting */
-
-	if (in_atomic() || irqs_disabled()) {
+	/* Don't print warnings until system_running is set.  This avoids
+	   spurious warnings during boot before local_irq_enable() and
+	   init_idle(). */
+	if (system_running && (in_atomic() || irqs_disabled())) {
 		if (time_before(jiffies, prev_jiffy + HZ) && prev_jiffy)
 			return;
 		prev_jiffy = jiffies;
diff -Nurp linux-2.6.0-test8.orig/kernel/sys.c linux-2.6.0-test8/kernel/sys.c
--- linux-2.6.0-test8.orig/kernel/sys.c	Fri Oct 17 14:42:54 2003
+++ linux-2.6.0-test8/kernel/sys.c	Sat Oct 18 17:58:18 2003
@@ -78,8 +78,6 @@ EXPORT_SYMBOL(fs_overflowgid);
 int C_A_D = 1;
 int cad_pid = 1;
 
-extern int system_running;
-
 /*
  *	Notifier list for kernel code which wants to be called
  *	at shutdown. This is used to stop any idling DMA operations

Re: [PATCH] Linux 2.6.0-test8 __might_sleep warnings on boot

[Jesper Juhl]

On Sun, 18 Oct 2003, Roland Dreier wrote:

> -	if (in_atomic() || irqs_disabled()) {
> +	/* Don't print warnings until system_running is set.  This avoids
> +	   spurious warnings during boot before local_irq_enable() and
> +	   init_idle(). */
> +	if (system_running && (in_atomic() || irqs_disabled())) {

Wouldn't this :

if ((in_atomic() || irqs_disabled()) && system_running)

be slightly more efficient?   The reason I say that is that I would assume
that the chance of (in_atomic() || irqs_disabled()) being false is greater
than the chance of !system_running - if that is so, then reordering the if
will allow it to break out early more often...

Is that a completely rediculous thing to do?


Regards,

Jesper Juhl

Re: [PATCH] Linux 2.6.0-test8 __might_sleep warnings on boot

[Roland Dreier]

>>>>> " " == Jesper Juhl <juhl-lkml@dif.dk> writes:

    > On Sun, 18 Oct 2003, Roland Dreier wrote:
    > 
    > > -	if (in_atomic() || irqs_disabled()) {
    > > +	/* Don't print warnings until system_running is set.  This avoids
    > > +	   spurious warnings during boot before local_irq_enable() and
    > > +	   init_idle(). */
    > > +	if (system_running && (in_atomic() || irqs_disabled())) {
    > 
    > Wouldn't this :
    > 
    > if ((in_atomic() || irqs_disabled()) && system_running)
    > 
    > be slightly more efficient?   The reason I say that is that I would assume
    > that the chance of (in_atomic() || irqs_disabled()) being false is greater
    > than the chance of !system_running - if that is so, then reordering the if
    > will allow it to break out early more often...

Yes, I think you're right about the efficiency.  However, I didn't
think this was a performance-critical code path (especially since it
is only turned on by a debugging config option), and the way I wrote
it matched the way I was thinking about the test ("If the system is
running, then check if we're in atomic or have irqs disabled").
But I don't think it matters much either way.

Thanks,
  Roland

Re: [PATCH] Linux 2.6.0-test8 __might_sleep warnings on boot

[Jesper Juhl]

On Sun, 19 Oct 2003, Roland Dreier wrote:

> >>>>> " " == Jesper Juhl <juhl-lkml@dif.dk> writes:
>
>     > On Sun, 18 Oct 2003, Roland Dreier wrote:
>     >
>     > > -	if (in_atomic() || irqs_disabled()) {
>     > > +	/* Don't print warnings until system_running is set.  This avoids
>     > > +	   spurious warnings during boot before local_irq_enable() and
>     > > +	   init_idle(). */
>     > > +	if (system_running && (in_atomic() || irqs_disabled())) {
>     >
>     > Wouldn't this :
>     >
>     > if ((in_atomic() || irqs_disabled()) && system_running)
>     >
>     > be slightly more efficient?   The reason I say that is that I would assume
>     > that the chance of (in_atomic() || irqs_disabled()) being false is greater
>     > than the chance of !system_running - if that is so, then reordering the if
>     > will allow it to break out early more often...
>
> Yes, I think you're right about the efficiency.  However, I didn't
> think this was a performance-critical code path (especially since it
> is only turned on by a debugging config option),

No, I don't think it is critical in any way either. I just read the patch
and thought "why not do it the most efficient way even if it's not a
critical path?"...
But I'm probably just being a pedant here...

> and the way I wrote
> it matched the way I was thinking about the test ("If the system is
> running, then check if we're in atomic or have irqs disabled").
> But I don't think it matters much either way.
>
Probably not.


/Jesper Juhl