[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <df2f6c0b-fb4c-97e5-b607-7df626ea5933@crisal.io>
Date: Wed, 4 Jan 2023 17:06:50 +0100
From: Emilio Cobos Álvarez <emilio@...sal.io>
To: Wedson Almeida Filho <wedsonaf@...il.com>,
Gary Guo <gary@...yguo.net>
Cc: rust-for-linux@...r.kernel.org, Miguel Ojeda <ojeda@...nel.org>,
Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/7] rust: sync: introduce `ArcBorrow`
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@...yguo.net> wrote:
>>
>> On Wed, 28 Dec 2022 06:03:43 +0000
>> Wedson Almeida Filho <wedsonaf@...il.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@...il.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.
Cheers,
-- Emilio
Powered by blists - more mailing lists