[PATCH] Fix x86_64 _spin_lock_irqsave()

[PATCH] Fix x86_64 _spin_lock_irqsave()

[Edward Falk]

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

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

[-- Attachment #2: 2012926.patch --]
[-- Type: text/plain, Size: 1665 bytes --]

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

	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.
	
	PRESUBMIT=passed
	
	R=mbligh mikew
	OCL=2010261

diff -uprN linux-2.6.17/include/asm-x86_64/spinlock.h 2012926/include/asm-x86_64/spinlock.h
--- linux-2.6.17/include/asm-x86_64/spinlock.h	2006-06-17 18:49:35.000000000 -0700
+++ 2012926/include/asm-x86_64/spinlock.h	2006-07-12 16:09:50.000000000 -0700
@@ -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__(

Re: [PATCH] Fix x86_64 _spin_lock_irqsave()

[Nick Piggin]

Edward Falk wrote:
> 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.

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

Re: [PATCH] Fix x86_64 _spin_lock_irqsave()

[Andrew Morton]

On Thu, 24 Aug 2006 13:10:09 +1000
Nick Piggin <nickpiggin@yahoo.com.au> wrote:

> Edward Falk wrote:
> > 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.
> 

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.

Re: [PATCH] Fix x86_64 _spin_lock_irqsave()

[Andi Kleen]

Edward Falk <efalk@google.com> writes:

> 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

Re: [PATCH] Fix x86_64 _spin_lock_irqsave()

[Suleiman Souhlal]

Andi Kleen wrote:
> Edward Falk <efalk@google.com> writes:
> 
> 
>>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

Re: [PATCH] Fix x86_64 _spin_lock_irqsave()

[Arjan van de Ven]

On Thu, 2006-08-24 at 13:04 +0200, Suleiman Souhlal wrote:
> Andi Kleen wrote:
> > Edward Falk <efalk@google.com> writes:
> > 
> > 
> >>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,

then fix the drivers ;)

Re: [PATCH] Fix x86_64 _spin_lock_irqsave()

[Andi Kleen]

On Thursday 24 August 2006 13:04, Suleiman Souhlal wrote:
> Andi Kleen wrote:
> > Edward Falk <efalk@google.com> writes:
> > 
> > 
> >>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.

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


> 

Re: [PATCH] Fix x86_64 _spin_lock_irqsave()

[Suleiman Souhlal]

Andi Kleen wrote:
> On Thursday 24 August 2006 13:04, Suleiman Souhlal wrote:
> 
>>Andi Kleen wrote:
>>
>>>Edward Falk <efalk@google.com> writes:
>>>
>>>
>>>
>>>>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.
> 
> 
> 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

Re: [PATCH] Fix x86_64 _spin_lock_irqsave()

[Arjan van de Ven]

On Thu, 2006-08-24 at 14:33 +0200, Suleiman Souhlal wrote:
> Andi Kleen wrote:
> > On Thursday 24 August 2006 13:04, Suleiman Souhlal wrote:
> > 
> >>Andi Kleen wrote:
> >>
> >>>Edward Falk <efalk@google.com> writes:
> >>>
> >>>
> >>>
> >>>>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.
> > 
> > 
> > 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? 

Re: [PATCH] Fix x86_64 _spin_lock_irqsave()

[Suleiman Souhlal]

Arjan van de Ven wrote:
> On Thu, 2006-08-24 at 14:33 +0200, Suleiman Souhlal wrote:
> 
>>Andi Kleen wrote:
>>
>>>On Thursday 24 August 2006 13:04, Suleiman Souhlal wrote:
>>>
>>>
>>>>Andi Kleen wrote:
>>>>
>>>>
>>>>>Edward Falk <efalk@google.com> writes:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>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.
>>>
>>>
>>>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.

> Can you point at ANY place that does this? 

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

-- Suleiman

Re: [PATCH] Fix x86_64 _spin_lock_irqsave()

[Martin Bligh]

Andrew Morton wrote:
> On Thu, 24 Aug 2006 13:10:09 +1000
> Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> 
> 
>>Edward Falk wrote:
>>
>>>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.
>>
> 
> 
> 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.

M.

Re: [PATCH] Fix x86_64 _spin_lock_irqsave()

[Andrew Morton]

On 24 Aug 2006 08:45:11 +0200
Andi Kleen <ak@suse.de> wrote:

> Edward Falk <efalk@google.com> writes:
> 
> > 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);

Re: [PATCH] Fix x86_64 _spin_lock_irqsave()

[Nick Piggin]

Andrew Morton wrote:
> On 24 Aug 2006 08:45:11 +0200
> Andi Kleen <ak@suse.de> wrote:
> 
> 
>>Edward Falk <efalk@google.com> writes:
>>
>>
>>>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);

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...

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

Re: [PATCH] Fix x86_64 _spin_lock_irqsave()

[Andi Kleen]

On Friday 25 August 2006 06:38, Andrew Morton wrote:
> On 24 Aug 2006 08:45:11 +0200
> Andi Kleen <ak@suse.de> wrote:
> 
> > Edward Falk <efalk@google.com> writes:
> > 
> > > 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)

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

Re: [PATCH] Fix x86_64 _spin_lock_irqsave()

[Keith Owens]

Martin Bligh (on Thu, 24 Aug 2006 08:53:39 -0700) wrote:
>Andrew Morton wrote:
>> On Thu, 24 Aug 2006 13:10:09 +1000
>> Nick Piggin <nickpiggin@yahoo.com.au> wrote:
>> 
>> 
>>>Edward Falk wrote:
>>>
>>>>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).

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.