Message-Id: <DG82CZTISPBF.2B59PSYVD8Z00@garyguo.net>
Date: Fri, 06 Feb 2026 17:43:45 +0000
From: "Gary Guo" <gary@...yguo.net>
To: "Tamir Duberstein" <tamird@...nel.org>, "Andreas Hindborg"
 <a.hindborg@...nel.org>
Cc: "Matthew Wilcox" <willy@...radead.org>, "Miguel Ojeda"
 <ojeda@...nel.org>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
 <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Benno Lossin" <lossin@...nel.org>, "Alice
 Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>, "Danilo
 Krummrich" <dakr@...nel.org>, <rust-for-linux@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] rust: xarray: fix false positive lockdep warnings

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@...nel.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@...il.com>
>
> Please replace with:
>
> Acked-by: Tamir Duberstein <tamird@...nel.org>
>
>> Signed-off-by: Andreas Hindborg <a.hindborg@...nel.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@...radead.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@...nel.org>
>>
>>

Powered by blists - more mailing lists