[4/7] rust: sync: introduce `ArcBorrow`

  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(-)
  

On Dec 27, 2022, at 10:03 PM, 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> {}
> +
> +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) })
> +            .deref()
> +            .clone()

The same worries about safety apply here too.  You need to make this fallible—try_from is nice enough for that.

> +    }
> +}
> +
> +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 }
> +    }
> +}
> -- 
> 2.34.1
> 
> 

— Laine Taffin Altman
  

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

On 12/28/22 07:03, Wedson Almeida Filho 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> {}
> +
> +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) })
> +            .deref()
> +            .clone()
> +    }
> +}
> +
> +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 }
> +    }
> +}
  

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

> +
> +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) })
> +            .deref()
> +            .clone()
> +    }
> +}

It might be easier to follow if this is jsut `bindings::refcount_inc`
followed by `Arc::from_inner`?

> +
> +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 }
> +    }
> +}
  

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.

>
> > +
> > +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) })
> > +            .deref()
> > +            .clone()
> > +    }
> > +}
>
> It might be easier to follow if this is jsut `bindings::refcount_inc`
> followed by `Arc::from_inner`?

I'd prefer to keep the interactions with `refcount_t` in `Arc` only so
that we can more easily change it in the future if we so choose.

> > +
> > +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 }
> > +    }
> > +}
  

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.

Cheers,

  -- Emilio
  

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
  

On Wed, 4 Jan 2023 17:06:50 +0100
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:
> >>> +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.

In this case:

Reviewed-by: Gary Guo <gary@garyguo.net>
  

Ops! I fall asleep while waiting for the Copy to derive Copy debate!

Reviewed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>


On Wed, Dec 28, 2022 at 7:05 AM 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> {}
> +
> +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) })
> +            .deref()
> +            .clone()
> +    }
> +}
> +
> +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 }
> +    }
> +}
> --
> 2.34.1
>
  

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@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:
> > >>> +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@garyguo.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
     }
 }