Implements `alloc_cyclic` function to `XArray<T>` that
wraps `xa_alloc_cyclic` safely.

Resolves a task from the nova/core task list under the "XArray
bindings [XARR]" section in "Documentation/gpu/nova/core/todo.rst"
file.

Signed-off-by: Onur Özkan <work@onurozkan.dev>
---
 rust/kernel/xarray.rs | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
index 1b882cd2f58b..4c2fdf53c7af 100644
--- a/rust/kernel/xarray.rs
+++ b/rust/kernel/xarray.rs
@@ -305,6 +305,49 @@ pub fn alloc(

         Ok(id)
     }
+
+    /// Allocates an empty slot within the given `limit`, storing `value` and cycling from `*next`.
+    ///
+    /// May drop the lock if needed to allocate memory, and then reacquire it afterwards.
+    ///
+    /// On success, returns the allocated id.
+    ///
+    /// On failure, returns the element which was attempted to be stored.
+    pub fn alloc_cyclic(
+        &mut self,
+        limit: bindings::xa_limit,
+        next: &mut u32,
+        value: T,
+        gfp: alloc::Flags,
+    ) -> Result<u32, StoreError<T>> {
+        build_assert!(
+            T::FOREIGN_ALIGN >= 4,
+            "pointers stored in XArray must be 4-byte aligned"
+        );
+
+        let new = value.into_foreign();
+
+        // `__xa_alloc_cyclic` overwrites this.
+        let mut id: u32 = 0;
+
+        // SAFETY:
+        // - `self.xa.xa` is valid by the type invariant.
+        // - `new` came from `T::into_foreign`.
+        let ret = unsafe {
+            bindings::__xa_alloc_cyclic(self.xa.xa.get(), &mut id, new, limit, next, gfp.as_raw())
+        };
+
+        if ret < 0 {
+            // SAFETY: `__xa_alloc_cyclic` doesn't take ownership on error.
+            let value = unsafe { T::from_foreign(new) };
+            return Err(StoreError {
+                value,
+                error: Error::from_errno(ret),
+            });
+        }
+
+        Ok(id)
+    }
 }

 // SAFETY: `XArray<T>` has no shared mutable state so it is `Send` iff `T` is `Send`.
--
2.51.0

On Mon, Oct 06, 2025 at 07:30:23PM +0300, Onur Özkan wrote:
> Implements `alloc_cyclic` function to `XArray<T>` that
> wraps `xa_alloc_cyclic` safely.
> 
> Resolves a task from the nova/core task list under the "XArray
> bindings [XARR]" section in "Documentation/gpu/nova/core/todo.rst"
> file.
> 
> Signed-off-by: Onur Özkan <work@onurozkan.dev>
> ---
>  rust/kernel/xarray.rs | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
> index 1b882cd2f58b..4c2fdf53c7af 100644
> --- a/rust/kernel/xarray.rs
> +++ b/rust/kernel/xarray.rs
> @@ -305,6 +305,49 @@ pub fn alloc(
> 
>          Ok(id)
>      }
> +
> +    /// Allocates an empty slot within the given `limit`, storing `value` and cycling from `*next`.
> +    ///
> +    /// May drop the lock if needed to allocate memory, and then reacquire it afterwards.
> +    ///
> +    /// On success, returns the allocated id.
> +    ///
> +    /// On failure, returns the element which was attempted to be stored.
> +    pub fn alloc_cyclic(
> +        &mut self,
> +        limit: bindings::xa_limit,

Could we use a Range<u32> type or similar here? I don't think we want a
bindings type.

> +        next: &mut u32,

So this is a mutable reference because it writes `*id + 1` to next,
taking wrap-around into account? The docs should probably explain that.

> +        value: T,
> +        gfp: alloc::Flags,
> +    ) -> Result<u32, StoreError<T>> {
> +        build_assert!(
> +            T::FOREIGN_ALIGN >= 4,
> +            "pointers stored in XArray must be 4-byte aligned"
> +        );

It should be enough to have this in the constructor. I don't think it's
needed here.

Alice

On Tue, 7 Oct 2025 10:56:56 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> On Mon, Oct 06, 2025 at 07:30:23PM +0300, Onur Özkan wrote:
> > Implements `alloc_cyclic` function to `XArray<T>` that
> > wraps `xa_alloc_cyclic` safely.
> > 
> > Resolves a task from the nova/core task list under the "XArray
> > bindings [XARR]" section in "Documentation/gpu/nova/core/todo.rst"
> > file.
> > 
> > Signed-off-by: Onur Özkan <work@onurozkan.dev>
> > ---
> >  rust/kernel/xarray.rs | 43
> > +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43
> > insertions(+)
> > 
> > diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
> > index 1b882cd2f58b..4c2fdf53c7af 100644
> > --- a/rust/kernel/xarray.rs
> > +++ b/rust/kernel/xarray.rs
> > @@ -305,6 +305,49 @@ pub fn alloc(
> > 
> >          Ok(id)
> >      }
> > +
> > +    /// Allocates an empty slot within the given `limit`, storing
> > `value` and cycling from `*next`.
> > +    ///
> > +    /// May drop the lock if needed to allocate memory, and then
> > reacquire it afterwards.
> > +    ///
> > +    /// On success, returns the allocated id.
> > +    ///
> > +    /// On failure, returns the element which was attempted to be
> > stored.
> > +    pub fn alloc_cyclic(
> > +        &mut self,
> > +        limit: bindings::xa_limit,
> 
> Could we use a Range<u32> type or similar here? I don't think we want
> a bindings type.
> 

Why do we not like to use the bindings type directly?


> > +        next: &mut u32,
> 
> So this is a mutable reference because it writes `*id + 1` to next,
> taking wrap-around into account? The docs should probably explain
> that.
> 

Sure. To be honest, I didn't really like doing this in the first place.
I can drop the mutable reference and return the next value as part of
the result to make it more idiomatic. This way, it will be easier for
the caller to use, especially for those who don't care about the next
value.

> > +        value: T,
> > +        gfp: alloc::Flags,
> > +    ) -> Result<u32, StoreError<T>> {
> > +        build_assert!(
> > +            T::FOREIGN_ALIGN >= 4,
> > +            "pointers stored in XArray must be 4-byte aligned"
> > +        );
> 
> It should be enough to have this in the constructor. I don't think
> it's needed here.
> 
> Alice

Regards,
Onur

On Tue, Oct 7, 2025 at 7:19 PM Onur Özkan <work@onurozkan.dev> wrote:
>
> Why do we not like to use the bindings type directly?

For public APIs, we generally try to avoid exposing C types:

    https://docs.kernel.org/rust/general-information.html#abstractions-vs-bindings

Sometimes it still makes sense, of course, e.g. a method may return
an inner type so that it gets used by other abstractions to call into
C. But generally we want to avoid exposing those for drivers and other
abstractions wherever possible.

Cheers,
Miguel

On Tue, 7 Oct 2025 19:28:31 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Tue, Oct 7, 2025 at 7:19 PM Onur Özkan <work@onurozkan.dev> wrote:
> >
> > Why do we not like to use the bindings type directly?
> 
> For public APIs, we generally try to avoid exposing C types:
> 
>     https://docs.kernel.org/rust/general-information.html#abstractions-vs-bindings
> 
> Sometimes it still makes sense, of course, e.g. a method may return
> an inner type so that it gets used by other abstractions to call into
> C. But generally we want to avoid exposing those for drivers and other
> abstractions wherever possible.
> 
> Cheers,
> Miguel

Thank you for explaining it, I will drop the bindings type in the next
version.

Regards,
Onur