[PATCH] m32r: Revise __raw_read_trylock()

[PATCH] m32r: Revise __raw_read_trylock()

[Hirokazu Takata]

Hi,

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.

Signed-off-by: Hirokazu Takata <takata@linux-m32r.org>
Cc: Matthew Wilcox <matthew@wil.cx>
---
 include/asm-m32r/spinlock.h |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/include/asm-m32r/spinlock.h b/include/asm-m32r/spinlock.h
index f94c1a6..78205f0 100644
--- a/include/asm-m32r/spinlock.h
+++ b/include/asm-m32r/spinlock.h
@@ -298,7 +298,15 @@ #endif	/* CONFIG_CHIP_M32700_TS1 */
 	);
 }
 
-#define __raw_read_trylock(lock) generic__raw_read_trylock(lock)
+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;
+}
 
 static inline int __raw_write_trylock(raw_rwlock_t *lock)
 {
-- 
Hirokazu Takata <takata@linux-m32r.org>
Linux/M32R Project:  http://www.linux-m32r.org/

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.

Re: [PATCH] m32r: Revise __raw_read_trylock()

[Matthew Wilcox]

On Fri, Sep 22, 2006 at 03:29:53PM +0900, Hirokazu Takata wrote:
>  
> -#define __raw_read_trylock(lock) generic__raw_read_trylock(lock)
> +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;
> +}
>  

Is there a race here between __raw_read_trylock and __raw_write_trylock?

CPU A			CPU B
__raw_read_trylock
atomic_dec(count);
			__raw_write_trylock
			atomic_sub_and_test(RW_LOCK_BIAS, count)
atomic_read(count)

It'd be fairly harmless as neither would manage to get the lock.  But
I think it's not too hard to fix.  Seems to me you want to do:

static inline int __raw_read_trylock(raw_rwlock_t *lock)
{
	atomic_t *count = (atomic_t*)lock;
	if (atomic_dec_return(count) >= 0)
		return 1;
	atomic_inc(count);
	return 0;
}

eliminating the race.

Re: [PATCH] m32r: Revise __raw_read_trylock()

[Hirokazu Takata]

From: Matthew Wilcox <matthew@wil.cx>
Subject: Re: [PATCH] m32r: Revise __raw_read_trylock()
Date: Sun, 24 Sep 2006 00:20:36 -0600
> On Fri, Sep 22, 2006 at 03:29:53PM +0900, Hirokazu Takata wrote:
> >  
> > -#define __raw_read_trylock(lock) generic__raw_read_trylock(lock)
> > +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;
> > +}
> >  
> 
> Is there a race here between __raw_read_trylock and __raw_write_trylock?
> 
> CPU A			CPU B
> __raw_read_trylock
> atomic_dec(count);
> 			__raw_write_trylock
> 			atomic_sub_and_test(RW_LOCK_BIAS, count)
> atomic_read(count)

Indeed, it is a possible race.

> It'd be fairly harmless as neither would manage to get the lock.  But
> I think it's not too hard to fix.  Seems to me you want to do:

I agree with you.
As you said, I think the above case is harmless.

> static inline int __raw_read_trylock(raw_rwlock_t *lock)
> {
> 	atomic_t *count = (atomic_t*)lock;
> 	if (atomic_dec_return(count) >= 0)
> 		return 1;
> 	atomic_inc(count);
> 	return 0;
> }
> 
> eliminating the race.
> 

All right, I think this fix is preferable.

Andrew, please drop and replace the previous my patch with the following
Matthew's fix.

Thank you.

Signed-off-by: Hirokazu Takata <takata@linux-m32r.org>
Cc: Matthew Wilcox <matthew@wil.cx>
--
 include/asm-m32r/spinlock.h |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/asm-m32r/spinlock.h b/include/asm-m32r/spinlock.h
index f94c1a6..f9f9072 100644
--- a/include/asm-m32r/spinlock.h
+++ b/include/asm-m32r/spinlock.h
@@ -298,7 +298,14 @@ #endif	/* CONFIG_CHIP_M32700_TS1 */
 	);
 }
 
-#define __raw_read_trylock(lock) generic__raw_read_trylock(lock)
+static inline int __raw_read_trylock(raw_rwlock_t *lock)
+{
+	atomic_t *count = (atomic_t*)lock;
+	if (atomic_dec_return(count) >= 0)
+		return 1;
+	atomic_inc(count);
+	return 0;
+}
 
 static inline int __raw_write_trylock(raw_rwlock_t *lock)
 {
--
Hirokazu Takata <takata@linux-m32r.org>
Linux/M32R Project:  http://www.linux-m32r.org/

Re: [PATCH] m32r: Revise __raw_read_trylock()

[Andrew Morton]

On Mon, 25 Sep 2006 16:47:22 +0900
Hirokazu Takata <takata.hirokazu@renesas.com> wrote:

> Andrew, please drop and replace the previous my patch with the following
> Matthew's fix.
> 
> Thank you.
> 
> Signed-off-by: Hirokazu Takata <takata@linux-m32r.org>
> Cc: Matthew Wilcox <matthew@wil.cx>
> --
>  include/asm-m32r/spinlock.h |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/include/asm-m32r/spinlock.h b/include/asm-m32r/spinlock.h
> index f94c1a6..f9f9072 100644
> --- a/include/asm-m32r/spinlock.h
> +++ b/include/asm-m32r/spinlock.h
> @@ -298,7 +298,14 @@ #endif	/* CONFIG_CHIP_M32700_TS1 */
>  	);
>  }
>  
> -#define __raw_read_trylock(lock) generic__raw_read_trylock(lock)
> +static inline int __raw_read_trylock(raw_rwlock_t *lock)
> +{
> +	atomic_t *count = (atomic_t*)lock;
> +	if (atomic_dec_return(count) >= 0)
> +		return 1;
> +	atomic_inc(count);
> +	return 0;
> +}

We don't have a changelog for this patch.  My usual technique when this
happens is to mutter something unprintable then go on a hunt through the
mailing list archives.

But all I have is "Matthew Wilcox pointed out that
generic__raw_read_trylock() is unfit for use.".

What's wrong with it?

Re: [PATCH] m32r: Revise __raw_read_trylock()

[Matthew Wilcox]

On Tue, Sep 26, 2006 at 02:33:44PM -0700, Andrew Morton wrote:
> We don't have a changelog for this patch.  My usual technique when this
> happens is to mutter something unprintable then go on a hunt through the
> mailing list archives.
> 
> But all I have is "Matthew Wilcox pointed out that
> generic__raw_read_trylock() is unfit for use.".
> 
> What's wrong with it?

I pointed it out on linux-arch a couple of weeks ago.  Ever look at the
generic__raw_read_trylock implementation?

$ git-diff linus spinlock.c 
diff --git a/kernel/spinlock.c b/kernel/spinlock.c
index fb524b0..6fc4c92 100644
--- a/kernel/spinlock.c
+++ b/kernel/spinlock.c
@@ -16,17 +16,6 @@ #include <linux/interrupt.h>
 #include <linux/debug_locks.h>
 #include <linux/module.h>
 
-/*
- * Generic declaration of the raw read_trylock() function,
- * architectures are supposed to optimize this:
- */
-int __lockfunc generic__raw_read_trylock(raw_rwlock_t *lock)
-{
-       __raw_read_lock(lock);
-       return 1;
-}
-EXPORT_SYMBOL(generic__raw_read_trylock);
-

If the cpu has the lock held for write, is interrupted, and the interrupt
handler calls read_trylock(), it's an instant deadlock.

Now, Dave Miller has subsequently pointed out that we don't have any
situations where this can occur.  Nevertheless, we should delete
generic__raw_read_lock (and its associated EXPORT to make Arjan happy)
so that nobody thinks they can use it.