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__(
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
On Thu, 24 Aug 2006 13:10:09 +1000
Nick Piggin <[email protected]> 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.
Edward Falk <[email protected]> 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
Andi Kleen wrote:
> Edward Falk <[email protected]> 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
On Thu, 2006-08-24 at 13:04 +0200, Suleiman Souhlal wrote:
> Andi Kleen wrote:
> > Edward Falk <[email protected]> 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 ;)
On Thursday 24 August 2006 13:04, Suleiman Souhlal wrote:
> Andi Kleen wrote:
> > Edward Falk <[email protected]> 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
>
Andi Kleen wrote:
> On Thursday 24 August 2006 13:04, Suleiman Souhlal wrote:
>
>>Andi Kleen wrote:
>>
>>>Edward Falk <[email protected]> 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
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 <[email protected]> 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?
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 <[email protected]> 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
Andrew Morton wrote:
> On Thu, 24 Aug 2006 13:10:09 +1000
> Nick Piggin <[email protected]> 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.
On 24 Aug 2006 08:45:11 +0200
Andi Kleen <[email protected]> wrote:
> Edward Falk <[email protected]> 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);
Andrew Morton wrote:
> On 24 Aug 2006 08:45:11 +0200
> Andi Kleen <[email protected]> wrote:
>
>
>>Edward Falk <[email protected]> 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
On Friday 25 August 2006 06:38, Andrew Morton wrote:
> On 24 Aug 2006 08:45:11 +0200
> Andi Kleen <[email protected]> wrote:
>
> > Edward Falk <[email protected]> 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
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 <[email protected]> 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.