Re: [PATCH v11 2/4] rust: Split `AlwaysRefCounted` into two traits

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

On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index c12ff4d2a3f2d79b760c34c0b84a51b507d0cfb1..40c0138bd336057e7d3a835a9e81391baa2fd2b1 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -418,11 +418,9 @@ pub const fn raw_get(this: *const Self) -> *mut T {
>      }
>  }
>  
> -/// Types that are _always_ reference counted.
> +/// Types that are internally reference counted.
>  ///
>  /// It allows such types to define their own custom ref increment and decrement functions.
> -/// Additionally, it allows users to convert from a shared reference `&T` to an owned reference
> -/// [`ARef<T>`].
>  ///
>  /// This is usually implemented by wrappers to existing structures on the C side of the code. For
>  /// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
> @@ -438,9 +436,8 @@ pub const fn raw_get(this: *const Self) -> *mut T {
>  /// at least until matching decrements are performed.
>  ///
>  /// Implementers must also ensure that all instances are reference-counted. (Otherwise they
> -/// won't be able to honour the requirement that [`AlwaysRefCounted::inc_ref`] keep the object
> -/// alive.)
> -pub unsafe trait AlwaysRefCounted {
> +/// won't be able to honour the requirement that [`RefCounted::inc_ref`] keep the object alive.)
> +pub unsafe trait RefCounted {
>      /// Increments the reference count on the object.
>      fn inc_ref(&self);

This seems a bit problematic for `Owned`, since now I can do:

    fn bad<T: Ownable + RefCounted>(t: &Owned<T>) {
        t.inc_ref();
    }

And now the `Owned<T>` is no longer "unique" in the sense that the
refcount is 1...

Similarly, we should probably make this an associated function, such
that people don't accidentally call `.inc_ref()` on `ARef<T>`.

> @@ -453,11 +450,21 @@ pub unsafe trait AlwaysRefCounted {
>      /// Callers must ensure that there was a previous matching increment to the reference count,
>      /// and that the object is no longer used after its reference count is decremented (as it may
>      /// result in the object being freed), unless the caller owns another increment on the refcount
> -    /// (e.g., it calls [`AlwaysRefCounted::inc_ref`] twice, then calls
> -    /// [`AlwaysRefCounted::dec_ref`] once).
> +    /// (e.g., it calls [`RefCounted::inc_ref`] twice, then calls [`RefCounted::dec_ref`] once).
>      unsafe fn dec_ref(obj: NonNull<Self>);
>  }
>  
> +/// An extension to RefCounted, which declares that it is allowed to convert from a shared reference
> +/// `&T` to an owned reference [`ARef<T>`].

This is a bit too long for the first sentence... How about

    Always reference counted type.

    Allows the creation of `ARef<T>` from `&T`.

Feel free to add more information.

> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that no safety invariants are violated by upgrading an `&T` to an
> +/// [`ARef<T>`]. In particular that implies [`AlwaysRefCounted`] and [`Ownable`] cannot be
> +/// implemented for the same type, as this would allow to violate the uniqueness guarantee of
> +/// [`Owned<T>`] by derefencing it into an `&T` and obtaining an [`ARef`] from that.
> +pub unsafe trait AlwaysRefCounted: RefCounted {}

It's a bit sad that we can't just say `: !Ownable` (or rather a
blanket-implemented marker trait, since that might land earlier). Anyone
aware of progress in this area?

---
Cheers,
Benno

> +
>  /// An owned reference to an always-reference-counted object.
>  ///
>  /// The object's reference count is automatically decremented when an instance of [`ARef`] is