Message-ID: <CAJ-ks9nQhjXYfjr=kPU1RQON83iWkFXrxM7oXzSrSxRVbg_xTg@mail.gmail.com>
Date: Fri, 21 Feb 2025 12:32:06 -0500
From: Tamir Duberstein <tamird@...il.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, 
	Matthew Wilcox <willy@...radead.org>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	FUJITA Tomonori <fujita.tomonori@...il.com>, "Rob Herring (Arm)" <robh@...nel.org>, 
	Maíra Canal <mcanal@...lia.com>, 
	Asahi Lina <lina@...hilina.net>, rust-for-linux@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-pci@...r.kernel.org
Subject: Re: [PATCH v17 2/3] rust: xarray: Add an abstraction for XArray

(resending because gmail decided 👍 is only allowed in HTML)

On Fri, Feb 21, 2025 at 11:14 AM Danilo Krummrich <dakr@...nel.org> wrote:
>
> On Tue, Feb 18, 2025 at 09:37:44AM -0500, Tamir Duberstein wrote:
>
> [...]
>
> > +
> > +impl<T: ForeignOwnable> XArray<T> {
> > +    /// Creates a new [`XArray`].
> > +    pub fn new(kind: AllocKind) -> impl PinInit<Self> {
> > +        let flags = match kind {
> > +            AllocKind::Alloc => bindings::XA_FLAGS_ALLOC,
> > +            AllocKind::Alloc1 => bindings::XA_FLAGS_ALLOC1,
> > +        };
> > +        pin_init!(Self {
> > +            // SAFETY: `xa` is valid while the closure is called.
> > +            xa <- Opaque::ffi_init(|xa| unsafe {
> > +                bindings::xa_init_flags(xa, flags)
> > +            }),
> > +            _p: PhantomData,
> > +        })
>
> I think this needs an `INVARIANT` comment.

👍

> [...]
>
> > +/// The error returned by [`store`](Guard::store).
> > +///
> > +/// Contains the underlying error and the value that was not stored.
> > +pub struct StoreError<T> {
> > +    /// The error that occurred.
> > +    pub error: Error,
> > +    /// The value that was not stored.
> > +    pub value: T,
> > +}
> > +
> > +impl<T> From<StoreError<T>> for Error {
> > +    fn from(value: StoreError<T>) -> Self {
> > +        let StoreError { error, value: _ } = value;
> > +        error
> > +    }
>
> Still think this should just be `value.error`.
>
> If it is important to especially point out that `value` is dropped, maybe a
> comment is the better option.
>
> IMHO, adding additionally code here just throws up questions on why that
> additional code is needed.

OK.

> > +}
> > +
> > +impl<'a, T: ForeignOwnable> Guard<'a, T> {
> > +    fn load<F, U>(&self, index: usize, f: F) -> Option<U>
> > +    where
> > +        F: FnOnce(NonNull<T::PointedTo>) -> U,
> > +    {
> > +        // SAFETY: `self.xa.xa` is always valid by the type invariant.
> > +        let ptr = unsafe { bindings::xa_load(self.xa.xa.get(), index) };
> > +        let ptr = NonNull::new(ptr.cast())?;
> > +        Some(f(ptr))
> > +    }
> > +
> > +    /// Provides a reference to the element at the given index.
> > +    pub fn get(&self, index: usize) -> Option<T::Borrowed<'_>> {
> > +        self.load(index, |ptr| {
> > +            // SAFETY: `ptr` came from `T::into_foreign`.
> > +            unsafe { T::borrow(ptr.as_ptr()) }
> > +        })
> > +    }
> > +
> > +    /// Provides a mutable reference to the element at the given index.
> > +    pub fn get_mut(&mut self, index: usize) -> Option<T::BorrowedMut<'_>> {
> > +        self.load(index, |ptr| {
> > +            // SAFETY: `ptr` came from `T::into_foreign`.
> > +            unsafe { T::borrow_mut(ptr.as_ptr()) }
> > +        })
> > +    }
> > +
> > +    /// Removes and returns the element 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.
>
> I think we only want one `SAFETY` section with an enumeration.

👍

> > +        let ptr = unsafe { bindings::__xa_erase(self.xa.xa.get(), index) }.cast();
> > +        // SAFETY: `ptr` is either NULL or came from `T::into_foreign`.
> > +        //
> > +        // SAFETY: `&mut self` guarantees that the lifetimes of [`T::Borrowed`] and
> > +        // [`T::BorrowedMut`] borrowed from `self` have ended.
>
> Same here...

👍

> > +        unsafe { T::try_from_foreign(ptr) }
> > +    }
> > +
> > +    /// Stores an element at the given index.
> > +    ///
> > +    /// May drop the lock if needed to allocate memory, and then reacquire it afterwards.
> > +    ///
> > +    /// On success, returns the element which was previously at the given index.
> > +    ///
> > +    /// On failure, returns the element which was attempted to be stored.
> > +    pub fn store(
> > +        &mut self,
> > +        index: usize,
> > +        value: T,
> > +        gfp: alloc::Flags,
> > +    ) -> Result<Option<T>, StoreError<T>> {
> > +        build_assert!(
> > +            mem::align_of::<T::PointedTo>() >= 4,
> > +            "pointers stored in XArray must be 4-byte aligned"
> > +        );
> > +        let new = value.into_foreign();
> > +
> > +        let old = {
> > +            let new = new.cast();
> > +            // SAFETY: `self.xa.xa` is always valid by the type invariant.
> > +            //
> > +            // SAFETY: The caller holds the lock.
>
> ...and here.

👍

> > +            //
> > +            // INVARIANT: `new` came from `T::into_foreign`.
> > +            unsafe { bindings::__xa_store(self.xa.xa.get(), index, new, gfp.as_raw()) }
> > +        };

Powered by blists - more mailing lists