linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Ingo Molnar <mingo@kernel.org>
To: Byungchul Park <max.byungchul.park@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Peter Zijlstra <peterz@infradead.org>,
	david@fromorbit.com, tytso@mit.edu, willy@infradead.org,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Amir Goldstein <amir73il@gmail.com>,
	byungchul.park@lge.com, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, linux-block@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, oleg@redhat.com
Subject: [PATCH] locking/lockdep: Remove the cross-release locking checks
Date: Wed, 13 Dec 2017 11:46:17 +0100	[thread overview]
Message-ID: <20171213104617.7lffucjhaa6xb7lp@gmail.com> (raw)
In-Reply-To: <CANrsvRPQcWz-p_3TYfNf+Waek3bcNNPniXhFzyyS=7qbCqzGyg@mail.gmail.com>


* Byungchul Park <max.byungchul.park@gmail.com> wrote:

> Lockdep works, based on the following:
> 
>    (1) Classifying locks properly
>    (2) Checking relationship between the classes
> 
> If (1) is not good or (2) is not good, then we
> might get false positives.
> 
> For (1), we don't have to classify locks 100%
> properly but need as enough as lockdep works.
> 
> For (2), we should have a mechanism w/o
> logical defects.
> 
> Cross-release added an additional capacity to
> (2) and requires (1) to get more precisely classified.
> 
> Since the current classification level is too low for
> cross-release to work, false positives are being
> reported frequently with enabling cross-release.
> Yes. It's a obvious problem. It needs to be off by
> default until the classification is done by the level
> that cross-release requires.
> 
> But, the logic (2) is valid and logically true. Please
> keep the code, mechanism, and logic.

Just to give a full context to everyone: the patch that removes the cross-release 
locking checks was Cc:-ed to lkml, I've attached the patch below again.

In general, as described in the changelog, the cross-release checks were 
historically just too painful (first they were too slow, and they also had a lot 
of false positives), and today, 4 months after its introduction, the cross-release 
checks *still* produce numerous false positives, especially in the filesystem 
space, but the continuous-integration testing folks were still having trouble with 
kthread locking patterns causing false positives:

  https://bugs.freedesktop.org/show_bug.cgi?id=103950

which were resulting in two bad reactions:

 - turning off lockdep

 - writing patches that uglified unrelated subsystems

So while I appreciate the fixes that resulted from running cross-release, there's 
still numerous false positives, months after its interaction, which is 
unacceptable. For us to have this feature it has to have roughly similar qualities 
as compiler warnings:

 - there's a "zero false positive warnings" policy

 - plus any widespread changes to avoid warnings has to improve the code,
   not make it uglier.

Lockdep itself is a following that policy: the default state is that it produces 
no warnings upstream, and any annotations added to unrelated code documents the 
locking hierarchies.

While technically we could keep the cross-release checking code upstream and turn 
it off by default via the Kconfig switch, I'm not a big believer in such a policy 
for complex debugging code:

 - We already did that for v4.14, two months ago:

     b483cf3bc249: locking/lockdep: Disable cross-release features for now

   ... and re-enabled it for v4.15 - but the false positives are still not fixed.

 - either the cross-release checking code can be fixed and then having it off by
   default is just wrong, because we can apply the fixed code again once it's
   fixed.

 - or it cannot be fixed (or we don't have the manpower/interest to fix it),
   in which case having it off is only delaying the inevitable.

In any case, for v4.15 it's clear that the false positives are too numerous.

Thanks,

	Ingo


=============================>

  parent reply	other threads:[~2017-12-13 10:46 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-13  6:24 About the try to remove cross-release feature entirely by Ingo Byungchul Park
2017-12-13  7:13 ` Byungchul Park
2017-12-13 15:23   ` Bart Van Assche
2017-12-14  3:07   ` Theodore Ts'o
2017-12-14  5:58     ` Byungchul Park
2017-12-14 11:18     ` Peter Zijlstra
2017-12-14 13:30       ` Byungchul Park
2017-12-13 10:46 ` Ingo Molnar [this message]
2017-12-14  5:01   ` [PATCH] locking/lockdep: Remove the cross-release locking checks Byungchul Park
2017-12-15  4:05     ` Byungchul Park
2017-12-15  6:24       ` Theodore Ts'o
2017-12-15  7:38         ` Byungchul Park
2017-12-15  8:39         ` Byungchul Park
2017-12-15 21:15           ` Theodore Ts'o
2017-12-16  2:41             ` Byungchul Park
2017-12-29  1:47 ` About the try to remove cross-release feature entirely by Ingo Byungchul Park
2017-12-29  2:02   ` Byungchul Park
2017-12-29  3:51   ` Theodore Ts'o
2017-12-29  7:28     ` Byungchul Park
2017-12-30  6:16       ` Matthew Wilcox
2017-12-30 15:40         ` Theodore Ts'o
2017-12-30 20:44           ` Matthew Wilcox
2017-12-30 22:40             ` Theodore Ts'o
2017-12-30 23:00               ` Theodore Ts'o
2018-01-01 10:18                 ` Matthew Wilcox
2018-01-01 16:00                   ` Theodore Ts'o
2018-01-03  2:38                     ` Byungchul Park
2018-01-03  2:28                   ` Byungchul Park
2018-01-03  2:58                     ` Dave Chinner
2018-01-03  5:48                       ` Byungchul Park
2018-01-05 16:49                   ` J. Bruce Fields
2018-01-05 17:05                     ` J. Bruce Fields
2018-01-03  2:10               ` Byungchul Park
2018-01-03  7:05                 ` Theodore Ts'o
2018-01-03  8:10                   ` Byungchul Park
2018-01-03  8:23                     ` Byungchul Park
2018-01-03  1:57           ` Byungchul Park
2018-01-02  7:57         ` Byungchul Park
2017-12-29  8:09   ` Amir Goldstein
2017-12-29  9:46     ` Byungchul Park

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171213104617.7lffucjhaa6xb7lp@gmail.com \
    --to=mingo@kernel.org \
    --cc=amir73il@gmail.com \
    --cc=byungchul.park@lge.com \
    --cc=david@fromorbit.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=max.byungchul.park@gmail.com \
    --cc=oleg@redhat.com \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=tytso@mit.edu \
    --cc=willy@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox