Message-ID: <67dd95be.050a0220.ff22e.716f@mx.google.com>
Date: Fri, 21 Mar 2025 09:37:16 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Oliver Mangold <oliver.mangold@...me>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
	Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Asahi Lina <lina@...hilina.net>, rust-for-linux@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 4/4] rust: adding OwnableRefCounted and
 SimpleOwnableRefCounted

On Thu, Mar 13, 2025 at 07:00:26AM +0000, Oliver Mangold wrote:
> Types implementing one of these traits can safely convert between an
> ARef<T> and an Owned<T>.
> 

Again, you need to mention why we need this.

> Signed-off-by: Oliver Mangold <oliver.mangold@...me>
> ---
>  rust/kernel/types.rs | 250 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 250 insertions(+)
> 
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 5a96da714348cc2369969200e6070972226c00fe..cca3d65782eecc7e2ada91f116eb885391c454bf 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -552,6 +552,12 @@ fn from(b: &T) -> Self {
>      }
>  }
>  
> +impl<T: OwnableRefCounted> From<Owned<T>> for ARef<T> {
> +    fn from(b: Owned<T>) -> Self {
> +        T::into_shared(b)
> +    }
> +}
> +
>  impl<T: RefCounted> Drop for ARef<T> {
>      fn drop(&mut self) {
>          // SAFETY: The type invariants guarantee that the `ARef` owns the reference
> @@ -669,6 +675,250 @@ fn drop(&mut self) {
>      }
>  }
>  
> +/// A trait for objects that can be wrapped in either one of the reference types [`Owned`] and
> +/// [`ARef`].
> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that:
> +///
> +/// - Both the safety requirements for [`Ownable`] and [`RefCounted`] are fulfilled.
> +/// - The uniqueness invariant of [`Owned`] is upheld until dropped.

Could you explain what this safety requirement means? Isn't this part of
the safe requirement of `impl Ownable`?

> +/// - [`try_from_shared()`](OwnableRefCounted::into_shared) only returns an [`Owned`] if exactly
> +///   one [`ARef`] exists.
> +/// - [`into_shared()`](OwnableRefCounted::into_shared) set the reference count to the value which
> +///   the returned [`ARef`] expects for an object with a single reference
> +///   in existence. This implies that if [`into_shared()`](OwnableRefCounted::into_shared) is left
> +///   on the default implementation, which just rewraps the underlying object, the reference count
> +///   needs not to be modified when converting a [`Owned`] to an [`ARef`].
> +///
> +/// # Examples
> +///
> +/// A minimal example implementation of [`OwnableRefCounted`], [`Ownable`] and its usage with
> +/// [`ARef`] and [`Owned`] looks like this:
> +///
> +/// ```
> +/// # #![expect(clippy::disallowed_names)]
> +/// use core::cell::Cell;
> +/// use core::ptr::NonNull;
> +/// use kernel::alloc::{flags, kbox::KBox, AllocError};
> +/// use kernel::types::{
> +///     ARef, RefCounted, Owned, Ownable, OwnableRefCounted,
> +/// };
> +///
> +/// struct Foo {
> +///     refcount: Cell<usize>,

It's fine to use a Cell for now, but eventually we want to replace this
with either Gary's Refcount [1] or LKMM atomics.

[1]: https://lore.kernel.org/rust-for-linux/20250219201602.1898383-1-gary@garyguo.net/

(just keeping a note here)

> +/// }
> +///
> +/// impl Foo {
> +///     fn new() -> Result<Owned<Self>, AllocError> {
> +///         // Use a `KBox` to handle the actual allocation.
> +///         let result = KBox::new(
> +///             Foo {
> +///                 refcount: Cell::new(1),
> +///             },
> +///             flags::GFP_KERNEL,
> +///         )?;
> +///         let result = NonNull::new(KBox::into_raw(result))
> +///             .expect("Raw pointer to newly allocation KBox is null, this should never happen.");
> +///         // SAFETY: We just allocated the `Foo`, thus it is valid.
> +///         Ok(unsafe { Owned::from_raw(result) })
> +///     }
> +/// }
> +///
> +/// // SAFETY: We increment and decrement each time the respective function is called and only free
> +/// // the `Foo` when the refcount reaches zero.
> +/// unsafe impl RefCounted for Foo {
> +///     fn inc_ref(&self) {
> +///         self.refcount.replace(self.refcount.get() + 1);
> +///     }
> +///
> +///     unsafe fn dec_ref(this: NonNull<Self>) {
> +///         // SAFETY: The underlying object is always valid when the function is called.
> +///         let refcount = unsafe { &this.as_ref().refcount };
> +///         let new_refcount = refcount.get() - 1;
> +///         if new_refcount == 0 {
> +///             // The `Foo` will be dropped when `KBox` goes out of scope.
> +///             // SAFETY: The `Box<Foo>` is still alive as the old refcount is 1.
> +///             unsafe { KBox::from_raw(this.as_ptr()) };
> +///         } else {
> +///             refcount.replace(new_refcount);
> +///         }
> +///     }
> +/// }
> +///
> +/// // SAFETY: We only convert into an `Owned` when the refcount is 1.
> +/// unsafe impl OwnableRefCounted for Foo {
> +///     fn try_from_shared(this: ARef<Self>) -> Result<Owned<Self>, ARef<Self>> {
> +///         if this.refcount.get() == 1 {
> +///             // SAFETY: The `Foo` is still alive as the refcount is 1.
> +///             Ok(unsafe { Owned::from_raw(ARef::into_raw(this)) })
> +///         } else {
> +///             Err(this)
> +///         }
> +///     }
> +/// }
> +///
> +/// // SAFETY: We are not `AlwaysRefCounted`.
> +/// unsafe impl Ownable for Foo {
> +///     unsafe fn release(this: NonNull<Self>) {
> +///         // SAFETY: Using `dec_ref()` from `RefCounted` to release is okay, as the refcount is
> +///         // always 1 for an `Owned<Foo>`.
> +///         unsafe{ Foo::dec_ref(this) };
> +///     }
> +/// }
> +///
> +/// let foo = Foo::new().unwrap();
> +/// let mut foo = ARef::from(foo);
> +/// {
> +///     let bar = foo.clone();
> +///     assert!(Owned::try_from(bar).is_err());
> +/// }
> +/// assert!(Owned::try_from(foo).is_ok());
> +/// ```
> +pub unsafe trait OwnableRefCounted: RefCounted + Ownable + Sized {
> +    /// Checks if the [`ARef`] is unique and convert it to an [`Owned`] it that is that case.
> +    /// Otherwise it returns again an [`ARef`] to the same underlying object.
> +    fn try_from_shared(this: ARef<Self>) -> Result<Owned<Self>, ARef<Self>>;
> +    /// Converts the [`Owned`] into an [`ARef`].
> +

^ this blanket line seems to be at the wrong place.

Regards,
Boqun

> +    fn into_shared(this: Owned<Self>) -> ARef<Self> {
> +        // SAFETY: Safe by the requirements on implementing the trait.
> +        unsafe { ARef::from_raw(Owned::into_raw(this)) }
> +    }
> +}
> +
> +/// This trait allows to implement [`Ownable`] and [`OwnableRefCounted`] together in a simplified
> +/// way, only requiring to implement [`RefCounted`] and providing the method
> +/// [`is_unique()`](SimpleOwnableRefCounted::is_unique).
> +///
> +/// For non-standard cases where conversion between [`Ownable`] and [`RefCounted`] does not allow
> +/// [`Ownable::release()`] and [`RefCounted::dec_ref()`] to be the same method, [`Ownable`]
> +/// and [`OwnableRefCounted`] should be implemented separately.
> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that:
> +///
> +/// - The safety requirements for [`Ownable`] are fulfilled and [`RefCounted::dec_ref()`] can
> +///   be used for [`Ownable::release()`].
> +/// - [`is_unique`](SimpleOwnableRefCounted::is_unique) must only return `true` in case only one
> +///   [`ARef`] exists and it is impossible for one to be obtained other than by cloning an existing
> +///   [`ARef`] or converting an [`Owned`] to an [`ARef`].
> +/// - It is safe to convert an unique [`ARef`] into an [`Owned`] simply by re-wrapping the
> +///   underlying object without modifying the refcount.
> +///
> +/// # Examples
> +///
> +/// A minimal example implementation of [`RefCounted`] and [`SimpleOwnableRefCounted`]
> +/// and its usage with [`ARef`] and [`Owned`] looks like this:
> +///
> +/// ```
> +/// # #![expect(clippy::disallowed_names)]
> +/// use core::cell::Cell;
> +/// use core::ptr::NonNull;
> +/// use kernel::alloc::{flags, kbox::KBox, AllocError};
> +/// use kernel::types::{
> +///     ARef, Owned, RefCounted, SimpleOwnableRefCounted,
> +/// };
> +///
> +/// struct Foo {
> +///     refcount: Cell<usize>,
> +/// }
> +///
> +/// impl Foo {
> +///     fn new() -> Result<Owned<Self>, AllocError> {
> +///         // Use a KBox to handle the actual allocation.
> +///         let result = KBox::new(
> +///             Foo {
> +///                 refcount: Cell::new(1),
> +///             },
> +///             flags::GFP_KERNEL,
> +///         )?;
> +///         let result = NonNull::new(KBox::into_raw(result))
> +///             .expect("Raw pointer to newly allocation KBox is null, this should never happen.");
> +///         // SAFETY: We just allocated the `Foo`, thus it is valid.
> +///         Ok(unsafe { Owned::from_raw(result) })
> +///     }
> +/// }
> +///
> +/// // SAFETY: we ensure that:
> +/// // - The `Foo` is only dropped when the refcount is zero.
> +/// // - `is_unique()` only returns `true` when the refcount is 1.
> +/// unsafe impl RefCounted for Foo {
> +///     fn inc_ref(&self) {
> +///         self.refcount.replace(self.refcount.get() + 1);
> +///     }
> +///
> +///     unsafe fn dec_ref(this: NonNull<Self>) {
> +///         // SAFETY: The underlying object is always valid when the function is called.
> +///         let refcount = unsafe { &this.as_ref().refcount };
> +///         let new_refcount = refcount.get() - 1;
> +///         if new_refcount == 0 {
> +///             // The `Foo` will be dropped when KBox goes out of scope.
> +///             // SAFETY: The `Box<Foo>` is still alive as the old refcount is 1.
> +///             unsafe { KBox::from_raw(this.as_ptr()) };
> +///         } else {
> +///             refcount.replace(new_refcount);
> +///         }
> +///     }
> +/// }
> +///
> +/// // SAFETY: we ensure that:
> +/// // - `is_unique()` only returns `true` when the refcount is 1.
> +/// unsafe impl SimpleOwnableRefCounted for Foo {
> +///     fn is_unique(&self) -> bool {
> +///         self.refcount.get() == 1
> +///     }
> +/// }
> +///
> +/// let foo = Foo::new().unwrap();
> +/// let mut foo = ARef::from(foo);
> +/// {
> +///     let bar = foo.clone();
> +///     assert!(Owned::try_from(bar).is_err());
> +/// }
> +/// assert!(Owned::try_from(foo).is_ok());
> +/// ```
> +pub unsafe trait SimpleOwnableRefCounted: RefCounted {
> +    /// Checks if exactly one [`ARef`] to the object exists. In case the object is [`Sync`], the
> +    /// check needs to be race-free.
> +    fn is_unique(&self) -> bool;
> +}
> +
> +#[cfg_attr(RUSTC_HAS_DO_NOT_RECOMMEND, diagnostic::do_not_recommend)]
> +// SAFETY: Safe by the requirements on implementation of [`SimpleOwnableRefCounted`].
> +unsafe impl<T: SimpleOwnableRefCounted> OwnableRefCounted for T {
> +    fn try_from_shared(this: ARef<Self>) -> Result<Owned<Self>, ARef<Self>> {
> +        if T::is_unique(&*this) {
> +            // SAFETY: Safe by the requirements on implementation of [`SimpleOwnable`].
> +            Ok(unsafe { Owned::from_raw(ARef::into_raw(this)) })
> +        } else {
> +            Err(this)
> +        }
> +    }
> +}
> +
> +#[cfg_attr(RUSTC_HAS_DO_NOT_RECOMMEND, diagnostic::do_not_recommend)]
> +// SAFETY: Safe by the requirements on implementation of [`SimpleOwnableRefCounted`].
> +unsafe impl<T: SimpleOwnableRefCounted> Ownable for T {
> +    unsafe fn release(this: NonNull<Self>) {
> +        // SAFETY: Safe by the requirements on implementation of
> +        // [`SimpleOwnableRefCounted::dec_ref()`].
> +        unsafe { RefCounted::dec_ref(this) };
> +    }
> +}
> +
> +impl<T: OwnableRefCounted> TryFrom<ARef<T>> for Owned<T> {
> +    type Error = ARef<T>;
> +    /// Tries to convert the [`ARef`] to an [`Owned`] by calling
> +    /// [`try_from_shared()`](OwnableRefCounted::try_from_shared). In case the [`ARef`] is not
> +    /// unique, it returns again an [`ARef`] to the same underlying object.
> +    fn try_from(b: ARef<T>) -> Result<Owned<T>, Self::Error> {
> +        T::try_from_shared(b)
> +    }
> +}
> +
>  /// A sum type that always holds either a value of type `L` or `R`.
>  ///
>  /// # Examples
> 
> -- 
> 2.48.1
> 
> 

Powered by blists - more mailing lists