Re: [PATCH v4 3/5] rust: convert `Arc` to use `Refcount`

["Benno Lossin" <lossin@kernel.org>]

On Sun Jun 22, 2025 at 2:57 PM CEST, Gary Guo wrote:
> @@ -428,14 +422,10 @@ fn as_ref(&self) -> &T {
>  
>  impl<T: ?Sized> Clone for Arc<T> {
>      fn clone(&self) -> Self {
> -        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> -        // safe to dereference it.
> -        let refcount = unsafe { self.ptr.as_ref() }.refcount.get();
> -
> -        // INVARIANT: C `refcount_inc` saturates the refcount, so it cannot overflow to zero.
> +        // INVARIANT: `Refcount` saturates the refcount, so it cannot overflow to zero.
>          // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
>          // safe to increment the refcount.
> -        unsafe { bindings::refcount_inc(refcount) };
> +        unsafe { self.ptr.as_ref().refcount.inc() };

The `.refcount.inc()` can be outside of the `unsafe` block.

>  
>          // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
>          unsafe { Self::from_inner(self.ptr) }
> @@ -444,16 +434,10 @@ fn clone(&self) -> Self {
>  
>  impl<T: ?Sized> Drop for Arc<T> {
>      fn drop(&mut self) {
> -        // SAFETY: By the type invariant, there is necessarily a reference to the object. We cannot
> -        // touch `refcount` after it's decremented to a non-zero value because another thread/CPU
> -        // may concurrently decrement it to zero and free it. It is ok to have a raw pointer to
> -        // freed/invalid memory as long as it is never dereferenced.
> -        let refcount = unsafe { self.ptr.as_ref() }.refcount.get();
> -
>          // INVARIANT: If the refcount reaches zero, there are no other instances of `Arc`, and
>          // this instance is being dropped, so the broken invariant is not observable.
> -        // SAFETY: Also by the type invariant, we are allowed to decrement the refcount.
> -        let is_zero = unsafe { bindings::refcount_dec_and_test(refcount) };
> +        // SAFETY: By the type invariant, there is necessarily a reference to the object.
> +        let is_zero = unsafe { self.ptr.as_ref().refcount.dec_and_test() };

Ditto.

>          if is_zero {
>              // The count reached zero, we must free the memory.
>              //
> @@ -747,8 +731,7 @@ pub fn new_uninit(flags: Flags) -> Result<UniqueArc<MaybeUninit<T>>, AllocError>
>          // INVARIANT: The refcount is initialised to a non-zero value.
>          let inner = KBox::try_init::<AllocError>(
>              try_init!(ArcInner {
> -                // SAFETY: There are no safety requirements for this FFI call.
> -                refcount: Opaque::new(unsafe { bindings::REFCOUNT_INIT(1) }),
> +                refcount: Refcount::new(1),
>                  data <- pin_init::uninit::<T, AllocError>(),
>              }? AllocError),
>              flags,
> diff --git a/rust/kernel/sync/refcount.rs b/rust/kernel/sync/refcount.rs
> index a0fc22f6d645..8e7b9b0c1979 100644
> --- a/rust/kernel/sync/refcount.rs
> +++ b/rust/kernel/sync/refcount.rs
> @@ -71,6 +71,14 @@ pub fn dec(&self) {
>      /// must come after.
>      ///
>      /// Returns true if the resulting refcount is 0, false otherwise.
> +    ///
> +    /// # Notes
> +    ///
> +    /// A common pattern of using `Refcount` is to free memory when the reference count reaches
> +    /// zero. This means that the reference to `Refcount` could become invalid after calling this
> +    /// function. This is fine as long as the reference to `Refcount` is no longer used when this
> +    /// function returns `false`. It is not necessary to use raw pointers in this scenario, see
> +    /// https://github.com/rust-lang/rust/issues/55005.

This should be in patch 1?

---
Cheers,
Benno

>      #[inline]
>      #[must_use = "use `dec` instead if you do not need to test if it is 0"]
>      pub fn dec_and_test(&self) -> bool {