[PATCH 0/2] Remove `ArcBorrow`

[PATCH 0/2] Remove `ArcBorrow`

[Wedson Almeida Filho]

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

In this small series we remove `ArcBorrow<'_, T>` and replace it with
`&WithRef<T>`, which we used to call `ArcInner` --  it is renamed
because it is now public, so we chose a more meaningful name.

This simplifies the code because we remove a type and have simpler
syntax to refer to borrowed arcs: `ArcBorrow<'a, T>` vs 
`&'a WithRef<T>`.

This became possible when we adopted GATs in 2021 but we only realised
it recently (thanks Boqun!), more details on this zulip thread:
https://rust-for-linux.zulipchat.com/#narrow/stream/291566-Library/topic/Isn't.20.60ArcBorrow.60.20just.20.60.26ArcInner.3CT.3E.60.3F

Wedson Almeida Filho (2):
  rust: arc: rename `ArcInner` to `WithRef`
  rust: arc: remove `ArcBorrow` in favour of `WithRef`

 rust/kernel/sync.rs                |   2 +-
 rust/kernel/sync/arc.rs            | 192 ++++++++++-------------------
 rust/kernel/sync/arc/std_vendor.rs |   4 +-
 3 files changed, 70 insertions(+), 128 deletions(-)


base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
-- 
2.34.1

[PATCH 1/2] rust: arc: rename `ArcInner` to `WithRef`

[Wedson Almeida Filho]

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

This is in preparation for removing `ArcBorrow` and making `WithRef`
public.

This is a pure name change with no functional changes intended.

Suggested-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
---
 rust/kernel/sync/arc.rs            | 20 ++++++++++----------
 rust/kernel/sync/arc/std_vendor.rs |  4 ++--
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 3d496391a9bd..86bff1e0002c 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -126,13 +126,13 @@
 /// # Ok::<(), Error>(())
 /// ```
 pub struct Arc<T: ?Sized> {
-    ptr: NonNull<ArcInner<T>>,
-    _p: PhantomData<ArcInner<T>>,
+    ptr: NonNull<WithRef<T>>,
+    _p: PhantomData<WithRef<T>>,
 }
 
 #[pin_data]
 #[repr(C)]
-struct ArcInner<T: ?Sized> {
+struct WithRef<T: ?Sized> {
     refcount: Opaque<bindings::refcount_t>,
     data: T,
 }
@@ -164,7 +164,7 @@ impl<T> Arc<T> {
     /// Constructs a new reference counted instance of `T`.
     pub fn try_new(contents: T) -> Result<Self, AllocError> {
         // INVARIANT: The refcount is initialised to a non-zero value.
-        let value = ArcInner {
+        let value = WithRef {
             // SAFETY: There are no safety requirements for this FFI call.
             refcount: Opaque::new(unsafe { bindings::REFCOUNT_INIT(1) }),
             data: contents,
@@ -201,13 +201,13 @@ pub fn init<E>(init: impl Init<T, E>) -> error::Result<Self>
 }
 
 impl<T: ?Sized> Arc<T> {
-    /// Constructs a new [`Arc`] from an existing [`ArcInner`].
+    /// Constructs a new [`Arc`] from an existing [`WithRef`].
     ///
     /// # Safety
     ///
     /// The caller must ensure that `inner` points to a valid location and has a non-zero reference
     /// count, one of which will be owned by the new [`Arc`] instance.
-    unsafe fn from_inner(inner: NonNull<ArcInner<T>>) -> Self {
+    unsafe fn from_inner(inner: NonNull<WithRef<T>>) -> Self {
         // INVARIANT: By the safety requirements, the invariants hold.
         Arc {
             ptr: inner,
@@ -243,7 +243,7 @@ fn into_foreign(self) -> *const core::ffi::c_void {
     unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, 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 ArcInner<T>).unwrap();
+        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.
@@ -376,7 +376,7 @@ fn from(item: Pin<UniqueArc<T>>) -> Self {
 /// # Ok::<(), Error>(())
 /// ```
 pub struct ArcBorrow<'a, T: ?Sized + 'a> {
-    inner: NonNull<ArcInner<T>>,
+    inner: NonNull<WithRef<T>>,
     _p: PhantomData<&'a ()>,
 }
 
@@ -406,7 +406,7 @@ impl<T: ?Sized> ArcBorrow<'_, T> {
     /// 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 {
+    unsafe fn new(inner: NonNull<WithRef<T>>) -> Self {
         // INVARIANT: The safety requirements guarantee the invariants.
         Self {
             inner,
@@ -526,7 +526,7 @@ pub fn try_new(value: T) -> Result<Self, AllocError> {
     /// Tries to allocate a new [`UniqueArc`] instance whose contents are not initialised yet.
     pub fn try_new_uninit() -> Result<UniqueArc<MaybeUninit<T>>, AllocError> {
         // INVARIANT: The refcount is initialised to a non-zero value.
-        let inner = Box::try_init::<AllocError>(try_init!(ArcInner {
+        let inner = Box::try_init::<AllocError>(try_init!(WithRef {
             // SAFETY: There are no safety requirements for this FFI call.
             refcount: Opaque::new(unsafe { bindings::REFCOUNT_INIT(1) }),
             data <- init::uninit::<T, AllocError>(),
diff --git a/rust/kernel/sync/arc/std_vendor.rs b/rust/kernel/sync/arc/std_vendor.rs
index a66a0c2831b3..4b30e5597ba5 100644
--- a/rust/kernel/sync/arc/std_vendor.rs
+++ b/rust/kernel/sync/arc/std_vendor.rs
@@ -5,7 +5,7 @@
 //! "Apache-2.0 OR MIT" and adapted for kernel use. For copyright details,
 //! see <https://github.com/rust-lang/rust/blob/master/COPYRIGHT>.
 
-use crate::sync::{arc::ArcInner, Arc};
+use crate::sync::{arc::WithRef, Arc};
 use core::any::Any;
 
 impl Arc<dyn Any + Send + Sync> {
@@ -17,7 +17,7 @@ pub fn downcast<T>(self) -> core::result::Result<Arc<T>, Self>
         if (*self).is::<T>() {
             // SAFETY: We have just checked that the type is correct, so we can cast the pointer.
             unsafe {
-                let ptr = self.ptr.cast::<ArcInner<T>>();
+                let ptr = self.ptr.cast::<WithRef<T>>();
                 core::mem::forget(self);
                 Ok(Arc::from_inner(ptr))
             }
-- 
2.34.1

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

[Wedson Almeida Filho]

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`].
     ///
-    /// 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

Re: [PATCH 1/2] rust: arc: rename `ArcInner` to `WithRef`

[Finn Behrens]

On 21 Sep 2023, at 23:34, Wedson Almeida Filho wrote:

> From: Wedson Almeida Filho <walmeida@microsoft.com>
>
> This is in preparation for removing `ArcBorrow` and making `WithRef`
> public.
>
> This is a pure name change with no functional changes intended.
>
> Suggested-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---

Reviewed-by: Finn Behrens <me@kloenk.dev>

Re: [PATCH 1/2] rust: arc: rename `ArcInner` to `WithRef`

[Benno Lossin]

On 21.09.23 23:34, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> This is in preparation for removing `ArcBorrow` and making `WithRef`
> public.
> 
> This is a pure name change with no functional changes intended.
> 
> Suggested-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>

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

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

[Benno Lossin]

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
> 

Re: [PATCH 1/2] rust: arc: rename `ArcInner` to `WithRef`

[Alice Ryhl]

On Thu, Sep 21, 2023 at 11:35 PM Wedson Almeida Filho
<wedsonaf@gmail.com> wrote:
>
> From: Wedson Almeida Filho <walmeida@microsoft.com>
>
> This is in preparation for removing `ArcBorrow` and making `WithRef`
> public.
>
> This is a pure name change with no functional changes intended.
>
> Suggested-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>

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

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

[Alice Ryhl]

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>

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

[Benno Lossin]

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

-- 
Cheers,
Benno

Re: [PATCH 1/2] rust: arc: rename `ArcInner` to `WithRef`

[Martin Rodriguez Reboredo]

On 9/21/23 18:34, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> This is in preparation for removing `ArcBorrow` and making `WithRef`
> public.
> 
> This is a pure name change with no functional changes intended.
> 
> Suggested-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
> [...]

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

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

[Martin Rodriguez Reboredo]

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>

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

[Martin Rodriguez Reboredo]

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.

Re: [PATCH 1/2] rust: arc: rename `ArcInner` to `WithRef`

[Gary Guo]

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

> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> This is in preparation for removing `ArcBorrow` and making `WithRef`
> public.
> 
> This is a pure name change with no functional changes intended.
> 
> Suggested-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>

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

> ---
>  rust/kernel/sync/arc.rs            | 20 ++++++++++----------
>  rust/kernel/sync/arc/std_vendor.rs |  4 ++--
>  2 files changed, 12 insertions(+), 12 deletions(-)

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

[Gary Guo]

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

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

[Gary Guo]

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

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

[Jianguo Bao]

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
> 

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

[Wedson Almeida Filho]

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

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

[Wedson Almeida Filho]

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.

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

[Wedson Almeida Filho]

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

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

[Wedson Almeida Filho]

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

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

[Alice Ryhl]

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

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

[Wedson Almeida Filho]

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?