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

[Liam R. Howlett]

* Andreas Hindborg <a.hindborg@xxxxxxxxxx> [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.

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?

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

> 
> Signed-off-by: Andreas Hindborg <a.hindborg@xxxxxxxxxx>
> ---
>  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()?))
>      }
>  
>      /// 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
> 
> 
>