[Patch] don't spin with irq disabled

[Patch] don't spin with irq disabled

[Juergen Gross]

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

Attached patch reduces interrupt latency in lock handling.
spin_lock_irq and spin_lock_irqsave used to turn off IRQs and then tried to
get the lock. If the lock was already held, waiting for the lock was done with
IRQs still off.
The patch reenables IRQs during the wait loop.

read/write locks seem to be rarely used, so I didn't change them.

In favor for ia64 I chose not to modify the assembler code :-)


Juergen

-- 
Juergen Gross                             Principal Developer
IP SW OS6                      Telephone: +49 (0) 89 636 47950
Fujitsu Siemens Computers         e-mail: juergen.gross@fujitsu-siemens.com
Otto-Hahn-Ring 6                Internet: www.fujitsu-siemens.com
D-81739 Muenchen         Company details: www.fujitsu-siemens.com/imprint.html

[-- Attachment #2: spinirq.patch --]
[-- Type: text/x-patch, Size: 1267 bytes --]

Signed-off-by: juergen.gross@fujitsu-siemens.com

# HG changeset patch
# User juergen.gross@fujitsu-siemens.com
# Date 1238057346 -3600
# Node ID b42d379abeb555152c835b1fe065666cb2438c28
# Parent  0b13d9787622d5e1d447a21657394805bb96d26f
don't spin with irq disabled

diff -r 0b13d9787622 -r b42d379abeb5 xen/common/spinlock.c
--- a/xen/common/spinlock.c	Tue Mar 24 06:55:29 2009 +0000
+++ b/xen/common/spinlock.c	Thu Mar 26 09:49:06 2009 +0100
@@ -2,6 +2,7 @@
 #include <xen/irq.h>
 #include <xen/smp.h>
 #include <xen/spinlock.h>
+#include <asm/processor.h>
 
 #ifndef NDEBUG
 
@@ -51,7 +52,13 @@
     ASSERT(local_irq_is_enabled());
     local_irq_disable();
     check_lock(&lock->debug);
-    _raw_spin_lock(&lock->raw);
+    while ( unlikely(!_raw_spin_trylock(&lock->raw)) )
+    {
+        local_irq_enable();
+        cpu_relax();
+        local_irq_disable();
+    }
+    return;
 }
 
 unsigned long _spin_lock_irqsave(spinlock_t *lock)
@@ -59,7 +66,12 @@
     unsigned long flags;
     local_irq_save(flags);
     check_lock(&lock->debug);
-    _raw_spin_lock(&lock->raw);
+    while ( unlikely(!_raw_spin_trylock(&lock->raw)) )
+    {
+        local_irq_restore(flags);
+        cpu_relax();
+        local_irq_save(flags);
+    }
     return flags;
 }
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [Patch] don't spin with irq disabled

[Jan Beulich]

>>> Juergen Gross <juergen.gross@fujitsu-siemens.com> 26.03.09 10:00 >>>
>Attached patch reduces interrupt latency in lock handling.
>spin_lock_irq and spin_lock_irqsave used to turn off IRQs and then tried to
>get the lock. If the lock was already held, waiting for the lock was done with
>IRQs still off.
>The patch reenables IRQs during the wait loop.

This is wrong - you must not enable interrupts if they weren't enabled upon
entry to these two functions.

Jan

Re: [Patch] don't spin with irq disabled

[Juergen Gross]

Jan Beulich wrote:
>>>> Juergen Gross <juergen.gross@fujitsu-siemens.com> 26.03.09 10:00 >>>
>> Attached patch reduces interrupt latency in lock handling.
>> spin_lock_irq and spin_lock_irqsave used to turn off IRQs and then tried to
>> get the lock. If the lock was already held, waiting for the lock was done with
>> IRQs still off.
>> The patch reenables IRQs during the wait loop.
> 
> This is wrong - you must not enable interrupts if they weren't enabled upon
> entry to these two functions.

spin_lock_irq disables always IRQs. spin_unlock_irq enables always IRQs. They
are always used in pairs, so IRQs should always be enabled on entry of
spin_lock_irq.
I'm not enabling IRQs unconditionally in spin_lock_irqsave, of course, but use
the flags value saved before...

Did I miss something? Or did you refer only to my inexact comment above?


Juergen

-- 
Juergen Gross                             Principal Developer
IP SW OS6                      Telephone: +49 (0) 89 636 47950
Fujitsu Siemens Computers         e-mail: juergen.gross@fujitsu-siemens.com
Otto-Hahn-Ring 6                Internet: www.fujitsu-siemens.com
D-81739 Muenchen         Company details: www.fujitsu-siemens.com/imprint.html

Re: [Patch] don't spin with irq disabled

[Jan Beulich]

>>> Juergen Gross <juergen.gross@fujitsu-siemens.com> 26.03.09 13:36 >>>
>Jan Beulich wrote:
>>>>> Juergen Gross <juergen.gross@fujitsu-siemens.com> 26.03.09 10:00 >>>
>>> Attached patch reduces interrupt latency in lock handling.
>>> spin_lock_irq and spin_lock_irqsave used to turn off IRQs and then tried to
>>> get the lock. If the lock was already held, waiting for the lock was done with
>>> IRQs still off.
>>> The patch reenables IRQs during the wait loop.
>> 
>> This is wrong - you must not enable interrupts if they weren't enabled upon
>> entry to these two functions.
>
>spin_lock_irq disables always IRQs. spin_unlock_irq enables always IRQs. They
>are always used in pairs, so IRQs should always be enabled on entry of
>spin_lock_irq.

No, I wouldn't suggest making an assumption like this - some code could allow
interrupts to be disabled when acquiring the lock, but intentionally enabling
them when releasing it. (Personally, I think there shouldn't be any users of
this function pair in the first place, as I don't think forcibly enabling interrupts
is a correct thing to do in all but *very* few cases.)

>I'm not enabling IRQs unconditionally in spin_lock_irqsave, of course, but use
>the flags value saved before...

Oh, sorry, I blindly implied the second function to use the same methods as
the first one. And really I'd think it's cheaper to use local_irq_disable() here
(but of course retain local_irq_restore()).

Btw., why do you think the issue is more important to address in Xen than
in Linux (where not to long ago the opposite move happened, since re-
enabling interrupts with ticket locks is much trickier and hence wasn't
considered worthwhile afair.

Jan

Re: [Patch] don't spin with irq disabled

[Keir Fraser]

On 27/03/2009 07:41, "Jan Beulich" <jbeulich@novell.com> wrote:

>> spin_lock_irq disables always IRQs. spin_unlock_irq enables always IRQs. They
>> are always used in pairs, so IRQs should always be enabled on entry of
>> spin_lock_irq.
> 
> No, I wouldn't suggest making an assumption like this - some code could allow
> interrupts to be disabled when acquiring the lock, but intentionally enabling
> them when releasing it. (Personally, I think there shouldn't be any users of
> this function pair in the first place, as I don't think forcibly enabling
> interrupts
> is a correct thing to do in all but *very* few cases.)

It seems to me a perfectly reasonable function pair to use when using an
IRQ-safe lock from code that you know cannot possibly already have IRQs
disabled. Anyone using the function pair to implicitly enable interrupts is
an evil person, and I already put assertions in to prevent that kind of
thing, in _{spin,read,write}_lock_irq(). The assertions only fired in a few
cases, and all of them looked like genuine bugs.

Anyway, as to the more general question of is this all worthwhile, I'm not
sure. I actually like the potential advantage of needing fewer and simpler
asm stubs in arch-specific spinlock.h, since Juergen has accidentally fallen
on an opportunity to get rid of _raw_spin_lock. I'm not sure whether
_raw_trylock is more expensive than _raw_lock though. On x86 it's the
difference between lock;dec and xchg -- I don't see any reason why one would
be more expensive than the other.

 -- Keir

spinlock requests (was RE: [Patch] don't spin with irq disabled)

[Dan Magenheimer]

Keir (and/or others) --

If you are messing around in the spinlock code anyway,
I have a couple of requests.  Tmem needs:

rw_is_write_locked(rwlock_t *lock)

and

write_trylock(rwlock_t *lock)

I implemented the latter by grabbing the C code from Linux
and it seems to work, but it would be nice if it was
consistent with the other lock code in xen and
my asm statement understanding is too poor to try.

For rw_is_write_locked(), I had a devil of a time
because of what appeared to be a weird code generated
race; the obvious simple implementation failed
periodically... apparently due to racing against
try_readlock attempts!  (I use it in an ASSERT so it
was a rather noticeable and spectacular failure!)
The workaround I used below is a horrible hack
but I haven't had problems since.

Thanks,
Dan

(include/asm-x86/spinlock.h)
+static inline int write_trylock(rwlock_t *lock)
+{
+	atomic_t *count = (atomic_t *)lock;
+	if (atomic_sub_and_test(RW_LOCK_BIAS, count))
+		return 1;
+	atomic_add(RW_LOCK_BIAS, count);
+	return 0;
+}

(include/asm-x86/spinlock.h)
+#define _raw_rw_is_write_locked(x) (((x)->lock) <= 0)

(common/spinlock.c))
+int _rw_is_write_locked(rwlock_t *lock)
+{
+#if 0
+    check_lock(&lock->debug);
+    return _raw_rw_is_write_locked(&lock->raw);
+#else
+    int val;
+
+    /* some weird code generation race? this works, above doesn't */
+    check_lock(&lock->debug);
+    val = (&lock->raw)->lock;
+    if (val > 0)  /* FIXME remove if ever called when expects not locked */
+        printk("*** val=%d\n",val);
+    return val <= 0;
+#endif
+}


> -----Original Message-----
> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
> Sent: Friday, March 27, 2009 3:10 AM
> To: Jan Beulich; Juergen Gross
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [Patch] don't spin with irq disabled
> 
> 
> On 27/03/2009 07:41, "Jan Beulich" <jbeulich@novell.com> wrote:
> 
> >> spin_lock_irq disables always IRQs. spin_unlock_irq 
> enables always IRQs. They
> >> are always used in pairs, so IRQs should always be enabled 
> on entry of
> >> spin_lock_irq.
> > 
> > No, I wouldn't suggest making an assumption like this - 
> some code could allow
> > interrupts to be disabled when acquiring the lock, but 
> intentionally enabling
> > them when releasing it. (Personally, I think there 
> shouldn't be any users of
> > this function pair in the first place, as I don't think 
> forcibly enabling
> > interrupts
> > is a correct thing to do in all but *very* few cases.)
> 
> It seems to me a perfectly reasonable function pair to use 
> when using an
> IRQ-safe lock from code that you know cannot possibly already 
> have IRQs
> disabled. Anyone using the function pair to implicitly enable 
> interrupts is
> an evil person, and I already put assertions in to prevent 
> that kind of
> thing, in _{spin,read,write}_lock_irq(). The assertions only 
> fired in a few
> cases, and all of them looked like genuine bugs.
> 
> Anyway, as to the more general question of is this all 
> worthwhile, I'm not
> sure. I actually like the potential advantage of needing 
> fewer and simpler
> asm stubs in arch-specific spinlock.h, since Juergen has 
> accidentally fallen
> on an opportunity to get rid of _raw_spin_lock. I'm not sure whether
> _raw_trylock is more expensive than _raw_lock though. On x86 it's the
> difference between lock;dec and xchg -- I don't see any 
> reason why one would
> be more expensive than the other.
> 
>  -- Keir
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>

Re: spinlock requests (was RE: [Patch] don't spin with irq disabled)

[Keir Fraser]

On 27/03/2009 17:00, "Dan Magenheimer" <dan.magenheimer@oracle.com> wrote:

> For rw_is_write_locked(), I had a devil of a time
> because of what appeared to be a weird code generated
> race; the obvious simple implementation failed
> periodically... apparently due to racing against
> try_readlock attempts!  (I use it in an ASSERT so it
> was a rather noticeable and spectacular failure!)
> The workaround I used below is a horrible hack
> but I haven't had problems since.

What try_readlock would that be? There is no such function. The failing
version of rw_is_write_locked() is exactly what I would implement. I don't
see how it could race other lock acquisition attempts -- if anyone could see
the lock field as >0 then read_lock attempts could spuriously fail. It
should especially definitely work if you are ASSERTing that the CPU you are
running on has the write lock. If you are ASSERTing about the lock being
held by other CPUs, perhaps there could be races in that case?

Actually I would argue that rw_is_write_locked() is hard to implement
accurately -- a reader and a writer can both update the lock field; only the
first to update wins the race; and an external observer of the lock field
cannot tell whether the reader or writer won unless it spins and waits for
the failed party to undo its update. But this can only result in false
positives (returns TRUE when the writer has actually failed to grab the
lock) I think, which will generally be benign for the kinds of assertion
statements you want to write. OTOH it makes me uncertain about providing
this function, and I wonder if you should just use rw_is_locked().

 -- Keir

RE: spinlock requests (was RE: [Patch] don't spin with irq disabled)

[Dan Magenheimer]

> What try_readlock would that be? There is no such function. 

Oops, my memory was faulty.  It is read_lock() itself
that decrements the lock and then re-increments it.
This appeared to be causing the race.  When I added
debug code, the problem went away (which was what led
me to the "hack" working version).

> I wonder if you should just use rw_is_locked().

IIRC, this doesn't do the job.

When I have a chance and a spare test machine, I'll
try to reproduce/reconfirm the problem.

But in any case I'd still like to see an asm version of
write_trylock.

Thanks,
Dan

> -----Original Message-----
> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
> Sent: Friday, March 27, 2009 11:46 AM
> To: Dan Magenheimer; Jan Beulich; Juergen Gross
> Cc: xen-devel@lists.xensource.com
> Subject: Re: spinlock requests (was RE: [Xen-devel] [Patch] don't spin
> with irq disabled)
> 
> 
> On 27/03/2009 17:00, "Dan Magenheimer" 
> <dan.magenheimer@oracle.com> wrote:
> 
> > For rw_is_write_locked(), I had a devil of a time
> > because of what appeared to be a weird code generated
> > race; the obvious simple implementation failed
> > periodically... apparently due to racing against
> > try_readlock attempts!  (I use it in an ASSERT so it
> > was a rather noticeable and spectacular failure!)
> > The workaround I used below is a horrible hack
> > but I haven't had problems since.
> 
> What try_readlock would that be? There is no such function. 
> The failing
> version of rw_is_write_locked() is exactly what I would 
> implement. I don't
> see how it could race other lock acquisition attempts -- if 
> anyone could see
> the lock field as >0 then read_lock attempts could spuriously fail. It
> should especially definitely work if you are ASSERTing that 
> the CPU you are
> running on has the write lock. If you are ASSERTing about the 
> lock being
> held by other CPUs, perhaps there could be races in that case?
> 
> Actually I would argue that rw_is_write_locked() is hard to implement
> accurately -- a reader and a writer can both update the lock 
> field; only the
> first to update wins the race; and an external observer of 
> the lock field
> cannot tell whether the reader or writer won unless it spins 
> and waits for
> the failed party to undo its update. But this can only result in false
> positives (returns TRUE when the writer has actually failed 
> to grab the
> lock) I think, which will generally be benign for the kinds 
> of assertion
> statements you want to write. OTOH it makes me uncertain 
> about providing
> this function, and I wonder if you should just use rw_is_locked().
> 
>  -- Keir
> 
> 
>

Re: spinlock requests (was RE: [Patch] don't spin with irq disabled)

[Keir Fraser]

On 27/03/2009 18:00, "Dan Magenheimer" <dan.magenheimer@oracle.com> wrote:

>> What try_readlock would that be? There is no such function.
> 
> Oops, my memory was faulty.  It is read_lock() itself
> that decrements the lock and then re-increments it.
> This appeared to be causing the race.  When I added
> debug code, the problem went away (which was what led
> me to the "hack" working version).

Well the race is still impossible afaics. Unless your
_raw_rw_is_write_locked() was checking for ==0 rather than <=0. I think
something fishy was going on in your actual original implementation if
is_write_locked(). :-)

 -- Keir

RE: spinlock requests (was RE: [Patch] don't spin with irq disabled)

[Dan Magenheimer]

> Well the race is still impossible afaics. Unless your
> _raw_rw_is_write_locked() was checking for ==0 rather than 
> <=0.

Finally got a chance to go back and reproduce and figure
this one out.  The crash is still occurring.  My code
definitely checks for <=0.  BUT the declaration of
raw_rwlock_t declares the lock as "volatile UNSIGNED int",
so the compiler blithely generates a check for == 0.

Can I assume the fix is to change the declaration to
int instead of unsigned int, or will that cause problems
elsewhere?

Thanks,
Dan

> -----Original Message-----
> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
> Sent: Friday, March 27, 2009 12:12 PM
> To: Dan Magenheimer; Jan Beulich; Juergen Gross
> Cc: xen-devel@lists.xensource.com
> Subject: Re: spinlock requests (was RE: [Xen-devel] [Patch] don't spin
> with irq disabled)
> 
> 
> On 27/03/2009 18:00, "Dan Magenheimer" 
> <dan.magenheimer@oracle.com> wrote:
> 
> >> What try_readlock would that be? There is no such function.
> > 
> > Oops, my memory was faulty.  It is read_lock() itself
> > that decrements the lock and then re-increments it.
> > This appeared to be causing the race.  When I added
> > debug code, the problem went away (which was what led
> > me to the "hack" working version).
> 
> Well the race is still impossible afaics. Unless your
> _raw_rw_is_write_locked() was checking for ==0 rather than 
> <=0. I think
> something fishy was going on in your actual original implementation if
> is_write_locked(). :-)
> 
>  -- Keir
> 
> 
>

Re: spinlock requests (was RE: [Patch] don't spin with irq disabled)

[Keir Fraser]

On 03/04/2009 00:08, "Dan Magenheimer" <dan.magenheimer@oracle.com> wrote:

>> Well the race is still impossible afaics. Unless your
>> _raw_rw_is_write_locked() was checking for ==0 rather than
>> <=0.
> 
> Finally got a chance to go back and reproduce and figure
> this one out.  The crash is still occurring.  My code
> definitely checks for <=0.  BUT the declaration of
> raw_rwlock_t declares the lock as "volatile UNSIGNED int",
> so the compiler blithely generates a check for == 0.
> 
> Can I assume the fix is to change the declaration to
> int instead of unsigned int, or will that cause problems
> elsewhere?

Oh dear. Yes, that's the fix!

 -- Keir

Re: spinlock requests (was RE: [Patch] don't spin with irq disabled)

[Juergen Gross]

Dan Magenheimer wrote:
> Keir (and/or others) --
> 
> If you are messing around in the spinlock code anyway,
> I have a couple of requests.  Tmem needs:
> 
> rw_is_write_locked(rwlock_t *lock)
> 
> and
> 
> write_trylock(rwlock_t *lock)
> 
> I implemented the latter by grabbing the C code from Linux
> and it seems to work, but it would be nice if it was
> consistent with the other lock code in xen and
> my asm statement understanding is too poor to try.
> 
> For rw_is_write_locked(), I had a devil of a time
> because of what appeared to be a weird code generated
> race; the obvious simple implementation failed
> periodically... apparently due to racing against
> try_readlock attempts!  (I use it in an ASSERT so it
> was a rather noticeable and spectacular failure!)
> The workaround I used below is a horrible hack
> but I haven't had problems since.
> 
> Thanks,
> Dan

Dan,

if you are planning to use rw_locks you should be aware that the current
implementation in Xen is sub-optimal on systems with high processor counts.
Read locks always succeed when other readers are already holding the lock,
even if a writer is waiting for the lock. If there are many potential readers
they might (in theory) lock out a writer for rather long times.
A better solution would be to stop further readers to acquire the lock if a
writer is waiting for it.

Juergen

-- 
Juergen Gross                             Principal Developer
IP SW OS6                      Telephone: +49 (0) 89 636 47950
Fujitsu Siemens Computers         e-mail: juergen.gross@fujitsu-siemens.com
Otto-Hahn-Ring 6                Internet: www.fujitsu-siemens.com
D-81739 Muenchen         Company details: www.fujitsu-siemens.com/imprint.html

RE: spinlock requests (was RE: [Patch] don't spin with irq disabled)

[Dan Magenheimer]

Thanks Juergen.  Do you know of any GPLv2 code that implements
this improved rwlock solution?  (I don't think Linux does,
does it?)

Dan

> -----Original Message-----
> From: Juergen Gross [mailto:juergen.gross@fujitsu-siemens.com]
> Sent: Monday, March 30, 2009 12:12 AM
> To: Dan Magenheimer
> Cc: Keir Fraser; Jan Beulich; xen-devel@lists.xensource.com
> Subject: Re: spinlock requests (was RE: [Xen-devel] [Patch] don't spin
> with irq disabled)
> 
> 
> Dan Magenheimer wrote:
> > Keir (and/or others) --
> > 
> > If you are messing around in the spinlock code anyway,
> > I have a couple of requests.  Tmem needs:
> > 
> > rw_is_write_locked(rwlock_t *lock)
> > 
> > and
> > 
> > write_trylock(rwlock_t *lock)
> > 
> > I implemented the latter by grabbing the C code from Linux
> > and it seems to work, but it would be nice if it was
> > consistent with the other lock code in xen and
> > my asm statement understanding is too poor to try.
> > 
> > For rw_is_write_locked(), I had a devil of a time
> > because of what appeared to be a weird code generated
> > race; the obvious simple implementation failed
> > periodically... apparently due to racing against
> > try_readlock attempts!  (I use it in an ASSERT so it
> > was a rather noticeable and spectacular failure!)
> > The workaround I used below is a horrible hack
> > but I haven't had problems since.
> > 
> > Thanks,
> > Dan
> 
> Dan,
> 
> if you are planning to use rw_locks you should be aware that 
> the current
> implementation in Xen is sub-optimal on systems with high 
> processor counts.
> Read locks always succeed when other readers are already 
> holding the lock,
> even if a writer is waiting for the lock. If there are many 
> potential readers
> they might (in theory) lock out a writer for rather long times.
> A better solution would be to stop further readers to acquire 
> the lock if a
> writer is waiting for it.
> 
> Juergen
> 
> -- 
> Juergen Gross                             Principal Developer
> IP SW OS6                      Telephone: +49 (0) 89 636 47950
> Fujitsu Siemens Computers         e-mail: 
> juergen.gross@fujitsu-siemens.com
> Otto-Hahn-Ring 6                Internet: www.fujitsu-siemens.com
> D-81739 Muenchen         Company details: 
www.fujitsu-siemens.com/imprint.html

Re: spinlock requests (was RE: [Patch] don't spin with irq disabled)

[Juergen Gross]

Dan Magenheimer wrote:
> Thanks Juergen.  Do you know of any GPLv2 code that implements
> this improved rwlock solution?  (I don't think Linux does,
> does it?)

Good question.
I just looked into the Linux code and decided not to analyse it. :-)
I have implemented a solution for our BS2000 system on Xen. It is just
a rather simple state machine using the cmpxchg instruction for the
update of the (structured) lock word.
If there is common interest for this solution I could prepare a patch.


Juergen

>> -----Original Message-----
>> From: Juergen Gross [mailto:juergen.gross@fujitsu-siemens.com]
>> Sent: Monday, March 30, 2009 12:12 AM
>> To: Dan Magenheimer
>> Cc: Keir Fraser; Jan Beulich; xen-devel@lists.xensource.com
>> Subject: Re: spinlock requests (was RE: [Xen-devel] [Patch] don't spin
>> with irq disabled)
>>
>> if you are planning to use rw_locks you should be aware that 
>> the current
>> implementation in Xen is sub-optimal on systems with high 
>> processor counts.
>> Read locks always succeed when other readers are already 
>> holding the lock,
>> even if a writer is waiting for the lock. If there are many 
>> potential readers
>> they might (in theory) lock out a writer for rather long times.
>> A better solution would be to stop further readers to acquire 
>> the lock if a
>> writer is waiting for it.



-- 
Juergen Gross                             Principal Developer
IP SW OS6                      Telephone: +49 (0) 89 636 47950
Fujitsu Siemens Computers         e-mail: juergen.gross@fujitsu-siemens.com
Otto-Hahn-Ring 6                Internet: www.fujitsu-siemens.com
D-81739 Muenchen         Company details: www.fujitsu-siemens.com/imprint.html

Re: spinlock requests (was RE: [Patch] don't spin with irq disabled)

[Keir Fraser]

On 31/03/2009 14:40, "Juergen Gross" <juergen.gross@fujitsu-siemens.com>
wrote:

> Dan Magenheimer wrote:
>> Thanks Juergen.  Do you know of any GPLv2 code that implements
>> this improved rwlock solution?  (I don't think Linux does,
>> does it?)
> 
> Good question.
> I just looked into the Linux code and decided not to analyse it. :-)
> I have implemented a solution for our BS2000 system on Xen. It is just
> a rather simple state machine using the cmpxchg instruction for the
> update of the (structured) lock word.
> If there is common interest for this solution I could prepare a patch.

If we care that much about fairness we should use ticket- or queue-based
locks. I don't believe any of our locks are contended enough to be a
concern. If they were, that would be a concern in itself.

 -- Keir

Re: spinlock requests (was RE: [Patch] don't spin with irq disabled)

[Jeremy Fitzhardinge]

Keir Fraser wrote:
> On 31/03/2009 14:40, "Juergen Gross" <juergen.gross@fujitsu-siemens.com>
> wrote:
>
>   
>> Dan Magenheimer wrote:
>>     
>>> Thanks Juergen.  Do you know of any GPLv2 code that implements
>>> this improved rwlock solution?  (I don't think Linux does,
>>> does it?)
>>>       
>> Good question.
>> I just looked into the Linux code and decided not to analyse it. :-)
>> I have implemented a solution for our BS2000 system on Xen. It is just
>> a rather simple state machine using the cmpxchg instruction for the
>> update of the (structured) lock word.
>> If there is common interest for this solution I could prepare a patch.
>>     
>
> If we care that much about fairness we should use ticket- or queue-based
> locks. I don't believe any of our locks are contended enough to be a
> concern. If they were, that would be a concern in itself.
>   

Writer vs reader fairness in rwlocks is different from normal spinlock 
fairness.  One presumes that you're expecting to get multiple readers if 
you choose to use a rwlock, but that can end up excluding writers for an 
unbounded amount of time.

There was a big discussion of this on lkml about 6-9 months ago, because 
people were seeing writers held off for long periods of time.  I think 
the kernel's rwlock now blocks new readers if a writer if waiting for 
the lock.

    J

Re: spinlock requests (was RE: [Patch] don't spin with irq disabled)

[Keir Fraser]

On 31/03/2009 20:00, "Jeremy Fitzhardinge" <jeremy@goop.org> wrote:

>> If we care that much about fairness we should use ticket- or queue-based
>> locks. I don't believe any of our locks are contended enough to be a
>> concern. If they were, that would be a concern in itself.
> 
> Writer vs reader fairness in rwlocks is different from normal spinlock
> fairness.  One presumes that you're expecting to get multiple readers if
> you choose to use a rwlock, but that can end up excluding writers for an
> unbounded amount of time.

I suspect the existing uses of rwlock in Xen actually are because that
seemed a natural fit for the code -- obvious split between reader and writer
critical sections -- rather than because of excessive serialisation if using
a normal spinlock. I strongly disbelieve that lock acquire/release is a
significant performance bottleneck for us right now.

 -- Keir

Re: spinlock requests (was RE: [Patch] don't spin with irq disabled)

[Jeremy Fitzhardinge]

Keir Fraser wrote:
> I suspect the existing uses of rwlock in Xen actually are because that
> seemed a natural fit for the code -- obvious split between reader and writer
> critical sections -- rather than because of excessive serialisation if using
> a normal spinlock. I strongly disbelieve that lock acquire/release is a
> significant performance bottleneck for us right now.
>   

Aren't rwlocks sufficiently less efficient than spinlocks that you'd 
tend to use the latter if you don't think contention is an issue?

    J

Re: [Patch] don't spin with irq disabled

[Keir Fraser]

On 26/03/2009 09:00, "Juergen Gross" <juergen.gross@fujitsu-siemens.com>
wrote:

> Attached patch reduces interrupt latency in lock handling.
> spin_lock_irq and spin_lock_irqsave used to turn off IRQs and then tried to
> get the lock. If the lock was already held, waiting for the lock was done with
> IRQs still off.
> The patch reenables IRQs during the wait loop.
> 
> read/write locks seem to be rarely used, so I didn't change them.
> 
> In favor for ia64 I chose not to modify the assembler code :-)

This will ping-pong the spinlock cache line when spinning among multiple
processors. Still, getting rid of _raw_spin_lock is an interesting idea, and
perhaps your scheme will work okay if modified as:
 while (unlikely(!raw_spin_trylock))
  while (likely(raw_spin_is_locked))
    ...;
I'll think about it -- certainly it would cull loads of crap from ia64's
spinlock.h. No need to send another patch.

 -- Keir