Re: [PATCH] m32r: Revise __raw_read_trylock()

Re: [PATCH] m32r: Revise __raw_read_trylock()

[Paul Mundt]

On Fri, Sep 22, 2006 at 03:29:53PM +0900, Hirokazu Takata wrote:
> Matthew Wilcox pointed out that generic__raw_read_trylock() is
> unfit for use.
> 
> Here is a patch to fix __raw_read_trylock() for m32r.
> It is taken from the i386 implementation.
> 
This might be a stupid question, but why exactly are we ripping out
generic__raw_read_trylock() if architectures are going to implement a
generic implementation anyways, rather than just changing it to match
the proper semantics?

With this the m32r patch can be dropped and the rest of the
architectures can switch over as necessary to optimized versions, rather
than being fundamentally broken.

Signed-off-by: Paul Mundt <lethal@linux-sh.org>

--

 include/asm-i386/spinlock.h |   10 +---------
 kernel/spinlock.c           |    9 +++++++--
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/include/asm-i386/spinlock.h b/include/asm-i386/spinlock.h
index d102036..2aba6b3 100644
--- a/include/asm-i386/spinlock.h
+++ b/include/asm-i386/spinlock.h
@@ -169,15 +169,7 @@ static inline void __raw_write_lock(raw_
 	__build_write_lock(rw, "__write_lock_failed");
 }
 
-static inline int __raw_read_trylock(raw_rwlock_t *lock)
-{
-	atomic_t *count = (atomic_t *)lock;
-	atomic_dec(count);
-	if (atomic_read(count) >= 0)
-		return 1;
-	atomic_inc(count);
-	return 0;
-}
+#define __raw_read_trylock(lock) generic__raw_read_trylock(lock)
 
 static inline int __raw_write_trylock(raw_rwlock_t *lock)
 {
diff --git a/kernel/spinlock.c b/kernel/spinlock.c
index fb524b0..8d50b9d 100644
--- a/kernel/spinlock.c
+++ b/kernel/spinlock.c
@@ -15,6 +15,7 @@ #include <linux/spinlock.h>
 #include <linux/interrupt.h>
 #include <linux/debug_locks.h>
 #include <linux/module.h>
+#include <asm/atomic.h>
 
 /*
  * Generic declaration of the raw read_trylock() function,
@@ -22,8 +23,12 @@ #include <linux/module.h>
  */
 int __lockfunc generic__raw_read_trylock(raw_rwlock_t *lock)
 {
-	__raw_read_lock(lock);
-	return 1;
+	atomic_t *count = (atomic_t *)lock;
+	atomic_dec(count);
+	if (atomic_read(count) >= 0)
+		return 1;
+	atomic_inc(count);
+	return 0;
 }
 EXPORT_SYMBOL(generic__raw_read_trylock);

Re: [PATCH] m32r: Revise __raw_read_trylock()

[Matthew Wilcox]

On Fri, Sep 22, 2006 at 04:48:13PM +0900, Paul Mundt wrote:
> This might be a stupid question, but why exactly are we ripping out
> generic__raw_read_trylock() if architectures are going to implement a
> generic implementation anyways, rather than just changing it to match
> the proper semantics?

Because there is no generic definition of struct spinlock.

>  int __lockfunc generic__raw_read_trylock(raw_rwlock_t *lock)
>  {
> -	__raw_read_lock(lock);
> -	return 1;
> +	atomic_t *count = (atomic_t *)lock;
> +	atomic_dec(count);
> +	if (atomic_read(count) >= 0)
> +		return 1;
> +	atomic_inc(count);
> +	return 0;
>  }

You're assuming:

 - a spinlock is an atomic_t.
 - Said atomic_t uses RW_LOCK_BIAS to indicate locked/unlocked.

This is true for m32r, but not for sparc.  SuperH looks completely
broken -- I don't see how holding a read lock prevents someone else from
getting a write lock.  The SH write_trylock uses RW_LOCK_BIAS, but
write_lock doesn't.  Are there any SMP SH machines?

Re: [PATCH] m32r: Revise __raw_read_trylock()

[Paul Mundt]

On Fri, Sep 22, 2006 at 05:27:08AM -0600, Matthew Wilcox wrote:
> You're assuming:
> 
>  - a spinlock is an atomic_t.
>  - Said atomic_t uses RW_LOCK_BIAS to indicate locked/unlocked.
> 
> This is true for m32r, but not for sparc.

That makes sense, thanks for the clarification.

> SuperH looks completely broken -- I don't see how holding a read lock
> prevents someone else from getting a write lock.  The SH write_trylock
> uses RW_LOCK_BIAS, but write_lock doesn't.  Are there any SMP SH
> machines?

Yes, it's broken, most of the work for that has been happening out of
tree, and we'll have to sync it up again. The initial work was targetted
at a pair of microcontrollers, but there were too many other issues
there that the work was eventually abandoned. Recently it's started up
again on more reasonable CPUs, so we'll be fixing these things up in
order.