Xen devel - Mar 2009 - [Patch] don''t spin with irq disabled

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


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

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


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

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

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

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



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

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


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

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

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
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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



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

> 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
> 
> 
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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



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

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

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

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

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

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

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

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



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

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

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

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



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

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

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

> 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
> 
> 
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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



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