[2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef`

  From: Wedson Almeida Filho <walmeida@microsoft.com>

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<T>` instead of `ArcBorrow<'_, T>`.

Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
---
 rust/kernel/sync.rs     |   2 +-
 rust/kernel/sync/arc.rs | 180 ++++++++++++++--------------------------
 2 files changed, 62 insertions(+), 120 deletions(-)
  

On 21.09.23 23:34, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> 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<T>` instead of `ArcBorrow<'_, T>`.
> 
> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>

I only have a very small comment below, so

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

> ---
>   rust/kernel/sync.rs     |   2 +-
>   rust/kernel/sync/arc.rs | 180 ++++++++++++++--------------------------
>   2 files changed, 62 insertions(+), 120 deletions(-)
> 
> 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;
> 
> -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<Example>` to `Arc<dyn MyTrait>`:
>   ///
>   /// ```
> -/// use kernel::sync::{Arc, ArcBorrow};
> +/// use kernel::sync::{Arc, WithRef};
>   ///
>   /// trait MyTrait {
>   ///     // Trait has a function whose `self` type is `Arc<Self>`.
>   ///     fn example1(self: Arc<Self>) {}
>   ///
> -///     // Trait has a function whose `self` type is `ArcBorrow<'_, Self>`.
> -///     fn example2(self: ArcBorrow<'_, Self>) {}
> +///     // Trait has a function whose `self` type is `&WithRef<Self>`.
> +///     fn example2(self: &WithRef<Self>) {}
>   /// }
>   ///
>   /// struct Example;
> @@ -130,9 +130,48 @@ pub struct Arc<T: ?Sized> {
>       _p: PhantomData<WithRef<T>>,
>   }
> 
> +/// An instance of `T` with an attached reference count.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// use kernel::sync::{Arc, WithRef};
> +///
> +/// struct Example;
> +///
> +/// fn do_something(e: &WithRef<Example>) -> Arc<Example> {
> +///     e.into()
> +/// }
> +///
> +/// let obj = Arc::try_new(Example)?;
> +/// let cloned = 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<T>` as the type of `self`:
> +///
> +/// ```
> +/// use kernel::sync::{Arc, WithRef};
> +///
> +/// struct Example {
> +///     _a: u32,
> +///     _b: u32,
> +/// }
> +///
> +/// impl Example {
> +///     fn use_reference(self: &WithRef<Self>) {
> +///         // ...
> +///     }
> +/// }
> +///
> +/// let obj = Arc::try_new(Example { _a: 10, _b: 20 })?;
> +/// obj.as_with_ref().use_reference();
> +/// ```
>   #[pin_data]
>   #[repr(C)]
> -struct WithRef<T: ?Sized> {
> +pub struct WithRef<T: ?Sized> {
>       refcount: Opaque<bindings::refcount_t>,
>       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<T: ?Sized> From<&WithRef<T>> for Arc<T>`).

> @@ -215,16 +254,16 @@ unsafe fn from_inner(inner: NonNull<WithRef<T>>) -> Self {
>           }
>       }
> 
> -    /// 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 [`ArcBorrow`] (e.g., in a method
> -    /// receiver), but we have an [`Arc`] instead. Getting an [`ArcBorrow`] is free when optimised.
> +    /// This is useful when the argument of a function call is a [`WithRef`] (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<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
> +        // 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() }
>       }
> 
>       /// Compare whether two [`Arc`] pointers reference the same underlying object.
> @@ -234,20 +273,17 @@ pub fn ptr_eq(this: &Self, other: &Self) -> bool {
>   }
> 
>   impl<T: 'static> ForeignOwnable for Arc<T> {
> -    type Borrowed<'a> = ArcBorrow<'a, T>;
> +    type Borrowed<'a> = &'a WithRef<T>;
> 
>       fn into_foreign(self) -> *const core::ffi::c_void {
>           ManuallyDrop::new(self).ptr.as_ptr() as _
>       }
> 
> -    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<T> {
>           // SAFETY: By the safety requirement of this function, we know that `ptr` came from
> -        // a previous call to `Arc::into_foreign`.
> -        let inner = NonNull::new(ptr as *mut WithRef<T>).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 requirements of `from_foreign` ensure
> +        // that the object remains alive for the lifetime of the returned value.
> +        unsafe { &*(ptr.cast::<WithRef<T>>()) }
>       }
> 
>       unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
> @@ -320,119 +356,25 @@ fn from(item: Pin<UniqueArc<T>>) -> Self {
>       }
>   }
> 
> -/// 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 kernel::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));
> -/// # Ok::<(), Error>(())
> -/// ```
> -///
> -/// Using `ArcBorrow<T>` 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 = 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<WithRef<T>>,
> -    _p: PhantomData<&'a ()>,
> -}
> -
> -// 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<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<WithRef<T>>) -> 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 contribution.

> +// This is to allow [`WithRef`] (and variants) to be used as the type of `self`.
> +impl<T: ?Sized> core::ops::Receiver for WithRef<T> {}
> 
> -impl<T: ?Sized> From<ArcBorrow<'_, T>> for Arc<T> {
> -    fn from(b: ArcBorrow<'_, T>) -> Self {
> +impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
> +    fn from(b: &WithRef<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.into()) })
>               .deref()
>               .clone()
>       }
>   }
> 
> -impl<T: ?Sized> Deref for ArcBorrow<'_, T> {
> +impl<T: ?Sized> Deref for WithRef<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.data
>       }
>   }
> 
> --
> 2.34.1
>
  

On Thu, Sep 21, 2023 at 11:35 PM Wedson Almeida Filho
<wedsonaf@gmail.com> wrote:
>
> From: Wedson Almeida Filho <walmeida@microsoft.com>
>
> 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<T>` instead of `ArcBorrow<'_, T>`.
>
> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>

I wonder whether "WithArc" is a better name? After all, arc is short
for "atomic refcount".
I also think it's nice to include Arc in the name of the type.

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

On 22.09.23 17:29, Alice Ryhl wrote:
> On Thu, Sep 21, 2023 at 11:35 PM Wedson Almeida Filho
> <wedsonaf@gmail.com> wrote:
>>
>> From: Wedson Almeida Filho <walmeida@microsoft.com>
>>
>> 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<T>` instead of `ArcBorrow<'_, T>`.
>>
>> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> I wonder whether "WithArc" is a better name? After all, arc is short
> for "atomic refcount".
> I also think it's nice to include Arc in the name of the type.

I think `WithArc` would be confusing, since `Arc` is the smartpointer
that points to a `WithRef`.
  

On 9/21/23 18:34, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> 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<T>` instead of `ArcBorrow<'_, T>`.
> 
> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
> [...]
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
  

On 9/22/23 12:29, Alice Ryhl wrote:
> On Thu, Sep 21, 2023 at 11:35 PM Wedson Almeida Filho
> <wedsonaf@gmail.com> wrote:
>>
>> From: Wedson Almeida Filho <walmeida@microsoft.com>
>>
>> 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<T>` instead of `ArcBorrow<'_, T>`.
>>
>> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> I wonder whether "WithArc" is a better name? After all, arc is short
> for "atomic refcount".
> I also think it's nice to include Arc in the name of the type.
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> 

It can be read as `arc::WithRef`, which makes sense.
  

On Thu, 21 Sep 2023 18:34:40 -0300
Wedson Almeida Filho <wedsonaf@gmail.com> wrote:

> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> 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<T>` instead of `ArcBorrow<'_, T>`.
> 
> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>

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

The implementation looks good to me, thanks Wedson.

A minor thing that worth considering is to implement `AlwaysRefCounted`
to `WithRef` and reimplement `Arc` with `ARef<TaskRef<T>>` or add
conversion functions between them.

It feels natural to have this this impl (because `WithRef` is indeed
always ref counted), but on the other hand I don't currently foresee
anyone to currently want to use this API :(

> ---
>  rust/kernel/sync.rs     |   2 +-
>  rust/kernel/sync/arc.rs | 180 ++++++++++++++--------------------------
>  2 files changed, 62 insertions(+), 120 deletions(-)
  

On Fri, 22 Sep 2023 17:29:58 +0200
Alice Ryhl <aliceryhl@google.com> wrote:

> On Thu, Sep 21, 2023 at 11:35 PM Wedson Almeida Filho
> <wedsonaf@gmail.com> wrote:
> >
> > From: Wedson Almeida Filho <walmeida@microsoft.com>
> >
> > 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<T>` instead of `ArcBorrow<'_, T>`.
> >
> > Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>  
> 
> I wonder whether "WithArc" is a better name? After all, arc is short
> for "atomic refcount".
> I also think it's nice to include Arc in the name of the type.
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Or just call it with `WithRc` -- we don't use `aloc::rc::Rc` in the
kernel so there's no need to emphasis that this one is atomic :)

I personally think the `WithRef` name is fine, we have a smart pointer
called `ARef` after all :)

Best,
Gary
  

On Thu, Sep 21, 2023 at 06:34:40PM -0300, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> 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<T>` instead of `ArcBorrow<'_, T>`.
> 
> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
>  rust/kernel/sync.rs     |   2 +-
>  rust/kernel/sync/arc.rs | 180 ++++++++++++++--------------------------
>  2 files changed, 62 insertions(+), 120 deletions(-)
> 
> 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;
>  
> -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<Example>` to `Arc<dyn MyTrait>`:
>  ///
>  /// ```
> -/// use kernel::sync::{Arc, ArcBorrow};
> +/// use kernel::sync::{Arc, WithRef};
>  ///
>  /// trait MyTrait {
>  ///     // Trait has a function whose `self` type is `Arc<Self>`.
>  ///     fn example1(self: Arc<Self>) {}
>  ///
> -///     // Trait has a function whose `self` type is `ArcBorrow<'_, Self>`.
> -///     fn example2(self: ArcBorrow<'_, Self>) {}
> +///     // Trait has a function whose `self` type is `&WithRef<Self>`.
> +///     fn example2(self: &WithRef<Self>) {}
>  /// }
>  ///
>  /// struct Example;
> @@ -130,9 +130,48 @@ pub struct Arc<T: ?Sized> {
>      _p: PhantomData<WithRef<T>>,
>  }
>  
> +/// An instance of `T` with an attached reference count.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// use kernel::sync::{Arc, WithRef};
> +///
> +/// struct Example;
> +///
> +/// fn do_something(e: &WithRef<Example>) -> Arc<Example> {
> +///     e.into()
> +/// }
> +///
> +/// let obj = Arc::try_new(Example)?;
> +/// let cloned = 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<T>` as the type of `self`:
> +///
> +/// ```
> +/// use kernel::sync::{Arc, WithRef};
> +///
> +/// struct Example {
> +///     _a: u32,
> +///     _b: u32,
> +/// }
> +///
> +/// impl Example {
> +///     fn use_reference(self: &WithRef<Self>) {
> +///         // ...
> +///     }
> +/// }
> +///
> +/// let obj = Arc::try_new(Example { _a: 10, _b: 20 })?;
> +/// obj.as_with_ref().use_reference();
> +/// ```
>  #[pin_data]
>  #[repr(C)]
> -struct WithRef<T: ?Sized> {
> +pub struct WithRef<T: ?Sized> {
>      refcount: Opaque<bindings::refcount_t>,
>      data: T,
>  }
> @@ -215,16 +254,16 @@ unsafe fn from_inner(inner: NonNull<WithRef<T>>) -> Self {
>          }
>      }
>  
> -    /// Returns an [`ArcBorrow`] from the given [`Arc`].
> +    /// Returns a [`WithRef`] from the given [`Arc`].

       /// Returns a shared reference of [`WithRef`] 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.
> +    /// This is useful when the argument of a function call is a [`WithRef`] (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<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
> +        // 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() }
>      }
>  
>      /// Compare whether two [`Arc`] pointers reference the same underlying object.
> @@ -234,20 +273,17 @@ pub fn ptr_eq(this: &Self, other: &Self) -> bool {
>  }
>  
>  impl<T: 'static> ForeignOwnable for Arc<T> {
> -    type Borrowed<'a> = ArcBorrow<'a, T>;
> +    type Borrowed<'a> = &'a WithRef<T>;
>  
>      fn into_foreign(self) -> *const core::ffi::c_void {
>          ManuallyDrop::new(self).ptr.as_ptr() as _
>      }
>  
> -    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<T> {
>          // SAFETY: By the safety requirement of this function, we know that `ptr` came from
> -        // a previous call to `Arc::into_foreign`.
> -        let inner = NonNull::new(ptr as *mut WithRef<T>).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 requirements of `from_foreign` ensure
> +        // that the object remains alive for the lifetime of the returned value.
> +        unsafe { &*(ptr.cast::<WithRef<T>>()) }
>      }
>  
>      unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
> @@ -320,119 +356,25 @@ fn from(item: Pin<UniqueArc<T>>) -> Self {
>      }
>  }
>  
> -/// 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 kernel::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));
> -/// # Ok::<(), Error>(())
> -/// ```
> -///
> -/// Using `ArcBorrow<T>` 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 = 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<WithRef<T>>,
> -    _p: PhantomData<&'a ()>,
> -}
> -
> -// 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<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<WithRef<T>>) -> Self {
> -        // INVARIANT: The safety requirements guarantee the invariants.
> -        Self {
> -            inner,
> -            _p: PhantomData,
> -        }
> -    }
> -}
> +// This is to allow [`WithRef`] (and variants) to be used as the type of `self`.
> +impl<T: ?Sized> core::ops::Receiver for WithRef<T> {}
>  
> -impl<T: ?Sized> From<ArcBorrow<'_, T>> for Arc<T> {
> -    fn from(b: ArcBorrow<'_, T>) -> Self {
> +impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
> +    fn from(b: &WithRef<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.into()) })
>              .deref()
>              .clone()
>      }
>  }
>  
> -impl<T: ?Sized> Deref for ArcBorrow<'_, T> {
> +impl<T: ?Sized> Deref for WithRef<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.data
>      }
>  }
>  
> -- 
> 2.34.1
>
  

On Fri, 22 Sept 2023 at 05:53, Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 21.09.23 23:34, Wedson Almeida Filho wrote:
> > From: Wedson Almeida Filho <walmeida@microsoft.com>
> >
> > 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<T>` instead of `ArcBorrow<'_, T>`.
> >
> > Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
>
> I only have a very small comment below, so
>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
>
> > ---
> >   rust/kernel/sync.rs     |   2 +-
> >   rust/kernel/sync/arc.rs | 180 ++++++++++++++--------------------------
> >   2 files changed, 62 insertions(+), 120 deletions(-)
> >
> > 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;
> >
> > -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<Example>` to `Arc<dyn MyTrait>`:
> >   ///
> >   /// ```
> > -/// use kernel::sync::{Arc, ArcBorrow};
> > +/// use kernel::sync::{Arc, WithRef};
> >   ///
> >   /// trait MyTrait {
> >   ///     // Trait has a function whose `self` type is `Arc<Self>`.
> >   ///     fn example1(self: Arc<Self>) {}
> >   ///
> > -///     // Trait has a function whose `self` type is `ArcBorrow<'_, Self>`.
> > -///     fn example2(self: ArcBorrow<'_, Self>) {}
> > +///     // Trait has a function whose `self` type is `&WithRef<Self>`.
> > +///     fn example2(self: &WithRef<Self>) {}
> >   /// }
> >   ///
> >   /// struct Example;
> > @@ -130,9 +130,48 @@ pub struct Arc<T: ?Sized> {
> >       _p: PhantomData<WithRef<T>>,
> >   }
> >
> > +/// An instance of `T` with an attached reference count.
> > +///
> > +/// # Examples
> > +///
> > +/// ```
> > +/// use kernel::sync::{Arc, WithRef};
> > +///
> > +/// struct Example;
> > +///
> > +/// fn do_something(e: &WithRef<Example>) -> Arc<Example> {
> > +///     e.into()
> > +/// }
> > +///
> > +/// let obj = Arc::try_new(Example)?;
> > +/// let cloned = 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<T>` as the type of `self`:
> > +///
> > +/// ```
> > +/// use kernel::sync::{Arc, WithRef};
> > +///
> > +/// struct Example {
> > +///     _a: u32,
> > +///     _b: u32,
> > +/// }
> > +///
> > +/// impl Example {
> > +///     fn use_reference(self: &WithRef<Self>) {
> > +///         // ...
> > +///     }
> > +/// }
> > +///
> > +/// let obj = Arc::try_new(Example { _a: 10, _b: 20 })?;
> > +/// obj.as_with_ref().use_reference();
> > +/// ```
> >   #[pin_data]
> >   #[repr(C)]
> > -struct WithRef<T: ?Sized> {
> > +pub struct WithRef<T: ?Sized> {
> >       refcount: Opaque<bindings::refcount_t>,
> >       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<T: ?Sized> From<&WithRef<T>> for Arc<T>`).

Yeah, I think it was ok originally because it was just a private
standalone auxiliary type. Since this is not the case anymore, it's
probably better to move it down. Will do for v2.

> > @@ -215,16 +254,16 @@ unsafe fn from_inner(inner: NonNull<WithRef<T>>) -> Self {
> >           }
> >       }
> >
> > -    /// 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 [`ArcBorrow`] (e.g., in a method
> > -    /// receiver), but we have an [`Arc`] instead. Getting an [`ArcBorrow`] is free when optimised.
> > +    /// This is useful when the argument of a function call is a [`WithRef`] (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<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
> > +        // 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() }
> >       }
> >
> >       /// Compare whether two [`Arc`] pointers reference the same underlying object.
> > @@ -234,20 +273,17 @@ pub fn ptr_eq(this: &Self, other: &Self) -> bool {
> >   }
> >
> >   impl<T: 'static> ForeignOwnable for Arc<T> {
> > -    type Borrowed<'a> = ArcBorrow<'a, T>;
> > +    type Borrowed<'a> = &'a WithRef<T>;
> >
> >       fn into_foreign(self) -> *const core::ffi::c_void {
> >           ManuallyDrop::new(self).ptr.as_ptr() as _
> >       }
> >
> > -    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<T> {
> >           // SAFETY: By the safety requirement of this function, we know that `ptr` came from
> > -        // a previous call to `Arc::into_foreign`.
> > -        let inner = NonNull::new(ptr as *mut WithRef<T>).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 requirements of `from_foreign` ensure
> > +        // that the object remains alive for the lifetime of the returned value.
> > +        unsafe { &*(ptr.cast::<WithRef<T>>()) }
> >       }
> >
> >       unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
> > @@ -320,119 +356,25 @@ fn from(item: Pin<UniqueArc<T>>) -> Self {
> >       }
> >   }
> >
> > -/// 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 kernel::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));
> > -/// # Ok::<(), Error>(())
> > -/// ```
> > -///
> > -/// Using `ArcBorrow<T>` 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 = 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<WithRef<T>>,
> > -    _p: PhantomData<&'a ()>,
> > -}
> > -
> > -// 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<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<WithRef<T>>) -> 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 contribution.
>
> > +// This is to allow [`WithRef`] (and variants) to be used as the type of `self`.
> > +impl<T: ?Sized> core::ops::Receiver for WithRef<T> {}
> >
> > -impl<T: ?Sized> From<ArcBorrow<'_, T>> for Arc<T> {
> > -    fn from(b: ArcBorrow<'_, T>) -> Self {
> > +impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
> > +    fn from(b: &WithRef<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.into()) })
> >               .deref()
> >               .clone()
> >       }
> >   }
> >
> > -impl<T: ?Sized> Deref for ArcBorrow<'_, T> {
> > +impl<T: ?Sized> Deref for WithRef<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.data
> >       }
> >   }
> >
> > --
> > 2.34.1
> >
>
>
  

On Fri, 22 Sept 2023 at 16:50, Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 22.09.23 17:29, Alice Ryhl wrote:
> > On Thu, Sep 21, 2023 at 11:35 PM Wedson Almeida Filho
> > <wedsonaf@gmail.com> wrote:
> >>
> >> From: Wedson Almeida Filho <walmeida@microsoft.com>
> >>
> >> 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<T>` instead of `ArcBorrow<'_, T>`.
> >>
> >> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> >> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> >> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> >
> > I wonder whether "WithArc" is a better name? After all, arc is short
> > for "atomic refcount".
> > I also think it's nice to include Arc in the name of the type.
>
> I think `WithArc` would be confusing, since `Arc` is the smartpointer
> that points to a `WithRef`.

I find `WithArc` confusing as well: is it a `T` with an `Arc`?. And I
agree that with Martin's point that it being in the `arc` module
should (although feebly) establish the relationship with `Arc`.

I'm not married to the `WithRef` name, but so far it's been the best
option I've seen.
  

On Fri, 22 Sept 2023 at 21:13, Gary Guo <gary@garyguo.net> wrote:
>
> On Thu, 21 Sep 2023 18:34:40 -0300
> Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>
> > From: Wedson Almeida Filho <walmeida@microsoft.com>
> >
> > 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<T>` instead of `ArcBorrow<'_, T>`.
> >
> > Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
>
> Reviewed-by: Gary Guo <gary@garyguo.net>
>
> The implementation looks good to me, thanks Wedson.
>
> A minor thing that worth considering is to implement `AlwaysRefCounted`
> to `WithRef` and reimplement `Arc` with `ARef<TaskRef<T>>` or add
> conversion functions between them.
>
> It feels natural to have this this impl (because `WithRef` is indeed
> always ref counted), but on the other hand I don't currently foresee
> anyone to currently want to use this API :(

Yes, I like the idea of defining `Arc<T>` as `ARef<WithRef<T>>`. My
concern had to do with error messages for type aliases but you or
Benno (or Bjorn?) had a link to plans from the compiler team to
improve this. And we're using type aliases for our locks anyway.

So this change is in my queue. (As well as the changes to the guard we
discussed a couple of months ago.)

Thanks,
-Wedson
  

On Sat, 23 Sept 2023 at 02:17, Jianguo Bao <roidinev@gmail.com> wrote:
>
> On Thu, Sep 21, 2023 at 06:34:40PM -0300, Wedson Almeida Filho wrote:
> > From: Wedson Almeida Filho <walmeida@microsoft.com>
> >
> > 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<T>` instead of `ArcBorrow<'_, T>`.
> >
> > Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> > ---
> >  rust/kernel/sync.rs     |   2 +-
> >  rust/kernel/sync/arc.rs | 180 ++++++++++++++--------------------------
> >  2 files changed, 62 insertions(+), 120 deletions(-)
> >
> > 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;
> >
> > -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<Example>` to `Arc<dyn MyTrait>`:
> >  ///
> >  /// ```
> > -/// use kernel::sync::{Arc, ArcBorrow};
> > +/// use kernel::sync::{Arc, WithRef};
> >  ///
> >  /// trait MyTrait {
> >  ///     // Trait has a function whose `self` type is `Arc<Self>`.
> >  ///     fn example1(self: Arc<Self>) {}
> >  ///
> > -///     // Trait has a function whose `self` type is `ArcBorrow<'_, Self>`.
> > -///     fn example2(self: ArcBorrow<'_, Self>) {}
> > +///     // Trait has a function whose `self` type is `&WithRef<Self>`.
> > +///     fn example2(self: &WithRef<Self>) {}
> >  /// }
> >  ///
> >  /// struct Example;
> > @@ -130,9 +130,48 @@ pub struct Arc<T: ?Sized> {
> >      _p: PhantomData<WithRef<T>>,
> >  }
> >
> > +/// An instance of `T` with an attached reference count.
> > +///
> > +/// # Examples
> > +///
> > +/// ```
> > +/// use kernel::sync::{Arc, WithRef};
> > +///
> > +/// struct Example;
> > +///
> > +/// fn do_something(e: &WithRef<Example>) -> Arc<Example> {
> > +///     e.into()
> > +/// }
> > +///
> > +/// let obj = Arc::try_new(Example)?;
> > +/// let cloned = 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<T>` as the type of `self`:
> > +///
> > +/// ```
> > +/// use kernel::sync::{Arc, WithRef};
> > +///
> > +/// struct Example {
> > +///     _a: u32,
> > +///     _b: u32,
> > +/// }
> > +///
> > +/// impl Example {
> > +///     fn use_reference(self: &WithRef<Self>) {
> > +///         // ...
> > +///     }
> > +/// }
> > +///
> > +/// let obj = Arc::try_new(Example { _a: 10, _b: 20 })?;
> > +/// obj.as_with_ref().use_reference();
> > +/// ```
> >  #[pin_data]
> >  #[repr(C)]
> > -struct WithRef<T: ?Sized> {
> > +pub struct WithRef<T: ?Sized> {
> >      refcount: Opaque<bindings::refcount_t>,
> >      data: T,
> >  }
> > @@ -215,16 +254,16 @@ unsafe fn from_inner(inner: NonNull<WithRef<T>>) -> Self {
> >          }
> >      }
> >
> > -    /// Returns an [`ArcBorrow`] from the given [`Arc`].
> > +    /// Returns a [`WithRef`] from the given [`Arc`].
>
>        /// Returns a shared reference of [`WithRef`] from the given [`Arc`].

Thanks Jianguo.. I'll make this change in v2.

> >      ///
> > -    /// 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.
> > +    /// This is useful when the argument of a function call is a [`WithRef`] (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<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
> > +        // 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() }
> >      }
> >
> >      /// Compare whether two [`Arc`] pointers reference the same underlying object.
> > @@ -234,20 +273,17 @@ pub fn ptr_eq(this: &Self, other: &Self) -> bool {
> >  }
> >
> >  impl<T: 'static> ForeignOwnable for Arc<T> {
> > -    type Borrowed<'a> = ArcBorrow<'a, T>;
> > +    type Borrowed<'a> = &'a WithRef<T>;
> >
> >      fn into_foreign(self) -> *const core::ffi::c_void {
> >          ManuallyDrop::new(self).ptr.as_ptr() as _
> >      }
> >
> > -    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<T> {
> >          // SAFETY: By the safety requirement of this function, we know that `ptr` came from
> > -        // a previous call to `Arc::into_foreign`.
> > -        let inner = NonNull::new(ptr as *mut WithRef<T>).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 requirements of `from_foreign` ensure
> > +        // that the object remains alive for the lifetime of the returned value.
> > +        unsafe { &*(ptr.cast::<WithRef<T>>()) }
> >      }
> >
> >      unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
> > @@ -320,119 +356,25 @@ fn from(item: Pin<UniqueArc<T>>) -> Self {
> >      }
> >  }
> >
> > -/// 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 kernel::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));
> > -/// # Ok::<(), Error>(())
> > -/// ```
> > -///
> > -/// Using `ArcBorrow<T>` 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 = 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<WithRef<T>>,
> > -    _p: PhantomData<&'a ()>,
> > -}
> > -
> > -// 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<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<WithRef<T>>) -> Self {
> > -        // INVARIANT: The safety requirements guarantee the invariants.
> > -        Self {
> > -            inner,
> > -            _p: PhantomData,
> > -        }
> > -    }
> > -}
> > +// This is to allow [`WithRef`] (and variants) to be used as the type of `self`.
> > +impl<T: ?Sized> core::ops::Receiver for WithRef<T> {}
> >
> > -impl<T: ?Sized> From<ArcBorrow<'_, T>> for Arc<T> {
> > -    fn from(b: ArcBorrow<'_, T>) -> Self {
> > +impl<T: ?Sized> From<&WithRef<T>> for Arc<T> {
> > +    fn from(b: &WithRef<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.into()) })
> >              .deref()
> >              .clone()
> >      }
> >  }
> >
> > -impl<T: ?Sized> Deref for ArcBorrow<'_, T> {
> > +impl<T: ?Sized> Deref for WithRef<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.data
> >      }
> >  }
> >
> > --
> > 2.34.1
> >
  

On Sat, Sep 23, 2023 at 4:11 PM Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>
> On Fri, 22 Sept 2023 at 21:13, Gary Guo <gary@garyguo.net> wrote:
> >
> > On Thu, 21 Sep 2023 18:34:40 -0300
> > Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
> >
> > > From: Wedson Almeida Filho <walmeida@microsoft.com>
> > >
> > > 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<T>` instead of `ArcBorrow<'_, T>`.
> > >
> > > Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > > Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> >
> > Reviewed-by: Gary Guo <gary@garyguo.net>
> >
> > The implementation looks good to me, thanks Wedson.
> >
> > A minor thing that worth considering is to implement `AlwaysRefCounted`
> > to `WithRef` and reimplement `Arc` with `ARef<TaskRef<T>>` or add
> > conversion functions between them.
> >
> > It feels natural to have this this impl (because `WithRef` is indeed
> > always ref counted), but on the other hand I don't currently foresee
> > anyone to currently want to use this API :(
>
> Yes, I like the idea of defining `Arc<T>` as `ARef<WithRef<T>>`. My
> concern had to do with error messages for type aliases but you or
> Benno (or Bjorn?) had a link to plans from the compiler team to
> improve this. And we're using type aliases for our locks anyway.
>
> So this change is in my queue. (As well as the changes to the guard we
> discussed a couple of months ago.)

Will this not cause issues with DispatchFromDyn and the Receiver
trait? Maybe it's best to have Arc be a real type and just provide a
conversion?

Alice
  

On Sat, 23 Sept 2023 at 11:15, Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Sat, Sep 23, 2023 at 4:11 PM Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
> >
> > On Fri, 22 Sept 2023 at 21:13, Gary Guo <gary@garyguo.net> wrote:
> > >
> > > On Thu, 21 Sep 2023 18:34:40 -0300
> > > Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
> > >
> > > > From: Wedson Almeida Filho <walmeida@microsoft.com>
> > > >
> > > > 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<T>` instead of `ArcBorrow<'_, T>`.
> > > >
> > > > Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> > > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > > > Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> > >
> > > Reviewed-by: Gary Guo <gary@garyguo.net>
> > >
> > > The implementation looks good to me, thanks Wedson.
> > >
> > > A minor thing that worth considering is to implement `AlwaysRefCounted`
> > > to `WithRef` and reimplement `Arc` with `ARef<TaskRef<T>>` or add
> > > conversion functions between them.
> > >
> > > It feels natural to have this this impl (because `WithRef` is indeed
> > > always ref counted), but on the other hand I don't currently foresee
> > > anyone to currently want to use this API :(
> >
> > Yes, I like the idea of defining `Arc<T>` as `ARef<WithRef<T>>`. My
> > concern had to do with error messages for type aliases but you or
> > Benno (or Bjorn?) had a link to plans from the compiler team to
> > improve this. And we're using type aliases for our locks anyway.
> >
> > So this change is in my queue. (As well as the changes to the guard we
> > discussed a couple of months ago.)
>
> Will this not cause issues with DispatchFromDyn and the Receiver
> trait? Maybe it's best to have Arc be a real type and just provide a
> conversion?

I don't think it will but I haven't really tried it yet so can't say for sure.

One thing I learned (obvious in hindsight) when doing the locks is
that we can have impl blocks for specialisations of a type, so
`DispatchFromDyn<Arc<T>>` will expand to
`DispatchFromDyn<ARef<WithRef<T>>>` and it should be ok.

Or are you worried about something else?