Re: [PATCH 2/3] rust: xarray: abstract `xa_alloc_cyclic`

["Onur Özkan" <work@onurozkan.dev>]

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