interrupt checks for spinlocks

I recently thought about interrupt disablement and locking again, or at least such as has gone about in various places, and I got scared.

Hence, a wee addition to CONFIG_DEBUG_SPINLOCK:

(1) check that spinlocks are not taken in interrupt context without interrupts disabled
(2) taint spinlocks taken in interrupt context
(3) check for tainted spinlocks taken without interrupts disabled
(4) check for spinlocking calls unconditionally disabling (and hence later re-enabling) interrupts with interrupts disabled

The only action taken is printk() and dump_stack(). No arch code has been futzed with to provide irq tainting yet. Looks like a good way to shake out lurking bugs to me (somewhat like may_sleep() etc.).

vs. 2.5.x-bk as of 2PM PST 3 Nov

Bill

diff -urN linux-virgin/include/linux/spinlock.h linux-wli/include/linux/spinlock.h
@@ -38,6 +38,48 @@
 #include <asm/spinlock.h>
 
 /*
+ * if a lock is ever taken in interrupt context, it must always be
+ * taken with interrupts disabled. If a locking call is made that
+ * unconditionally disables and then re-enables interrupts, it must
+ * be made with interrupts enabled.
+ */
+#ifndef irq_tainted_lock
+#define irq_tainted_lock(lock)	0 
+#endif
+
+#ifndef irq_taint_lock
+#define irq_taint_lock(lock)	do { } while (0)
+#define
+
+#ifndef CONFIG_DEBUG_SPINLOCK
+#define check_spinlock_irq(lock)		do { } while (0)
+#define check_spinlock_irqs_disabled(lock)	do { } while (0)
+#else
+#define check_spinlock_irq(lock)					\
+	do {								\
+		if (irqs_disabled()) {					\
+			printk("spinlock taken unconditionally "	\
+				"re-enabling interrupts\n");		\
+			dump_stack();					\
+		}							\
+	} while (0)
+#define check_spinlock_irqs_disabled(lock)				\
+	do {								\
+		if (in_interrupt() && !irqs_disabled()) {		\
+			printk("spinlock taken in interrupt context "	\
+				"without disabling interrupts\n");	\
+			dump_stack();					\
+		} else if (in_interrupt())				\
+			irq_taint_lock(lock);				\
+		else if (irq_tainted_lock(lock)) {			\
+			printk("spinlock taken in process context "	\
+				"without disabling interrupts\n");	\
+			dump_stack();					\
+		}							\
+	} while (0)
+#endif
+
+/*
  * !CONFIG_SMP and spin_lock_init not previously defined
  * (e.g. by including include/asm/spinlock.h)
  */

@@ -87,6 +129,7 @@
  */
 #define spin_lock(lock)	\
 do { \
+	check_spinlock_irqs_disabled(lock); \
 	preempt_disable(); \
 	_raw_spin_lock(lock); \
 } while(0)

@@ -102,6 +145,7 @@
 
 #define read_lock(lock)	\
 do { \
+	check_spinlock_irqs_disabled(lock); \
 	preempt_disable(); \
 	_raw_read_lock(lock); \
 } while(0)

@@ -114,6 +158,7 @@
 
 #define write_lock(lock) \
 do { \
+	check_spinlock_irqs_disabled(lock); \
 	preempt_disable(); \
 	_raw_write_lock(lock); \
 } while(0)

@@ -136,6 +181,7 @@
 
 #define spin_lock_irq(lock) \
 do { \
+	check_spinlock_irq(lock); \
 	local_irq_disable(); \
 	preempt_disable(); \
 	_raw_spin_lock(lock); \

@@ -157,6 +203,7 @@
 
 #define read_lock_irq(lock) \
 do { \
+	check_spinlock_irq(lock); \
 	local_irq_disable(); \
 	preempt_disable(); \
 	_raw_read_lock(lock); \

@@ -178,6 +225,7 @@
 
 #define write_lock_irq(lock) \
 do { \
+	check_spinlock_irq(lock); \
 	local_irq_disable(); \
 	preempt_disable(); \
 	_raw_write_lock(lock); \

Wouldn't it be interesting to keep a ( per task ) list of acquired spinlocks to be able to diagnose cross locks in case of stall ? ( obviously under CONFIG_DEBUG_SPINLOCK )

- Davide

On Sun, 3 Nov 2002, William Lee Irwin III wrote:
[...]

That would appear to require cycle detection, but it sounds like a potential breakthrough usage of graph algorithms in the kernel. (I've always been told graph algorithms would come back to haunt me.) Or maybe not, deadlock detection has been done before.

A separate patch/feature/whatever for deadlock detection could do that nicely, though. What I've presented here is meant only to flag far more trivial errors with interrupt enablement/disablement than the full deadlock detection problem.

Bill

It's not realy a graph Bill. Each task has a list of acquired locks ( by address ). You keep __LINE__ and __FILE__ with you list items. When there's a deadlock you'll have somewhere :

TSK#N TSK#M
-------------
... ...
LCK#I LCK#J
... ...
-> LCK#J LCK#I

Then with a SysReq key you dump the list of acquired locks for each task who's spinning for a lock. IMO it might be usefull ...

- Davide

Then you had something different in mind. I *thought* you meant maintaining a graph's arcs and dumping the specific deadlocking processes and their acquired locks at failure time. This scheme with limited reporting requires less work/code, but is still beyond the scope of what I was doing.

Bill

On Sun, 3 Nov 2002, Davide Libenzi wrote:

| On Sun, 3 Nov 2002, William Lee Irwin III wrote:
|
| > On Sun, 3 Nov 2002, William Lee Irwin III wrote:
| > [...]
| > >> The only action taken is printk() and dump_stack(). No arch code has
| > >> been futzed with to provide irq tainting yet. Looks like a good way
| > >> to shake out lurking bugs to me (somewhat like may_sleep() etc.).
| >
| > On Sun, Nov 03, 2002 at 04:15:46PM -0800, Davide Libenzi wrote:
| > > Wouldn't it be interesting to keep a ( per task ) list of acquired
| > > spinlocks to be able to diagnose cross locks in case of stall ?
| > > ( obviously under CONFIG_DEBUG_SPINLOCK )
| >
| > That would appear to require cycle detection, but it sounds like a
| > potential breakthrough usage of graph algorithms in the kernel.
| > (I've always been told graph algorithms would come back to haunt me.)
| > Or maybe not, deadlock detection has been done before.
| >
| > A separate patch/feature/whatever for deadlock detection could do that
| > nicely, though. What I've presented here is meant only to flag far more
| > trivial errors with interrupt enablement/disablement than the full
| > deadlock detection problem.
|
| It's not realy a graph Bill.  Each task has a list of acquired locks (
| by address ). You keep __LINE__ and __FILE__ with you list items. When
| there's a deadlock you'll have somewhere :
|
|    TSK#N	TSK#M
|    -------------
|    ...		...
|    LCK#I	LCK#J
|    ...		...
| -> LCK#J	LCK#I
|
| Then with a SysReq key you dump the list of acquired locks for each task
| who's spinning for a lock. IMO it might be usefull ...

What's a task in this context? Are we (you) talking about kernel threads/drivers etc. or userspace?

~Randy
"I'm a healthy mushroom."

Hi Randy,

a task here is anything you can identify with a task_struct*

- Davide

For a spinlock it doesn't appear to be insoluble.

Suppose we do the following

For

	spin_lock(&foo)

	current->waiting = foo;
	foo->waiting += current;

	If foo is held
		Check if foo is on current->locks
			If it is then we shot ourself in the foot
		Check if any member of foo->waiting is waiting on a lock
			we hold (in current->locks)
			If it is then we shot ourselves in both feet
		
	When we get the lock
	foo->waiting -= current;
	foo->held = current;
	current->locks = foo;

For

	spin_unlock(&foo)
	
	if(current->locks != foo)
		We released the locks in the wrong order

	remoe foo from current->locks

Alan