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

[Alice Ryhl]

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.

> > > +    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.

> > > +    /// 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.

> > > +        // 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.

Alice