[patch 1/1] Fix spinlock debugging delays to not time out too early

[patch 1/1] Fix spinlock debugging delays to not time out too early

[akpm]

From: Ingo Molnar <mingo@elte.hu>

The spinlock-debug wait-loop was using loops_per_jiffy to detect too long
spinlock waits - but on fast CPUs this led to a way too fast timeout and false
messages.

The fix is to include a __delay(1) call in the loop, to correctly approximate
the intended delay timeout of 1 second.  The code assumes that every
architecture implements __delay(1) to last around 1/(loops_per_jiffy*HZ)
seconds.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
Cc: Andi Kleen <ak@muc.de>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 lib/spinlock_debug.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff -puN lib/spinlock_debug.c~fix-spinlock-debugging-delays-to-not-time-out-too-early lib/spinlock_debug.c
--- devel/lib/spinlock_debug.c~fix-spinlock-debugging-delays-to-not-time-out-too-early	2006-02-07 09:40:57.000000000 -0800
+++ devel-akpm/lib/spinlock_debug.c	2006-02-07 09:40:57.000000000 -0800
@@ -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 1/1] Fix spinlock debugging delays to not time out too early

[Luck, Tony]

> The fix is to include a __delay(1) call in the loop, to correctly approximate
> the intended delay timeout of 1 second.  The code assumes that every
> architecture implements __delay(1) to last around 1/(loops_per_jiffy*HZ)
> seconds.

But we calculate loops_per_jiffy based on somewhat larger vales passed
to __delay() ... where the function call overhead is amortized away.

I'd expect that __delay(1) would last quite a bit longer than
1/(loops_per_jiffy * HZ) ... so we'll wait a lot more than a
second.

-Tony

Re: [patch 1/1] Fix spinlock debugging delays to not time out too early

[Andi Kleen]

On Tuesday 07 February 2006 19:20, Luck, Tony wrote:
> > The fix is to include a __delay(1) call in the loop, to correctly approximate
> > the intended delay timeout of 1 second.  The code assumes that every
> > architecture implements __delay(1) to last around 1/(loops_per_jiffy*HZ)
> > seconds.
> 
> But we calculate loops_per_jiffy based on somewhat larger vales passed
> to __delay() ... where the function call overhead is amortized away.
>
> I'd expect that __delay(1) would last quite a bit longer than
> 1/(loops_per_jiffy * HZ) ... so we'll wait a lot more than a
> second.

For this case that would be ok, as long as it isn't a hour or so,
but let's say < 1 minute.

-Andi

Re: [patch 1/1] Fix spinlock debugging delays to not time out too early

[Luck, Tony]

On Tue, Feb 07, 2006 at 07:29:24PM +0100, Andi Kleen wrote:
> For this case that would be ok, as long as it isn't a hour or so,
> but let's say < 1 minute.

About 9.7 seconds on ia64 for:

	for (i = 0; i < loops_per_jiffy * HZ; i++) {
		__delay(1);
	}

so yes, well under a minute.

-Tony

Re: [patch 1/1] Fix spinlock debugging delays to not time out too early

[Ingo Molnar]

* Luck, Tony <tony.luck@intel.com> wrote:

> On Tue, Feb 07, 2006 at 07:29:24PM +0100, Andi Kleen wrote:
> > For this case that would be ok, as long as it isn't a hour or so,
> > but let's say < 1 minute.
> 
> About 9.7 seconds on ia64 for:
> 
> 	for (i = 0; i < loops_per_jiffy * HZ; i++) {
> 		__delay(1);
> 	}
> 
> so yes, well under a minute.

yeah, that's good enough. We want to have a minimum delay of 1 second, 
because otherwise we'd be getting false positives under high load (and 
under some bad drivers). Having a larger delay is not an issue, as long 
as it's below the average hit-reset latency of users ;)

i'd expect the HPET-based __delay(1) to be the heaviest.

	Ingo

Re: [patch 1/1] Fix spinlock debugging delays to not time out too early

[Andi Kleen]

On Tuesday 07 February 2006 20:14, Ingo Molnar wrote:
> 
> * Luck, Tony <tony.luck@intel.com> wrote:
> 
> > On Tue, Feb 07, 2006 at 07:29:24PM +0100, Andi Kleen wrote:
> > > For this case that would be ok, as long as it isn't a hour or so,
> > > but let's say < 1 minute.
> > 
> > About 9.7 seconds on ia64 for:
> > 
> > 	for (i = 0; i < loops_per_jiffy * HZ; i++) {
> > 		__delay(1);
> > 	}
> > 
> > so yes, well under a minute.
> 
> yeah, that's good enough. We want to have a minimum delay of 1 second, 
> because otherwise we'd be getting false positives under high load (and 
> under some bad drivers). Having a larger delay is not an issue, as long 
> as it's below the average hit-reset latency of users ;)

This reminds me - if we can find a cheaper way than __delay(1) it 
would be actually quite reasonable to do a timeout in the non debug
spinlock. Since it's always out of line it doesn't matter if the code
is bigger and spinning cycles are free anyways, so some checks there
wouldn't matter.

Maybe go through a 32bit counter to overflow a few times? A

It's probably better longer than a second by default.

-Andi