[PATCH 0/2] Fix boot regression for s390 and remove break_lock

[PATCH 0/2] Fix boot regression for s390 and remove break_lock

[Will Deacon]

Hi all,

The following two patches do the following:

  1. Fix boot breakage reported on s390 caused by a8a217c22116
  2. Kill off the break_lock field entirely, since it's not actually
     that useful

I didn't go the whole hog and remove CONFIG_GENERIC_LOCKBREAK, since the
"do something different with locks in preemptible kernels" isn't necessarily
a bad idea, and PowerPC actually implements things like arch_spin_relax
for that.

The first patch should go in to 4.15.

Thanks,

Will

--->8

Will Deacon (2):
  locking/core: Fix deadlock during boot on systems with
    GENERIC_LOCKBREAK
  locking/core: Remove break_lock field when CONFIG_GENERIC_LOCKBREAK=y

 include/linux/rwlock_types.h   |  3 ---
 include/linux/spinlock.h       |  5 -----
 include/linux/spinlock_types.h |  3 ---
 kernel/locking/spinlock.c      | 13 +++----------
 4 files changed, 3 insertions(+), 21 deletions(-)

-- 
2.1.4

[PATCH 1/2] locking/core: Fix deadlock during boot on systems with GENERIC_LOCKBREAK

[Will Deacon]

Commit a8a217c22116 ("locking/core: Remove {read,spin,write}_can_lock()")
removed the definition of raw_spin_can_lock, causing the GENERIC_LOCKBREAK
spin_lock routines to poll the break_lock field when waiting on a lock.

This has been reported to cause a deadlock during boot on s390, because
the break_lock field is also set by the waiters, and can potentially
remain set indefinitely if no other CPUs come in to take the lock after
it has been released.

This patch removes the explicit spinning on break_lock from the waiters,
instead relying on the outer trylock operation to determine when the
lock is available.

Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>
Fixes: a8a217c22116 ("locking/core: Remove {read,spin,write}_can_lock()")
Reported-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
Tested-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 kernel/locking/spinlock.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/locking/spinlock.c b/kernel/locking/spinlock.c
index 1fd1a7543cdd..0ebb253e2199 100644
--- a/kernel/locking/spinlock.c
+++ b/kernel/locking/spinlock.c
@@ -68,8 +68,8 @@ void __lockfunc __raw_##op##_lock(locktype##_t *lock)			\
 									\
 		if (!(lock)->break_lock)				\
 			(lock)->break_lock = 1;				\
-		while ((lock)->break_lock)				\
-			arch_##op##_relax(&lock->raw_lock);		\
+									\
+		arch_##op##_relax(&lock->raw_lock);			\
 	}								\
 	(lock)->break_lock = 0;						\
 }									\
@@ -88,8 +88,8 @@ unsigned long __lockfunc __raw_##op##_lock_irqsave(locktype##_t *lock)	\
 									\
 		if (!(lock)->break_lock)				\
 			(lock)->break_lock = 1;				\
-		while ((lock)->break_lock)				\
-			arch_##op##_relax(&lock->raw_lock);		\
+									\
+		arch_##op##_relax(&lock->raw_lock);			\
 	}								\
 	(lock)->break_lock = 0;						\
 	return flags;							\
-- 
2.1.4

[PATCH 2/2] locking/core: Remove break_lock field when CONFIG_GENERIC_LOCKBREAK=y

[Will Deacon]

When CONFIG_GENERIC_LOCKBEAK=y, locking structures grow an extra int break_lock
field which is used to implement raw_spin_is_contended() by setting the field
to 1 when waiting on a lock and clearing it to zero when holding a lock.
However, there are a few problems with this approach:

  - There is a write-write race between a CPU successfully taking the lock
    (and subsequently writing break_lock = 0) and a waiter waiting on
    the lock (and subsequently writing break_lock = 1). This could result
    in a contended lock being reported as uncontended and vice-versa.

  - On machines with store buffers, nothing guarantees that the writes
    to break_lock are visible to other CPUs at any particular time.

  - READ_ONCE/WRITE_ONCE are not used, so the field is potentially
    susceptible to harmful compiler optimisations,

Consequently, the usefulness of this field is unclear and we'd be better off
removing it and allowing architectures to implement raw_spin_is_contended() by
providing a definition of arch_spin_is_contended(), as they can when
CONFIG_GENERIC_LOCKBREAK=n.

Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Sebastian Ott <sebott@linux.vnet.ibm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 include/linux/rwlock_types.h   | 3 ---
 include/linux/spinlock.h       | 5 -----
 include/linux/spinlock_types.h | 3 ---
 kernel/locking/spinlock.c      | 9 +--------
 4 files changed, 1 insertion(+), 19 deletions(-)

diff --git a/include/linux/rwlock_types.h b/include/linux/rwlock_types.h
index cc0072e93e36..857a72ceb794 100644
--- a/include/linux/rwlock_types.h
+++ b/include/linux/rwlock_types.h
@@ -10,9 +10,6 @@
  */
 typedef struct {
 	arch_rwlock_t raw_lock;
-#ifdef CONFIG_GENERIC_LOCKBREAK
-	unsigned int break_lock;
-#endif
 #ifdef CONFIG_DEBUG_SPINLOCK
 	unsigned int magic, owner_cpu;
 	void *owner;
diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index a39186194cd6..3bf273538840 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -107,16 +107,11 @@ do {								\
 
 #define raw_spin_is_locked(lock)	arch_spin_is_locked(&(lock)->raw_lock)
 
-#ifdef CONFIG_GENERIC_LOCKBREAK
-#define raw_spin_is_contended(lock) ((lock)->break_lock)
-#else
-
 #ifdef arch_spin_is_contended
 #define raw_spin_is_contended(lock)	arch_spin_is_contended(&(lock)->raw_lock)
 #else
 #define raw_spin_is_contended(lock)	(((void)(lock), 0))
 #endif /*arch_spin_is_contended*/
-#endif
 
 /*
  * This barrier must provide two things:
diff --git a/include/linux/spinlock_types.h b/include/linux/spinlock_types.h
index 73548eb13a5d..24b4e6f2c1a2 100644
--- a/include/linux/spinlock_types.h
+++ b/include/linux/spinlock_types.h
@@ -19,9 +19,6 @@
 
 typedef struct raw_spinlock {
 	arch_spinlock_t raw_lock;
-#ifdef CONFIG_GENERIC_LOCKBREAK
-	unsigned int break_lock;
-#endif
 #ifdef CONFIG_DEBUG_SPINLOCK
 	unsigned int magic, owner_cpu;
 	void *owner;
diff --git a/kernel/locking/spinlock.c b/kernel/locking/spinlock.c
index 0ebb253e2199..936f3d14dd6b 100644
--- a/kernel/locking/spinlock.c
+++ b/kernel/locking/spinlock.c
@@ -66,12 +66,8 @@ void __lockfunc __raw_##op##_lock(locktype##_t *lock)			\
 			break;						\
 		preempt_enable();					\
 									\
-		if (!(lock)->break_lock)				\
-			(lock)->break_lock = 1;				\
-									\
 		arch_##op##_relax(&lock->raw_lock);			\
 	}								\
-	(lock)->break_lock = 0;						\
 }									\
 									\
 unsigned long __lockfunc __raw_##op##_lock_irqsave(locktype##_t *lock)	\
@@ -86,12 +82,9 @@ unsigned long __lockfunc __raw_##op##_lock_irqsave(locktype##_t *lock)	\
 		local_irq_restore(flags);				\
 		preempt_enable();					\
 									\
-		if (!(lock)->break_lock)				\
-			(lock)->break_lock = 1;				\
-									\
 		arch_##op##_relax(&lock->raw_lock);			\
 	}								\
-	(lock)->break_lock = 0;						\
+									\
 	return flags;							\
 }									\
 									\
-- 
2.1.4

Re: [PATCH 0/2] Fix boot regression for s390 and remove break_lock

[Heiko Carstens]

On Tue, Nov 28, 2017 at 06:42:17PM +0000, Will Deacon wrote:
> Hi all,
> 
> The following two patches do the following:
> 
>   1. Fix boot breakage reported on s390 caused by a8a217c22116
>   2. Kill off the break_lock field entirely, since it's not actually
>      that useful
> 
> I didn't go the whole hog and remove CONFIG_GENERIC_LOCKBREAK, since the
> "do something different with locks in preemptible kernels" isn't necessarily
> a bad idea, and PowerPC actually implements things like arch_spin_relax
> for that.
> 
> The first patch should go in to 4.15.
> 
> Thanks,
> 
> Will
> 
> --->8
> 
> Will Deacon (2):
>   locking/core: Fix deadlock during boot on systems with
>     GENERIC_LOCKBREAK
>   locking/core: Remove break_lock field when CONFIG_GENERIC_LOCKBREAK=y
> 
>  include/linux/rwlock_types.h   |  3 ---
>  include/linux/spinlock.h       |  5 -----
>  include/linux/spinlock_types.h |  3 ---
>  kernel/locking/spinlock.c      | 13 +++----------
>  4 files changed, 3 insertions(+), 21 deletions(-)

*ping* ... these patches haven't been picked up yet as far as I can tell.
Peter, Ingo?

Re: [PATCH 0/2] Fix boot regression for s390 and remove break_lock

[Will Deacon]

On Sat, Dec 02, 2017 at 10:02:08AM +0100, Heiko Carstens wrote:
> On Tue, Nov 28, 2017 at 06:42:17PM +0000, Will Deacon wrote:
> > The following two patches do the following:
> > 
> >   1. Fix boot breakage reported on s390 caused by a8a217c22116
> >   2. Kill off the break_lock field entirely, since it's not actually
> >      that useful
> > 
> > I didn't go the whole hog and remove CONFIG_GENERIC_LOCKBREAK, since the
> > "do something different with locks in preemptible kernels" isn't necessarily
> > a bad idea, and PowerPC actually implements things like arch_spin_relax
> > for that.
> > 
> > The first patch should go in to 4.15.
> > 
> > Thanks,
> > 
> > Will
> > 
> > --->8
> > 
> > Will Deacon (2):
> >   locking/core: Fix deadlock during boot on systems with
> >     GENERIC_LOCKBREAK
> >   locking/core: Remove break_lock field when CONFIG_GENERIC_LOCKBREAK=y
> > 
> >  include/linux/rwlock_types.h   |  3 ---
> >  include/linux/spinlock.h       |  5 -----
> >  include/linux/spinlock_types.h |  3 ---
> >  kernel/locking/spinlock.c      | 13 +++----------
> >  4 files changed, 3 insertions(+), 21 deletions(-)
> 
> *ping* ... these patches haven't been picked up yet as far as I can tell.
> Peter, Ingo?

Damn, this first patch didn't appear in -rc3.

Ingo -- please can you pick this up? Without it, s390 doesn't boot with
mainline.

Thanks,

Will

Re: [PATCH 0/2] Fix boot regression for s390 and remove break_lock

[Ingo Molnar]

* Will Deacon <will.deacon@arm.com> wrote:

> On Sat, Dec 02, 2017 at 10:02:08AM +0100, Heiko Carstens wrote:
> > On Tue, Nov 28, 2017 at 06:42:17PM +0000, Will Deacon wrote:
> > > The following two patches do the following:
> > > 
> > >   1. Fix boot breakage reported on s390 caused by a8a217c22116
> > >   2. Kill off the break_lock field entirely, since it's not actually
> > >      that useful
> > > 
> > > I didn't go the whole hog and remove CONFIG_GENERIC_LOCKBREAK, since the
> > > "do something different with locks in preemptible kernels" isn't necessarily
> > > a bad idea, and PowerPC actually implements things like arch_spin_relax
> > > for that.
> > > 
> > > The first patch should go in to 4.15.
> > > 
> > > Thanks,
> > > 
> > > Will
> > > 
> > > --->8
> > > 
> > > Will Deacon (2):
> > >   locking/core: Fix deadlock during boot on systems with
> > >     GENERIC_LOCKBREAK
> > >   locking/core: Remove break_lock field when CONFIG_GENERIC_LOCKBREAK=y
> > > 
> > >  include/linux/rwlock_types.h   |  3 ---
> > >  include/linux/spinlock.h       |  5 -----
> > >  include/linux/spinlock_types.h |  3 ---
> > >  kernel/locking/spinlock.c      | 13 +++----------
> > >  4 files changed, 3 insertions(+), 21 deletions(-)
> > 
> > *ping* ... these patches haven't been picked up yet as far as I can tell.
> > Peter, Ingo?
> 
> Damn, this first patch didn't appear in -rc3.
> 
> Ingo -- please can you pick this up? Without it, s390 doesn't boot with
> mainline.

Applied now, and will get it to Linus ASAP, sorry about that!

Thanks,

	Ingo

[tip:locking/urgent] locking/core: Fix deadlock during boot on systems with GENERIC_LOCKBREAK

[tip-bot for Will Deacon]

Commit-ID:  f87f3a328dbbb3e79dd53e7e889ced9222512649
Gitweb:     https://git.kernel.org/tip/f87f3a328dbbb3e79dd53e7e889ced9222512649
Author:     Will Deacon <will.deacon@arm.com>
AuthorDate: Tue, 28 Nov 2017 18:42:18 +0000
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 12 Dec 2017 11:24:01 +0100

locking/core: Fix deadlock during boot on systems with GENERIC_LOCKBREAK

Commit:

  a8a217c22116 ("locking/core: Remove {read,spin,write}_can_lock()")

removed the definition of raw_spin_can_lock(), causing the GENERIC_LOCKBREAK
spin_lock() routines to poll the ->break_lock field when waiting on a lock.

This has been reported to cause a deadlock during boot on s390, because
the ->break_lock field is also set by the waiters, and can potentially
remain set indefinitely if no other CPUs come in to take the lock after
it has been released.

This patch removes the explicit spinning on ->break_lock from the waiters,
instead relying on the outer trylock() operation to determine when the
lock is available.

Reported-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
Tested-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
Acked-by: Peter Zijlstra <peterz@infradead.org>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: a8a217c22116 ("locking/core: Remove {read,spin,write}_can_lock()")
Link: http://lkml.kernel.org/r/1511894539-7988-2-git-send-email-will.deacon@arm.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/locking/spinlock.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/locking/spinlock.c b/kernel/locking/spinlock.c
index 1fd1a75..0ebb253 100644
--- a/kernel/locking/spinlock.c
+++ b/kernel/locking/spinlock.c
@@ -68,8 +68,8 @@ void __lockfunc __raw_##op##_lock(locktype##_t *lock)			\
 									\
 		if (!(lock)->break_lock)				\
 			(lock)->break_lock = 1;				\
-		while ((lock)->break_lock)				\
-			arch_##op##_relax(&lock->raw_lock);		\
+									\
+		arch_##op##_relax(&lock->raw_lock);			\
 	}								\
 	(lock)->break_lock = 0;						\
 }									\
@@ -88,8 +88,8 @@ unsigned long __lockfunc __raw_##op##_lock_irqsave(locktype##_t *lock)	\
 									\
 		if (!(lock)->break_lock)				\
 			(lock)->break_lock = 1;				\
-		while ((lock)->break_lock)				\
-			arch_##op##_relax(&lock->raw_lock);		\
+									\
+		arch_##op##_relax(&lock->raw_lock);			\
 	}								\
 	(lock)->break_lock = 0;						\
 	return flags;							\

[tip:locking/urgent] locking/core: Remove break_lock field when CONFIG_GENERIC_LOCKBREAK=y

[tip-bot for Will Deacon]

Commit-ID:  d89c70356acf11b7cf47ca5cfcafae5062a85451
Gitweb:     https://git.kernel.org/tip/d89c70356acf11b7cf47ca5cfcafae5062a85451
Author:     Will Deacon <will.deacon@arm.com>
AuthorDate: Tue, 28 Nov 2017 18:42:19 +0000
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 12 Dec 2017 11:24:01 +0100

locking/core: Remove break_lock field when CONFIG_GENERIC_LOCKBREAK=y

When CONFIG_GENERIC_LOCKBEAK=y, locking structures grow an extra int ->break_lock
field which is used to implement raw_spin_is_contended() by setting the field
to 1 when waiting on a lock and clearing it to zero when holding a lock.
However, there are a few problems with this approach:

  - There is a write-write race between a CPU successfully taking the lock
    (and subsequently writing break_lock = 0) and a waiter waiting on
    the lock (and subsequently writing break_lock = 1). This could result
    in a contended lock being reported as uncontended and vice-versa.

  - On machines with store buffers, nothing guarantees that the writes
    to break_lock are visible to other CPUs at any particular time.

  - READ_ONCE/WRITE_ONCE are not used, so the field is potentially
    susceptible to harmful compiler optimisations,

Consequently, the usefulness of this field is unclear and we'd be better off
removing it and allowing architectures to implement raw_spin_is_contended() by
providing a definition of arch_spin_is_contended(), as they can when
CONFIG_GENERIC_LOCKBREAK=n.

Signed-off-by: Will Deacon <will.deacon@arm.com>
Acked-by: Peter Zijlstra <peterz@infradead.org>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Sebastian Ott <sebott@linux.vnet.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/1511894539-7988-3-git-send-email-will.deacon@arm.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/rwlock_types.h   | 3 ---
 include/linux/spinlock.h       | 5 -----
 include/linux/spinlock_types.h | 3 ---
 kernel/locking/spinlock.c      | 9 +--------
 4 files changed, 1 insertion(+), 19 deletions(-)

diff --git a/include/linux/rwlock_types.h b/include/linux/rwlock_types.h
index cc0072e..857a72c 100644
--- a/include/linux/rwlock_types.h
+++ b/include/linux/rwlock_types.h
@@ -10,9 +10,6 @@
  */
 typedef struct {
 	arch_rwlock_t raw_lock;
-#ifdef CONFIG_GENERIC_LOCKBREAK
-	unsigned int break_lock;
-#endif
 #ifdef CONFIG_DEBUG_SPINLOCK
 	unsigned int magic, owner_cpu;
 	void *owner;
diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index a391861..3bf2735 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -107,16 +107,11 @@ do {								\
 
 #define raw_spin_is_locked(lock)	arch_spin_is_locked(&(lock)->raw_lock)
 
-#ifdef CONFIG_GENERIC_LOCKBREAK
-#define raw_spin_is_contended(lock) ((lock)->break_lock)
-#else
-
 #ifdef arch_spin_is_contended
 #define raw_spin_is_contended(lock)	arch_spin_is_contended(&(lock)->raw_lock)
 #else
 #define raw_spin_is_contended(lock)	(((void)(lock), 0))
 #endif /*arch_spin_is_contended*/
-#endif
 
 /*
  * This barrier must provide two things:
diff --git a/include/linux/spinlock_types.h b/include/linux/spinlock_types.h
index 73548eb..24b4e6f 100644
--- a/include/linux/spinlock_types.h
+++ b/include/linux/spinlock_types.h
@@ -19,9 +19,6 @@
 
 typedef struct raw_spinlock {
 	arch_spinlock_t raw_lock;
-#ifdef CONFIG_GENERIC_LOCKBREAK
-	unsigned int break_lock;
-#endif
 #ifdef CONFIG_DEBUG_SPINLOCK
 	unsigned int magic, owner_cpu;
 	void *owner;
diff --git a/kernel/locking/spinlock.c b/kernel/locking/spinlock.c
index 0ebb253..936f3d1 100644
--- a/kernel/locking/spinlock.c
+++ b/kernel/locking/spinlock.c
@@ -66,12 +66,8 @@ void __lockfunc __raw_##op##_lock(locktype##_t *lock)			\
 			break;						\
 		preempt_enable();					\
 									\
-		if (!(lock)->break_lock)				\
-			(lock)->break_lock = 1;				\
-									\
 		arch_##op##_relax(&lock->raw_lock);			\
 	}								\
-	(lock)->break_lock = 0;						\
 }									\
 									\
 unsigned long __lockfunc __raw_##op##_lock_irqsave(locktype##_t *lock)	\
@@ -86,12 +82,9 @@ unsigned long __lockfunc __raw_##op##_lock_irqsave(locktype##_t *lock)	\
 		local_irq_restore(flags);				\
 		preempt_enable();					\
 									\
-		if (!(lock)->break_lock)				\
-			(lock)->break_lock = 1;				\
-									\
 		arch_##op##_relax(&lock->raw_lock);			\
 	}								\
-	(lock)->break_lock = 0;						\
+									\
 	return flags;							\
 }									\
 									\