Message-ID: <Y8s1evi1wY1XlVgG@boqun-archlinux>
Date:   Fri, 20 Jan 2023 16:44:42 -0800
From:   Boqun Feng <boqun.feng@...il.com>
To:     Gary Guo <gary@...yguo.net>
Cc:     Emilio Cobos Álvarez <emilio@...sal.io>,
        Wedson Almeida Filho <wedsonaf@...il.com>,
        rust-for-linux@...r.kernel.org, Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/7] rust: sync: introduce `ArcBorrow`

Hi,

On Mon, Jan 16, 2023 at 10:07:36PM +0000, Gary Guo wrote:
> On Wed, 4 Jan 2023 17:06:50 +0100
> Emilio Cobos Álvarez <emilio@...sal.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@...yguo.net> wrote:  
> > >>
> > >> On Wed, 28 Dec 2022 06:03:43 +0000
> > >> Wedson Almeida Filho <wedsonaf@...il.com> wrote:
> > >>> +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.
> 
> Thanks for pointing out, I neglected that.
> 

Somehow I run into this code again, and after a few thoughts, I'm
wondering: isn't ArcBorrow just &ArcInner<T>?

I've tried the following diff, and it seems working. The better parts
are 1) #[derive(Clone, Copy)] works and 2) I'm able to remove a few code
including one "unsafe" in ArcBorrow::deref().

But sure, more close look is needed. Thoughts?

Regards,
Boqun

> In this case:
> 
> Reviewed-by: Gary Guo <gary@...yguo.net>

--------------------------------->8
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index ff73f9240ca1..48f919878f44 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -185,7 +185,7 @@ impl<T: ?Sized> Arc<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) }
+        ArcBorrow(unsafe { self.ptr.as_ref() })
     }
 }
 
@@ -298,52 +298,25 @@ impl<T: ?Sized> From<Pin<UniqueArc<T>>> for Arc<T> {
 /// let obj = Arc::try_new(Example { a: 10, b: 20 })?;
 /// obj.as_arc_borrow().use_reference();
 /// ```
-pub struct ArcBorrow<'a, T: ?Sized + 'a> {
-    inner: NonNull<ArcInner<T>>,
-    _p: PhantomData<&'a ()>,
-}
+#[derive(Clone, Copy)]
+pub struct ArcBorrow<'a, T: ?Sized>(&'a ArcInner<T>);
 
 // This is to allow [`ArcBorrow`] (and variants) to be used as the type of `self`.
 impl<T: ?Sized> core::ops::Receiver for ArcBorrow<'_, T> {}
 
 // This is to allow `ArcBorrow<U>` to be dispatched on when `ArcBorrow<T>` can be coerced into
 // `ArcBorrow<U>`.
-impl<T: ?Sized + Unsize<U>, U: ?Sized> core::ops::DispatchFromDyn<ArcBorrow<'_, U>>
-    for ArcBorrow<'_, T>
+impl<'a, T: ?Sized + Unsize<U>, U: ?Sized> core::ops::DispatchFromDyn<ArcBorrow<'a, U>>
+    for ArcBorrow<'a, T>
 {
 }
 
-impl<T: ?Sized> Clone for ArcBorrow<'_, T> {
-    fn clone(&self) -> Self {
-        *self
-    }
-}
-
-impl<T: ?Sized> Copy for ArcBorrow<'_, T> {}
-
-impl<T: ?Sized> ArcBorrow<'_, T> {
-    /// Creates a new [`ArcBorrow`] instance.
-    ///
-    /// # Safety
-    ///
-    /// Callers must ensure the following for the lifetime of the returned [`ArcBorrow`] instance:
-    /// 1. That `inner` remains valid;
-    /// 2. That no mutable references to `inner` are created.
-    unsafe fn new(inner: NonNull<ArcInner<T>>) -> Self {
-        // INVARIANT: The safety requirements guarantee the invariants.
-        Self {
-            inner,
-            _p: PhantomData,
-        }
-    }
-}
-
 impl<T: ?Sized> From<ArcBorrow<'_, T>> for Arc<T> {
     fn from(b: ArcBorrow<'_, T>) -> 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 temporary `Arc` doesn't own the
         // increment.
-        ManuallyDrop::new(unsafe { Arc::from_inner(b.inner) })
+        ManuallyDrop::new(unsafe { Arc::from_inner(b.0.into()) })
             .deref()
             .clone()
     }
@@ -353,9 +326,7 @@ impl<T: ?Sized> Deref for ArcBorrow<'_, T> {
     type Target = T;
 
     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.0.data
     }
 }
 

Powered by blists - more mailing lists