[PATCH v3 0/3] locking/spinlock_debug: Change it to a mostly fair lock

[PATCH v3 0/3] locking/spinlock_debug: Change it to a mostly fair lock

[Waiman Long]

 v2->v3:
  - Keep the original v1 patches but move patch 3 of v2 in front so
    as to disable GENERIC_LOCKBREAK when DEBUG_LOCK_ALLOC is on.

 v1->v2:
  - Pack lockup and break_lock into a single 4-byte slot so as not
    to in increase spinlock size when GENERIC_LOCKBREAK is
    on. Hopefully that will be enough to fix a frame size too large
    warning in 0-day build.
  - Add a new patch to disable GENERIC_LOCKBREAK when DEBUG_LOCK_ALLOC
    is on.

The current debug spinlock implementation is a TATAS unfair lock. This
can occasionally lead to system lockup with a debug kernel because
of the unfairness of the lock rather than inherent locking problem.

This patch set changes the debug spinlock implementation to a
mostly fair spinlock based on the MCS lock similar to what is done
in qspinlock. It also includes a patch that disable GENERIC_LOCKBREAK
when DEBUG_LOCK_ALLOC is on.

Waiman Long (3):
  locking/spinlock: Disable GENERIC_LOCKBREAK when DEBUG_LOCK_ALLOC is
    on
  locking/spinlock_debug: Reduce lockup suspected message clutter
  locking/spinlock_debug: Reduce lock cacheline contention

 arch/m32r/Kconfig               |  2 +-
 arch/parisc/Kconfig             |  2 +-
 arch/powerpc/Kconfig            |  2 +-
 arch/s390/Kconfig               |  2 +-
 arch/sh/Kconfig                 |  2 +-
 arch/sparc/Kconfig              |  2 +-
 include/linux/spinlock_types.h  |  8 +++--
 kernel/locking/spinlock_debug.c | 73 ++++++++++++++++++++++++++++++-----------
 8 files changed, 64 insertions(+), 29 deletions(-)

-- 
1.8.3.1

[PATCH v3 1/3] locking/spinlock: Disable GENERIC_LOCKBREAK when DEBUG_LOCK_ALLOC is on

[Waiman Long]

The break_lock variable defined when GENERIC_LOCKBREAK is on is used
only in kernel/locking/spinlock.c when

defined(CONFIG_GENERIC_LOCKBREAK) && !defined(CONFIG_DEBUG_LOCK_ALLOC).

As a result, there is no point in enabling GENERIC_LOCKBREAK to
define one more variable in the spinlock structure that is not going
to be used when DEBUG_LOCK_ALLOC is also on. This patch disables
GENERIC_LOCKBREAK under this circumstance.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 arch/m32r/Kconfig    | 2 +-
 arch/parisc/Kconfig  | 2 +-
 arch/powerpc/Kconfig | 2 +-
 arch/s390/Kconfig    | 2 +-
 arch/sh/Kconfig      | 2 +-
 arch/sparc/Kconfig   | 2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/m32r/Kconfig b/arch/m32r/Kconfig
index d227a69..c0922e0 100644
--- a/arch/m32r/Kconfig
+++ b/arch/m32r/Kconfig
@@ -242,7 +242,7 @@ config IRAM_SIZE
 config GENERIC_LOCKBREAK
 	bool
 	default y
-	depends on SMP && PREEMPT
+	depends on SMP && PREEMPT && !DEBUG_LOCK_ALLOC
 
 config RWSEM_GENERIC_SPINLOCK
 	bool
diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
index 3a71f38..bd8dbb9 100644
--- a/arch/parisc/Kconfig
+++ b/arch/parisc/Kconfig
@@ -56,7 +56,7 @@ config STACK_GROWSUP
 config GENERIC_LOCKBREAK
 	bool
 	default y
-	depends on SMP && PREEMPT
+	depends on SMP && PREEMPT && !DEBUG_LOCK_ALLOC
 
 config RWSEM_GENERIC_SPINLOCK
 	def_bool y
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index a8ee573..feae0a3 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -60,7 +60,7 @@ config RWSEM_XCHGADD_ALGORITHM
 config GENERIC_LOCKBREAK
 	bool
 	default y
-	depends on SMP && PREEMPT
+	depends on SMP && PREEMPT && !DEBUG_LOCK_ALLOC
 
 config ARCH_HAS_ILOG2_U32
 	bool
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index c6722112..a2a33a6 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -38,7 +38,7 @@ config ARCH_DMA_ADDR_T_64BIT
 	def_bool y
 
 config GENERIC_LOCKBREAK
-	def_bool y if SMP && PREEMPT
+	def_bool y if SMP && PREEMPT && !DEBUG_LOCK_ALLOC
 
 config PGSTE
 	def_bool y if KVM
diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
index ee08695..e2eb35c 100644
--- a/arch/sh/Kconfig
+++ b/arch/sh/Kconfig
@@ -105,7 +105,7 @@ config GENERIC_CALIBRATE_DELAY
 
 config GENERIC_LOCKBREAK
 	def_bool y
-	depends on SMP && PREEMPT
+	depends on SMP && PREEMPT && !DEBUG_LOCK_ALLOC
 
 config ARCH_SUSPEND_POSSIBLE
 	def_bool n
diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
index cf4034c..695a31a 100644
--- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig
@@ -286,7 +286,7 @@ config US3_MC
 config GENERIC_LOCKBREAK
 	bool
 	default y
-	depends on SPARC64 && SMP && PREEMPT
+	depends on SPARC64 && SMP && PREEMPT && !DEBUG_LOCK_ALLOC
 
 config NUMA
 	bool "NUMA support"
-- 
1.8.3.1

[PATCH v3 2/3] locking/spinlock_debug: Reduce lockup suspected message clutter

[Waiman Long]

When the debug spinlock code detects a lockup, it will print out an
error messages as well as the backtraces of all the CPUs. However, if
more than one CPUs are waiting on that lock, multiple lockup messages
will be printed leading to garbled output.

To reduce clutter in the console log, now only one of the lock waiters
will be allowed to print out the CPU backtraces.

Since break_lock, like lockup, can only have a value of 0 or 1, its
size is now reduced so that on a 64-bit architecture, the size of the
raw_spinlock structure won't increase whether CONFIG_GENERIC_LOCKBREAK
is defined or not.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 include/linux/spinlock_types.h  |  3 ++-
 kernel/locking/spinlock_debug.c | 26 +++++++++++++++++++++-----
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/include/linux/spinlock_types.h b/include/linux/spinlock_types.h
index 73548eb..ef28ce5 100644
--- a/include/linux/spinlock_types.h
+++ b/include/linux/spinlock_types.h
@@ -23,7 +23,7 @@
 	unsigned int break_lock;
 #endif
 #ifdef CONFIG_DEBUG_SPINLOCK
-	unsigned int magic, owner_cpu;
+	unsigned int magic, owner_cpu, lockup;
 	void *owner;
 #endif
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
@@ -45,6 +45,7 @@
 # define SPIN_DEBUG_INIT(lockname)		\
 	.magic = SPINLOCK_MAGIC,		\
 	.owner_cpu = -1,			\
+	.lockup = 0,				\
 	.owner = SPINLOCK_OWNER_INIT,
 #else
 # define SPIN_DEBUG_INIT(lockname)
diff --git a/kernel/locking/spinlock_debug.c b/kernel/locking/spinlock_debug.c
index 0374a59..0f880a8 100644
--- a/kernel/locking/spinlock_debug.c
+++ b/kernel/locking/spinlock_debug.c
@@ -27,6 +27,7 @@ void __raw_spin_lock_init(raw_spinlock_t *lock, const char *name,
 	lock->magic = SPINLOCK_MAGIC;
 	lock->owner = SPINLOCK_OWNER_INIT;
 	lock->owner_cpu = -1;
+	lock->lockup = 0;
 }
 
 EXPORT_SYMBOL(__raw_spin_lock_init);
@@ -101,6 +102,24 @@ static inline void debug_spin_unlock(raw_spinlock_t *lock)
 							lock, "wrong CPU");
 	lock->owner = SPINLOCK_OWNER_INIT;
 	lock->owner_cpu = -1;
+	lock->lockup = 0;
+}
+
+static inline void __spin_lockup(raw_spinlock_t *lock)
+{
+	/*
+	 * lockup suspected:
+	 *
+	 * Only one of the lock waiters will be allowed to print the lockup
+	 * message in order to avoid an avalanche of lockup and backtrace
+	 * messages from different lock waiters of the same lock.
+	 */
+	if (!xchg(&lock->lockup, 1)) {
+		spin_dump(lock, "lockup suspected");
+#ifdef CONFIG_SMP
+		trigger_all_cpu_backtrace();
+#endif
+	}
 }
 
 static void __spin_lock_debug(raw_spinlock_t *lock)
@@ -113,11 +132,8 @@ static void __spin_lock_debug(raw_spinlock_t *lock)
 			return;
 		__delay(1);
 	}
-	/* lockup suspected: */
-	spin_dump(lock, "lockup suspected");
-#ifdef CONFIG_SMP
-	trigger_all_cpu_backtrace();
-#endif
+
+	__spin_lockup(lock);
 
 	/*
 	 * The trylock above was causing a livelock.  Give the lower level arch
-- 
1.8.3.1

[PATCH v3 3/3] locking/spinlock_debug: Reduce lock cacheline contention

[Waiman Long]

The debug spinlock code is a basic TATAS unfair lock irrespective
of what the underlying architecture specific spinlock implementation
is. As a result, it is sometimes possible to trigger a false positive
"lockup suspected" warning with all the cpu backtraces.

This patch re-implements the debug spinlock as a fair MCS lock. This
reduces the chance of false positive warning messages. At the
same time, it also improves performance by reducing lock cacheline
contention.

Because there is a trylock before entering the MCS queue, this new
debug spinlock code also perform pretty well in a virtual machine
even if its vCPUSs are over-committed.

On a 4-socket 32-core 64-thread system, the performance of a locking
microbenchmark (locking rate and standard deviation) on a 4.9.6 based
debug kernel with and without the patch was as follows:

  32 locking threads:

  Kernel       Locking Rate    SD (execution time)
  ------       ------------    -------------------
  w/o patch     263.1 Mop/s         1.39s
  with patch    917.6 Mop/s         0.07s

  64 locking threads:

  Kernel       Locking Rate    SD (execution time)
  ------       ------------    -------------------
  w/o patch     368.3 Mop/s         6.88s
  with patch    733.0 Mop/s         0.09s

On a 2-socket 24-core 48-thread system, the performance of the same
locking microbenchmark (# of locking threads = # of vCPUs) on a KVM
guest are as follows:

  24 vCPUs:

  Kernel       Locking Rate    SD (execution time)
  ------       ------------    -------------------
  w/o patch     746.4 Mop/s         1.07s
  with patch   1323.6 Mop/s         0.20s

  48 vCPUs:

  Kernel       Locking Rate    SD (execution time)
  ------       ------------    -------------------
  w/o patch    1077.8 Mop/s         3.34s
  with patch   1090.4 Mop/s         0.29s

  72 vCPUs:

  Kernel       Locking Rate    SD (execution time)
  ------       ------------    -------------------
  w/o patch     944.5 Mop/s         3.96s
  with patch   1176.7 Mop/s         0.44s

  96 vCPUs:

  Kernel       Locking Rate    SD (execution time)
  ------       ------------    -------------------
  w/o patch     878.0 Mop/s         5.19s
  with patch   1017.0 Mop/s         0.83s

Signed-off-by: Waiman Long <longman@redhat.com>
---
 include/linux/spinlock_types.h  |  5 ++--
 kernel/locking/spinlock_debug.c | 53 +++++++++++++++++++++++++++--------------
 2 files changed, 38 insertions(+), 20 deletions(-)

diff --git a/include/linux/spinlock_types.h b/include/linux/spinlock_types.h
index ef28ce5..895da96 100644
--- a/include/linux/spinlock_types.h
+++ b/include/linux/spinlock_types.h
@@ -24,7 +24,7 @@
 #endif
 #ifdef CONFIG_DEBUG_SPINLOCK
 	unsigned int magic, owner_cpu, lockup;
-	void *owner;
+	void *owner, *tail;
 #endif
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 	struct lockdep_map dep_map;
@@ -46,7 +46,8 @@
 	.magic = SPINLOCK_MAGIC,		\
 	.owner_cpu = -1,			\
 	.lockup = 0,				\
-	.owner = SPINLOCK_OWNER_INIT,
+	.owner = SPINLOCK_OWNER_INIT,		\
+	.tail = NULL,
 #else
 # define SPIN_DEBUG_INIT(lockname)
 #endif
diff --git a/kernel/locking/spinlock_debug.c b/kernel/locking/spinlock_debug.c
index 0f880a8..c58b61f 100644
--- a/kernel/locking/spinlock_debug.c
+++ b/kernel/locking/spinlock_debug.c
@@ -12,6 +12,7 @@
 #include <linux/debug_locks.h>
 #include <linux/delay.h>
 #include <linux/export.h>
+#include "mcs_spinlock.h"
 
 void __raw_spin_lock_init(raw_spinlock_t *lock, const char *name,
 			  struct lock_class_key *key)
@@ -26,6 +27,7 @@ void __raw_spin_lock_init(raw_spinlock_t *lock, const char *name,
 	lock->raw_lock = (arch_spinlock_t)__ARCH_SPIN_LOCK_UNLOCKED;
 	lock->magic = SPINLOCK_MAGIC;
 	lock->owner = SPINLOCK_OWNER_INIT;
+	lock->tail = NULL;
 	lock->owner_cpu = -1;
 	lock->lockup = 0;
 }
@@ -105,7 +107,7 @@ static inline void debug_spin_unlock(raw_spinlock_t *lock)
 	lock->lockup = 0;
 }
 
-static inline void __spin_lockup(raw_spinlock_t *lock)
+static inline void __spin_chk_lockup(raw_spinlock_t *lock, u64 loops)
 {
 	/*
 	 * lockup suspected:
@@ -113,37 +115,52 @@ static inline void __spin_lockup(raw_spinlock_t *lock)
 	 * Only one of the lock waiters will be allowed to print the lockup
 	 * message in order to avoid an avalanche of lockup and backtrace
 	 * messages from different lock waiters of the same lock.
+	 *
+	 * With the original __deley(1) call, lockup can happen when both
+	 * threads of a hyperthreaded CPU core contend on the same lock. So
+	 * cpu_relax() is used here instead.
 	 */
-	if (!xchg(&lock->lockup, 1)) {
+	if (unlikely(!loops && !xchg(&lock->lockup, 1))) {
 		spin_dump(lock, "lockup suspected");
 #ifdef CONFIG_SMP
 		trigger_all_cpu_backtrace();
 #endif
 	}
+	cpu_relax();
 }
 
+/*
+ * The lock waiters are put into a MCS queue to maintain lock fairness
+ * as well as avoiding excessive contention on the lock cacheline. It
+ * also helps to reduce false positive because of unfairness instead
+ * of real lockup.
+ *
+ * The trylock before entering the MCS queue makes this code perform
+ * reasonably well in a virtual machine where some of the lock waiters
+ * may have their vCPUs preempted.
+ */
 static void __spin_lock_debug(raw_spinlock_t *lock)
 {
-	u64 i;
 	u64 loops = loops_per_jiffy * HZ;
-
-	for (i = 0; i < loops; i++) {
-		if (arch_spin_trylock(&lock->raw_lock))
-			return;
-		__delay(1);
+	struct mcs_spinlock node, *prev;
+
+	node.next = NULL;
+	node.locked = 0;
+	prev = xchg(&lock->tail, &node);
+	if (prev) {
+		WRITE_ONCE(prev->next, &node);
+		while (!READ_ONCE(node.locked))
+			__spin_chk_lockup(lock, loops--);
 	}
 
-	__spin_lockup(lock);
+	while (!arch_spin_trylock(&lock->raw_lock))
+		__spin_chk_lockup(lock, loops--);
 
-	/*
-	 * The trylock above was causing a livelock.  Give the lower level arch
-	 * specific lock code a chance to acquire the lock. We have already
-	 * printed a warning/backtrace at this point. The non-debug arch
-	 * specific code might actually succeed in acquiring the lock.  If it is
-	 * not successful, the end-result is the same - there is no forward
-	 * progress.
-	 */
-	arch_spin_lock(&lock->raw_lock);
+	if (cmpxchg(&lock->tail, &node, NULL) == &node)
+		return;
+	while (!READ_ONCE(node.next))
+		cpu_relax();
+	WRITE_ONCE(node.next->locked, 1);
 }
 
 void do_raw_spin_lock(raw_spinlock_t *lock)
-- 
1.8.3.1

Re: [PATCH v3 0/3] locking/spinlock_debug: Change it to a mostly fair lock

[Peter Zijlstra]

On Thu, Feb 02, 2017 at 09:15:26AM -0500, Waiman Long wrote:
>  v2->v3:
>   - Keep the original v1 patches but move patch 3 of v2 in front so
>     as to disable GENERIC_LOCKBREAK when DEBUG_LOCK_ALLOC is on.
> 
>  v1->v2:
>   - Pack lockup and break_lock into a single 4-byte slot so as not
>     to in increase spinlock size when GENERIC_LOCKBREAK is
>     on. Hopefully that will be enough to fix a frame size too large
>     warning in 0-day build.
>   - Add a new patch to disable GENERIC_LOCKBREAK when DEBUG_LOCK_ALLOC
>     is on.
> 
> The current debug spinlock implementation is a TATAS unfair lock. This
> can occasionally lead to system lockup with a debug kernel because
> of the unfairness of the lock rather than inherent locking problem.
> 
> This patch set changes the debug spinlock implementation to a
> mostly fair spinlock based on the MCS lock similar to what is done
> in qspinlock. It also includes a patch that disable GENERIC_LOCKBREAK
> when DEBUG_LOCK_ALLOC is on.


An alternative is to just delete the thing entirely..

Ingo, much of what this thing does seems to be superseded by both
lockdep and a reliable NMI watchdog. Is there still value in
spinlock_debug?

Re: [PATCH v3 0/3] locking/spinlock_debug: Change it to a mostly fair lock

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Feb 02, 2017 at 09:15:26AM -0500, Waiman Long wrote:
> >  v2->v3:
> >   - Keep the original v1 patches but move patch 3 of v2 in front so
> >     as to disable GENERIC_LOCKBREAK when DEBUG_LOCK_ALLOC is on.
> > 
> >  v1->v2:
> >   - Pack lockup and break_lock into a single 4-byte slot so as not
> >     to in increase spinlock size when GENERIC_LOCKBREAK is
> >     on. Hopefully that will be enough to fix a frame size too large
> >     warning in 0-day build.
> >   - Add a new patch to disable GENERIC_LOCKBREAK when DEBUG_LOCK_ALLOC
> >     is on.
> > 
> > The current debug spinlock implementation is a TATAS unfair lock. This
> > can occasionally lead to system lockup with a debug kernel because
> > of the unfairness of the lock rather than inherent locking problem.
> > 
> > This patch set changes the debug spinlock implementation to a
> > mostly fair spinlock based on the MCS lock similar to what is done
> > in qspinlock. It also includes a patch that disable GENERIC_LOCKBREAK
> > when DEBUG_LOCK_ALLOC is on.
> 
> 
> An alternative is to just delete the thing entirely..
> 
> Ingo, much of what this thing does seems to be superseded by both
> lockdep and a reliable NMI watchdog. Is there still value in
> spinlock_debug?

So there's still early stages when the NMI watchdog is not running, and 
spinlock-debug can detect lockups in raw locks that lockdep does not cover, right?

But yeah ... it would simplify things all around, so I'm not unsympathetic to the 
idea...

I've Cc:-ed a few other locking gents.

Thanks,

	Ingo

Re: [PATCH v3 0/3] locking/spinlock_debug: Change it to a mostly fair lock

[Waiman Long]

On 02/07/2017 04:48 AM, Ingo Molnar wrote:
> * Peter Zijlstra <peterz@infradead.org> wrote:
>
>> On Thu, Feb 02, 2017 at 09:15:26AM -0500, Waiman Long wrote:
>>>  v2->v3:
>>>   - Keep the original v1 patches but move patch 3 of v2 in front so
>>>     as to disable GENERIC_LOCKBREAK when DEBUG_LOCK_ALLOC is on.
>>>
>>>  v1->v2:
>>>   - Pack lockup and break_lock into a single 4-byte slot so as not
>>>     to in increase spinlock size when GENERIC_LOCKBREAK is
>>>     on. Hopefully that will be enough to fix a frame size too large
>>>     warning in 0-day build.
>>>   - Add a new patch to disable GENERIC_LOCKBREAK when DEBUG_LOCK_ALLOC
>>>     is on.
>>>
>>> The current debug spinlock implementation is a TATAS unfair lock. This
>>> can occasionally lead to system lockup with a debug kernel because
>>> of the unfairness of the lock rather than inherent locking problem.
>>>
>>> This patch set changes the debug spinlock implementation to a
>>> mostly fair spinlock based on the MCS lock similar to what is done
>>> in qspinlock. It also includes a patch that disable GENERIC_LOCKBREAK
>>> when DEBUG_LOCK_ALLOC is on.
>>
>> An alternative is to just delete the thing entirely..
>>
>> Ingo, much of what this thing does seems to be superseded by both
>> lockdep and a reliable NMI watchdog. Is there still value in
>> spinlock_debug?
> So there's still early stages when the NMI watchdog is not running, and 
> spinlock-debug can detect lockups in raw locks that lockdep does not cover, right?
>
> But yeah ... it would simplify things all around, so I'm not unsympathetic to the 
> idea...
>
> I've Cc:-ed a few other locking gents.
>
> Thanks,
>
> 	Ingo

I have no problem deleting the debug_spinlock code entirely. I can
update the patch to delete the code if you guys think that is the right
thing to do.

Cheers,
Longman

Re: [PATCH v3 0/3] locking/spinlock_debug: Change it to a mostly fair lock

[Paul E. McKenney]

On Tue, Feb 07, 2017 at 02:46:31PM -0500, Waiman Long wrote:
> On 02/07/2017 04:48 AM, Ingo Molnar wrote:
> > * Peter Zijlstra <peterz@infradead.org> wrote:
> >
> >> On Thu, Feb 02, 2017 at 09:15:26AM -0500, Waiman Long wrote:
> >>>  v2->v3:
> >>>   - Keep the original v1 patches but move patch 3 of v2 in front so
> >>>     as to disable GENERIC_LOCKBREAK when DEBUG_LOCK_ALLOC is on.
> >>>
> >>>  v1->v2:
> >>>   - Pack lockup and break_lock into a single 4-byte slot so as not
> >>>     to in increase spinlock size when GENERIC_LOCKBREAK is
> >>>     on. Hopefully that will be enough to fix a frame size too large
> >>>     warning in 0-day build.
> >>>   - Add a new patch to disable GENERIC_LOCKBREAK when DEBUG_LOCK_ALLOC
> >>>     is on.
> >>>
> >>> The current debug spinlock implementation is a TATAS unfair lock. This
> >>> can occasionally lead to system lockup with a debug kernel because
> >>> of the unfairness of the lock rather than inherent locking problem.
> >>>
> >>> This patch set changes the debug spinlock implementation to a
> >>> mostly fair spinlock based on the MCS lock similar to what is done
> >>> in qspinlock. It also includes a patch that disable GENERIC_LOCKBREAK
> >>> when DEBUG_LOCK_ALLOC is on.
> >>
> >> An alternative is to just delete the thing entirely..
> >>
> >> Ingo, much of what this thing does seems to be superseded by both
> >> lockdep and a reliable NMI watchdog. Is there still value in
> >> spinlock_debug?
> > So there's still early stages when the NMI watchdog is not running, and 
> > spinlock-debug can detect lockups in raw locks that lockdep does not cover, right?
> >
> > But yeah ... it would simplify things all around, so I'm not unsympathetic to the 
> > idea...
> >
> > I've Cc:-ed a few other locking gents.
> >
> > Thanks,
> >
> > 	Ingo
> 
> I have no problem deleting the debug_spinlock code entirely. I can
> update the patch to delete the code if you guys think that is the right
> thing to do.
> 
> Cheers,
> Longman

My usual question is "how often does the spinlock_debug code find a
problem that would be hard to find otherwise?"  Probably unanswerable
given the nature of Linux-kernel development, but I figured I would ask
anyway.  ;-)

							Thanx, Paul

Re: [PATCH v3 0/3] locking/spinlock_debug: Change it to a mostly fair lock

[Peter Zijlstra]

On Tue, Feb 07, 2017 at 11:53:08AM -0800, Paul E. McKenney wrote:
> My usual question is "how often does the spinlock_debug code find a
> problem that would be hard to find otherwise?"  Probably unanswerable
> given the nature of Linux-kernel development, but I figured I would ask
> anyway.  ;-)

So I've not found it useful in many years, and quite to the contrary,
its proven prone to generate false positives because the lock timeout
gets hit because of various reasons.

But that's just me of course..

Re: [PATCH v3 0/3] locking/spinlock_debug: Change it to a mostly fair lock

[Paul E. McKenney]

On Wed, Feb 08, 2017 at 10:27:26AM +0100, Peter Zijlstra wrote:
> On Tue, Feb 07, 2017 at 11:53:08AM -0800, Paul E. McKenney wrote:
> > My usual question is "how often does the spinlock_debug code find a
> > problem that would be hard to find otherwise?"  Probably unanswerable
> > given the nature of Linux-kernel development, but I figured I would ask
> > anyway.  ;-)
> 
> So I've not found it useful in many years, and quite to the contrary,
> its proven prone to generate false positives because the lock timeout
> gets hit because of various reasons.
> 
> But that's just me of course..

I have seen neither useful information nor false positives from it, nor
have I heard of anyone else having recently done so.  So I have nothing
indicating that I should either defend it on the one hand or advocate
for its removal on the other.  But I was asked, so...

One approach would be to push the patch and see if anyone complained.
But there are quite a few people who wouldn't complain about anything
until it bit them, which wouldn't happen for some time.  Which -might-
be OK, but...

Another would be to announce the patch widely, via social media, LWN
articles, presentations, etc., in order to increase the probability of
finding out if anyone relies on it sooner rather than later.

My recommendation is to do both in parallel.  Get the patch out there
for testing, get it headed for mainline, and make noise about it along
the way.  If no one complains, it goes in.  If someone does complain,
we can see what they really need, which would hopefully point the way
to an improved mechanism.

But that's just me of course...  ;-)

							Thanx, Paul