Message-ID: <2f25f21e-fad8-48bb-aa2b-d61bf8909a41@proton.me>
Date: Wed, 03 Apr 2024 15:51:42 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Alice Ryhl <aliceryhl@...gle.com>, Miguel Ojeda <ojeda@...nel.org>, Andrew Morton <akpm@...ux-foundation.org>
Cc: Alex Gaynor <alex.gaynor@...il.com>, Wedson Almeida Filho <wedsonaf@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...sung.com>, Marco Elver <elver@...gle.com>, Kees Cook <keescook@...omium.org>, Coly Li <colyli@...e.de>, Paolo Abeni <pabeni@...hat.com>, Pierre Gondois <pierre.gondois@....com>, Ingo Molnar <mingo@...nel.org>, Jakub Kicinski <kuba@...nel.org>, Wei Yang <richard.weiyang@...il.com>, Matthew Wilcox <willy@...radead.org>, linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH 1/9] rust: list: add ListArc

On 02.04.24 14:16, Alice Ryhl wrote:
> +impl<T: ListArcSafe<ID>, const ID: u64> ListArc<T, ID> {
> +    /// Constructs a new reference counted instance of `T`.
> +    pub fn try_new(contents: T) -> Result<Self, AllocError> {
> +        Ok(Self::from_unique(UniqueArc::try_new(contents)?))
> +    }
> +
> +    /// Use the given initializer to in-place initialize a `T`.
> +    ///
> +    /// If `T: !Unpin` it will not be able to move afterwards.
> +    pub fn pin_init<E>(init: impl PinInit<T, E>) -> error::Result<Self>
> +    where
> +        Error: From<E>,
> +    {
> +        Ok(Self::from_pin_unique(UniqueArc::pin_init(init)?))
> +    }

pin-init has a general trait for this: InPlaceInit. I don't know if the
other functions that it provides would help you.

> +}
> +
> +impl<T, const ID: u64> ListArc<T, ID>
> +where
> +    T: ListArcSafe<ID> + ?Sized,
> +{
> +    /// Convert a [`UniqueArc`] into a [`ListArc`].
> +    pub fn from_unique(mut unique: UniqueArc<T>) -> Self {
> +        // SAFETY: We have a `UniqueArc`, so there is no `ListArc`.
> +        unsafe { T::on_create_list_arc_from_unique(&mut unique) };
> +        let arc = Arc::from(unique);
> +        // SAFETY: We just called `on_create_list_arc_from_unique` on an arc without a `ListArc`,
> +        // so we can create a `ListArc`.
> +        unsafe { Self::transmute_from_arc(arc) }
> +    }
> +
> +    /// Convert a pinned [`UniqueArc`] into a [`ListArc`].
> +    pub fn from_pin_unique(unique: Pin<UniqueArc<T>>) -> Self {
> +        // SAFETY: We continue to treat this pointer as pinned after this call, since `ListArc`
> +        // implicitly pins its value.

This is not sufficient, since you also rely on `Self::from_unique` to
handle the parameter as if it were pinned, which it does not. Since it
calls `T::on_create_list_arc_from_unique` which just gets a `&mut self`
as a parameter and it could move stuff out.

> +        Self::from_unique(unsafe { Pin::into_inner_unchecked(unique) })
> +    }
> +
> +    /// Like [`from_unique`], but creates two `ListArcs`.
> +    ///
> +    /// The two ids must be different.
> +    ///
> +    /// [`from_unique`]: ListArc::from_unique
> +    pub fn pair_from_unique<const ID2: u64>(mut unique: UniqueArc<T>) -> (Self, ListArc<T, ID2>)
> +    where
> +        T: ListArcSafe<ID2>,
> +    {
> +        assert_ne!(ID, ID2);

Can this be a `build_assert!`?

> +
> +        // SAFETY: We have a `UniqueArc`, so we can call this method.

I liked the comment from above better:

     // SAFETY: We have a `UniqueArc`, so there is no `ListArc`.

Maybe use the variation "so there are no `ListArc`s for any ID.".

> +        unsafe { <T as ListArcSafe<ID>>::on_create_list_arc_from_unique(&mut unique) };
> +        // SAFETY: We have a `UniqueArc`, so we can call this method. The two ids are not equal.
> +        unsafe { <T as ListArcSafe<ID2>>::on_create_list_arc_from_unique(&mut unique) };
> +
> +        let arc1 = Arc::from(unique);
> +        let arc2 = Arc::clone(&arc1);
> +
> +        // SAFETY: We just called `on_create_list_arc_from_unique` on an arc without a `ListArc`,
> +        // so we can create a `ListArc`.

I would mention the two different IDs again.

> +        unsafe {
> +            (
> +                Self::transmute_from_arc(arc1),
> +                ListArc::transmute_from_arc(arc2),
> +            )
> +        }
> +    }
> +
> +    /// Like [`pair_from_unique`], but uses a pinned arc.
> +    ///
> +    /// The two ids must be different.
> +    ///
> +    /// [`pair_from_unique`]: ListArc::pair_from_unique
> +    pub fn pair_from_pin_unique<const ID2: u64>(
> +        unique: Pin<UniqueArc<T>>,
> +    ) -> (Self, ListArc<T, ID2>)
> +    where
> +        T: ListArcSafe<ID2>,
> +    {
> +        // SAFETY: We continue to treat this pointer as pinned after this call, since `ListArc`
> +        // implicitly pins its value.
> +        Self::pair_from_unique(unsafe { Pin::into_inner_unchecked(unique) })
> +    }
> +
> +    /// Transmutes an [`Arc`] into a `ListArc` without updating the tracking inside `T`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// * The value must not already have a `ListArc` reference.
> +    /// * The tracking inside `T` must think that there is a `ListArc` reference.
> +    #[inline]
> +    unsafe fn transmute_from_arc(me: Arc<T>) -> Self {
> +        // INVARIANT: By the safety requirements, the invariants on `ListArc` are satisfied.
> +        // SAFETY: ListArc is repr(transparent).
> +        unsafe { core::mem::transmute(me) }

Why do you need a transmute here? Can't you just construct the struct?

     Self { arc: me }

-- 
Cheers,
Benno

> +    }
> +

Powered by blists - more mailing lists