enhanced spinlock debugging code for intel

Hi -

I'm having a problem with my USB CD burner that involves spinlocks - in particular, some code that trys to grab a spinlock that's already locked (this on a uni-processor machine).

The existing spinlock debug code on intel only checked for unlocking an unlocked spinlock, so I added code to check for locking a locked spinlock as well - it now catches my problem.

I'm attaching the code. The basic operation is to add a field (when SPINLOCK_DEBUG is set in include/asm-i386/spinlock.h; no config option) to the spinlock structure that contains the processor ID that set the lock. Then, when we try to grab the lock, check to see if 1) it's already locked and 2) the current processor is the one that holds the lock.

I've had to add some hideous code to get the processor ID:

	#define my_processor_id (((int *)current)[13])

since sched.h includes spinlock.h, so task_struct isn't defined when this file is parsed, so we can't just dereference current to find the processor ID. Any better suggestions would be welcome.

The code also adds fields to record the PC and current task_struct when the lock is grabbed, so if somebody comes along later and trys to grab it again, we can figure out who already has it. This information doesn't show up in a oops, but is easily extracted using remote gdb (see my previous post).

Try it if you'd like; it's not very complex, but I would like a better solution for getting the processor ID before I make it an "official" submission to Linus.

Signature · 11 lines

                                        -bwb

                                        Brent Baccala
                                        [email protected]

==============================================================================
       For news from freesoft.org, subscribe to [email protected]:
   
mailto:[email protected]?subject=subscribe&body=subscribe
==============================================================================
diff -ru linux-2.4.6-dist/include/asm-i386/spinlock.h linux-2.4.6-kgdb/include/asm-i386/spinlock.h

include/asm-i386/spinlock.h +24 -2

@@ -1,6 +1,8 @@
 #ifndef __ASM_SPINLOCK_H
 #define __ASM_SPINLOCK_H
 
+#include <asm/smp.h>
+#include <asm/current.h>
 #include <asm/atomic.h>
 #include <asm/rwlock.h>
 #include <asm/page.h>

@@ -12,7 +14,7 @@
  * initialize their spinlocks properly, tsk tsk.
  * Remember to turn this off in 2.4. -ben
  */
-#define SPINLOCK_DEBUG	0
+#define SPINLOCK_DEBUG	1
 
 /*
  * Your basic SMP spinlocks, allowing only a single CPU anywhere

@@ -22,13 +24,16 @@
 	volatile unsigned int lock;
 #if SPINLOCK_DEBUG
 	unsigned magic;
+	void *last_lock_addr;
+	void *last_lock_current;
+	int last_lock_processor;
 #endif
 } spinlock_t;
 
 #define SPINLOCK_MAGIC	0xdead4ead
 
 #if SPINLOCK_DEBUG
-#define SPINLOCK_MAGIC_INIT	, SPINLOCK_MAGIC
+#define SPINLOCK_MAGIC_INIT	, SPINLOCK_MAGIC, NULL
 #else
 #define SPINLOCK_MAGIC_INIT	/* */
 #endif

@@ -75,6 +80,15 @@
 	return oldval > 0;
 }
 
+/* This is here because the definition of smp_processor_id() pulls the
+ * processor id out of the current task_struct, which is defined in
+ * linux/sched.h, which includes this file because it declares spinlocks.
+ * So we can't use smp_processor_id() because task_struct hasn't been
+ * defined yet.  Damn these computers.
+ */
+
+#define my_processor_id (((int *)current)[13])
+
 static inline void spin_lock(spinlock_t *lock)
 {
 #if SPINLOCK_DEBUG

@@ -84,10 +98,18 @@
 printk("eip: %p\n", &&here);
 		BUG();
 	}
+	if (spin_is_locked(lock)
+	    && lock->last_lock_processor == my_processor_id)
+		BUG();
 #endif
 	__asm__ __volatile__(
 		spin_lock_string
 		:"=m" (lock->lock) : : "memory");
+#if SPINLOCK_DEBUG
+	lock->last_lock_addr = &&here;
+	lock->last_lock_processor = my_processor_id;
+	lock->last_lock_current = current;
+#endif
 }
 
 static inline void spin_unlock(spinlock_t *lock)

Hello,

At Thu, 02 Aug 2001 03:02:12 -0400,

Quoted 7 lines · Brent Baccala

Hi -

I'm having a problem with my USB CD burner that involves spinlocks - in particular, some code that trys to grab a spinlock that's already locked (this on a uni-processor machine).

Quoted 3 lines

The existing spinlock debug code on intel only checked for unlocking an unlocked spinlock, so I added code to check for locking a locked spinlock as well - it now catches my problem.

I think your code has a race. See following sequence.

Quoted 8 lines · Brent Baccala

I've had to add some hideous code to get the processor ID:

      #define my_processor_id (((int *)current)[13])

since sched.h includes spinlock.h, so task_struct isn't defined when this file is parsed, so we can't just dereference current to find the processor ID. Any better suggestions would be welcome.