[v1] rust: xarray: add entry API with preloading

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

Posted by Andreas Hindborg 2 months ago

Replace the call to `xa_load` with `xas_load` in `Guard::load`. The
`xa_load` function takes the XArray lock internally, which would cause
a double lock since the `Guard` already holds the lock. The `xas_load`
function operates on XArray state and assumes the lock is already held,
which is the correct API to use when holding a `Guard`.

This change also removes the `#[expect(dead_code)]` annotation from
`XArrayState` and its constructor, as they are now in use.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/xarray.rs | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
index f6e610b059625..0f69a523b72bf 100644
--- a/rust/kernel/xarray.rs
+++ b/rust/kernel/xarray.rs
@@ -215,8 +215,10 @@ 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 mut state = XArrayState::new(self, index);
+        // SAFETY: `state.state` is always valid by the type invariant of
+        // `XArrayState`.
+        let ptr = unsafe { bindings::xas_load(&raw mut state.state) };
         let ptr = NonNull::new(ptr.cast())?;
         Some(f(ptr))
     }
@@ -327,7 +329,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 +337,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.

-- 
2.51.2

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

Posted by Tamir Duberstein 1 month, 1 week ago

On Wed, Dec 3, 2025 at 5:27 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Replace the call to `xa_load` with `xas_load` in `Guard::load`. The
> `xa_load` function takes the XArray lock internally, which would cause
> a double lock since the `Guard` already holds the lock.

This is not correct. `xa_load` takes and releases the RCU lock only,
not the XArray lock.

> The `xas_load`
> function operates on XArray state and assumes the lock is already held,
> which is the correct API to use when holding a `Guard`.
>
> This change also removes the `#[expect(dead_code)]` annotation from
> `XArrayState` and its constructor, as they are now in use.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  rust/kernel/xarray.rs | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
> index f6e610b059625..0f69a523b72bf 100644
> --- a/rust/kernel/xarray.rs
> +++ b/rust/kernel/xarray.rs
> @@ -215,8 +215,10 @@ 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 mut state = XArrayState::new(self, index);
> +        // SAFETY: `state.state` is always valid by the type invariant of
> +        // `XArrayState`.
> +        let ptr = unsafe { bindings::xas_load(&raw mut state.state) };

Can this be a method on XArrayState?

>          let ptr = NonNull::new(ptr.cast())?;
>          Some(f(ptr))
>      }
> @@ -327,7 +329,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 +337,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.
>
> --
> 2.51.2
>
>

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

Posted by Andreas Hindborg 1 month ago

Tamir Duberstein <tamird@gmail.com> writes:

> On Wed, Dec 3, 2025 at 5:27 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Replace the call to `xa_load` with `xas_load` in `Guard::load`. The
>> `xa_load` function takes the XArray lock internally, which would cause
>> a double lock since the `Guard` already holds the lock.
>
> This is not correct. `xa_load` takes and releases the RCU lock only,
> not the XArray lock.

You are right. However, we do not need to take the RCU lock either in
this case, right?

>
>> The `xas_load`
>> function operates on XArray state and assumes the lock is already held,
>> which is the correct API to use when holding a `Guard`.
>>
>> This change also removes the `#[expect(dead_code)]` annotation from
>> `XArrayState` and its constructor, as they are now in use.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>>  rust/kernel/xarray.rs | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
>> index f6e610b059625..0f69a523b72bf 100644
>> --- a/rust/kernel/xarray.rs
>> +++ b/rust/kernel/xarray.rs
>> @@ -215,8 +215,10 @@ 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 mut state = XArrayState::new(self, index);
>> +        // SAFETY: `state.state` is always valid by the type invariant of
>> +        // `XArrayState`.
>> +        let ptr = unsafe { bindings::xas_load(&raw mut state.state) };
>
> Can this be a method on XArrayState?

OK.


Best regards,
Andreas Hindborg

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

Posted by Tamir Duberstein 1 month ago

On Wed, Jan 7, 2026 at 2:37 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Tamir Duberstein <tamird@gmail.com> writes:
>
> > On Wed, Dec 3, 2025 at 5:27 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>
> >> Replace the call to `xa_load` with `xas_load` in `Guard::load`. The
> >> `xa_load` function takes the XArray lock internally, which would cause
> >> a double lock since the `Guard` already holds the lock.
> >
> > This is not correct. `xa_load` takes and releases the RCU lock only,
> > not the XArray lock.
>
> You are right. However, we do not need to take the RCU lock either in
> this case, right?

Seems reasonable, but the language mentioning double locking should be
removed please.

>
> >
> >> The `xas_load`
> >> function operates on XArray state and assumes the lock is already held,
> >> which is the correct API to use when holding a `Guard`.
> >>
> >> This change also removes the `#[expect(dead_code)]` annotation from
> >> `XArrayState` and its constructor, as they are now in use.
> >>
> >> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> >> ---
> >>  rust/kernel/xarray.rs | 8 ++++----
> >>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
> >> index f6e610b059625..0f69a523b72bf 100644
> >> --- a/rust/kernel/xarray.rs
> >> +++ b/rust/kernel/xarray.rs
> >> @@ -215,8 +215,10 @@ 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 mut state = XArrayState::new(self, index);
> >> +        // SAFETY: `state.state` is always valid by the type invariant of
> >> +        // `XArrayState`.
> >> +        let ptr = unsafe { bindings::xas_load(&raw mut state.state) };
> >
> > Can this be a method on XArrayState?
>
> OK.
>
>
> Best regards,
> Andreas Hindborg
>
>

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

Posted by Andreas Hindborg 1 month ago

Tamir Duberstein <tamird@gmail.com> writes:

> On Wed, Jan 7, 2026 at 2:37 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Tamir Duberstein <tamird@gmail.com> writes:
>>
>> > On Wed, Dec 3, 2025 at 5:27 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>> >>
>> >> Replace the call to `xa_load` with `xas_load` in `Guard::load`. The
>> >> `xa_load` function takes the XArray lock internally, which would cause
>> >> a double lock since the `Guard` already holds the lock.
>> >
>> > This is not correct. `xa_load` takes and releases the RCU lock only,
>> > not the XArray lock.
>>
>> You are right. However, we do not need to take the RCU lock either in
>> this case, right?
>
> Seems reasonable, but the language mentioning double locking should be
> removed please.

For sure.


Best regards,
Andreas Hindborg

[PATCH 01/10] rust: xarray: minor formatting fixes
[PATCH 02/10] rust: xarray: add debug format for `StoreError`
[PATCH 03/10] rust: xarray: add `contains_index` method
[PATCH 04/10] rust: xarray: add `XArrayState`
[PATCH 05/10] rust: xarray: use `xas_load` instead of `xa_load` in `Guard::load`
[PATCH 06/10] rust: xarray: simplify `Guard::load`
[PATCH 07/10] rust: xarray: add `find_next` and `find_next_mut`
[PATCH 08/10] rust: xarray: add entry API
[PATCH 09/10] rust: xarray: add preload API
[PATCH 10/10] rust: xarray: fix false positive lockdep warnings