Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1572399 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/3] locking/spinlock_debug: Change it to a mostly fair lock Waiman Long <longman@redhat.com> - 2017-02-02 15:20 +0100
    Re: [PATCH v3 0/3] locking/spinlock_debug: Change it to a mostly  fair lock Peter Zijlstra <peterz@infradead.org> - 2017-02-07 09:50 +0100
      Re: [PATCH v3 0/3] locking/spinlock_debug: Change it to a mostly  fair lock Ingo Molnar <mingo@kernel.org> - 2017-02-07 10:50 +0100
        Re: [PATCH v3 0/3] locking/spinlock_debug: Change it to a mostly fair  lock Waiman Long <longman@redhat.com> - 2017-02-07 20:50 +0100
          Re: [PATCH v3 0/3] locking/spinlock_debug: Change it to a mostly  fair lock "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-02-07 21:00 +0100
            Re: [PATCH v3 0/3] locking/spinlock_debug: Change it to a mostly  fair lock Peter Zijlstra <peterz@infradead.org> - 2017-02-08 10:50 +0100
              Re: [PATCH v3 0/3] locking/spinlock_debug: Change it to a mostly  fair lock "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-02-08 14:20 +0100

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

 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

[toc] | [next] | [standalone]

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

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?

[toc] | [prev] | [next] | [standalone]

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

* 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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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..

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web