Messages in this thread | | | From | Wedson Almeida Filho <> | Date | Wed, 4 Jan 2023 17:52:46 +0000 | Subject | Re: [PATCH 4/7] rust: sync: introduce `ArcBorrow` |
| |
On Wed, 4 Jan 2023 at 16:06, Emilio Cobos Álvarez <emilio@crisal.io> wrote: > > Sorry for the drive-by comment, but maybe it saves some work. > > On 1/4/23 16:29, Wedson Almeida Filho wrote: > > On Sat, 31 Dec 2022 at 19:43, Gary Guo <gary@garyguo.net> wrote: > >> > >> On Wed, 28 Dec 2022 06:03:43 +0000 > >> Wedson Almeida Filho <wedsonaf@gmail.com> wrote: > >> > >>> This allows us to create references to a ref-counted allocation without > >>> double-indirection and that still allow us to increment the refcount to > >>> a new `Arc<T>`. > >>> > >>> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com> > >>> --- > >>> rust/kernel/sync.rs | 2 +- > >>> rust/kernel/sync/arc.rs | 97 +++++++++++++++++++++++++++++++++++++++++ > >>> 2 files changed, 98 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs > >>> index 39b379dd548f..5de03ea83ea1 100644 > >>> --- a/rust/kernel/sync.rs > >>> +++ b/rust/kernel/sync.rs > >>> @@ -7,4 +7,4 @@ > >>> > >>> mod arc; > >>> > >>> -pub use arc::Arc; > >>> +pub use arc::{Arc, ArcBorrow}; > >>> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs > >>> index dbc7596cc3ce..f68bfc02c81a 100644 > >>> --- a/rust/kernel/sync/arc.rs > >>> +++ b/rust/kernel/sync/arc.rs > >>> @@ -19,6 +19,7 @@ use crate::{bindings, error::Result, types::Opaque}; > >>> use alloc::boxed::Box; > >>> use core::{ > >>> marker::{PhantomData, Unsize}, > >>> + mem::ManuallyDrop, > >>> ops::Deref, > >>> ptr::NonNull, > >>> }; > >>> @@ -164,6 +165,18 @@ impl<T: ?Sized> Arc<T> { > >>> _p: PhantomData, > >>> } > >>> } > >>> + > >>> + /// Returns an [`ArcBorrow`] from the given [`Arc`]. > >>> + /// > >>> + /// This is useful when the argument of a function call is an [`ArcBorrow`] (e.g., in a method > >>> + /// receiver), but we have an [`Arc`] instead. Getting an [`ArcBorrow`] is free when optimised. > >>> + #[inline] > >>> + pub fn as_arc_borrow(&self) -> ArcBorrow<'_, T> { > >>> + // SAFETY: The constraint that the lifetime of the shared reference must outlive that of > >>> + // the returned `ArcBorrow` ensures that the object remains alive and that no mutable > >>> + // reference can be created. > >>> + unsafe { ArcBorrow::new(self.ptr) } > >>> + } > >>> } > >>> > >>> impl<T: ?Sized> Deref for Arc<T> { > >>> @@ -208,3 +221,87 @@ impl<T: ?Sized> Drop for Arc<T> { > >>> } > >>> } > >>> } > >>> + > >>> +/// A borrowed reference to an [`Arc`] instance. > >>> +/// > >>> +/// For cases when one doesn't ever need to increment the refcount on the allocation, it is simpler > >>> +/// to use just `&T`, which we can trivially get from an `Arc<T>` instance. > >>> +/// > >>> +/// However, when one may need to increment the refcount, it is preferable to use an `ArcBorrow<T>` > >>> +/// over `&Arc<T>` because the latter results in a double-indirection: a pointer (shared reference) > >>> +/// to a pointer (`Arc<T>`) to the object (`T`). An [`ArcBorrow`] eliminates this double > >>> +/// indirection while still allowing one to increment the refcount and getting an `Arc<T>` when/if > >>> +/// needed. > >>> +/// > >>> +/// # Invariants > >>> +/// > >>> +/// There are no mutable references to the underlying [`Arc`], and it remains valid for the > >>> +/// lifetime of the [`ArcBorrow`] instance. > >>> +/// > >>> +/// # Example > >>> +/// > >>> +/// ``` > >>> +/// use crate::sync::{Arc, ArcBorrow}; > >>> +/// > >>> +/// struct Example; > >>> +/// > >>> +/// fn do_something(e: ArcBorrow<'_, Example>) -> Arc<Example> { > >>> +/// e.into() > >>> +/// } > >>> +/// > >>> +/// let obj = Arc::try_new(Example)?; > >>> +/// let cloned = do_something(obj.as_arc_borrow()); > >>> +/// > >>> +/// // Assert that both `obj` and `cloned` point to the same underlying object. > >>> +/// assert!(core::ptr::eq(&*obj, &*cloned)); > >>> +/// ``` > >>> +pub struct ArcBorrow<'a, T: ?Sized + 'a> { > >>> + inner: NonNull<ArcInner<T>>, > >>> + _p: PhantomData<&'a ()>, > >>> +} > >>> + > >>> +impl<T: ?Sized> Clone for ArcBorrow<'_, T> { > >>> + fn clone(&self) -> Self { > >>> + *self > >>> + } > >>> +} > >>> + > >>> +impl<T: ?Sized> Copy for ArcBorrow<'_, T> {} > >> > >> Couldn't this just be derived `Clone` and `Copy`? > > > > Indeed. I'll send a v2 with this. > > I'm not sure this is true. Deriving will add the T: Copy and T: Clone > bound, which I think is not what you want here. > > i.e., I assume you want an ArcBorrow to be Copy even if the underlying T > is not. > > See <https://github.com/rust-lang/rust/issues/26925> for the relevant > (really long-standing) Rust issue.
Thanks for the heads up, Emilio!
After trying this out, derive doesn't work. The errors brought me back memories of when I first implemented this over a year ago, though I didn't take the time to try to understand why it was failing.
So no v2. The series will remain as is.
Cheers
> > Cheers, > > -- Emilio
| |