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

From: Benno Lossin <benno.lossin@proton.me>
To: "Maíra Canal" <mcanal@igalia.com>,
	"Asahi Lina" <lina@asahilina.net>,
	"Miguel Ojeda" <ojeda@kernel.org>,
	"Alex Gaynor" <alex.gaynor@gmail.com>,
	"Wedson Almeida Filho" <wedsonaf@gmail.com>,
	"Boqun Feng" <boqun.feng@gmail.com>,
	"Gary Guo" <gary@garyguo.net>,
	"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
	"Andreas Hindborg" <a.hindborg@samsung.com>,
	"Alice Ryhl" <aliceryhl@google.com>,
	"Matthew Wilcox" <willy@infradead.org>
Cc: rust-for-linux@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	kernel-dev@igalia.com
Subject: Re: [PATCH v8 2/2] rust: xarray: Add an abstraction for XArray
Date: Fri, 22 Mar 2024 11:12:00 +0000	[thread overview]
Message-ID: <8af4aa57-b1c4-44e4-a281-24ed45bbc312@proton.me> (raw)
In-Reply-To: <20240309235927.168915-4-mcanal@igalia.com>

On 3/10/24 00:57, Maíra Canal wrote:

[...]

> +// Convenience impl for [`ForeignOwnable`] types whose [`Borrowed`] form
> +// implements [`Deref`].
> +impl<'a, T: ForeignOwnable> Deref for Guard<'a, T>
> +where
> +    T::Borrowed<'a>: Deref,
> +    for<'b> T::Borrowed<'b>: Into<&'b <T::Borrowed<'a> as Deref>::Target>,

I took a look at this again and it currently is not really helpful.
`ArcBorrow<'b, T>` does not implement `Into<&'b T>` and it never is
going to, since it is impossible due to Rust's orphan rules. We have two
options:
1. Remove this impl.
2. Create a `IntoRef` trait that does the job instead and implement it
    for `ArcBorrow`.

If you go for option 2, I think `IntoRef` should look like this:
     pub trait IntoRef<'a, T: ?Sized> {
         fn into_ref(self) -> &'a T;
     }

But this can also easily be done in a future patch, so feel free to
choose option 1.

> +{
> +    type Target = <T::Borrowed<'a> as Deref>::Target;
> +
> +    fn deref(&self) -> &Self::Target {
> +        self.borrow().into()
> +    }
> +}
> +
> +impl<'a, T: ForeignOwnable> Drop for Guard<'a, T> {
> +    fn drop(&mut self) {
> +        // SAFETY: By the type invariant, we own the [`XArray`] lock, so we must
> +        // unlock it here.

I think the "so we must unlock it here" part is unnecessary.

> +        unsafe { bindings::xa_unlock(self.1.xa.get()) };
> +    }
> +}

[...]

> +/// # Examples
> +///
> +/// ```rust
> +/// use kernel::xarray::{XArray, flags};
> +/// use kernel::sync::Arc;
> +///
> +/// struct Foo {
> +///     a: u32,
> +///     b: u32,
> +/// }
> +///
> +/// let arr = Box::pin_init(XArray::<Arc<Foo>>::new(flags::ALLOC1))?;
> +///
> +/// let item = Arc::try_new(Foo { a : 1, b: 2 })?;
> +/// let index = arr.alloc(item)?;
> +///
> +/// if let Some(guard) = arr.get_locked(index) {
> +///     assert_eq!(guard.borrow().a, 1);
> +///     assert_eq!(guard.borrow().b, 2);
> +/// } else {
> +///     pr_info!("No value found in index {}", index);
> +/// }
> +///
> +/// let item = Arc::try_new(Foo { a : 3, b: 4 })?;
> +/// let index = arr.alloc(item)?;
> +///
> +/// if let Some(removed_data) = arr.remove(index) {
> +///     assert_eq!(removed_data.a, 3);
> +///     assert_eq!(removed_data.b, 4);
> +/// } else {
> +///     pr_info!("No value found in index {}", index);
> +/// }
> +/// # Ok::<(), Error>(())
> +/// ```

I think it would make more sense to move these examples into the
documentation of `XArray<T>` itself.

> +impl<T: ForeignOwnable> XArray<T> {
> +    /// Creates a new [`XArray`] with the given flags.
> +    pub fn new(flags: Flags) -> impl PinInit<Self> {
> +        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,
> +        })
> +    }
> +
> +    /// Converts [`usize`] to `unsigned long`.
> +    fn to_index(i: usize) -> c_ulong {
> +        // The type is `unsigned long`, which is always the same as `usize` in
> +        // the kernel.
> +        build_assert!(mem::size_of::<usize>() == mem::size_of::<c_ulong>());
> +        i as c_ulong
> +    }
> +
> +    /// Replaces an entry with a new value, returning the old value (if any).
> +    pub fn replace(&self, index: usize, value: T) -> Result<Option<T>> {
> +        let new = value.into_foreign();
> +
> +        build_assert!(T::FOREIGN_ALIGN >= 4);

This looks good. Which unsafe function call required this again? I can't
find it in the safety comments below.

As I wrote in my mail to the first patch, `ForeignOwnable` needs to
guarantee that `T::FOREIGN_ALIGN` actually is the alignment of the
pointers returned by `into_foreign()`, so it must be an `unsafe` trait.

> +
> +        // SAFETY: `new` just came from [`into_foreign()`], and we dismiss this guard
> +        // if the `xa_store` operation succeeds and takes ownership of the pointer.
> +        let guard = ScopeGuard::new(|| unsafe {
> +            drop(T::from_foreign(new));
> +        });
> +
> +        // SAFETY: `self.xa` is always valid by the type invariant, and we are
> +        // storing a [`T::into_foreign()`] result which upholds the later invariants.

The second part of this comment should be an `INVARIANT` comment
instead. So "INVARIANT: `new` came from [`T::into_foreign()`]."

> +        let old = unsafe {
> +            bindings::xa_store(
> +                self.xa.get(),
> +                Self::to_index(index),
> +                new.cast_mut(),
> +                bindings::GFP_KERNEL,
> +            )
> +        };
> +
> +        // SAFETY: `xa_store` returns the old entry at this index on success or
> +        // a [`XArray`] result, which can be turn into an errno through `xa_err`.
> +        to_result(unsafe { bindings::xa_err(old) })?;
> +        guard.dismiss();
> +
> +        Ok(if old.is_null() {
> +            None
> +        } else {
> +            // SAFETY: The old value must have been stored by either this function or
> +            // `insert_between`, both of which ensure non-NULL entries are valid
> +            // [`ForeignOwnable`] pointers.

This should be replaced by "by the type invariant of `Self`, `old` is
either NULL, or came from `T::into_foreign()`.".

> +            Some(unsafe { T::from_foreign(old) })
> +        })
> +    }

[...]

> +    /// Allocates a new index in the array, optionally storing a new value into
> +    /// it, with configurable bounds for the index range to allocate from.
> +    ///
> +    /// If `value` is [`None`], then the index is reserved from further allocation
> +    /// but remains free for storing a value into it.
> +    fn insert_between(&self, value: Option<T>, min: u32, max: u32) -> Result<usize> {
> +        let new = value.map_or(core::ptr::null(), |a| a.into_foreign());
> +        let mut id: u32 = 0;
> +
> +        let guard = ScopeGuard::new(|| {
> +            if !new.is_null() {
> +                // SAFETY: If `new` is not NULL, it came from the [`ForeignOwnable`]
> +                // we got from the caller.
> +                unsafe { drop(T::from_foreign(new)) };
> +            }
> +        });
> +
> +        // SAFETY: `self.xa` is always valid by the type invariant.
> +        //
> +        // If this succeeds, it takes ownership of the passed `T` (if any).
> +        // If it fails, we must drop the `T` again.
> +        let ret = unsafe {
> +            bindings::xa_alloc(
> +                self.xa.get(),
> +                &mut id,
> +                new.cast_mut(),
> +                bindings::xa_limit { min, max },
> +                bindings::GFP_KERNEL,
> +            )
> +        };

This function call is missing an INVARIANT comment stating that `new` is
either NULL or came from `T::into_foreign()`.

> +
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            guard.dismiss();
> +            Ok(id as usize)
> +        }
> +    }

[...]

> +// SAFETY: It is safe to send `XArray<T>` to another thread when the underlying
> +// `T` is `Send` because XArray is thread-safe and all mutation operations are
> +// internally locked. `T` must be `ForeignOwnable` because all pointers stored
> +// in the array are pointers obtained by calling `T::into_foreign` or are NULL
> +// pointers.
> +unsafe impl<T: Send + ForeignOwnable> Send for XArray<T> {}
> +//
> +// SAFETY: It is safe to send `&XArray<T>` to another thread when the underlying
> +// `T` is `Sync` because it effectively means sharing `&T` (which is safe because
> +// `T` is `Sync`). Additionally, `T` is `Send` because XArray is thread-safe and
> +// all mutation operations are internally locked. `T` must be `ForeignOwnable`
> +// because all pointers stored in the array are pointers obtained by calling
> +// `T::into_foreign` or are NULL pointers.
> +unsafe impl<T: Send + Sync + ForeignOwnable> Sync for XArray<T> {}

The part about "`T` must be `ForeignOwnable`..." is not needed here.

-- 
Cheers,
Benno

> --
> 2.43.0
> 