Re: [PATCH v10 2/2] rust: xarray: Add an abstraction for XArray

[Tamir Duberstein]

On Tue, Dec 3, 2024 at 10:12 AM Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:
>
> On Tue, Dec 3, 2024 at 4:00 PM Tamir Duberstein <tamird@xxxxxxxxx> wrote:
> >
> > On Tue, Dec 3, 2024 at 7:30 AM Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:
> > >
> > > On Wed, Nov 20, 2024 at 12:48 PM Tamir Duberstein <tamird@xxxxxxxxx> wrote:
> > > > +use crate::{
> > > > +    alloc, bindings, build_assert, build_error,
> > > > +    error::{Error, Result},
> > > > +    init::PinInit,
> > > > +    pin_init,
> > > > +    types::{ForeignOwnable, NotThreadSafe, Opaque},
> > > > +};
> > > > +use core::{iter, marker::PhantomData, mem};
> > > > +use macros::{pin_data, pinned_drop};
> > >
> > > I think these are in crate::prelude.
> >
> > I prefer to be explicit, unless there's guidance on this somewhere?
>
> I don't think I've ever seen anyone do a direct import from macros.

This exact stanza is also in `rust/kernel/block/mq/tag_set.rs`.

> > > > +    fn iter(&self) -> impl Iterator<Item = core::ptr::NonNull<T::PointedTo>> + '_ {
> > > > +        // TODO: Remove when https://lore.kernel.org/all/20240913213041.395655-5-gary@xxxxxxxxxxx/ is applied.
> > > > +        const MIN: core::ffi::c_ulong = core::ffi::c_ulong::MIN;
> > > > +        const MAX: core::ffi::c_ulong = core::ffi::c_ulong::MAX;
> > >
> > > Isn't MIN just zero?
> >
> > I liked the symmetry, but I could change it if you feel strongly.
>
> I commented because I thought it was confusing; I spent some time
> figuring out whether the integer was signed.

Alright, will replace with 0.

> > > > +    /// Erases an entry from the array.
> > > > +    ///
> > > > +    /// Returns the entry which was previously at the given index.
> > > > +    pub fn remove(&mut self, index: usize) -> Option<T> {
> > > > +        // SAFETY: `self.xa.xa` is always valid by the type invariant.
> > > > +        //
> > > > +        // SAFETY: The caller holds the lock.
> > > > +        let ptr = unsafe { bindings::__xa_erase(self.xa.xa.get(), to_index(index)) }.cast();
> > >
> > > Two safety comments?
> >
> > There are two properties that must be upheld. How would you like to
> > see it formatted?
>
> Usually multiple preconditions are listed using a bulleted list:
>
> // SAFETY:
> // - `self.xa.xa` is always valid by the type invariant.
> // - The caller holds the lock.

Thanks for the suggestion, done.

> > > > +        // SAFETY: `ptr` is either NULL or came from `T::into_foreign`.
> > > > +        unsafe { T::try_from_foreign(ptr) }
> > > > +    }
> > > > +
> > > > +    /// Stores an entry in the array.
> > > > +    ///
> > > > +    /// On success, returns the entry which was previously at the given index.
> > > > +    ///
> > > > +    /// On failure, returns the entry which was attempted to be stored.
> > >
> > > I'd like to see documentation about the gfp flags. This may unlock the
> > > spinlock temporarily if GFP_KERNEL is used.
> >
> > Will add the language from the C documentation: "May drop the lock if
> > needed to allocate memory, and then reacquire it afterwards."
>
> SGTM.
>
> > > > +        // SAFETY: `__xa_store` returns the old entry at this index on success or `xa_err` if an
> > > > +        // error happened.
> > > > +        match unsafe { bindings::xa_err(old) } {
> > > > +            0 => {
> > > > +                let old = old.cast();
> > > > +                // SAFETY: `ptr` is either NULL or came from `T::into_foreign`.
> > > > +                Ok(unsafe { T::try_from_foreign(old) })
> > >
> > > It can't be XA_ZERO_ENTRY?
> >
> > No it can't. XA_ZERO_ENTRY is never returned from the "normal" API.
> > XA_ZERO_ENTRY presents as NULL.
>
> It's probably worth mentioning in the safety comment.

Will do.