Fix x86_64 _spin_lock_irqsave()

PATCH In review Edward Falk 8 participants 15 messages Aug 24, 2006 – Aug 26, 2006

Share & tools

Edward Falk

author

2006-08-24 02:57 UTC #0 lore raw

PATCH 1/1 Fix x86_64 _spin_lock_irqsave()

Add spin_lock_string_flags and _raw_spin_lock_flags() to asm-x86_64/spinlock.h so that _spin_lock_irqsave() has the same semantics on x86_64 as it does on i386 and does *not* have interrupts disabled while it is waiting for the lock.

This fix is courtesy of Michael Davidson

Change 2012926 by md@md-test on 2006/01/23 18:28:12

PRESUBMIT=passed

R=mbligh mikew
OCL=2010261

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

diff -uprN linux-2.6.17/include/asm-x86_64/spinlock.h 2012926/include/asm-x86_64/spinlock.h
@@ -32,6 +32,23 @@
 	"jmp 1b\n" \
 	LOCK_SECTION_END
 
+#define __raw_spin_lock_string_flags \
+	"\n1:\t" \
+	"lock ; decb %0\n\t" \
+	"js 2f\n\t" \
+	LOCK_SECTION_START("") \
+	"2:\t" \
+	"test $0x200, %1\n\t" \
+	"jz 3f\n\t" \
+	"sti\n\t" \
+	"3:\t" \
+	"rep;nop\n\t" \
+	"cmpb $0, %0\n\t" \
+	"jle 3b\n\t" \
+	"cli\n\t" \
+	"jmp 1b\n" \
+	LOCK_SECTION_END
+
 #define __raw_spin_unlock_string \
 	"movl $1,%0" \
 		:"=m" (lock->slock) : : "memory"

@@ -43,8 +60,6 @@ static inline void __raw_spin_lock(raw_s
 		:"=m" (lock->slock) : : "memory");
 }
 
-#define __raw_spin_lock_flags(lock, flags) __raw_spin_lock(lock)
-
 static inline int __raw_spin_trylock(raw_spinlock_t *lock)
 {
 	int oldval;

@@ -57,6 +72,13 @@ static inline int __raw_spin_trylock(raw
 	return oldval > 0;
 }
 
+static inline void __raw_spin_lock_flags(raw_spinlock_t *lock, unsigned long flags)
+{
+	__asm__ __volatile__(
+		__raw_spin_lock_string_flags
+		:"=m" (lock->slock) : "r" (flags) : "memory");
+}
+
 static inline void __raw_spin_unlock(raw_spinlock_t *lock)
 {
 	__asm__ __volatile__(

Nick Piggin

2006-08-24 03:10 UTC #1 lore raw

Quoted 3 lines · Edward Falk

Add spin_lock_string_flags and _raw_spin_lock_flags() to asm-x86_64/spinlock.h so that _spin_lock_irqsave() has the same semantics on x86_64 as it does on i386 and does *not* have interrupts disabled while it is waiting for the lock.
This fix is courtesy of Michael Davidson

So, what's the bug? You shouldn't rely on these semantics anyway because you should never expect to wait for a spinlock for so long (and it may be the case that irqs can't be enabled anyway).

BTW. you should be cc'ing Andi Kleen (x86+/-64 maintainer) on this type of stuff.

No comments on the merits of adding this feature. I suppose parity with i386 is a good thing, though.

Signature · 2 lines

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com

Andrew Morton

2006-08-24 04:48 UTC #2 lore raw

Quoted 12 lines · On Thu, 24 Aug 2006 13:10:09 +1000 Nick Piggin <[email protected]> wrote:

Quoted 6 lines · Edward Falk
Add spin_lock_string_flags and _raw_spin_lock_flags() to asm-x86_64/spinlock.h so that _spin_lock_irqsave() has the same semantics on x86_64 as it does on i386 and does *not* have interrupts disabled while it is waiting for the lock.
This fix is courtesy of Michael Davidson
So, what's the bug? You shouldn't rely on these semantics anyway because you should never expect to wait for a spinlock for so long (and it may be the case that irqs can't be enabled anyway).

Quoted 6 lines

BTW. you should be cc'ing Andi Kleen (x86+/-64 maintainer) on this type of stuff.
No comments on the merits of adding this feature. I suppose parity with i386 is a good thing, though.

We put this into x86 ages ago and Andi ducked the x86_64 patch at the time.

I don't recall any reports about the x86 patch (Zwane?) improving or worsening anything. I guess there are some theoretical interrupt latency benefits.

Andi Kleen

2006-08-24 06:45 UTC #5 lore raw

In reply to Edward Falk

Quoted 4 lines · Edward Falk

Add spin_lock_string_flags and _raw_spin_lock_flags() to asm-x86_64/spinlock.h so that _spin_lock_irqsave() has the same semantics on x86_64 as it does on i386 and does *not* have interrupts disabled while it is waiting for the lock.

Did it fix anything for you?

-Andi

Suleiman Souhlal

2006-08-24 11:04 UTC #6 lore raw

Quoted 10 lines · Andi Kleen

Quoted 4 lines · Edward Falk
Add spin_lock_string_flags and _raw_spin_lock_flags() to asm-x86_64/spinlock.h so that _spin_lock_irqsave() has the same semantics on x86_64 as it does on i386 and does *not* have interrupts disabled while it is waiting for the lock.
Did it fix anything for you?

I think this was to work around the fact that some buggy drivers try to grab spinlocks without disabling interrupts when they should, which would cause deadlocks when trying to rendez-vous every cpu via IPIs.

It's a bad idea to spin with interrupts disabled when they could very well be enabled, anyway.

-- Suleiman

Arjan van de Ven

2006-08-24 11:13 UTC #7 lore raw

Quoted 10 lines · Suleiman Souhlal

Quoted 9 lines · Andi Kleen
Quoted 4 lines · Edward Falk
Add spin_lock_string_flags and _raw_spin_lock_flags() to asm-x86_64/spinlock.h so that _spin_lock_irqsave() has the same semantics on x86_64 as it does on i386 and does *not* have interrupts disabled while it is waiting for the lock.

Quoted 4 lines

Quoted 1 line
Did it fix anything for you?
I think this was to work around the fact that some buggy drivers try to grab spinlocks without disabling interrupts when they should,

then fix the drivers ;)

Andi Kleen

2006-08-24 11:33 UTC #8 lore raw

In reply to Suleiman Souhlal

Quoted 10 lines · Suleiman Souhlal

Quoted 9 lines · Andi Kleen
Quoted 4 lines · Edward Falk
Add spin_lock_string_flags and _raw_spin_lock_flags() to asm-x86_64/spinlock.h so that _spin_lock_irqsave() has the same semantics on x86_64 as it does on i386 and does *not* have interrupts disabled while it is waiting for the lock.

Quoted 5 lines

Quoted 1 line
Did it fix anything for you?
I think this was to work around the fact that some buggy drivers try to grab spinlocks without disabling interrupts when they should, which would cause deadlocks when trying to rendez-vous every cpu via IPIs.

That doesn't help them at all because they could then deadlock later.

In theory it is just a quite cheesy way to make lock contended code work a little better, but I was not aware of it actually helping in practice.

-Andi

Quoted 1 line

Suleiman Souhlal

2006-08-24 12:34 UTC #9 lore raw

Quoted 14 lines · Andi Kleen

Quoted 12 lines · Suleiman Souhlal
Quoted 10 lines · Andi Kleen
Quoted 4 lines · Edward Falk
Add spin_lock_string_flags and _raw_spin_lock_flags() to asm-x86_64/spinlock.h so that _spin_lock_irqsave() has the same semantics on x86_64 as it does on i386 and does *not* have interrupts disabled while it is waiting for the lock.

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
Did it fix anything for you?
I think this was to work around the fact that some buggy drivers try to grab spinlocks without disabling interrupts when they should, which would cause deadlocks when trying to rendez-vous every cpu via IPIs.
That doesn't help them at all because they could then deadlock later.

If the driver uses spin_lock() when it knows that the hardware won't generate the interrupt that would need to be masked, and spin_lock_irqsave() elsewhere, there shouldn't be any deadlocks unless IPIs are involved.

For example, say a driver uses spin_lock(&driver_lock) in its interrupt handler and spin_lock_irqsave(&driver_lock) elsewhere. Imagine CPU1 is handling a such a interrupt while CPU2 is trying to send a packet (for example).

In a regular situation, CPU1 shouldn't be interrupted by anything needing driver_lock, and so it should be able to complete and let CPU2 acquire the lock.

Now, if CPU3 is trying to do an IPI rendez-vous, it will interrupt CPU1 and try to interrupt CPU2 too. However, since spin_lock_irqsave() spins with interrupts disabled, the system will deadlock.

With this patch, IPI rendez-vous shouldn't cause these problems, since it will let the rendez-vous will be able to complete. Or am I missing something?

-- Suleiman

Arjan van de Ven

2006-08-24 13:22 UTC #10 lore raw

Quoted 22 lines · Suleiman Souhlal

Quoted 21 lines · Andi Kleen
Quoted 17 lines · Suleiman Souhlal
Quoted 11 lines · Andi Kleen
Quoted 4 lines · Edward Falk
Add spin_lock_string_flags and _raw_spin_lock_flags() to asm-x86_64/spinlock.h so that _spin_lock_irqsave() has the same semantics on x86_64 as it does on i386 and does *not* have interrupts disabled while it is waiting for the lock.
Did it fix anything for you?
I think this was to work around the fact that some buggy drivers try to grab spinlocks without disabling interrupts when they should, which would cause deadlocks when trying to rendez-vous every cpu via IPIs.

Quoted 6 lines

Quoted 1 line
That doesn't help them at all because they could then deadlock later.
If the driver uses spin_lock() when it knows that the hardware won't generate the interrupt that would need to be masked, and spin_lock_irqsave() elsewhere, there shouldn't be any deadlocks unless IPIs are involved.

this still is bad practice and lockdep will also scream about it

Can you point at ANY place that does this?

Suleiman Souhlal

2006-08-24 13:45 UTC #11 lore raw

Quoted 26 lines · Arjan van de Ven

Quoted 24 lines · Suleiman Souhlal
Quoted 22 lines · Andi Kleen
Quoted 17 lines · Suleiman Souhlal
Quoted 5 lines · Andi Kleen
Edward Falk <[email protected]> writes:
Quoted 7 lines
Quoted 4 lines
Add spin_lock_string_flags and _raw_spin_lock_flags() to asm-x86_64/spinlock.h so that _spin_lock_irqsave() has the same semantics on x86_64 as it does on i386 and does *not* have interrupts disabled while it is waiting for the lock.
Did it fix anything for you?
I think this was to work around the fact that some buggy drivers try to grab spinlocks without disabling interrupts when they should, which would cause deadlocks when trying to rendez-vous every cpu via IPIs.

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
That doesn't help them at all because they could then deadlock later.
If the driver uses spin_lock() when it knows that the hardware won't generate the interrupt that would need to be masked, and spin_lock_irqsave() elsewhere, there shouldn't be any deadlocks unless IPIs are involved.
this still is bad practice and lockdep will also scream about it

Great.

Quoted 1 line

Can you point at ANY place that does this?

From a quick inspection, drivers/net/forcedeth.c appears to do this.

-- Suleiman

Martin Bligh

2006-08-24 15:53 UTC #3 lore raw

In reply to Andrew Morton

Quoted 22 lines · Andrew Morton

Quoted 10 lines · On Thu, 24 Aug 2006 13:10:09 +1000 Nick Piggin <[email protected]> wrote:
Quoted 3 lines · Edward Falk
Add spin_lock_string_flags and _raw_spin_lock_flags() to asm-x86_64/spinlock.h so that _spin_lock_irqsave() has the same semantics on x86_64 as it does on i386 and does *not* have interrupts disabled while it is waiting for the lock.
This fix is courtesy of Michael Davidson
So, what's the bug? You shouldn't rely on these semantics anyway because you should never expect to wait for a spinlock for so long (and it may be the case that irqs can't be enabled anyway).
Quoted 6 lines
BTW. you should be cc'ing Andi Kleen (x86+/-64 maintainer) on this type of stuff.
No comments on the merits of adding this feature. I suppose parity with i386 is a good thing, though.

Quoted 5 lines

We put this into x86 ages ago and Andi ducked the x86_64 patch at the time.
I don't recall any reports about the x86 patch (Zwane?) improving or worsening anything. I guess there are some theoretical interrupt latency benefits.

Spinlocks are indeed meant to be held for a short time, but irq disabling is meant to be shorter.

I think the real question is: what is the justification for disabling interrupts when spinning for a lock? We should never disable interrupts unless we have to.

Andrew Morton

2006-08-25 04:38 UTC #12 lore raw

In reply to Andi Kleen

Quoted 9 lines · On 24 Aug 2006 08:45:11 +0200 Andi Kleen <[email protected]> wrote:

Quoted 4 lines · Edward Falk
Add spin_lock_string_flags and _raw_spin_lock_flags() to asm-x86_64/spinlock.h so that _spin_lock_irqsave() has the same semantics on x86_64 as it does on i386 and does *not* have interrupts disabled while it is waiting for the lock.
Did it fix anything for you?

It's the rendezvous-via-IPI problem. Suppose we want to capture all CPUs in an IPI handler (TSC sync, for example).

- CPUa holds read_lock(&tasklist_lock)
- CPUb is spinning in write_lock_irq(&taslist_lock)
- CPUa enters its IPI handler and spins
- CPUb never takes the IPI and we're dead.

Re-enabling interrupts while we spin will prevent that. But I suspect that if we ever want to implement IPI rendezvous (and cannot use the stop_machine_run() thing) then we might still have problems. A valid optimisation (which we use in some places) is:

	local_irq_save(flags);
	<stuff>
	write_lock(lock);

Nick Piggin

2006-08-25 05:34 UTC #13 lore raw

Quoted 24 lines · Andrew Morton

Quoted 10 lines · On 24 Aug 2006 08:45:11 +0200 Andi Kleen <[email protected]> wrote:
Quoted 4 lines · Edward Falk
Add spin_lock_string_flags and _raw_spin_lock_flags() to asm-x86_64/spinlock.h so that _spin_lock_irqsave() has the same semantics on x86_64 as it does on i386 and does *not* have interrupts disabled while it is waiting for the lock.
Did it fix anything for you?
It's the rendezvous-via-IPI problem. Suppose we want to capture all CPUs in an IPI handler (TSC sync, for example).
- CPUa holds read_lock(&tasklist_lock)
- CPUb is spinning in write_lock_irq(&taslist_lock)
- CPUa enters its IPI handler and spins
- CPUb never takes the IPI and we're dead.

Quoted 8 lines

Re-enabling interrupts while we spin will prevent that. But I suspect that if we ever want to implement IPI rendezvous (and cannot use the stop_machine_run() thing) then we might still have problems. A valid optimisation (which we use in some places) is:
	local_irq_save(flags);
	<stuff>
	write_lock(lock);

Yes, or it may be taken inside a section that needs interrupts off for correctness (eg. if it is holding an irq safe lock). And in the current implementation I don't think the plain _irq variants reenable interrupts because that would require reading the register.

Would it be sufficient to just do pair-wise rendezvous, where the initiating CPU is in a known good state? For TSC sync it might be...

Signature · 2 lines

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com

Andi Kleen

2006-08-25 06:22 UTC #14 lore raw

In reply to Andrew Morton

Quoted 10 lines · Andrew Morton

Quoted 7 lines · On 24 Aug 2006 08:45:11 +0200 Andi Kleen <[email protected]> wrote:
Quoted 4 lines · Edward Falk
Add spin_lock_string_flags and _raw_spin_lock_flags() to asm-x86_64/spinlock.h so that _spin_lock_irqsave() has the same semantics on x86_64 as it does on i386 and does *not* have interrupts disabled while it is waiting for the lock.

Quoted 8 lines

Quoted 2 lines
Did it fix anything for you?
It's the rendezvous-via-IPI problem. Suppose we want to capture all CPUs in an IPI handler (TSC sync, for example).
- CPUa holds read_lock(&tasklist_lock)
- CPUb is spinning in write_lock_irq(&taslist_lock)

But he didn't actually change the rwlocks, only the plain old spinlocks!

Anyways I applied the patch for now (and cleaned it up in the next patch), but I could have probably gotten away with not.

Edward, next time please add a Signed-off-by line.

-Andi

Keith Owens

2006-08-26 07:52 UTC #4 lore raw

In reply to Martin Bligh

Quoted 12 lines · Martin Bligh (on Thu, 24 Aug 2006 08:53:39 -0700) wrote:

Quoted 11 lines · Andrew Morton
Quoted 7 lines · On Thu, 24 Aug 2006 13:10:09 +1000 Nick Piggin <[email protected]> wrote:
Quoted 5 lines · Edward Falk
Add spin_lock_string_flags and _raw_spin_lock_flags() to asm-x86_64/spinlock.h so that _spin_lock_irqsave() has the same semantics on x86_64 as it does on i386 and does *not* have interrupts disabled while it is waiting for the lock.

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Quoted 1 line
This fix is courtesy of Michael Davidson
So, what's the bug? You shouldn't rely on these semantics anyway because you should never expect to wait for a spinlock for so long (and it may be the case that irqs can't be enabled anyway).

AFAICT the first architecture that enabled interrupts while spinning was IA64. http://www.gelato.unsw.edu.au/archives/linux-ia64/0404/9161.html
I did that patch because large NUMA systems were suffering from cache line bouncing on spinlocks which increased the time that interrupts were disabled. This effect tends not to show up on small systems, but it does make a measurable difference for large systems.

An additional benefit of enabling interrupts in the contention path is that it lets you get in with a profiler or debugger to track down deadlock or long lock problems. Enabling interrupts in the contention path has no disadvantages and it increases system responsiveness and availability.