[PATCH 0/2] locking/rwsem: disable optimistic spinning for PA-RISC

[PATCH 0/2] locking/rwsem: disable optimistic spinning for PA-RISC

[Davidlohr Bueso]

Patch 1 adds a much needed CONFIG_RWSEM_SPIN_ON_OWNER option.
Patch 2 is a quick fix to disable optimistic spinning on PA-RISC.

Thanks!

Davidlohr Bueso (2):
  locking/rwsem: Add CONFIG_RWSEM_SPIN_ON_OWNER
  locking/rwsem: Disable optimistic spinning for PA-RISC

 include/linux/rwsem.h  | 4 ++--
 kernel/Kconfig.locks   | 4 ++++
 kernel/locking/rwsem.c | 2 +-
 3 files changed, 7 insertions(+), 3 deletions(-)

-- 
1.8.1.4

[PATCH 1/2] locking/rwsem: Add CONFIG_RWSEM_SPIN_ON_OWNER

[Davidlohr Bueso]

Just like with mutexes (CONFIG_MUTEX_SPIN_ON_OWNER),
encapsulate the dependencies for rwsem optimistic spinning.
No logical changes here as it continues to depend on both
SMP and the XADD algorithm variant.

Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
---
 include/linux/rwsem.h  | 4 ++--
 kernel/Kconfig.locks   | 4 ++++
 kernel/locking/rwsem.c | 2 +-
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index 8d79708..accdef7 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -27,7 +27,7 @@ struct rw_semaphore {
 	long count;
 	raw_spinlock_t wait_lock;
 	struct list_head wait_list;
-#ifdef CONFIG_SMP
+#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
 	/*
 	 * Write owner. Used as a speculative check to see
 	 * if the owner is running on the cpu.
@@ -64,7 +64,7 @@ static inline int rwsem_is_locked(struct rw_semaphore *sem)
 # define __RWSEM_DEP_MAP_INIT(lockname)
 #endif
 
-#if defined(CONFIG_SMP) && !defined(CONFIG_RWSEM_GENERIC_SPINLOCK)
+#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
 #define __RWSEM_INITIALIZER(name)			\
 	{ RWSEM_UNLOCKED_VALUE,				\
 	  __RAW_SPIN_LOCK_UNLOCKED(name.wait_lock),	\
diff --git a/kernel/Kconfig.locks b/kernel/Kconfig.locks
index 35536d9..e4c3162 100644
--- a/kernel/Kconfig.locks
+++ b/kernel/Kconfig.locks
@@ -224,6 +224,10 @@ config MUTEX_SPIN_ON_OWNER
 	def_bool y
 	depends on SMP && !DEBUG_MUTEXES
 
+config RWSEM_SPIN_ON_OWNER
+       def_bool y
+       depends on SMP && RWSEM_XCHGADD_ALGORITHM
+
 config ARCH_USE_QUEUE_RWLOCK
 	bool
 
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index 42f806d..e2d3bc7 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -12,7 +12,7 @@
 
 #include <linux/atomic.h>
 
-#if defined(CONFIG_SMP) && defined(CONFIG_RWSEM_XCHGADD_ALGORITHM)
+#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
 static inline void rwsem_set_owner(struct rw_semaphore *sem)
 {
 	sem->owner = current;
-- 
1.8.1.4

Re: [PATCH 1/2] locking/rwsem: Add CONFIG_RWSEM_SPIN_ON_OWNER

[Jason Low]

On Fri, 2014-06-06 at 08:55 -0700, Davidlohr Bueso wrote:
> Just like with mutexes (CONFIG_MUTEX_SPIN_ON_OWNER),
> encapsulate the dependencies for rwsem optimistic spinning.
> No logical changes here as it continues to depend on both
> SMP and the XADD algorithm variant.
> 
> Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
> ---
>  include/linux/rwsem.h  | 4 ++--
>  kernel/Kconfig.locks   | 4 ++++
>  kernel/locking/rwsem.c | 2 +-
>  3 files changed, 7 insertions(+), 3 deletions(-)

Do we also want to add an #ifdef CONFIG_RWSEM_SPIN_ON_OWNER in
__init_rwsem() and in the optimistic spinning functions for rwsem-xadd?


Besides that:

Acked-by: Jason Low <jason.low2@hp.com>

Re: [PATCH 1/2] locking/rwsem: Add CONFIG_RWSEM_SPIN_ON_OWNER

[Davidlohr Bueso]

On Fri, 2014-06-06 at 10:13 -0700, Jason Low wrote:
> On Fri, 2014-06-06 at 08:55 -0700, Davidlohr Bueso wrote:
> > Just like with mutexes (CONFIG_MUTEX_SPIN_ON_OWNER),
> > encapsulate the dependencies for rwsem optimistic spinning.
> > No logical changes here as it continues to depend on both
> > SMP and the XADD algorithm variant.
> > 
> > Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
> > ---
> >  include/linux/rwsem.h  | 4 ++--
> >  kernel/Kconfig.locks   | 4 ++++
> >  kernel/locking/rwsem.c | 2 +-
> >  3 files changed, 7 insertions(+), 3 deletions(-)
> 
> Do we also want to add an #ifdef CONFIG_RWSEM_SPIN_ON_OWNER in
> __init_rwsem() and in the optimistic spinning functions for rwsem-xadd?
> 

Not really, as we conditionally build rwsem-xadd.o based on
CONFIG_RWSEM_XCHGADD_ALGORITHM:

obj-$(CONFIG_RWSEM_GENERIC_SPINLOCK) += rwsem-spinlock.o
obj-$(CONFIG_RWSEM_XCHGADD_ALGORITHM) += rwsem-xadd.o

So leaving just SMP is ok there.

> 
> Besides that:
> 
> Acked-by: Jason Low <jason.low2@hp.com>
> 

Thanks,
Davidlohr

Re: [PATCH 1/2] locking/rwsem: Add CONFIG_RWSEM_SPIN_ON_OWNER

[Davidlohr Bueso]

On Fri, 2014-06-06 at 11:12 -0700, Davidlohr Bueso wrote:
> On Fri, 2014-06-06 at 10:13 -0700, Jason Low wrote:
> > On Fri, 2014-06-06 at 08:55 -0700, Davidlohr Bueso wrote:
> > > Just like with mutexes (CONFIG_MUTEX_SPIN_ON_OWNER),
> > > encapsulate the dependencies for rwsem optimistic spinning.
> > > No logical changes here as it continues to depend on both
> > > SMP and the XADD algorithm variant.
> > > 
> > > Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
> > > ---
> > >  include/linux/rwsem.h  | 4 ++--
> > >  kernel/Kconfig.locks   | 4 ++++
> > >  kernel/locking/rwsem.c | 2 +-
> > >  3 files changed, 7 insertions(+), 3 deletions(-)
> > 
> > Do we also want to add an #ifdef CONFIG_RWSEM_SPIN_ON_OWNER in
> > __init_rwsem() and in the optimistic spinning functions for rwsem-xadd?
> > 
> 
> Not really, as we conditionally build rwsem-xadd.o based on
> CONFIG_RWSEM_XCHGADD_ALGORITHM:
> 
> obj-$(CONFIG_RWSEM_GENERIC_SPINLOCK) += rwsem-spinlock.o
> obj-$(CONFIG_RWSEM_XCHGADD_ALGORITHM) += rwsem-xadd.o
> 
> So leaving just SMP is ok there.

Of course that's bogus when we add more dependencies (ie 2/2), so yeah,
we need to add the whole CONFIG_RWSEM_SPIN_ON_OWNER option.

Thanks.

8<---------------------------------------------------------
From: Davidlohr Bueso <davidlohr@hp.com>
Date: Fri, 6 Jun 2014 11:45:34 -0700
Subject: [PATCH v2 1/2] locking/rwsem: Add CONFIG_RWSEM_SPIN_ON_OWNER

Just like with mutexes (CONFIG_MUTEX_SPIN_ON_OWNER),
encapsulate the dependencies for rwsem optimistic spinning.
No logical changes here as it continues to depend on both
SMP and the XADD algorithm variant.

Acked-by: Jason Low <jason.low2@hp.com>
Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
---
 include/linux/rwsem.h       | 4 ++--
 kernel/Kconfig.locks        | 4 ++++
 kernel/locking/rwsem-xadd.c | 2 +-
 kernel/locking/rwsem.c      | 2 +-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index 8d79708..accdef7 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -27,7 +27,7 @@ struct rw_semaphore {
 	long count;
 	raw_spinlock_t wait_lock;
 	struct list_head wait_list;
-#ifdef CONFIG_SMP
+#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
 	/*
 	 * Write owner. Used as a speculative check to see
 	 * if the owner is running on the cpu.
@@ -64,7 +64,7 @@ static inline int rwsem_is_locked(struct rw_semaphore *sem)
 # define __RWSEM_DEP_MAP_INIT(lockname)
 #endif
 
-#if defined(CONFIG_SMP) && !defined(CONFIG_RWSEM_GENERIC_SPINLOCK)
+#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
 #define __RWSEM_INITIALIZER(name)			\
 	{ RWSEM_UNLOCKED_VALUE,				\
 	  __RAW_SPIN_LOCK_UNLOCKED(name.wait_lock),	\
diff --git a/kernel/Kconfig.locks b/kernel/Kconfig.locks
index 35536d9..e4c3162 100644
--- a/kernel/Kconfig.locks
+++ b/kernel/Kconfig.locks
@@ -224,6 +224,10 @@ config MUTEX_SPIN_ON_OWNER
 	def_bool y
 	depends on SMP && !DEBUG_MUTEXES
 
+config RWSEM_SPIN_ON_OWNER
+       def_bool y
+       depends on SMP && RWSEM_XCHGADD_ALGORITHM
+
 config ARCH_USE_QUEUE_RWLOCK
 	bool
 
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index dacc321..f6b5b96 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -82,7 +82,7 @@ void __init_rwsem(struct rw_semaphore *sem, const char *name,
 	sem->count = RWSEM_UNLOCKED_VALUE;
 	raw_spin_lock_init(&sem->wait_lock);
 	INIT_LIST_HEAD(&sem->wait_list);
-#ifdef CONFIG_SMP
+#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
 	sem->owner = NULL;
 	sem->osq = NULL;
 #endif
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index 42f806d..e2d3bc7 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -12,7 +12,7 @@
 
 #include <linux/atomic.h>
 
-#if defined(CONFIG_SMP) && defined(CONFIG_RWSEM_XCHGADD_ALGORITHM)
+#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
 static inline void rwsem_set_owner(struct rw_semaphore *sem)
 {
 	sem->owner = current;
-- 
1.8.1.4

Re: [PATCH 1/2] locking/rwsem: Add CONFIG_RWSEM_SPIN_ON_OWNER

[Jason Low]

On Fri, 2014-06-06 at 11:48 -0700, Davidlohr Bueso wrote:
> On Fri, 2014-06-06 at 11:12 -0700, Davidlohr Bueso wrote:
> > On Fri, 2014-06-06 at 10:13 -0700, Jason Low wrote:
> > > On Fri, 2014-06-06 at 08:55 -0700, Davidlohr Bueso wrote:
> > > > Just like with mutexes (CONFIG_MUTEX_SPIN_ON_OWNER),
> > > > encapsulate the dependencies for rwsem optimistic spinning.
> > > > No logical changes here as it continues to depend on both
> > > > SMP and the XADD algorithm variant.
> > > > 
> > > > Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
> > > > ---
> > > >  include/linux/rwsem.h  | 4 ++--
> > > >  kernel/Kconfig.locks   | 4 ++++
> > > >  kernel/locking/rwsem.c | 2 +-
> > > >  3 files changed, 7 insertions(+), 3 deletions(-)
> > > 
> > > Do we also want to add an #ifdef CONFIG_RWSEM_SPIN_ON_OWNER in
> > > __init_rwsem() and in the optimistic spinning functions for rwsem-xadd?
> > > 
> > 
> > Not really, as we conditionally build rwsem-xadd.o based on
> > CONFIG_RWSEM_XCHGADD_ALGORITHM:
> > 
> > obj-$(CONFIG_RWSEM_GENERIC_SPINLOCK) += rwsem-spinlock.o
> > obj-$(CONFIG_RWSEM_XCHGADD_ALGORITHM) += rwsem-xadd.o
> > 
> > So leaving just SMP is ok there.
> 
> Of course that's bogus when we add more dependencies (ie 2/2), so yeah,
> we need to add the whole CONFIG_RWSEM_SPIN_ON_OWNER option.
> 
> Thanks.

> diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
> index dacc321..f6b5b96 100644
> --- a/kernel/locking/rwsem-xadd.c
> +++ b/kernel/locking/rwsem-xadd.c
> @@ -82,7 +82,7 @@ void __init_rwsem(struct rw_semaphore *sem, const char *name,
>  	sem->count = RWSEM_UNLOCKED_VALUE;
>  	raw_spin_lock_init(&sem->wait_lock);
>  	INIT_LIST_HEAD(&sem->wait_list);
> -#ifdef CONFIG_SMP
> +#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
>  	sem->owner = NULL;
>  	sem->osq = NULL;
>  #endif

And should we also change that in the optimistic spinning functions so
that it defaults to:

	static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
	{
        	return false;
	}

in the !CONFIG_RWSEM_SPIN_ON_OWNER case.

Thanks.

> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
> index 42f806d..e2d3bc7 100644
> --- a/kernel/locking/rwsem.c
> +++ b/kernel/locking/rwsem.c
> @@ -12,7 +12,7 @@
>  
>  #include <linux/atomic.h>
>  
> -#if defined(CONFIG_SMP) && defined(CONFIG_RWSEM_XCHGADD_ALGORITHM)
> +#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
>  static inline void rwsem_set_owner(struct rw_semaphore *sem)
>  {
>  	sem->owner = current;

Re: [PATCH 1/2] locking/rwsem: Add CONFIG_RWSEM_SPIN_ON_OWNER

[Davidlohr Bueso]

On Fri, 2014-06-06 at 12:08 -0700, Jason Low wrote:
> And should we also change that in the optimistic spinning functions so
> that it defaults to:
> 
> 	static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
> 	{
>         	return false;
> 	}

Oops Yep, afaict the last offending user.

8<----------------------------------
From: Davidlohr Bueso <davidlohr@hp.com>
Date: Fri, 6 Jun 2014 12:15:03 -0700
Subject: [PATCH v3 1/2] locking/rwsem: Add CONFIG_RWSEM_SPIN_ON_OWNER

Just like with mutexes (CONFIG_MUTEX_SPIN_ON_OWNER),
encapsulate the dependencies for rwsem optimistic spinning.
No logical changes here as it continues to depend on both
SMP and the XADD algorithm variant.

Acked-by: Jason Low <jason.low2@hp.com>
Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
---
 include/linux/rwsem.h       | 4 ++--
 kernel/Kconfig.locks        | 4 ++++
 kernel/locking/rwsem-xadd.c | 4 ++--
 kernel/locking/rwsem.c      | 2 +-
 4 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index 8d79708..accdef7 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -27,7 +27,7 @@ struct rw_semaphore {
 	long count;
 	raw_spinlock_t wait_lock;
 	struct list_head wait_list;
-#ifdef CONFIG_SMP
+#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
 	/*
 	 * Write owner. Used as a speculative check to see
 	 * if the owner is running on the cpu.
@@ -64,7 +64,7 @@ static inline int rwsem_is_locked(struct rw_semaphore *sem)
 # define __RWSEM_DEP_MAP_INIT(lockname)
 #endif
 
-#if defined(CONFIG_SMP) && !defined(CONFIG_RWSEM_GENERIC_SPINLOCK)
+#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
 #define __RWSEM_INITIALIZER(name)			\
 	{ RWSEM_UNLOCKED_VALUE,				\
 	  __RAW_SPIN_LOCK_UNLOCKED(name.wait_lock),	\
diff --git a/kernel/Kconfig.locks b/kernel/Kconfig.locks
index 35536d9..e4c3162 100644
--- a/kernel/Kconfig.locks
+++ b/kernel/Kconfig.locks
@@ -224,6 +224,10 @@ config MUTEX_SPIN_ON_OWNER
 	def_bool y
 	depends on SMP && !DEBUG_MUTEXES
 
+config RWSEM_SPIN_ON_OWNER
+       def_bool y
+       depends on SMP && RWSEM_XCHGADD_ALGORITHM
+
 config ARCH_USE_QUEUE_RWLOCK
 	bool
 
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index dacc321..abe6e06 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -82,7 +82,7 @@ void __init_rwsem(struct rw_semaphore *sem, const char *name,
 	sem->count = RWSEM_UNLOCKED_VALUE;
 	raw_spin_lock_init(&sem->wait_lock);
 	INIT_LIST_HEAD(&sem->wait_list);
-#ifdef CONFIG_SMP
+#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
 	sem->owner = NULL;
 	sem->osq = NULL;
 #endif
@@ -262,7 +262,7 @@ static inline bool rwsem_try_write_lock(long count, struct rw_semaphore *sem)
 	return false;
 }
 
-#ifdef CONFIG_SMP
+#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
 /*
  * Try to acquire write lock before the writer has been put on wait queue.
  */
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index 42f806d..e2d3bc7 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -12,7 +12,7 @@
 
 #include <linux/atomic.h>
 
-#if defined(CONFIG_SMP) && defined(CONFIG_RWSEM_XCHGADD_ALGORITHM)
+#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
 static inline void rwsem_set_owner(struct rw_semaphore *sem)
 {
 	sem->owner = current;
-- 
1.8.1.4

[PATCH 2/2] locking/rwsem: Disable optimistic spinning for PA-RISC

[Davidlohr Bueso]

PA-RISC's cmpxchg is not save against normal stores and the code used
for optimistic spinning is known broken because of this.

Disable for now.

[Changelog from PeterZ]
Reported-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
---
 kernel/Kconfig.locks | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/Kconfig.locks b/kernel/Kconfig.locks
index e4c3162..125c77a 100644
--- a/kernel/Kconfig.locks
+++ b/kernel/Kconfig.locks
@@ -226,7 +226,7 @@ config MUTEX_SPIN_ON_OWNER
 
 config RWSEM_SPIN_ON_OWNER
        def_bool y
-       depends on SMP && RWSEM_XCHGADD_ALGORITHM
+       depends on SMP && RWSEM_XCHGADD_ALGORITHM && !PARISC
 
 config ARCH_USE_QUEUE_RWLOCK
 	bool
-- 
1.8.1.4

Re: [PATCH 2/2] locking/rwsem: Disable optimistic spinning for PA-RISC

[James Bottomley]

On Fri, 2014-06-06 at 08:55 -0700, Davidlohr Bueso wrote:
> PA-RISC's cmpxchg is not save against normal stores and the code used
> for optimistic spinning is known broken because of this.

What about all the other identified architectures?  The problem is that
unless you can do an atomic Read Modify Write on your architecture, you
have to implement our exchange primitives with locking, and that makes
you unsafe against stores  We happen to be the architecture that
detected this, but I thought we agreed sparc32, metag, tile32, arc and
possibly hexagon have this problem.

Rather than naming all the failing architectures, we probably want an 

ARCH_NO_ATOMIC_RMW

symbol which they select to indicate they can't do atomic exchange and
then you make

depends on SMP && RWSEM_XCHGADD_ALGORITHM &!ARCH_NO_ATOMIC_RMW

So they can all self select (especially if more come crawling out of the
woodwork).

James

Re: [PATCH 2/2] locking/rwsem: Disable optimistic spinning for PA-RISC

[Peter Zijlstra]

On Fri, Jun 06, 2014 at 09:09:47AM -0700, James Bottomley wrote:
> On Fri, 2014-06-06 at 08:55 -0700, Davidlohr Bueso wrote:
> > PA-RISC's cmpxchg is not save against normal stores and the code used
> > for optimistic spinning is known broken because of this.
> 
> What about all the other identified architectures?  The problem is that
> unless you can do an atomic Read Modify Write on your architecture, you
> have to implement our exchange primitives with locking, and that makes
> you unsafe against stores  We happen to be the architecture that
> detected this, but I thought we agreed sparc32, metag, tile32, arc and
> possibly hexagon have this problem.
> 
> Rather than naming all the failing architectures, we probably want an 
> 
> ARCH_NO_ATOMIC_RMW

The thing is, all these archs are broken beyond this particular problem,
Mikulas Patocka found a number of other spots.

In any case, sure I can exclude more. Although ideally someone goes do
that __atomic sparse thing to flush out all this.

---
Subject: locking, mutex: Disable optimistic spinning on !RMW archs

For some archs a regular store does not play nice with cmpxchg(), the
optimistic spinning code (and various other places not caught by this)
break this assumption and make things go boom.

Until something better is found, disable optimistic spinning for these
archs.

Cc: James.Bottomley@HansenPartnership.com
Cc: davem@davemloft.net
Cc: james.hogan@imgtec.com
Cc: cmetcalf@tilera.com
Cc: vgupta@synopsys.com
Reported-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 kernel/Kconfig.locks | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/Kconfig.locks b/kernel/Kconfig.locks
index d2b32ac27a39..b9c132c48bf1 100644
--- a/kernel/Kconfig.locks
+++ b/kernel/Kconfig.locks
@@ -220,6 +220,10 @@ config INLINE_WRITE_UNLOCK_IRQRESTORE
 
 endif
 
+config ARCH_NO_ATOMIC_RMW
+	def_bool y
+	depends on PARISC || SPARC32 || METAG_ATOMICITY_LOCK1 || (TILE && !TILEGX) || (ARC && !ARC_HAS_LLSC)
+
 config MUTEX_SPIN_ON_OWNER
 	def_bool y
-	depends on SMP && !DEBUG_MUTEXES
+	depends on SMP && !DEBUG_MUTEXES && !ARCH_NO_ATOMIC_RMW

Re: [PATCH 2/2] locking/rwsem: Disable optimistic spinning for PA-RISC

[Chris Metcalf]

On 6/6/2014 1:11 PM, Peter Zijlstra wrote:
> The thing is, all these archs are broken beyond this particular problem,
> Mikulas Patocka found a number of other spots.
>
> In any case, sure I can exclude more. Although ideally someone goes do
> that __atomic sparse thing to flush out all this.
>
> ---
> Subject: locking, mutex: Disable optimistic spinning on !RMW archs
>
> For some archs a regular store does not play nice with cmpxchg(), the
> optimistic spinning code (and various other places not caught by this)
> break this assumption and make things go boom.
>
> Until something better is found, disable optimistic spinning for these
> archs.
>
> [..]
>   
> +config ARCH_NO_ATOMIC_RMW
> +	def_bool y
> +	depends on PARISC || SPARC32 || METAG_ATOMICITY_LOCK1 || (TILE && !TILEGX) || (ARC && !ARC_HAS_LLSC)

For tile:

Acked-by: Chris Metcalf <cmetcalf@tilera.com>

But you should use "TILEPRO" (added in kernel 3.5) instead of "(TILE && !TILEGX)".

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH 2/2] locking/rwsem: Disable optimistic spinning for PA-RISC

[Linus Torvalds]

On Fri, Jun 6, 2014 at 10:11 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> +config ARCH_NO_ATOMIC_RMW
> +       def_bool y
> +       depends on PARISC || SPARC32 || METAG_ATOMICITY_LOCK1 || (TILE && !TILEGX) || (ARC && !ARC_HAS_LLSC)

Ugh. We've had these kinds of things before, and they are broken and
nasty to maintain.

Just make it

    config ARCH_SUPPORTS_ATOMIC_RMW
        bool

which defaults to no. And then make MUTEX_SPIN_ON_OWNER depend on that.

And then we can add "select ARCH_SUPPORTS_ATOMIC_RMW" to the few
architectures we (a) care about and (b) know work. So start with x86,
arm, powerpc and sparc64, and then the rest can just add their own
oneliners if they care.

Remember, most people really won't ever care about this, simply
because it only matters if you have enough CPU's for the whole
spinning thing to make a noticeable difference.  So missing some odd
architecture _really_ doesn't matter.

And we really really *really* shouldn't have these kinds of "random
really odd architecture details" in the generic code, not even if it's
something as specific as kernel/Kconfig.locks.

               Linus

              Linus

Re: [PATCH 2/2] locking/rwsem: Disable optimistic spinning for PA-RISC

[Peter Zijlstra]

On Fri, Jun 06, 2014 at 10:22:21AM -0700, Linus Torvalds wrote:
> On Fri, Jun 6, 2014 at 10:11 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > +config ARCH_NO_ATOMIC_RMW
> > +       def_bool y
> > +       depends on PARISC || SPARC32 || METAG_ATOMICITY_LOCK1 || (TILE && !TILEGX) || (ARC && !ARC_HAS_LLSC)
> 
> Ugh. We've had these kinds of things before, and they are broken and
> nasty to maintain.
> 
> Just make it
> 
>     config ARCH_SUPPORTS_ATOMIC_RMW
>         bool
> 
> which defaults to no. And then make MUTEX_SPIN_ON_OWNER depend on that.

---
Subject: locking, mutex: Make optimistic spinning depend on ARCH_SUPPORTS_ATOMIC_RMW

The optimistic spin code assumes regular stores and cmpxchg() play nice;
this is found to not be true for at least: parisc, sparc32, tile32,
metag-lock1, arc-!llsc and hexagon.

There is further wreckage, but this in particular seemed easy to
trigger, so blacklist this.

Opt in for known good archs.

Reported-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 arch/arm/Kconfig     | 1 +
 arch/arm64/Kconfig   | 1 +
 arch/powerpc/Kconfig | 1 +
 arch/sparc/Kconfig   | 1 +
 arch/x86/Kconfig     | 1 +
 kernel/Kconfig.locks | 5 ++++-
 6 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index a2c1a18a7275..b8a6a5078eae 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -6,6 +6,7 @@ config ARM
 	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
 	select ARCH_HAVE_CUSTOM_GPIO_H
 	select ARCH_MIGHT_HAVE_PC_PARPORT
+	select ARCH_SUPPORTS_ATOMIC_RMW
 	select ARCH_USE_BUILTIN_BSWAP
 	select ARCH_USE_CMPXCHG_LOCKREF
 	select ARCH_WANT_IPC_PARSE_VERSION
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 5c30b10c0f6c..339dc07f15e2 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -3,6 +3,7 @@ config ARM64
 	select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
 	select ARCH_USE_CMPXCHG_LOCKREF
 	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
+	select ARCH_SUPPORTS_ATOMIC_RMW
 	select ARCH_WANT_OPTIONAL_GPIOLIB
 	select ARCH_WANT_COMPAT_IPC_PARSE_VERSION
 	select ARCH_WANT_FRAME_POINTERS
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index e0998997943b..5bb96f282373 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -145,6 +145,7 @@ config PPC
 	select HAVE_IRQ_EXIT_ON_IRQ_STACK
 	select ARCH_USE_CMPXCHG_LOCKREF if PPC64
 	select HAVE_ARCH_AUDITSYSCALL
+	select ARCH_SUPPORTS_ATOMIC_RMW
 
 config GENERIC_CSUM
 	def_bool CPU_LITTLE_ENDIAN
diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
index 29f2e988c56a..407c87d9879a 100644
--- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig
@@ -78,6 +78,7 @@ config SPARC64
 	select HAVE_C_RECORDMCOUNT
 	select NO_BOOTMEM
 	select HAVE_ARCH_AUDITSYSCALL
+	select ARCH_SUPPORTS_ATOMIC_RMW
 
 config ARCH_DEFCONFIG
 	string
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 3d4951d35587..2fe1f7c5994e 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -130,6 +130,7 @@ config X86
 	select HAVE_CC_STACKPROTECTOR
 	select GENERIC_CPU_AUTOPROBE
 	select HAVE_ARCH_AUDITSYSCALL
+	select ARCH_SUPPORTS_ATOMIC_RMW
 
 config INSTRUCTION_DECODER
 	def_bool y
diff --git a/kernel/Kconfig.locks b/kernel/Kconfig.locks
index d2b32ac27a39..ecee67a00f5f 100644
--- a/kernel/Kconfig.locks
+++ b/kernel/Kconfig.locks
@@ -220,6 +220,9 @@ config INLINE_WRITE_UNLOCK_IRQRESTORE
 
 endif
 
+config ARCH_SUPPORTS_ATOMIC_RMW
+	bool
+
 config MUTEX_SPIN_ON_OWNER
 	def_bool y
-	depends on SMP && !DEBUG_MUTEXES
+	depends on SMP && !DEBUG_MUTEXES && ARCH_SUPPORTS_ATOMIC_RMW

Re: [PATCH 2/2] locking/rwsem: Disable optimistic spinning for PA-RISC

[James Bottomley]

On Fri, 2014-06-06 at 19:53 +0200, Peter Zijlstra wrote:
> On Fri, Jun 06, 2014 at 10:22:21AM -0700, Linus Torvalds wrote:
> > On Fri, Jun 6, 2014 at 10:11 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > +config ARCH_NO_ATOMIC_RMW
> > > +       def_bool y
> > > +       depends on PARISC || SPARC32 || METAG_ATOMICITY_LOCK1 || (TILE && !TILEGX) || (ARC && !ARC_HAS_LLSC)
> > 
> > Ugh. We've had these kinds of things before, and they are broken and
> > nasty to maintain.
> > 
> > Just make it
> > 
> >     config ARCH_SUPPORTS_ATOMIC_RMW
> >         bool
> > 
> > which defaults to no. And then make MUTEX_SPIN_ON_OWNER depend on that.
> 
> ---
> Subject: locking, mutex: Make optimistic spinning depend on ARCH_SUPPORTS_ATOMIC_RMW
> 
> The optimistic spin code assumes regular stores and cmpxchg() play nice;
> this is found to not be true for at least: parisc, sparc32, tile32,
> metag-lock1, arc-!llsc and hexagon.
> 
> There is further wreckage, but this in particular seemed easy to
> trigger, so blacklist this.
> 
> Opt in for known good archs.
> 
> Reported-by: Mikulas Patocka <mpatocka@redhat.com>
> Signed-off-by: Peter Zijlstra <peterz@infradead.org>
> ---
>  arch/arm/Kconfig     | 1 +
>  arch/arm64/Kconfig   | 1 +
>  arch/powerpc/Kconfig | 1 +
>  arch/sparc/Kconfig   | 1 +
>  arch/x86/Kconfig     | 1 +
>  kernel/Kconfig.locks | 5 ++++-
>  6 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index a2c1a18a7275..b8a6a5078eae 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -6,6 +6,7 @@ config ARM
>  	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
>  	select ARCH_HAVE_CUSTOM_GPIO_H
>  	select ARCH_MIGHT_HAVE_PC_PARPORT
> +	select ARCH_SUPPORTS_ATOMIC_RMW
>  	select ARCH_USE_BUILTIN_BSWAP
>  	select ARCH_USE_CMPXCHG_LOCKREF
>  	select ARCH_WANT_IPC_PARSE_VERSION
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 5c30b10c0f6c..339dc07f15e2 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -3,6 +3,7 @@ config ARM64
>  	select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
>  	select ARCH_USE_CMPXCHG_LOCKREF
>  	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
> +	select ARCH_SUPPORTS_ATOMIC_RMW
>  	select ARCH_WANT_OPTIONAL_GPIOLIB
>  	select ARCH_WANT_COMPAT_IPC_PARSE_VERSION
>  	select ARCH_WANT_FRAME_POINTERS
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index e0998997943b..5bb96f282373 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -145,6 +145,7 @@ config PPC
>  	select HAVE_IRQ_EXIT_ON_IRQ_STACK
>  	select ARCH_USE_CMPXCHG_LOCKREF if PPC64
>  	select HAVE_ARCH_AUDITSYSCALL
> +	select ARCH_SUPPORTS_ATOMIC_RMW
>  
>  config GENERIC_CSUM
>  	def_bool CPU_LITTLE_ENDIAN
> diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
> index 29f2e988c56a..407c87d9879a 100644
> --- a/arch/sparc/Kconfig
> +++ b/arch/sparc/Kconfig
> @@ -78,6 +78,7 @@ config SPARC64
>  	select HAVE_C_RECORDMCOUNT
>  	select NO_BOOTMEM
>  	select HAVE_ARCH_AUDITSYSCALL
> +	select ARCH_SUPPORTS_ATOMIC_RMW

Sparc64 does, Sparc32 doesn't, so shouldn't that be

select ARCH_SUPPORTS_ATOMIC_RMW if 64BIT

?

Other than this, looks good to me (you can add reviewed by or acked by
from me to your taste).

James

Re: [PATCH 2/2] locking/rwsem: Disable optimistic spinning for PA-RISC

[James Bottomley]

On Fri, 2014-06-06 at 10:57 -0700, James Bottomley wrote:
> On Fri, 2014-06-06 at 19:53 +0200, Peter Zijlstra wrote:
> > On Fri, Jun 06, 2014 at 10:22:21AM -0700, Linus Torvalds wrote:
> > > On Fri, Jun 6, 2014 at 10:11 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > > >
> > > > +config ARCH_NO_ATOMIC_RMW
> > > > +       def_bool y
> > > > +       depends on PARISC || SPARC32 || METAG_ATOMICITY_LOCK1 || (TILE && !TILEGX) || (ARC && !ARC_HAS_LLSC)
> > > 
> > > Ugh. We've had these kinds of things before, and they are broken and
> > > nasty to maintain.
> > > 
> > > Just make it
> > > 
> > >     config ARCH_SUPPORTS_ATOMIC_RMW
> > >         bool
> > > 
> > > which defaults to no. And then make MUTEX_SPIN_ON_OWNER depend on that.
> > 
> > ---
> > Subject: locking, mutex: Make optimistic spinning depend on ARCH_SUPPORTS_ATOMIC_RMW
> > 
> > The optimistic spin code assumes regular stores and cmpxchg() play nice;
> > this is found to not be true for at least: parisc, sparc32, tile32,
> > metag-lock1, arc-!llsc and hexagon.
> > 
> > There is further wreckage, but this in particular seemed easy to
> > trigger, so blacklist this.
> > 
> > Opt in for known good archs.
> > 
> > Reported-by: Mikulas Patocka <mpatocka@redhat.com>
> > Signed-off-by: Peter Zijlstra <peterz@infradead.org>
> > ---
> >  arch/arm/Kconfig     | 1 +
> >  arch/arm64/Kconfig   | 1 +
> >  arch/powerpc/Kconfig | 1 +
> >  arch/sparc/Kconfig   | 1 +
> >  arch/x86/Kconfig     | 1 +
> >  kernel/Kconfig.locks | 5 ++++-
> >  6 files changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index a2c1a18a7275..b8a6a5078eae 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -6,6 +6,7 @@ config ARM
> >  	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
> >  	select ARCH_HAVE_CUSTOM_GPIO_H
> >  	select ARCH_MIGHT_HAVE_PC_PARPORT
> > +	select ARCH_SUPPORTS_ATOMIC_RMW
> >  	select ARCH_USE_BUILTIN_BSWAP
> >  	select ARCH_USE_CMPXCHG_LOCKREF
> >  	select ARCH_WANT_IPC_PARSE_VERSION
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index 5c30b10c0f6c..339dc07f15e2 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -3,6 +3,7 @@ config ARM64
> >  	select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
> >  	select ARCH_USE_CMPXCHG_LOCKREF
> >  	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
> > +	select ARCH_SUPPORTS_ATOMIC_RMW
> >  	select ARCH_WANT_OPTIONAL_GPIOLIB
> >  	select ARCH_WANT_COMPAT_IPC_PARSE_VERSION
> >  	select ARCH_WANT_FRAME_POINTERS
> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > index e0998997943b..5bb96f282373 100644
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -145,6 +145,7 @@ config PPC
> >  	select HAVE_IRQ_EXIT_ON_IRQ_STACK
> >  	select ARCH_USE_CMPXCHG_LOCKREF if PPC64
> >  	select HAVE_ARCH_AUDITSYSCALL
> > +	select ARCH_SUPPORTS_ATOMIC_RMW
> >  
> >  config GENERIC_CSUM
> >  	def_bool CPU_LITTLE_ENDIAN
> > diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
> > index 29f2e988c56a..407c87d9879a 100644
> > --- a/arch/sparc/Kconfig
> > +++ b/arch/sparc/Kconfig
> > @@ -78,6 +78,7 @@ config SPARC64
> >  	select HAVE_C_RECORDMCOUNT
> >  	select NO_BOOTMEM
> >  	select HAVE_ARCH_AUDITSYSCALL
> > +	select ARCH_SUPPORTS_ATOMIC_RMW
> 
> Sparc64 does, Sparc32 doesn't, so shouldn't that be
> 
> select ARCH_SUPPORTS_ATOMIC_RMW if 64BIT
> 
> ?
> 
> Other than this, looks good to me (you can add reviewed by or acked by
> from me to your taste).

Oh, wait, I see how sparc does it, never mind ...

James

Re: [PATCH 2/2] locking/rwsem: Disable optimistic spinning for PA-RISC

[Peter Zijlstra]

On Fri, Jun 06, 2014 at 10:57:07AM -0700, James Bottomley wrote:
> > diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
> > index 29f2e988c56a..407c87d9879a 100644
> > --- a/arch/sparc/Kconfig
> > +++ b/arch/sparc/Kconfig
> > @@ -78,6 +78,7 @@ config SPARC64
                      ^^^^^^^^^^^^^^


> >  	select HAVE_C_RECORDMCOUNT
> >  	select NO_BOOTMEM
> >  	select HAVE_ARCH_AUDITSYSCALL
> > +	select ARCH_SUPPORTS_ATOMIC_RMW
> 
> Sparc64 does, Sparc32 doesn't, so shouldn't that be
> 
> select ARCH_SUPPORTS_ATOMIC_RMW if 64BIT
> 
> ?

arch/sparc/Kconfig has 3 large sections:

config SPARC (included in both)
config SPARC32 (specific to 32bit)
config SPARC64 (specific to 64bit)

I added it to the last.