increase traffic on linux-kernel

[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(-)

@@ -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 */

@@ -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)));

@@ -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");

@@ -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

@@ -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

@@ -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 */

@@ -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);

.

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

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

or move sysrq outside CONFIG_DEBUG_KERNEL?

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

thanks,

greg k-h

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

Some value there I guess.

In article <[email protected]>,

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

Em Fri, Sep 27, 2002 at 01:31:17AM +0000, Linus Torvalds escreveu:

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