mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: "Gary Guo" <gary@garyguo.net>
To: "Tamir Duberstein" <tamird@kernel.org>,
	"Andreas Hindborg" <a.hindborg@kernel.org>
Cc: "Matthew Wilcox" <willy@infradead.org>,
	"Miguel Ojeda" <ojeda@kernel.org>,
	"Boqun Feng" <boqun.feng@gmail.com>,
	"Gary Guo" <gary@garyguo.net>,
	"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
	"Benno Lossin" <lossin@kernel.org>,
	"Alice Ryhl" <aliceryhl@google.com>,
	"Trevor Gross" <tmgross@umich.edu>,
	"Danilo Krummrich" <dakr@kernel.org>,
	rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] rust: xarray: fix false positive lockdep warnings
Date: Fri, 06 Feb 2026 17:43:45 +0000	[thread overview]
Message-ID: <DG82CZTISPBF.2B59PSYVD8Z00@garyguo.net> (raw)
In-Reply-To: <CAJ-ks9kUO3VNeOD2Qo5rpUj3sr2HW53dgx=YxpnPEupDP_0J4Q@mail.gmail.com>

On Fri Feb 6, 2026 at 5:19 PM GMT, Tamir Duberstein wrote:
> On Fri, Feb 6, 2026 at 12:11 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Replace the `xa_init_flags` helper with direct initialization of XArray
>> structures using `__spin_lock_init`. This allows each XArray to have
>> its own lock class key for lockdep, preventing false positive warnings
>> about lock ordering violations.
>>
>> Add a `new_xarray!` macro that automatically generates a unique lock
>> class key for each XArray instantiation site. The macro accepts an
>> optional name parameter and uses the `optional_name!` and
>> `static_lock_class!` macros to generate appropriate names and lock
>> classes.
>>
>> Update the `XArray::new` method signature to accept explicit name and
>> lock class key parameters. This enables proper lockdep tracking while
>> maintaining flexibility for advanced use cases.
>>
>> Remove `rust_helper_xa_init_flags` as it is now dead code.
>>
>> This was previously part of the series at [1]
>>
>> Link: https://lore.kernel.org/r/20251203-xarray-entry-send-v1-0-9e5ffd5e3cf0@kernel.org [1]
>> Fixes: 210b81578efbe ("rust: xarray: Add an abstraction for XArray")
>> Acked-by: Tamir Duberstein <tamird@gmail.com>
>
> Please replace with:
>
> Acked-by: Tamir Duberstein <tamird@kernel.org>
>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>>  rust/helpers/xarray.c |  5 -----
>>  rust/kernel/xarray.rs | 43 +++++++++++++++++++++++++++++++++++++------
>>  2 files changed, 37 insertions(+), 11 deletions(-)
>>
>> diff --git a/rust/helpers/xarray.c b/rust/helpers/xarray.c
>> index 60b299f11451d..644b8b35b1bee 100644
>> --- a/rust/helpers/xarray.c
>> +++ b/rust/helpers/xarray.c
>> @@ -7,11 +7,6 @@ int rust_helper_xa_err(void *entry)
>>         return xa_err(entry);
>>  }
>>
>> -void rust_helper_xa_init_flags(struct xarray *xa, gfp_t flags)
>> -{
>> -       return xa_init_flags(xa, flags);
>> -}
>
> I like Alice's suggestion in the previous version of doing the
> initialization here. It gives a natural place to explain why
> xa_init_flags cannot be used directly and avoids introducing a second
> user of `bindings:: __spin_lock_init` which seems like something we
> should discourage direct use of.

Or perhaps we should just add __xa_init_flags which accepts explicit lock
classes, and replace `xa_init_flags` to use it with a fresh lock class key.

I think C users benefit from having a fresh lock class key per xarray rather
than a single one per compilation unit, too.

Cc: Matthew Wilcox <willy@infradead.org>

Matthew, for context, the current xa_init_flags have an issue where, because it
is static inline function instead of macro, all users inside a single
compilation unit ended up get a single lock class key. For C users this means
that each .c file gets a single key, and this is more problematic for Rust as we
invoke via helpers, so all of Rust users share a single lockdep key.

Link: https://lore.kernel.org/all/DFUI6LMASTMK.2WI00LMY5FTSP@garyguo.net/

Best,
Gary

>
>> -
>>  int rust_helper_xa_trylock(struct xarray *xa)
>>  {
>>         return xa_trylock(xa);
>> diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
>> index a49d6db288458..62a2b2ebfb0bf 100644
>> --- a/rust/kernel/xarray.rs
>> +++ b/rust/kernel/xarray.rs
>> @@ -8,11 +8,31 @@
>>      alloc, bindings, build_assert,
>>      error::{Error, Result},
>>      ffi::c_void,
>> +    str::CStr,
>> +    sync::LockClassKey,
>>      types::{ForeignOwnable, NotThreadSafe, Opaque},
>>  };
>> -use core::{iter, marker::PhantomData, pin::Pin, ptr::NonNull};
>> +use core::{
>> +    iter,
>> +    marker::PhantomData,
>> +    pin::Pin,
>> +    ptr::{null_mut, NonNull},
>> +};
>>  use pin_init::{pin_data, pin_init, pinned_drop, PinInit};
>>
>> +/// Creates a [`XArray`] initialiser with the given name and a newly-created lock class.
>> +///
>> +/// It uses the name if one is given, otherwise it generates one based on the file name and line
>> +/// number.
>> +#[macro_export]
>> +macro_rules! new_xarray {
>> +    ($kind:expr $(, $name:literal)? $(,)?) => {
>> +        $crate::xarray::XArray::new(
>> +            $kind, $crate::optional_name!($($name)?), $crate::static_lock_class!())
>> +    };
>> +}
>> +pub use new_xarray;
>> +
>>  /// An array which efficiently maps sparse integer indices to owned objects.
>>  ///
>>  /// This is similar to a [`crate::alloc::kvec::Vec<Option<T>>`], but more efficient when there are
>> @@ -27,9 +47,10 @@
>>  ///
>>  /// ```rust
>>  /// use kernel::alloc::KBox;
>> -/// use kernel::xarray::{AllocKind, XArray};
>> +/// use kernel::xarray::{new_xarray, AllocKind, XArray};
>>  ///
>> -/// let xa = KBox::pin_init(XArray::new(AllocKind::Alloc1), GFP_KERNEL)?;
>> +/// let xa: Pin<KBox<XArray<KBox<u32>>>> =
>> +///     KBox::pin_init(new_xarray!(AllocKind::Alloc1), GFP_KERNEL)?;
>>  ///
>>  /// let dead = KBox::new(0xdead, GFP_KERNEL)?;
>>  /// let beef = KBox::new(0xbeef, GFP_KERNEL)?;
>> @@ -86,7 +107,11 @@ pub enum AllocKind {
>>
>>  impl<T: ForeignOwnable> XArray<T> {
>>      /// Creates a new initializer for this type.
>> -    pub fn new(kind: AllocKind) -> impl PinInit<Self> {
>> +    pub fn new(
>> +        kind: AllocKind,
>> +        name: &'static CStr,
>> +        key: Pin<&'static LockClassKey>,
>> +    ) -> impl PinInit<Self> {
>>          let flags = match kind {
>>              AllocKind::Alloc => bindings::XA_FLAGS_ALLOC,
>>              AllocKind::Alloc1 => bindings::XA_FLAGS_ALLOC1,
>> @@ -95,8 +120,14 @@ pub fn new(kind: AllocKind) -> impl PinInit<Self> {
>>              // SAFETY: `xa` is valid while the closure is called.
>>              //
>>              // INVARIANT: `xa` is initialized here to an empty, valid [`bindings::xarray`].
>> -            xa <- Opaque::ffi_init(|xa| unsafe {
>> -                bindings::xa_init_flags(xa, flags)
>> +            xa <- Opaque::ffi_init(|xa: *mut bindings::xarray| unsafe {
>> +                bindings::__spin_lock_init(
>> +                    &raw mut (*xa).xa_lock,
>> +                    name.as_ptr().cast(),
>> +                    key.as_ptr(),
>> +                );
>> +                (*xa).xa_flags = flags;
>> +                (*xa).xa_head = null_mut();
>>              }),
>>              _p: PhantomData,
>>          })
>>
>> ---
>> base-commit: 18f7fcd5e69a04df57b563360b88be72471d6b62
>> change-id: 20260206-xarray-lockdep-fix-10f1cc68e5d7
>>
>> Best regards,
>> --
>> Andreas Hindborg <a.hindborg@kernel.org>
>>
>>


      reply	other threads:[~2026-02-06 17:43 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-02-06 17:10 Andreas Hindborg
2026-02-06 17:19 ` Tamir Duberstein
2026-02-06 17:43   ` Gary Guo [this message]

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=DG82CZTISPBF.2B59PSYVD8Z00@garyguo.net \
    --to=gary@garyguo.net \
    --cc=a.hindborg@kernel.org \
    --cc=aliceryhl@google.com \
    --cc=bjorn3_gh@protonmail.com \
    --cc=boqun.feng@gmail.com \
    --cc=dakr@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lossin@kernel.org \
    --cc=ojeda@kernel.org \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=tamird@kernel.org \
    --cc=tmgross@umich.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

all inboxes | Powered by JetHome®