enhanced spinlock debugging code for intel

enhanced spinlock debugging code for intel

[Brent Baccala]

[-- Attachment #1: Type: text/plain, Size: 1963 bytes --]

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.

-- 
                                        -bwb

                                        Brent Baccala
                                        baccala@freesoft.org

==============================================================================
       For news from freesoft.org, subscribe to announce@freesoft.org:
   
mailto:announce-request@freesoft.org?subject=subscribe&body=subscribe
==============================================================================

[-- Attachment #2: linux-spinlock-diff --]
[-- Type: text/plain, Size: 2019 bytes --]

diff -ru linux-2.4.6-dist/include/asm-i386/spinlock.h linux-2.4.6-kgdb/include/asm-i386/spinlock.h
--- linux-2.4.6-dist/include/asm-i386/spinlock.h	Fri May 25 21:01:26 2001
+++ linux-2.4.6-kgdb/include/asm-i386/spinlock.h	Wed Aug  1 23:11:51 2001
@@ -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)

Re: enhanced spinlock debugging code for intel

[Tachino Nobuhiro]

Hello,

At Thu, 02 Aug 2001 03:02:12 -0400,
Brent Baccala wrote:
> 
> 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 think your code has a race. See following sequence.


	cpu0					cpu1
----------------------------------------------------------------------------------------

						call spin_unlock()
						
						last_lock_processor is 1 here.

	call spin_lock...

	if (spin_is_locked(lock)
	  && lock->last_lock_processor == my_processor_id)

	asm(spin_lock_string)
						call spin_lock()...

						if (spin_is_locked(lock)
						  && lock->last_lock_processor == my_processor_id)
							BUG();
						
						-------------------------------------------
						last_lock_processor and my_processor_id are
						both 1 here. So BUG() is called incorrectly.
						-------------------------------------------
						

        last_lock_processor = 0;
	
> 
> 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.
> 
> -- 
>                                         -bwb
> 
>                                         Brent Baccala
>                                         baccala@freesoft.org
> 
> ==============================================================================
>        For news from freesoft.org, subscribe to announce@freesoft.org:
>    
> mailto:announce-request@freesoft.org?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
> --- linux-2.4.6-dist/include/asm-i386/spinlock.h	Fri May 25 21:01:26 2001
> +++ linux-2.4.6-kgdb/include/asm-i386/spinlock.h	Wed Aug  1 23:11:51 2001
> @@ -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)

Re: enhanced spinlock debugging code for intel

[Brent Baccala]

Tachino Nobuhiro wrote:
> 
> Hello,
> 
> At Thu, 02 Aug 2001 03:02:12 -0400,
> Brent Baccala wrote:
> >
> > 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 think your code has a race. See following sequence.

You're right.  And I thought this code was simple :-)

I guess what suggests itself to me is something like:

  static inline void spin_unlock(spinlock_t *lock)
  {
  #if SPINLOCK_DEBUG
          if (lock->magic != SPINLOCK_MAGIC)
                  BUG();
          if (!spin_is_locked(lock))
                  BUG();
+         lock->last_lock_processor = -1;
  #endif
          __asm__ __volatile__(
                  spin_unlock_string
                  :"=m" (lock->lock) : : "memory");
  }

No longer any race (right?) and we don't lose anything since the one
processor is about to drop the lock it (presumably) held.  I wonder if
it should check to make sure the same processor that grabbed the lock is
releasing it.  Not exactly a bug, and somebody might write code like
that, but it seems suspicious.  Comments?

-- 
                                        -bwb

                                        Brent Baccala
                                        baccala@freesoft.org

==============================================================================
       For news from freesoft.org, subscribe to announce@freesoft.org:
   
mailto:announce-request@freesoft.org?subject=subscribe&body=subscribe
==============================================================================

Re: enhanced spinlock debugging code for intel

[Brent Baccala]

> Brent Baccala wrote:
> >
> > 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.


I've been thinking more about my own problem here.

I think it could be solved by splitting the spinlock include file in
two:

  spinlockdef.h   - the structure definitions for spinlocks and their
initializers
  spinlock.h      - includes spinlockdef.h and defines the functions to
manipulate spinlocks

This would have to been done in include/linux, as well as all the
include/asm* directories.

Most stuff would include spinlock.h, get both files, and see no change.

sched.h would be changed to include spinlockdef.h, since that's all it
needs.

asm-i386/spinlock.h could then include sched.h and spinlockdef.h without
creating a self-referential loop, so smp_processor_id would work in this
file.

Comments?  a new include file in all the asm dirs?  think Linus would
take it?

-- 
                                        -bwb

                                        Brent Baccala
                                        baccala@freesoft.org

==============================================================================
       For news from freesoft.org, subscribe to announce@freesoft.org:
   
mailto:announce-request@freesoft.org?subject=subscribe&body=subscribe
==============================================================================