[parisc-linux] some more questions about __raw_write_trylock() hppa implementation

[parisc-linux] some more questions about __raw_write_trylock() hppa implementation

[Joel Soete]

Hello all,

I noticed that:
static  __inline__ int __raw_write_trylock(raw_rwlock_t *rw)
{
         __raw_spin_lock(&rw->lock);
         if (rw->counter != 0) {
                 /* this basically never happens */
                 __raw_spin_unlock(&rw->lock);

                 return 0;
         }

         /* got it.  now leave without unlocking */
         rw->counter = -1; /* remember we are locked */
         return 1;
}


so depending the counter is null or not the lock is kept or relaxed?

following my smp pb invextigation the pb I encounter seems to occure rarely and so according to above comment "/* this basically 
never happens */" I place here a panic() in the hope to get a better trace if it start to hang by this place. Unfortunately the 
comment is wrong and system panicing at boot time ;-(

Looking to some other implementation (powerpc and sh in particular), I don't have the feeling that this fnct should have to keep the 
lock in any case. Any idea?

Thanks,
	Joel

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] some more questions about __raw_write_trylock() hppa implementation

[Matthew Wilcox]

On Wed, Aug 30, 2006 at 08:23:39PM +0000, Joel Soete wrote:
> Hello all,
> 
> I noticed that:
> static  __inline__ int __raw_write_trylock(raw_rwlock_t *rw)
> {
>         __raw_spin_lock(&rw->lock);
>         if (rw->counter != 0) {
>                 /* this basically never happens */
>                 __raw_spin_unlock(&rw->lock);
> 
>                 return 0;
>         }
> 
>         /* got it.  now leave without unlocking */
>         rw->counter = -1; /* remember we are locked */
>         return 1;
> }
> 
> 
> so depending the counter is null or not the lock is kept or relaxed?
> 
> following my smp pb invextigation the pb I encounter seems to occure rarely 
> and so according to above comment "/* this basically never happens */" I 
> place here a panic() in the hope to get a better trace if it start to hang 
> by this place. Unfortunately the comment is wrong and system panicing at 
> boot time ;-(

The comment isn't wrong.  It just means that the contention only happens
when there's actual write-lock contention.

> Looking to some other implementation (powerpc and sh in particular), I 
> don't have the feeling that this fnct should have to keep the lock in any 
> case. Any idea?

That's how it works.  Write locks keep the spinlock locked, read locks
release it.  Actually, there is a problem with the write locks that I
noticed during replying to this mail the first time.  Then palinux
crashed.  How ironic.  Grant and I are fixing that right now ...
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] some more questions about __raw_write_trylock() hppa implementation

[Grant Grundler]

On Wed, Aug 30, 2006 at 09:59:32PM -0600, Matthew Wilcox wrote:
> Actually, there is a problem with the write locks that I
> noticed during replying to this mail the first time.
> Then palinux crashed.  How ironic.
> Grant and I are fixing that right now ...

Willy,
Here's the patch I ended up with.
Diff is also parked on gsyprf11:~grundler/diff-2.6.18-rc4-pa4-rwlocks-01

This is currently running on iodine (a500-5x).
I don't know how this needs to be tested other than running
"make -j4" on the kernel build. Maybe glibc/toolchain build?

Please feel free to apply with your own commit comment, additional
changes, and S-o-B line.

And kudos for catching this. It's another non-trivial problem that's
been around for a while.

thanks,
grant

Commit Comment:
	We can't use generic__raw_read_trylock() since that will hang instead
	of returning a failure if the rwlock->lock isn't available.
	Similarly, __raw_write_trylock() is broken in that it would
	hang too instead of returning a failure.

Signed-off-by: Grant Grundler <grundler@parisc-linux.org>


diff --git a/include/asm-parisc/spinlock.h b/include/asm-parisc/spinlock.h
index a93960e..d4048a6 100644
--- a/include/asm-parisc/spinlock.h
+++ b/include/asm-parisc/spinlock.h
@@ -60,7 +60,6 @@ static inline int __raw_spin_trylock(raw
  * but only one writer.
  */
 
-#define __raw_read_trylock(lock) generic__raw_read_trylock(lock)
 
 /* read_lock, read_unlock are pretty straightforward.  Of course it somehow
  * sucks we end up saving/restoring flags twice for read_lock_irqsave aso. */
@@ -68,19 +67,25 @@ #define __raw_read_trylock(lock) generic
 static  __inline__ void __raw_read_lock(raw_rwlock_t *rw)
 {
 	__raw_spin_lock(&rw->lock);
-
 	rw->counter++;
-
 	__raw_spin_unlock(&rw->lock);
 }
 
 static  __inline__ void __raw_read_unlock(raw_rwlock_t *rw)
 {
 	__raw_spin_lock(&rw->lock);
-
 	rw->counter--;
+	__raw_spin_unlock(&rw->lock);
+}
+
+static  __inline__ int __raw_read_trylock(raw_rwlock_t *rw)
+{
+	if (!__raw_spin_trylock(&rw->lock))
+		return 0;
 
+	rw->counter++;
 	__raw_spin_unlock(&rw->lock);
+	return 1;
 }
 
 /* write_lock is less trivial.  We optimistically grab the lock and check
@@ -121,15 +126,15 @@ static  __inline__ void __raw_write_unlo
 
 static  __inline__ int __raw_write_trylock(raw_rwlock_t *rw)
 {
-	__raw_spin_lock(&rw->lock);
+	if (!__raw_spin_trylock(&rw->lock))
+		return 0;
+
 	if (rw->counter != 0) {
-		/* this basically never happens */
 		__raw_spin_unlock(&rw->lock);

Re: [parisc-linux] some more questions about __raw_write_trylock() hppa implementation

[Matthew Wilcox]

On Thu, Aug 31, 2006 at 12:06:37AM -0600, Grant Grundler wrote:
> Here's the patch I ended up with.
> Diff is also parked on gsyprf11:~grundler/diff-2.6.18-rc4-pa4-rwlocks-01
> 
> +static  __inline__ int __raw_read_trylock(raw_rwlock_t *rw)
> +{
> +	if (!__raw_spin_trylock(&rw->lock))
> +		return 0;
>  
> +	rw->counter++;
>  	__raw_spin_unlock(&rw->lock);
> +	return 1;
>  }

This has the false failure problem.  ie if another CPU is doing a
read_lock() at the same time, it will fail to acquire the lock, even
though it would succeed if it tried again.  I don't know if we have any
code which depends upon that not failing, but it does seem a bit
suboptimal.

Fixing this is somewhat Hard.  My original suggestion was this:

retry:
	if (__raw_spin_trylock(&rw->lock)) {
		rw->counter++;
		__raw_spin_unlock(&rw->lock);
		return 1;
	} else if (rw->counter < 0) {
		return 0;
	} else {
		goto retry;
	}

But this leads to deadlock if called from interrupt context and the CPU
had got to:

static  __inline__ void __raw_write_lock(raw_rwlock_t *rw)
{
retry:
        __raw_spin_lock(&rw->lock);

<<<- here

and hadn't set counter to -1.  Now, we can fix that:

static  __inline__ void __raw_write_lock(raw_rwlock_t *rw)
{
	unsigned long flags;
retry:
	local_irq_save(flags);
        __raw_spin_lock(&rw->lock);

        if (rw->counter != 0) {
                /* this basically never happens */
                __raw_spin_unlock(&rw->lock);
		local_irq_restore(flags);

                while (rw->counter != 0)
                        cpu_relax();

                goto retry;
        }

        /* got it.  now leave without unlocking */
        rw->counter = -1; /* remember we are locked */
	local_irq_restore(flags);
}

A similar change has to be made to __raw_write_trylock too:

static  __inline__ int __raw_write_trylock(raw_rwlock_t *rw)
{
	unsigned long flags;
	int result = 0;
	local_irq_save(flags);
	if (__raw_spin_trylock(&rw->lock)) {
		if (rw->counter == 0) {
			rw->counter = -1;
			result = 1;
		} else {
			__raw_spin_unlock(&rw->lock);
		}
	}
	local_irq_restore(flags);
	return result;
}

Better ideas, anyone?

(And perhaps more importantly, anyone spot any holes in my reasoning?
I'm not too happy disabling interrupts *after* disabling preemption.
And of course, we disable interrupts twice when called with
write_lock_irq.)
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] some more questions about __raw_write_trylock() hppa implementation

[Michael S. Zick]

On Thu August 31 2006 07:31, Matthew Wilcox wrote:
> 
> Better ideas, anyone?
> 

better? not sure, but one thing I am looking at using is
tri-state locks, defined as:

0 == Busy, change in process
&lock == Locked
other == Unlocked

Since that can be done with load&clear and the information
already on hand in the registers.

That is also compatible with the single-linked, bi-directional
lists of requesters - I.E: the values and the lock are also
the head (or tail) of the list. (race free)

No actual code to offer at this time.

Not my idea, took it from one of the references mentioned
in my pa-risc synchronization constructs write-up.

Hmmm...
That might be usable (the tri-state logic) in dealing with
a wrapping timer cr16 also.

Mike
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] some more questions about __raw_write_trylock() hppa implementation

[Matthew Wilcox]

On Thu, Aug 31, 2006 at 08:01:30AM -0500, Michael S. Zick wrote:
> better? not sure, but one thing I am looking at using is
> tri-state locks, defined as:
> 
> 0 == Busy, change in process
> &lock == Locked
> other == Unlocked
> 
> Since that can be done with load&clear and the information
> already on hand in the registers.

Yep, although I don't think that actually saves us from the
reader-interrupted-write-lock problem.  The first thing the writer does
is ldcw, then if it's interrupted, the reader-in-interrupt-context can't
tell if it's a heavily contended read-lock (which would eventually
work), or if it's interrupted a write lock attempt.

One thing we could do is limit the number of times we retry.

Hmm, I just thought.  If we interrupted a writer trying to acquire the
lock, the writer was about to successfully acquire the lock.  But we
interrupted them, so they can't be running.  And nobody else can acquire
the lock because the attempting writer holds the spinlock.  So the
reader in interrupt context can be deemed to have successfully acquired
the lock.  Now ... how do we handle releasing that lock, given that it
can't acquire the spinlock ...
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] some more questions about __raw_write_trylock() hppa implementation

[Grant Grundler]

On Thu, Aug 31, 2006 at 07:13:40AM -0600, Matthew Wilcox wrote:
> One thing we could do is limit the number of times we retry.

Yes, we have to or we face a variant of deadlock.

> Hmm, I just thought.  If we interrupted a writer trying to acquire the
> lock, the writer was about to successfully acquire the lock.  But we
> interrupted them, so they can't be running.  And nobody else can acquire
> the lock because the attempting writer holds the spinlock.  So the
> reader in interrupt context can be deemed to have successfully acquired
> the lock.  Now ... how do we handle releasing that lock, given that it
> can't acquire the spinlock ...

nononono...please don't go there. It's complicated enough already.
Adding more code just makes it slower and harder to maintain.
I'd much rather block interrupts in the writer code path as
proposed in your previous email.

grant
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] some more questions about __raw_write_trylock() hppa implementation

[Grant Grundler]

On Thu, Aug 31, 2006 at 06:31:35AM -0600, Matthew Wilcox wrote:
> On Thu, Aug 31, 2006 at 12:06:37AM -0600, Grant Grundler wrote:
> > Here's the patch I ended up with.
> > Diff is also parked on gsyprf11:~grundler/diff-2.6.18-rc4-pa4-rwlocks-01
> > 
> > +static  __inline__ int __raw_read_trylock(raw_rwlock_t *rw)
> > +{
> > +	if (!__raw_spin_trylock(&rw->lock))
> > +		return 0;
> >  
> > +	rw->counter++;
> >  	__raw_spin_unlock(&rw->lock);
> > +	return 1;
> >  }
> 
> This has the false failure problem.  ie if another CPU is doing a
> read_lock() at the same time, it will fail to acquire the lock, even
> though it would succeed if it tried again.  I don't know if we have any
> code which depends upon that not failing, but it does seem a bit
> suboptimal.

trylock() variants are expected to fail some of the time.
But I agree readers should never fail because of another reader.
I guess we have to implement some number of retries (less than 5?).

> Fixing this is somewhat Hard.  My original suggestion was this:
> 
> retry:
> 	if (__raw_spin_trylock(&rw->lock)) {
> 		rw->counter++;
> 		__raw_spin_unlock(&rw->lock);
> 		return 1;
> 	} else if (rw->counter < 0) {
> 		return 0;
> 	} else {
> 		goto retry;
> 	}
> 
> But this leads to deadlock if called from interrupt context and the CPU
> had got to:
> 
> static  __inline__ void __raw_write_lock(raw_rwlock_t *rw)
> {
> retry:
>         __raw_spin_lock(&rw->lock);
> 
> <<<- here
> 
> and hadn't set counter to -1.

Drop the "if (rw->counter < 0)" test and we won't have a deadlock.
But your next idea on fixing that sounds good to me for other reasons.

> Now, we can fix that:
> 
> static  __inline__ void __raw_write_lock(raw_rwlock_t *rw)
> {
> 	unsigned long flags;
> retry:
> 	local_irq_save(flags);
>         __raw_spin_lock(&rw->lock);
...

I'm thinking we want to block interrupts here anyway to make sure
the writer gets done and releases the spinlock.

I'm also wondering if the writer code paths need to include "mb()"
to prevent the compiler and/or other back-end optimizers from
re-organizing the instruction stream and "leaking" other code
before "counter = -1" is set. James Bottomley already
fixed our regular spinlocks for this problem once before.

> A similar change has to be made to __raw_write_trylock too:

*nod*

> Better ideas, anyone?
> (And perhaps more importantly, anyone spot any holes in my reasoning?

Nope and nope.

> I'm not too happy disabling interrupts *after* disabling preemption.
> And of course, we disable interrupts twice when called with
> write_lock_irq.)

yup.

thanks,
grant
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] some more questions about __raw_write_trylock() hppa implementation

[Matthew Wilcox]

On Thu, Aug 31, 2006 at 10:27:40AM -0600, Grant Grundler wrote:
> trylock() variants are expected to fail some of the time.
> But I agree readers should never fail because of another reader.
> I guess we have to implement some number of retries (less than 5?).

I don't think we need retries; we're guaranteed to make forward
progress.  If we fail to acquire the lock, it's because it's either held
for a short duration by a reader, or for a long duration by a writer.
If it's a writer, we'll fail due to the counter being negative; if it's
a reader, we'll succeed soon.  Mmm.  Unless, of course, we interrupted a
read-locker ... crap.  They need to take the lock in an irqsafe way too.

> > But this leads to deadlock if called from interrupt context and the CPU
> > had got to:
> > 
> > static  __inline__ void __raw_write_lock(raw_rwlock_t *rw)
> > {
> > retry:
> >         __raw_spin_lock(&rw->lock);
> > 
> > <<<- here
> > 
> > and hadn't set counter to -1.
> 
> Drop the "if (rw->counter < 0)" test and we won't have a deadlock.
> But your next idea on fixing that sounds good to me for other reasons.

I don't understand why you think that.  Can you explain?

> > Now, we can fix that:
> > 
> > static  __inline__ void __raw_write_lock(raw_rwlock_t *rw)
> > {
> > 	unsigned long flags;
> > retry:
> > 	local_irq_save(flags);
> >         __raw_spin_lock(&rw->lock);
> ...
> 
> I'm thinking we want to block interrupts here anyway to make sure
> the writer gets done and releases the spinlock.

Umm.  Sounds like a spectacularly bad idea.  If the caller wanted to do
that, they would have called write_lock_irqsave() or write_lock_irq().

> I'm also wondering if the writer code paths need to include "mb()"
> to prevent the compiler and/or other back-end optimizers from
> re-organizing the instruction stream and "leaking" other code
> before "counter = -1" is set. James Bottomley already
> fixed our regular spinlocks for this problem once before.

With the out-of-line spinlocks (and for that matter, write locks),
that's not going to matter.  The only place that calls
__raw_write_lock() is in kernel/spinlock.c, so there's no way for gcc to
optimise that away.  I can put it in anyway, since it's not going to
make a difference.

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] some more questions about __raw_write_trylock() hppa implementation

[Grant Grundler]

On Fri, Sep 01, 2006 at 08:01:23AM -0600, Matthew Wilcox wrote:
> On Thu, Aug 31, 2006 at 10:27:40AM -0600, Grant Grundler wrote:
> > trylock() variants are expected to fail some of the time.
> > But I agree readers should never fail because of another reader.
> > I guess we have to implement some number of retries (less than 5?).
> 
> I don't think we need retries; we're guaranteed to make forward
> progress.  If we fail to acquire the lock, it's because it's either held
> for a short duration by a reader, or for a long duration by a writer.
> If it's a writer, we'll fail due to the counter being negative; if it's
> a reader, we'll succeed soon.  Mmm.  Unless, of course, we interrupted a
> read-locker ... crap.  They need to take the lock in an irqsafe way too.

Yup...

> > Drop the "if (rw->counter < 0)" test and we won't have a deadlock.
> > But your next idea on fixing that sounds good to me for other reasons.
> 
> I don't understand why you think that.  Can you explain?

Without blocking interrupts, that test is reading a value
that's not deterministic. ie we don't when if/when we are
interrupting a writer. Failing the read lock is safe even
if it's not correct.


> > I'm thinking we want to block interrupts here anyway to make sure
> > the writer gets done and releases the spinlock.
> 
> Umm.  Sounds like a spectacularly bad idea.  If the caller wanted to do
> that, they would have called write_lock_irqsave() or write_lock_irq().

Well, ok - you're right about caller intentions. But the caller also has
no clue about parisc rw_locks and how fsck'd the implementation is.
I'm just "speculating out loud"  in order to make parisc implementation
work better in practice.

> With the out-of-line spinlocks (and for that matter, write locks),
> that's not going to matter.  The only place that calls
> __raw_write_lock() is in kernel/spinlock.c, so there's no way for gcc to
> optimise that away.  I can put it in anyway, since it's not going to
> make a difference.

I'm not sure that a good reason to put the mb() in.
Would "it's correct" be a better reason?
I'm thinking other people will look at the code when trying to
understand parisc.

thanks,
grant
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] some more questions about __raw_write_trylock() hppa implementation

[Matthew Wilcox]

On Fri, Sep 01, 2006 at 09:57:31AM -0600, Grant Grundler wrote:
> > > Drop the "if (rw->counter < 0)" test and we won't have a deadlock.
> > > But your next idea on fixing that sounds good to me for other reasons.
> > 
> > I don't understand why you think that.  Can you explain?
> 
> Without blocking interrupts, that test is reading a value
> that's not deterministic. ie we don't when if/when we are
> interrupting a writer. Failing the read lock is safe even
> if it's not correct.

Oh, OK.  Understood.  I thought you had a neat optimisation there ;-)

> > > I'm thinking we want to block interrupts here anyway to make sure
> > > the writer gets done and releases the spinlock.
> > 
> > Umm.  Sounds like a spectacularly bad idea.  If the caller wanted to do
> > that, they would have called write_lock_irqsave() or write_lock_irq().
> 
> Well, ok - you're right about caller intentions. But the caller also has
> no clue about parisc rw_locks and how fsck'd the implementation is.
> I'm just "speculating out loud"  in order to make parisc implementation
> work better in practice.

It's not that bad.  Really ;-)

> > With the out-of-line spinlocks (and for that matter, write locks),
> > that's not going to matter.  The only place that calls
> > __raw_write_lock() is in kernel/spinlock.c, so there's no way for gcc to
> > optimise that away.  I can put it in anyway, since it's not going to
> > make a difference.
> 
> I'm not sure that a good reason to put the mb() in.
> Would "it's correct" be a better reason?
> I'm thinking other people will look at the code when trying to
> understand parisc.

I doubt it, or somebody might've spotted the, what, three or four
problems with our rwlocks that I've found?

 - an interrupt while taking or releasing a read_lock can deadlock
   against a read_lock.
 - an interrupt while holding a read_lock will deadlock against a
   read_trylock.
 - an interrupt while holding a write_lock will deadlock against a
   read_trylock or write_trylock.

Anyway, here's the current diff; I have a make -j4; make clean running
in an endless loop on nicol right now.  Any further suggestions for
stress-testing are welcome ... perhaps I'll install apache and run an
apache benchmark.

diff --git a/include/asm-parisc/spinlock.h b/include/asm-parisc/spinlock.h
index a93960e..97115dc 100644
--- a/include/asm-parisc/spinlock.h
+++ b/include/asm-parisc/spinlock.h
@@ -56,50 +56,70 @@ static inline int __raw_spin_trylock(raw
 }
 
 /*
- * Read-write spinlocks, allowing multiple readers
- * but only one writer.
+ * Read-write spinlocks, allowing multiple readers but only one writer.
+ * The spinlock is held by the writer, preventing any readers or other
+ * writers from grabbing the rwlock.  Readers use the lock to serialise their
+ * access to the counter (which records how many readers currently hold the
+ * lock).  Linux rwlocks are unfair to writers; they can be starved for
+ * an indefinite time by readers.  They can also be taken in interrupt context,
+ * so we have to disable interrupts when acquiring the spin lock to be sure
+ * that an interrupting reader doesn't get an inconsistent view of the lock.
  */
 
-#define __raw_read_trylock(lock) generic__raw_read_trylock(lock)
-
-/* read_lock, read_unlock are pretty straightforward.  Of course it somehow
- * sucks we end up saving/restoring flags twice for read_lock_irqsave aso. */
-
 static  __inline__ void __raw_read_lock(raw_rwlock_t *rw)
 {
+	unsigned long flags;
+	local_irq_save(flags);
 	__raw_spin_lock(&rw->lock);
-
 	rw->counter++;
-
 	__raw_spin_unlock(&rw->lock);
+	local_irq_restore(flags);
 }
 
 static  __inline__ void __raw_read_unlock(raw_rwlock_t *rw)
 {
+	unsigned long flags;
+	local_irq_save(flags);
 	__raw_spin_lock(&rw->lock);
-
 	rw->counter--;
-
 	__raw_spin_unlock(&rw->lock);
+	local_irq_restore(flags);
 }
 
-/* write_lock is less trivial.  We optimistically grab the lock and check
- * if we surprised any readers.  If so we release the lock and wait till
- * they're all gone before trying again
- *
- * Also note that we don't use the _irqsave / _irqrestore suffixes here.
- * If we're called with interrupts enabled and we've got readers (or other
- * writers) in interrupt handlers someone fucked up and we'd dead-lock
- * sooner or later anyway.   prumpf */
+static __inline__ int __raw_read_trylock(raw_rwlock_t *rw)
+{
+	unsigned long flags;
+ retry:
+	local_irq_save(flags);
+	if (__raw_spin_trylock(&rw->lock)) {
+		rw->counter++;
+		__raw_spin_unlock(&rw->lock);
+		local_irq_restore(flags);
+		return 1;
+	}
+
+	local_irq_restore(flags);
+	/* If write-locked, we fail to acquire the lock */
+	if (rw->counter < 0)
+		return 0;
+
+	/* Wait until we have a realistic chance at the lock */
+	while (__raw_spin_is_locked(&rw->lock) && rw->counter >= 0)
+		cpu_relax();
+
+	goto retry;
+}
 
-static  __inline__ void __raw_write_lock(raw_rwlock_t *rw)
+static __inline__ void __raw_write_lock(raw_rwlock_t *rw)
 {
+	unsigned long flags;
 retry:
+	local_irq_save(flags);
 	__raw_spin_lock(&rw->lock);
 
-	if(rw->counter != 0) {
-		/* this basically never happens */
+	if (rw->counter != 0) {
 		__raw_spin_unlock(&rw->lock);
+		local_irq_restore(flags);
 
 		while (rw->counter != 0)
 			cpu_relax();
@@ -107,31 +127,35 @@ retry:
 		goto retry;
 	}
 
-	/* got it.  now leave without unlocking */
-	rw->counter = -1; /* remember we are locked */
+	rw->counter = -1; /* mark as write-locked */
+	mb();
+	local_irq_restore(flags);
 }
 
-/* write_unlock is absolutely trivial - we don't have to wait for anything */
-
-static  __inline__ void __raw_write_unlock(raw_rwlock_t *rw)
+static __inline__ void __raw_write_unlock(raw_rwlock_t *rw)
 {
 	rw->counter = 0;
 	__raw_spin_unlock(&rw->lock);
 }
 
-static  __inline__ int __raw_write_trylock(raw_rwlock_t *rw)
+static __inline__ int __raw_write_trylock(raw_rwlock_t *rw)
 {
-	__raw_spin_lock(&rw->lock);
-	if (rw->counter != 0) {
-		/* this basically never happens */
-		__raw_spin_unlock(&rw->lock);