All of lore.kernel.org
 help / color / mirror / Atom feed
From: Peter Zijlstra <peterz@infradead.org>
To: Marco Elver <elver@google.com>
Cc: Bart Van Assche <bvanassche@acm.org>,
	Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
	Boqun Feng <boqun@kernel.org>, Waiman Long <longman@redhat.com>,
	linux-kernel@vger.kernel.org,
	"Paul E . McKenney" <paulmck@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Nathan Chancellor <nathan@kernel.org>
Subject: Re: [PATCH 08/14] lockdep: Introduce lockdep_assume_ctx_held()
Date: Thu, 5 Nov 2026 15:58:44 +0100	[thread overview]
Message-ID: <20261105145844.GQ1813469@noisy.programming.kicks-ass.net> (raw)
In-Reply-To: <CANpmjNMaDwcm+tQ-cOUjnE3n+5x8WgB5CZ037uurbwVa4Jn0BA@mail.gmail.com>

On Thu, Nov 05, 2026 at 03:39:33PM +0100, Marco Elver wrote:
> On Thu, 5 Nov 2026 at 09:37, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Mon, Nov 02, 2026 at 01:34:38PM -0800, Bart Van Assche wrote:
> > > Introduce lockdep_assume_ctx_held() and lockdep_assume_shared_ctx_held()
> > > for code that needs to inform Clang's context analysis that a lock is
> > > held in code paths where lock ownership cannot be proven at compile
> > > time, e.g. because the lock is accessed through a pointer alias that
> > > the analysis cannot resolve. These macros call respectively
> > > __assume_ctx_lock() and __assume_shared_ctx_lock() and also verify at
> > > runtime with lockdep that the lock is held. Store the macro argument in
> > > a local variable with a unique name such that it is evaluated only once
> > > and such that nested use of these macros does not cause shadowing. Rely
> > > on Clang's context analysis resolving that local variable to the
> > > expression it has been initialized with. Document both macros with
> > > kernel-doc comments, including that __assume_ctx_lock() and
> > > __assume_shared_ctx_lock() must be used for context locks without
> > > lockdep support, e.g. bit spinlocks and the RCU context tokens.
> > >
> > > Convert the lockdep_assert_held() tests in lib/test_context-analysis.c
> > > into lockdep_assume_ctx_held() tests and add tests for
> > > lockdep_assume_shared_ctx_held().
> > >
> > > This patch prepares for removing the context assumptions from
> > > lockdep_assert_held*().
> >
> > So the only convincing rationale from 0/n for doing this was that the
> > current annotation can hide the need, and thus cause a lacking of
> > __must_hold() on the function.
> >
> > I'm not sure I understand how this alternative form of the same avoids
> > that same problem, and the above doesn't seem to mention the whole
> > problem -- either that, or I just can't read this morning.
> 
> There's no problem if you're dealing with new code. But we have a lot
> of lockdep_assert_held*() sprinkled all over existing code, which when
> compiling the kernel with WARN_CONTEXT_ANALYSIS_ALL will not warn
> where we would be better off with a __must_hold(). I believe Bart had
> been working on enabling this more broadly and doesn't want
> lockdep_assert_held*() hide warnings.
> 
> This latest iteration is a compromise for that problem, rather than
> the previous proposals of removing __assume_ctx_lock() from lockdep
> primitives entirely. The programming model is unchanged: we delegate
> to dynamic checks where the static analysis fails, and that's the main
> purpose of __assume_ctx_lock(). The only difference is that we'd have
> to be more explicit about it, just because of legacy use of
> lockdep_assert_held*().
> 
> I'm somewhat on the fence still, although would be willing to accept
> this compromise if it does help catch more bugs, and is neutral to
> only mildly more mentally taxing in writing new code.

Aaah, so the 'solution' is to rename lockdep_assert_held(), such that
the pre-existing annotation doesn't pre-pollute things? But if you start
by converting existing lockdep_assert_held() to the new
lockdep_assume_*_ctx_held() you'll run into the exact same problems?

That is a very minor and very subtle 'fix'.

  reply	other threads:[~2026-11-05 14:58 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-11-02 21:34 [PATCH 00/14] Stop lockdep_assert_held*() from assuming lock ownership Bart Van Assche
2026-11-02 21:34 ` [PATCH 01/14] bit_spinlock: Fix the __bitlock() definition Bart Van Assche
2026-11-02 21:34 ` [PATCH 02/14] cleanup: Disable context analysis for __free() helpers Bart Van Assche
2026-11-02 21:34 ` [PATCH 03/14] tracing: Disable branch profiling for COMPILE_TEST builds Bart Van Assche
2026-11-02 21:34 ` [PATCH 04/14] srcu: Remove check from srcu_dereference_check() Bart Van Assche
2026-11-02 21:34 ` [PATCH 05/14] futex: Annotate __attach_to_pi_owner() and fixup_pi_*owner() Bart Van Assche
2026-11-02 21:34 ` [PATCH 06/14] locking/rwsem: Annotate rwsem internal functions with __must_hold() Bart Van Assche
2026-11-02 21:34 ` [PATCH 07/14] nvme-fabrics: Annotate nvmf_lookup_transport() with __must_hold_shared() Bart Van Assche
2026-11-03  7:51   ` Christoph Hellwig
2026-11-03 20:37     ` Bart Van Assche
2026-11-04  7:13       ` Christoph Hellwig
2026-11-02 21:34 ` [PATCH 08/14] lockdep: Introduce lockdep_assume_ctx_held() Bart Van Assche
2026-11-05  8:37   ` Peter Zijlstra
2026-11-05 14:39     ` Marco Elver
2026-11-05 14:58       ` Peter Zijlstra [this message]
2026-11-05 17:00         ` Bart Van Assche
2026-11-05 16:52     ` Bart Van Assche
2026-11-02 21:34 ` [PATCH 09/14] kfence: Use lockdep_assume_ctx_held() in kfence_report_error() Bart Van Assche
2026-11-02 21:34 ` [PATCH 10/14] Bluetooth: Add lock context annotations for connection aliases Bart Van Assche
2026-11-02 21:34 ` [PATCH 11/14] futex: Verify exit_mutex ownership in futex_exit_recursive() Bart Van Assche
2026-11-02 21:34 ` [PATCH 12/14] lockdep: Stop lockdep_assert_held*() from assuming lock ownership Bart Van Assche
2026-11-02 21:34 ` [PATCH 13/14] compiler-context-analysis: Split the RCU assertion tests Bart Van Assche
2026-11-02 21:34 ` [PATCH 14/14] rcu: Introduce lockdep_assume_in_rcu_*() Bart Van Assche
2026-11-11 11:17 ` [PATCH 00/14] Stop lockdep_assert_held*() from assuming lock ownership Marco Elver
2026-11-11 18:41   ` Bart Van Assche
2026-11-11 19:14     ` Marco Elver
2026-11-11 21:05       ` Bart Van Assche
2026-11-13  9:11         ` Marco Elver
2026-11-13 20:27           ` Bart Van Assche

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=20261105145844.GQ1813469@noisy.programming.kicks-ass.net \
    --to=peterz@infradead.org \
    --cc=akpm@linux-foundation.org \
    --cc=boqun@kernel.org \
    --cc=bvanassche@acm.org \
    --cc=elver@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=longman@redhat.com \
    --cc=mingo@redhat.com \
    --cc=nathan@kernel.org \
    --cc=paulmck@kernel.org \
    --cc=will@kernel.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.