From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4468B134AB for ; Fri, 22 Sep 2023 08:53:46 +0000 (UTC) Received: from mail-0201.mail-europe.com (mail-0201.mail-europe.com [51.77.79.158]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CC3DCA9 for ; Fri, 22 Sep 2023 01:53:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=proton.me; s=protonmail; t=1695372809; x=1695632009; bh=ts4PF4d/DPQcSKzx9O++pjI9MrnIEG183Yqs4UuldsY=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: Feedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID: Message-ID:BIMI-Selector; b=TvvJFo9zKUKVOBKC0nkSJImrJdbhmBDXc23Nj6ybYmc2HCbZq0QEyF39+8gLQ9XSa D0L/gFVLKt0cG6YrIoTkVLSbDcfWjA7aywYBAPlUhd2Y2U9htOKZsNXbrrYz7ybkZI GrgJ48vEWz6pnIHypJu8yFo79GPsERs6RUVNXRnmNfMt00g45SP8fRPFbDulU9sbkV ix42urZucMwD4XR6XEfjKIa3uQwpG5QKn5OAvEGJh4fKm0P8kCY3GGzbyDRjnDGV6W L5UMpYYWg2QL7KBcgB0ceaniBsx2sfz6W90QwEGtMQKwc6Tfk2reejosBsbukrIi1J Kq3rVlplecK/w== Date: Fri, 22 Sep 2023 08:53:22 +0000 To: Wedson Almeida Filho , rust-for-linux@vger.kernel.org From: Benno Lossin Cc: Miguel Ojeda , Alex Gaynor , Boqun Feng , Gary Guo , =?utf-8?Q?Bj=C3=B6rn_Roy_Baron?= , Andreas Hindborg , Alice Ryhl , linux-kernel@vger.kernel.org, Wedson Almeida Filho Subject: Re: [PATCH 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef` Message-ID: <63223594-1bbe-c4f2-ea82-9b34c66ba1f6@proton.me> In-Reply-To: <20230921213440.202017-3-wedsonaf@gmail.com> References: <20230921213440.202017-1-wedsonaf@gmail.com> <20230921213440.202017-3-wedsonaf@gmail.com> Feedback-ID: 71780778:user:proton Precedence: bulk X-Mailing-List: rust-for-linux@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net On 21.09.23 23:34, Wedson Almeida Filho wrote: > From: Wedson Almeida Filho >=20 > With GATs, we don't need a separate type to represent a borrowed object > with a refcount, we can just use Rust's regular shared borrowing. In > this case, we use `&WithRef` instead of `ArcBorrow<'_, T>`. >=20 > Co-developed-by: Boqun Feng > Signed-off-by: Boqun Feng > Signed-off-by: Wedson Almeida Filho I only have a very small comment below, so Reviewed-by: Benno Lossin > --- > rust/kernel/sync.rs | 2 +- > rust/kernel/sync/arc.rs | 180 ++++++++++++++-------------------------- > 2 files changed, 62 insertions(+), 120 deletions(-) >=20 > diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs > index d219ee518eff..083494884500 100644 > --- a/rust/kernel/sync.rs > +++ b/rust/kernel/sync.rs > @@ -12,7 +12,7 @@ > pub mod lock; > mod locked_by; >=20 > -pub use arc::{Arc, ArcBorrow, UniqueArc}; > +pub use arc::{Arc, UniqueArc, WithRef}; > pub use condvar::CondVar; > pub use lock::{mutex::Mutex, spinlock::SpinLock}; > pub use locked_by::LockedBy; > diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs > index 86bff1e0002c..5948e42b9c8f 100644 > --- a/rust/kernel/sync/arc.rs > +++ b/rust/kernel/sync/arc.rs > @@ -105,14 +105,14 @@ > /// Coercion from `Arc` to `Arc`: > /// > /// ``` > -/// use kernel::sync::{Arc, ArcBorrow}; > +/// use kernel::sync::{Arc, WithRef}; > /// > /// trait MyTrait { > /// // Trait has a function whose `self` type is `Arc`. > /// fn example1(self: Arc) {} > /// > -/// // Trait has a function whose `self` type is `ArcBorrow<'_, Self= >`. > -/// fn example2(self: ArcBorrow<'_, Self>) {} > +/// // Trait has a function whose `self` type is `&WithRef`. > +/// fn example2(self: &WithRef) {} > /// } > /// > /// struct Example; > @@ -130,9 +130,48 @@ pub struct Arc { > _p: PhantomData>, > } >=20 > +/// An instance of `T` with an attached reference count. > +/// > +/// # Examples > +/// > +/// ``` > +/// use kernel::sync::{Arc, WithRef}; > +/// > +/// struct Example; > +/// > +/// fn do_something(e: &WithRef) -> Arc { > +/// e.into() > +/// } > +/// > +/// let obj =3D Arc::try_new(Example)?; > +/// let cloned =3D do_something(obj.as_with_ref()); > +/// > +/// // Assert that both `obj` and `cloned` point to the same underlying = object. > +/// assert!(core::ptr::eq(&*obj, &*cloned)); > +/// ``` > +/// > +/// Using `WithRef` as the type of `self`: > +/// > +/// ``` > +/// use kernel::sync::{Arc, WithRef}; > +/// > +/// struct Example { > +/// _a: u32, > +/// _b: u32, > +/// } > +/// > +/// impl Example { > +/// fn use_reference(self: &WithRef) { > +/// // ... > +/// } > +/// } > +/// > +/// let obj =3D Arc::try_new(Example { _a: 10, _b: 20 })?; > +/// obj.as_with_ref().use_reference(); > +/// ``` > #[pin_data] > #[repr(C)] > -struct WithRef { > +pub struct WithRef { > refcount: Opaque, > data: T, > } I do not really like the position of this definition in this file. It should be further down with the other code (like the `impl From<&WithRef> for Arc`). > @@ -215,16 +254,16 @@ unsafe fn from_inner(inner: NonNull>) ->= Self { > } > } >=20 > - /// Returns an [`ArcBorrow`] from the given [`Arc`]. > + /// Returns a [`WithRef`] from the given [`Arc`]. > /// > - /// This is useful when the argument of a function call is an [`ArcB= orrow`] (e.g., in a method > - /// receiver), but we have an [`Arc`] instead. Getting an [`ArcBorro= w`] is free when optimised. > + /// This is useful when the argument of a function call is a [`WithR= ef`] (e.g., in a method > + /// receiver), but we have an [`Arc`] instead. Getting a [`WithRef`]= is free when optimised. > #[inline] > - pub fn as_arc_borrow(&self) -> ArcBorrow<'_, T> { > + pub fn as_with_ref(&self) -> &WithRef { > // SAFETY: The constraint that the lifetime of the shared refer= ence must outlive that of > - // the returned `ArcBorrow` ensures that the object remains aliv= e and that no mutable > + // the returned `WithRef` ensures that the object remains alive = and that no mutable > // reference can be created. > - unsafe { ArcBorrow::new(self.ptr) } > + unsafe { self.ptr.as_ref() } > } >=20 > /// Compare whether two [`Arc`] pointers reference the same underly= ing object. > @@ -234,20 +273,17 @@ pub fn ptr_eq(this: &Self, other: &Self) -> bool { > } >=20 > impl ForeignOwnable for Arc { > - type Borrowed<'a> =3D ArcBorrow<'a, T>; > + type Borrowed<'a> =3D &'a WithRef; >=20 > fn into_foreign(self) -> *const core::ffi::c_void { > ManuallyDrop::new(self).ptr.as_ptr() as _ > } >=20 > - unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a,= T> { > + unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> &'a WithRef { > // SAFETY: By the safety requirement of this function, we know = that `ptr` came from > - // a previous call to `Arc::into_foreign`. > - let inner =3D NonNull::new(ptr as *mut WithRef).unwrap(); > - > - // SAFETY: The safety requirements of `from_foreign` ensure that= the object remains alive > - // for the lifetime of the returned value. > - unsafe { ArcBorrow::new(inner) } > + // a previous call to `Arc::into_foreign`. The safety requiremen= ts of `from_foreign` ensure > + // that the object remains alive for the lifetime of the returne= d value. > + unsafe { &*(ptr.cast::>()) } > } >=20 > unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self { > @@ -320,119 +356,25 @@ fn from(item: Pin>) -> Self { > } > } >=20 > -/// A borrowed reference to an [`Arc`] instance. > -/// > -/// For cases when one doesn't ever need to increment the refcount on th= e allocation, it is simpler > -/// to use just `&T`, which we can trivially get from an `Arc` instan= ce. > -/// > -/// However, when one may need to increment the refcount, it is preferab= le to use an `ArcBorrow` > -/// over `&Arc` because the latter results in a double-indirection: a= pointer (shared reference) > -/// to a pointer (`Arc`) to the object (`T`). An [`ArcBorrow`] elimin= ates this double > -/// indirection while still allowing one to increment the refcount and g= etting an `Arc` when/if > -/// needed. > -/// > -/// # Invariants > -/// > -/// There are no mutable references to the underlying [`Arc`], and it re= mains valid for the > -/// lifetime of the [`ArcBorrow`] instance. > -/// > -/// # Example > -/// > -/// ``` > -/// use kernel::sync::{Arc, ArcBorrow}; > -/// > -/// struct Example; > -/// > -/// fn do_something(e: ArcBorrow<'_, Example>) -> Arc { > -/// e.into() > -/// } > -/// > -/// let obj =3D Arc::try_new(Example)?; > -/// let cloned =3D do_something(obj.as_arc_borrow()); > -/// > -/// // Assert that both `obj` and `cloned` point to the same underlying = object. > -/// assert!(core::ptr::eq(&*obj, &*cloned)); > -/// # Ok::<(), Error>(()) > -/// ``` > -/// > -/// Using `ArcBorrow` as the type of `self`: > -/// > -/// ``` > -/// use kernel::sync::{Arc, ArcBorrow}; > -/// > -/// struct Example { > -/// a: u32, > -/// b: u32, > -/// } > -/// > -/// impl Example { > -/// fn use_reference(self: ArcBorrow<'_, Self>) { > -/// // ... > -/// } > -/// } > -/// > -/// let obj =3D Arc::try_new(Example { a: 10, b: 20 })?; > -/// obj.as_arc_borrow().use_reference(); > -/// # Ok::<(), Error>(()) > -/// ``` > -pub struct ArcBorrow<'a, T: ?Sized + 'a> { > - inner: NonNull>, > - _p: PhantomData<&'a ()>, > -} > - > -// This is to allow [`ArcBorrow`] (and variants) to be used as the type = of `self`. > -impl core::ops::Receiver for ArcBorrow<'_, T> {} > - > -// This is to allow `ArcBorrow` to be dispatched on when `ArcBorrow` can be coerced into > -// `ArcBorrow`. > -impl, U: ?Sized> core::ops::DispatchFromDyn> > - for ArcBorrow<'_, T> > -{ > -} > - > -impl Clone for ArcBorrow<'_, T> { > - fn clone(&self) -> Self { > - *self > - } > -} > - > -impl Copy for ArcBorrow<'_, T> {} > - > -impl ArcBorrow<'_, T> { > - /// Creates a new [`ArcBorrow`] instance. > - /// > - /// # Safety > - /// > - /// Callers must ensure the following for the lifetime of the return= ed [`ArcBorrow`] instance: > - /// 1. That `inner` remains valid; > - /// 2. That no mutable references to `inner` are created. > - unsafe fn new(inner: NonNull>) -> Self { > - // INVARIANT: The safety requirements guarantee the invariants. > - Self { > - inner, > - _p: PhantomData, > - } > - } > -} I think here would be the correct location. Another option would be to move this code up to the definition. I don't think this is important, so I can maybe just create an issue and let this be someone's first contrib= ution. > +// This is to allow [`WithRef`] (and variants) to be used as the type of= `self`. > +impl core::ops::Receiver for WithRef {} >=20 > -impl From> for Arc { > - fn from(b: ArcBorrow<'_, T>) -> Self { > +impl From<&WithRef> for Arc { > + fn from(b: &WithRef) -> Self { > // SAFETY: The existence of `b` guarantees that the refcount is= non-zero. `ManuallyDrop` > // guarantees that `drop` isn't called, so it's ok that the tem= porary `Arc` doesn't own the > // increment. > - ManuallyDrop::new(unsafe { Arc::from_inner(b.inner) }) > + ManuallyDrop::new(unsafe { Arc::from_inner(b.into()) }) > .deref() > .clone() > } > } >=20 > -impl Deref for ArcBorrow<'_, T> { > +impl Deref for WithRef { > type Target =3D T; >=20 > fn deref(&self) -> &Self::Target { > - // SAFETY: By the type invariant, the underlying object is still= alive with no mutable > - // references to it, so it is safe to create a shared reference. > - unsafe { &self.inner.as_ref().data } > + &self.data > } > } >=20 > -- > 2.34.1 >=20