Re: [PATCH v3 05/12] rust: xarray: use `xas_load` instead of `xa_load` in `Guard::load`

[Tamir Duberstein <tamird@kernel.org>]

On Tue, Feb 10, 2026 at 10:16 AM Liam R. Howlett
<Liam.Howlett@oracle.com> wrote:
>
> * Andreas Hindborg <a.hindborg@kernel.org> [260209 14:39]:
> > Replace the call to `xa_load` with `xas_load` in `Guard::load`. The
> > `xa_load` function takes the RCU lock internally, which we do not need,
> > since the `Guard` already holds an exclusive lock on the `XArray`. The
> > `xas_load` function operates on `xa_state` and assumes the required locks
> > are already held.
> >
> > This change also removes the `#[expect(dead_code)]` annotation from
> > `XArrayState` and its constructor, as they are now in use.
>
> I don't understand the locking here.
>
> You are saying that, since you hold the xarray write lock, you won't be
> taking the rcu read lock, but then you change the api of load?  That
> seems wrong to me.

This patch doesn't change the API of load. Andreas is saying that the
type system already requires the caller to hold the xarray spin lock
when load is called, meaning acquiring the RCU lock isn't necessary.

>
> Any readers of the api that calls load will now need to hold the rcu
> read lock externally.  If you're doing this, then you should indicate
> that is necessary in the function name, like the C side does.  Otherwise
> you are limiting the users to the advanced API, aren't you?

The existing API already requires users to hold the xarray lock.

>
> Or are you saying that xarray can only be used if you hold the exclusive
> lock, which is now a read and write lock?

Yes - except for the word "now"; I'm not sure what you mean by it.

>
> >
> > Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

Acked-by: Tamir Duberstein <tamird@kernel.org>

> > ---
> >  rust/kernel/xarray.rs | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
> > index d1246ec114898..eadddafb180ec 100644
> > --- a/rust/kernel/xarray.rs
> > +++ b/rust/kernel/xarray.rs
> > @@ -215,10 +215,8 @@ fn load<F, U>(&self, index: usize, f: F) -> Option<U>
> >      where
> >          F: FnOnce(NonNull<c_void>) -> U,
> >      {
> > -        // SAFETY: `self.xa.xa` is always valid by the type invariant.
> > -        let ptr = unsafe { bindings::xa_load(self.xa.xa.get(), index) };
> > -        let ptr = NonNull::new(ptr.cast())?;
> > -        Some(f(ptr))
> > +        let mut state = XArrayState::new(self, index);
> > +        Some(f(state.load()?))

This can probably be written as `state.load().map(f)`.

> >      }
> >
> >      /// Checks if the XArray contains an element at the specified index.
> > @@ -327,7 +325,6 @@ pub fn store(
> >  /// # Invariants
> >  ///
> >  /// - `state` is always a valid `bindings::xa_state`.
> > -#[expect(dead_code)]
> >  pub(crate) struct XArrayState<'a, 'b, T: ForeignOwnable> {
> >      /// Holds a reference to the lock guard to ensure the lock is not dropped
> >      /// while `Self` is live.
> > @@ -336,7 +333,6 @@ pub(crate) struct XArrayState<'a, 'b, T: ForeignOwnable> {
> >  }
> >
> >  impl<'a, 'b, T: ForeignOwnable> XArrayState<'a, 'b, T> {
> > -    #[expect(dead_code)]
> >      fn new(access: &'b Guard<'a, T>, index: usize) -> Self {
> >          let ptr = access.xa.xa.get();
> >          // INVARIANT: We initialize `self.state` to a valid value below.
> > @@ -356,6 +352,13 @@ fn new(access: &'b Guard<'a, T>, index: usize) -> Self {
> >              },
> >          }
> >      }
> > +
> > +    fn load(&mut self) -> Option<NonNull<c_void>> {
> > +        // SAFETY: `state.state` is always valid by the type invariant of
> > +        // `XArrayState and we hold the xarray lock`.
> > +        let ptr = unsafe { bindings::xas_load(&raw mut self.state) };
> > +        NonNull::new(ptr.cast())
> > +    }
> >  }
> >
> >  // SAFETY: `XArray<T>` has no shared mutable state so it is `Send` iff `T` is `Send`.
> >
> > --
> > 2.51.2
> >
> >
> >