Received-SPF: pass (google.com: domain of linux-kernel+bounces-131545-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240402-linked-list-v1-0-b1c59ba7ae3b@google.com>
 <20240402-linked-list-v1-1-b1c59ba7ae3b@google.com> <2f25f21e-fad8-48bb-aa2b-d61bf8909a41@proton.me>
In-Reply-To: <2f25f21e-fad8-48bb-aa2b-d61bf8909a41@proton.me>
From: Alice Ryhl <aliceryhl@google.com>
Date: Thu, 4 Apr 2024 16:00:53 +0200
Message-ID: <CAH5fLgjqyfExjckh7KnSLnT+Ok+yjcoJ+DpDkj0gUZRmoz=M8Q@mail.gmail.com>
Subject: Re: [PATCH 1/9] rust: list: add ListArc
To: Benno Lossin <benno.lossin@proton.me>
Cc: Miguel Ojeda <ojeda@kernel.org>, Andrew Morton <akpm@linux-foundation.org>, 
	Alex Gaynor <alex.gaynor@gmail.com>, Wedson Almeida Filho <wedsonaf@gmail.com>, 
	Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>, 
	=?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>, 
	Andreas Hindborg <a.hindborg@samsung.com>, Marco Elver <elver@google.com>, 
	Kees Cook <keescook@chromium.org>, Coly Li <colyli@suse.de>, Paolo Abeni <pabeni@redhat.com>, 
	Pierre Gondois <pierre.gondois@arm.com>, Ingo Molnar <mingo@kernel.org>, 
	Jakub Kicinski <kuba@kernel.org>, Wei Yang <richard.weiyang@gmail.com>, 
	Matthew Wilcox <willy@infradead.org>, linux-kernel@vger.kernel.org, 
	rust-for-linux@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 3, 2024 at 5:51=E2=80=AFPM Benno Lossin <benno.lossin@proton.me=
> wrote:
>
> 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.

I will use that.

> > +}
> > +
> > +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 =3D 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 t=
his 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.

I'll swap these so that from_unique calls from_pin_unique instead.
I'll also change on_create_list_arc_from_unique to take a Pin<&mut
Self>.

> > +        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!`?

Most likely.

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

Will do.

> > +        unsafe { <T as ListArcSafe<ID>>::on_create_list_arc_from_uniqu=
e(&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_uniq=
ue(&mut unique) };
> > +
> > +        let arc1 =3D Arc::from(unique);
> > +        let arc2 =3D 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.

Sure.

> > +        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 t=
his call, since `ListArc`
> > +        // implicitly pins its value.
> > +        Self::pair_from_unique(unsafe { Pin::into_inner_unchecked(uniq=
ue) })
> > +    }
> > +
> > +    /// Transmutes an [`Arc`] into a `ListArc` without updating the tr=
acking 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 `L=
istArc` 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 }

Yeah, I guess that works.

Alice