* [PATCH] Prevent spinlock debug from timing out too early
@ 2006-02-06 21:16 Andi Kleen
2006-02-06 21:36 ` Ingo Molnar
0 siblings, 1 reply; 4+ messages in thread
From: Andi Kleen @ 2006-02-06 21:16 UTC (permalink / raw)
To: torvalds; +Cc: mingo, akpm, linux-kernel
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) {
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Prevent spinlock debug from timing out too early
2006-02-06 21:16 [PATCH] Prevent spinlock debug from timing out too early Andi Kleen
@ 2006-02-06 21:36 ` Ingo Molnar
2006-02-06 21:42 ` Andi Kleen
0 siblings, 1 reply; 4+ messages in thread
From: Ingo Molnar @ 2006-02-06 21:36 UTC (permalink / raw)
To: Andi Kleen; +Cc: torvalds, akpm, linux-kernel
* 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) {
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Prevent spinlock debug from timing out too early
2006-02-06 21:36 ` Ingo Molnar
@ 2006-02-06 21:42 ` Andi Kleen
2006-02-06 23:22 ` Ingo Molnar
0 siblings, 1 reply; 4+ messages in thread
From: Andi Kleen @ 2006-02-06 21:42 UTC (permalink / raw)
To: Ingo Molnar; +Cc: torvalds, akpm, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Prevent spinlock debug from timing out too early
2006-02-06 21:42 ` Andi Kleen
@ 2006-02-06 23:22 ` Ingo Molnar
0 siblings, 0 replies; 4+ messages in thread
From: Ingo Molnar @ 2006-02-06 23:22 UTC (permalink / raw)
To: Andi Kleen; +Cc: torvalds, akpm, linux-kernel
* 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2006-02-06 23:24 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-02-06 21:16 [PATCH] Prevent spinlock debug from timing out too early Andi Kleen
2006-02-06 21:36 ` Ingo Molnar
2006-02-06 21:42 ` Andi Kleen
2006-02-06 23:22 ` Ingo Molnar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).