[PATCH] Prevent spinlock debug from timing out too early

[PATCH] Prevent spinlock debug from timing out too early

[Andi Kleen]

In modern Linux loops_per_jiffie doesn't have much relationship to how
fast the CPU can really execute loops because delay works in a different way. 
Unfortunately the spinlock debugging code used it for that, which
caused it it time timeout much earlier that a second.

A second is quite a long time for a spinlock, but there are situations
that can hold them quite long (debug output over slow serial
line, SCSI driver in recovery etc.), so it's better to not time out
too early.

This patch changes it to use jiffies to check for the timeout.
It has the small drawback over the previous case that if jiffies 
doesn't tick anymore you won't get any timed out spinlocks, but 
normally NMI watchdog should catch that anyways.

Signed-off-by:  Andi Kleen <ak@suse.de>

Index: linux-2.6.15/lib/spinlock_debug.c
===================================================================
--- linux-2.6.15.orig/lib/spinlock_debug.c
+++ linux-2.6.15/lib/spinlock_debug.c
@@ -68,13 +68,13 @@ static inline void debug_spin_unlock(spi
 static void __spin_lock_debug(spinlock_t *lock)
 {
 	int print_once = 1;
-	u64 i;
 
 	for (;;) {
-		for (i = 0; i < loops_per_jiffy * HZ; i++) {
-			cpu_relax();
+		unsigned long timeout = jiffies + HZ;
+		while (time_before(jiffies, timeout)) {
 			if (__raw_spin_trylock(&lock->raw_lock))
 				return;
+			cpu_relax();
 		}
 		/* lockup suspected: */
 		if (print_once) {

Re: [PATCH] Prevent spinlock debug from timing out too early

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> Index: linux-2.6.15/lib/spinlock_debug.c
> ===================================================================
> --- linux-2.6.15.orig/lib/spinlock_debug.c
> +++ linux-2.6.15/lib/spinlock_debug.c
> @@ -68,13 +68,13 @@ static inline void debug_spin_unlock(spi
>  static void __spin_lock_debug(spinlock_t *lock)
>  {
>  	int print_once = 1;
> -	u64 i;
>  
>  	for (;;) {
> -		for (i = 0; i < loops_per_jiffy * HZ; i++) {
> -			cpu_relax();
> +		unsigned long timeout = jiffies + HZ;
> +		while (time_before(jiffies, timeout)) {
>  			if (__raw_spin_trylock(&lock->raw_lock))
>  				return;
> +			cpu_relax();

The reason i added a loop counter was to solve the case where we are 
spinning with interrupts disabled - jiffies wont increase there!  But i 
agree that loops_per_jiffy is the wrong metric to use.

a better solution would be to call __delay(1) after the first failed 
attempt, that would make the delay at least 1 second long. It seems 
__delay() is de-facto exported by every architecture, so we can rely on 
it in the global spinlock code.

So how about the patch below instead?

[detail: i moved the __delay() after the second attempted trylock, this 
way we'll have 2 trylocks without a delay - for ultra-short critical 
sections.]

	Ingo

----
fix spinlock debugging delays to not time out too early.
Bug found by Andi Kleen.

Signed-off-by: Ingo Molnar <mingo@elte.hu>

--- linux/lib/spinlock_debug.c.orig
+++ linux/lib/spinlock_debug.c
@@ -72,9 +72,9 @@ static void __spin_lock_debug(spinlock_t
 
 	for (;;) {
 		for (i = 0; i < loops_per_jiffy * HZ; i++) {
-			cpu_relax();
 			if (__raw_spin_trylock(&lock->raw_lock))
 				return;
+			__delay(1);
 		}
 		/* lockup suspected: */
 		if (print_once) {
@@ -144,9 +144,9 @@ static void __read_lock_debug(rwlock_t *
 
 	for (;;) {
 		for (i = 0; i < loops_per_jiffy * HZ; i++) {
-			cpu_relax();
 			if (__raw_read_trylock(&lock->raw_lock))
 				return;
+			__delay(1);
 		}
 		/* lockup suspected: */
 		if (print_once) {
@@ -217,9 +217,9 @@ static void __write_lock_debug(rwlock_t 
 
 	for (;;) {
 		for (i = 0; i < loops_per_jiffy * HZ; i++) {
-			cpu_relax();
 			if (__raw_write_trylock(&lock->raw_lock))
 				return;
+			__delay(1);
 		}
 		/* lockup suspected: */
 		if (print_once) {

Re: [PATCH] Prevent spinlock debug from timing out too early

[Andi Kleen]

On Monday 06 February 2006 22:36, Ingo Molnar wrote:
> 
> * Andi Kleen <ak@suse.de> wrote:
> 
> > Index: linux-2.6.15/lib/spinlock_debug.c
> > ===================================================================
> > --- linux-2.6.15.orig/lib/spinlock_debug.c
> > +++ linux-2.6.15/lib/spinlock_debug.c
> > @@ -68,13 +68,13 @@ static inline void debug_spin_unlock(spi
> >  static void __spin_lock_debug(spinlock_t *lock)
> >  {
> >  	int print_once = 1;
> > -	u64 i;
> >  
> >  	for (;;) {
> > -		for (i = 0; i < loops_per_jiffy * HZ; i++) {
> > -			cpu_relax();
> > +		unsigned long timeout = jiffies + HZ;
> > +		while (time_before(jiffies, timeout)) {
> >  			if (__raw_spin_trylock(&lock->raw_lock))
> >  				return;
> > +			cpu_relax();
> 
> The reason i added a loop counter was to solve the case where we are 
> spinning with interrupts disabled - jiffies wont increase there! 

Yes but the NMI watchdog should catch it eventually

[we really should enable it by default on i386 too - local APIC 
NMI should work everywhere with APIC]

Oops I missed the write lock case. Thanks.

 
> a better solution would be to call __delay(1) after the first failed 
> attempt, that would make the delay at least 1 second long. It seems 
> __delay() is de-facto exported by every architecture, so we can rely on 
> it in the global spinlock code.
> 
> So how about the patch below instead?

Are you sure loops_per_jiffie is always in delay(1) units?


-Andi

Re: [PATCH] Prevent spinlock debug from timing out too early

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> > a better solution would be to call __delay(1) after the first failed 
> > attempt, that would make the delay at least 1 second long. It seems 
> > __delay() is de-facto exported by every architecture, so we can rely on 
> > it in the global spinlock code.
> > 
> > So how about the patch below instead?
> 
> Are you sure loops_per_jiffie is always in delay(1) units?

there are a few explicit calls to __delay() in drivers/*, so i'd assume 
so. A grep also seems to suggest so:

 ./ppc/xmon/xmon.c:extern inline void __delay(unsigned int loops)
 ./x86_64/lib/delay.c:void __delay(unsigned long loops)
 ./sparc64/lib/delay.c:void __delay(unsigned long loops)
 ./sh64/lib/udelay.c:void __delay(int loops)
 ./m32r/lib/delay.c:void __delay(unsigned long loops)
 ./i386/lib/delay.c:void __delay(unsigned long loops)
 ./s390/lib/delay.c:void __delay(unsigned long loops)
 ./sh/lib/delay.c:void __delay(unsigned long loops)
 ./powerpc/kernel/time.c:void __delay(unsigned long loops)

but yes, this is a non-specified thing so far, so there could be 
problems on some platforms. Worst-case we never time out - which could 
be detected via the NMI watchdog or the soft-lockup watchdog - so it's 
not like they would go unnoticed.

	Ingo