[patch 4/4] increase traffic on linux-kernel

[patch 4/4] increase traffic on linux-kernel

[Andrew Morton]

[This has four scalps already.  Thomas Molina has agreed
 to track things as they are identified ]

Infrastructure to detect sleep-inside-spinlock bugs.  Really only
useful if compiled with CONFIG_PREEMPT=y.  It prints out a whiny
message and a stack backtrace if someone calls a function which might
sleep from within an atomic region.

This patch generates a storm of output at boot, due to
drivers/ide/ide-probe.c:init_irq() calling lots of things which it
shouldn't under ide_lock.

It'll find other bugs too.



 include/asm-i386/semaphore.h |    4 ++--
 include/linux/kernel.h       |    7 +++++++
 include/linux/rwsem.h        |    2 ++
 kernel/ksyms.c               |    4 +++-
 kernel/sched.c               |   17 +++++++++++++++++
 mm/page_alloc.c              |    3 +++
 mm/slab.c                    |    3 +++
 7 files changed, 37 insertions(+), 3 deletions(-)

--- 2.5.38/include/asm-i386/semaphore.h~might_sleep	Wed Sep 25 20:15:27 2002
+++ 2.5.38-akpm/include/asm-i386/semaphore.h	Wed Sep 25 20:15:27 2002
@@ -116,7 +116,7 @@ static inline void down(struct semaphore
 #if WAITQUEUE_DEBUG
 	CHECK_MAGIC(sem->__magic);
 #endif
-
+	might_sleep();
 	__asm__ __volatile__(
 		"# atomic down operation\n\t"
 		LOCK "decl %0\n\t"     /* --sem->count */
@@ -142,7 +142,7 @@ static inline int down_interruptible(str
 #if WAITQUEUE_DEBUG
 	CHECK_MAGIC(sem->__magic);
 #endif
-
+	might_sleep();
 	__asm__ __volatile__(
 		"# atomic interruptible down operation\n\t"
 		LOCK "decl %1\n\t"     /* --sem->count */
--- 2.5.38/include/linux/kernel.h~might_sleep	Wed Sep 25 20:15:27 2002
+++ 2.5.38-akpm/include/linux/kernel.h	Wed Sep 25 20:15:27 2002
@@ -40,6 +40,13 @@
 
 struct completion;
 
+#ifdef CONFIG_DEBUG_KERNEL
+void __might_sleep(char *file, int line);
+#define might_sleep() __might_sleep(__FILE__, __LINE__)
+#else
+#define might_sleep() do {} while(0)
+#endif
+
 extern struct notifier_block *panic_notifier_list;
 NORET_TYPE void panic(const char * fmt, ...)
 	__attribute__ ((NORET_AND format (printf, 1, 2)));
--- 2.5.38/include/linux/rwsem.h~might_sleep	Wed Sep 25 20:15:27 2002
+++ 2.5.38-akpm/include/linux/rwsem.h	Wed Sep 25 20:15:27 2002
@@ -40,6 +40,7 @@ extern void FASTCALL(rwsemtrace(struct r
  */
 static inline void down_read(struct rw_semaphore *sem)
 {
+	might_sleep();
 	rwsemtrace(sem,"Entering down_read");
 	__down_read(sem);
 	rwsemtrace(sem,"Leaving down_read");
@@ -62,6 +63,7 @@ static inline int down_read_trylock(stru
  */
 static inline void down_write(struct rw_semaphore *sem)
 {
+	might_sleep();
 	rwsemtrace(sem,"Entering down_write");
 	__down_write(sem);
 	rwsemtrace(sem,"Leaving down_write");
--- 2.5.38/kernel/ksyms.c~might_sleep	Wed Sep 25 20:15:27 2002
+++ 2.5.38-akpm/kernel/ksyms.c	Wed Sep 25 20:15:27 2002
@@ -497,7 +497,9 @@ EXPORT_SYMBOL(jiffies_64);
 EXPORT_SYMBOL(xtime);
 EXPORT_SYMBOL(do_gettimeofday);
 EXPORT_SYMBOL(do_settimeofday);
-
+#ifdef CONFIG_DEBUG_KERNEL
+EXPORT_SYMBOL(__might_sleep);
+#endif
 #if !defined(__ia64__)
 EXPORT_SYMBOL(loops_per_jiffy);
 #endif
--- 2.5.38/kernel/sched.c~might_sleep	Wed Sep 25 20:15:27 2002
+++ 2.5.38-akpm/kernel/sched.c	Wed Sep 25 20:15:28 2002
@@ -2150,3 +2150,20 @@ void __init sched_init(void)
 	enter_lazy_tlb(&init_mm, current, smp_processor_id());
 }
 
+#ifdef CONFIG_DEBUG_KERNEL
+void __might_sleep(char *file, int line)
+{
+#if defined(in_atomic)
+	static unsigned long prev_jiffy;	/* ratelimiting */
+
+	if (in_atomic()) {
+		if (time_before(jiffies, prev_jiffy + HZ))
+			return;
+		prev_jiffy = jiffies;
+		printk("Sleeping function called from illegal"
+				" context at %s:%d\n", file, line);
+		dump_stack();
+	}
+#endif
+}
+#endif
--- 2.5.38/mm/page_alloc.c~might_sleep	Wed Sep 25 20:15:27 2002
+++ 2.5.38-akpm/mm/page_alloc.c	Wed Sep 25 20:15:28 2002
@@ -321,6 +321,9 @@ __alloc_pages(unsigned int gfp_mask, uns
 	struct page * page;
 	int freed, i;
 
+	if (gfp_mask & __GFP_WAIT)
+		might_sleep();
+
 	KERNEL_STAT_ADD(pgalloc, 1<<order);
 
 	zones = zonelist->zones;  /* the list of zones suitable for gfp_mask */
--- 2.5.38/mm/slab.c~might_sleep	Wed Sep 25 20:15:27 2002
+++ 2.5.38-akpm/mm/slab.c	Wed Sep 25 20:15:28 2002
@@ -1370,6 +1370,9 @@ static inline void * __kmem_cache_alloc 
 	unsigned long save_flags;
 	void* objp;
 
+	if (flags & __GFP_WAIT)
+		might_sleep();
+
 	kmem_cache_alloc_head(cachep, flags);
 try_again:
 	local_irq_save(save_flags);

.

Re: [patch 4/4] increase traffic on linux-kernel

[Jeff Garzik]

Andrew Morton wrote:
> --- 2.5.38/include/linux/kernel.h~might_sleep	Wed Sep 25 20:15:27 2002
> +++ 2.5.38-akpm/include/linux/kernel.h	Wed Sep 25 20:15:27 2002
> @@ -40,6 +40,13 @@
>  
>  struct completion;
>  
> +#ifdef CONFIG_DEBUG_KERNEL
> +void __might_sleep(char *file, int line);
> +#define might_sleep() __might_sleep(__FILE__, __LINE__)
> +#else
> +#define might_sleep() do {} while(0)
> +#endif


I disagree with this -- CONFIG_DEBUG_KERNEL should not enable any machinery.

Magic Sysrq should be enable-able without affecting any other parts of 
the kernel.

Re: [patch 4/4] increase traffic on linux-kernel

[Andrew Morton]

Jeff Garzik wrote:
> 
> Andrew Morton wrote:
> > --- 2.5.38/include/linux/kernel.h~might_sleep Wed Sep 25 20:15:27 2002
> > +++ 2.5.38-akpm/include/linux/kernel.h        Wed Sep 25 20:15:27 2002
> > @@ -40,6 +40,13 @@
> >
> >  struct completion;
> >
> > +#ifdef CONFIG_DEBUG_KERNEL
> > +void __might_sleep(char *file, int line);
> > +#define might_sleep() __might_sleep(__FILE__, __LINE__)
> > +#else
> > +#define might_sleep() do {} while(0)
> > +#endif
> 
> I disagree with this -- CONFIG_DEBUG_KERNEL should not enable any machinery.

I did this because adding a function call to down() and friends
has significant cost.

> Magic Sysrq should be enable-able without affecting any other parts of
> the kernel.

A separate config option then?

Re: [patch 4/4] increase traffic on linux-kernel

[Jeff Garzik]

Andrew Morton wrote:
> Jeff Garzik wrote:
>>I disagree with this -- CONFIG_DEBUG_KERNEL should not enable any machinery.

> A separate config option then?

please.

To answer your other email, moving magic-sysrq outside 
CONFIG_DEBUG_KERNEL should probably be done, though IMO that does not 
obviate the need for a separate config option here.

thanks,

	Jeff

Re: [patch 4/4] increase traffic on linux-kernel

[Andrew Morton]

Jeff Garzik wrote:
> 
> ...
> Magic Sysrq should be enable-able without affecting any other parts of
> the kernel.

or move sysrq outside CONFIG_DEBUG_KERNEL?

Re: [patch 4/4] increase traffic on linux-kernel

[Greg KH]

On Wed, Sep 25, 2002 at 09:09:08PM -0700, Andrew Morton wrote:
> 
> Infrastructure to detect sleep-inside-spinlock bugs.  Really only
> useful if compiled with CONFIG_PREEMPT=y.  It prints out a whiny
> message and a stack backtrace if someone calls a function which might
> sleep from within an atomic region.

Why not make this it's own config option, dependent on CONFIG_PREEMPT?

thanks,

greg k-h

Re: [patch 4/4] increase traffic on linux-kernel

[Andrew Morton]

Greg KH wrote:
> 
> On Wed, Sep 25, 2002 at 09:09:08PM -0700, Andrew Morton wrote:
> >
> > Infrastructure to detect sleep-inside-spinlock bugs.  Really only
> > useful if compiled with CONFIG_PREEMPT=y.  It prints out a whiny
> > message and a stack backtrace if someone calls a function which might
> > sleep from within an atomic region.
> 
> Why not make this it's own config option, dependent on CONFIG_PREEMPT?
> 

With CONFIG_PREEMPT=n, it'll detect might-sleep-inside-interrupt bugs.

Some value there I guess.

Re: [patch 4/4] increase traffic on linux-kernel

[Linus Torvalds]

In article <3D928864.23666D93@digeo.com>,
Andrew Morton  <akpm@digeo.com> wrote:
>
>Infrastructure to detect sleep-inside-spinlock bugs.  Really only
>useful if compiled with CONFIG_PREEMPT=y.  It prints out a whiny
>message and a stack backtrace if someone calls a function which might
>sleep from within an atomic region.

This is in my BK tree now, along with Ingo's symbolic backtraces, which
makes it possibly less tedious to read the output. 

I would suggest that all developers for a while run with 

	CONFIG_PREEMPT=y
	CONFIG_DEBUG_KERNEL=y
	CONFIG_KALLSYMS=y

and see if something shows up in their subsystem (but be careful about
the backtraces, since they often contain old crud, especially since gcc
does a horrible job at keeping the stack together and thus leaves unused
"holes" in the stack frame which then show old and stale info). 

It shows clearly that at least the sound PCM code does locking
completely the wrong way around, apparently at least partly because of
bad abstraction macros that hide the fact that some locks are semaphores
and others are spinlocks.

[ Rant: abstraction like this is _bad_, for christ sake! Don't hide what
  locks you're using just to make the code look simpler.  Hint: trying
  to do a "down()" within a spinlock is stupid and not produtive. ]

Thanks,

		Linus

Re: [patch 4/4] increase traffic on linux-kernel

[Arnaldo Carvalho de Melo]

Em Fri, Sep 27, 2002 at 01:31:17AM +0000, Linus Torvalds escreveu:
> In article <3D928864.23666D93@digeo.com>,
> Andrew Morton  <akpm@digeo.com> wrote:

> >Infrastructure to detect sleep-inside-spinlock bugs.  Really only
> >useful if compiled with CONFIG_PREEMPT=y.  It prints out a whiny
> >message and a stack backtrace if someone calls a function which might
> >sleep from within an atomic region.
> 
> This is in my BK tree now, along with Ingo's symbolic backtraces, which
> makes it possibly less tedious to read the output. 

Wheee! Thanks a LOT for merging both. We'll have lots of fun with these ones
while saving the old network protocols, that have lots of cases where we can
see problems even without these tools, imagine with them in place 8)

- Arnaldo