Re: [PATCH v13 4/4] rust: Add `OwnableRefCounted`

[Andreas Hindborg <a.hindborg@kernel.org>]

Daniel Almeida <daniel.almeida@collabora.com> writes:

> Hi Oliver,
>
>> On 17 Nov 2025, at 07:08, Oliver Mangold <oliver.mangold@pm.me> wrote:
>> 
>> Types implementing one of these traits can safely convert between an
>> `ARef<T>` and an `Owned<T>`.
>> 
>> This is useful for types which generally are accessed through an `ARef`
>> but have methods which can only safely be called when the reference is
>> unique, like e.g. `block::mq::Request::end_ok()`.
>> 
>> Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
>> Co-developed-by: Andreas Hindborg <a.hindborg@kernel.org>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>> rust/kernel/owned.rs     | 138 ++++++++++++++++++++++++++++++++++++++++++++---
>> rust/kernel/sync/aref.rs |  11 +++-
>> rust/kernel/types.rs     |   2 +-
>> 3 files changed, 141 insertions(+), 10 deletions(-)
>> 
>> diff --git a/rust/kernel/owned.rs b/rust/kernel/owned.rs
>> index a26747cbc13b..26ab2b00ada0 100644
>> --- a/rust/kernel/owned.rs
>> +++ b/rust/kernel/owned.rs
>> @@ -5,6 +5,7 @@
>> //! These pointer types are useful for C-allocated objects which by API-contract
>> //! are owned by Rust, but need to be freed through the C API.
>> 
>> +use crate::sync::aref::{ARef, RefCounted};
>> use core::{
>>     mem::ManuallyDrop,
>>     ops::{Deref, DerefMut},
>> @@ -14,14 +15,16 @@
>> 
>> /// Type allocated and destroyed on the C side, but owned by Rust.
>> ///
>> -/// Implementing this trait allows types to be referenced via the [`Owned<Self>`] pointer type. This
>> -/// is useful when it is desirable to tie the lifetime of the reference to an owned object, rather
>> -/// than pass around a bare reference. [`Ownable`] types can define custom drop logic that is
>> -/// executed when the owned reference [`Owned<Self>`] pointing to the object is dropped.
>> +/// Implementing this trait allows types to be referenced via the [`Owned<Self>`] pointer type.
>> +///  - This is useful when it is desirable to tie the lifetime of an object reference to an owned
>> +///    object, rather than pass around a bare reference.
>> +///  - [`Ownable`] types can define custom drop logic that is executed when the owned reference
>> +///    of type [`Owned<_>`] pointing to the object is dropped.
>> ///
>> /// Note: The underlying object is not required to provide internal reference counting, because it
>> /// represents a unique, owned reference. If reference counting (on the Rust side) is required,
>> -/// [`RefCounted`](crate::types::RefCounted) should be implemented.
>> +/// [`RefCounted`] should be implemented. [`OwnableRefCounted`] should be implemented if conversion
>> +/// between unique and shared (reference counted) ownership is needed.
>> ///
>> /// # Safety
>> ///
>> @@ -143,9 +146,7 @@ impl<T: Ownable> Owned<T> {
>>     ///   mutable reference requirements. That is, the kernel will not mutate or free the underlying
>>     ///   object and is okay with it being modified by Rust code.
>>     pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
>> -        Self {
>> -            ptr,
>> -        }
>> +        Self { ptr }
>>     }
>
> Unrelated change?

Looks like a rustfmt thing. I'll split it out or remove it.

>
>> 
>>     /// Consumes the [`Owned`], returning a raw pointer.
>> @@ -193,3 +194,124 @@ fn drop(&mut self) {
>>         unsafe { T::release(self.ptr) };
>>     }
>> }
>> +
>> +/// A trait for objects that can be wrapped in either one of the reference types [`Owned`] and
>> +/// [`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::sync::aref::{ARef, RefCounted};
>> +/// # use kernel::types::{Owned, Ownable, OwnableRefCounted};
>> +///
>> +/// // Example internally refcounted struct.
>
> nit: IMHO the wording could improve ^

/// // An internally refcounted struct for demonstration purposes.

>
>> +/// //
>> +/// // # Invariants
>> +/// //
>> +/// // - `refcount` is always non-zero for a valid object.
>> +/// // - `refcount` is >1 if there are more than 1 Rust reference to it.
>> +/// //
>> +/// struct Foo {
>> +///     refcount: Cell<usize>,
>> +/// }
>> +///
>> +/// impl Foo {
>> +///     fn new() -> Result<Owned<Self>, AllocError> {
>> +///         // We are just using a `KBox` here to handle the actual allocation, as our `Foo` is
>> +///         // not actually a C-allocated object.
>> +///         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 `Self`, thus it is valid and there cannot be any other
>> +///         // Rust references. Calling `into_raw()` makes us responsible for ownership and
>> +///         // we won't use the raw pointer anymore, thus we can transfer ownership to the `Owned`.
>> +///         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: By requirement on calling this function, the refcount is non-zero,
>> +///         // implying the underlying object is valid.
>> +///         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 [`KBox<Foo>`] is still alive as the old refcount is 1. We can pass
>> +///             // ownership to the [`KBox`] as by requirement on calling this function,
>> +///             // the `Self` will no longer be used by the caller.
>> +///             unsafe { KBox::from_raw(this.as_ptr()) };
>> +///         } else {
>> +///             refcount.replace(new_refcount);
>> +///         }
>> +///     }
>> +/// }
>> +///
>> +/// 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 and has no other Rust references as the refcount
>> +///             // is 1.
>> +///             Ok(unsafe { Owned::from_raw(ARef::into_raw(this)) })
>> +///         } else {
>> +///             Err(this)
>> +///         }
>> +///     }
>> +/// }
>> +///
>
> We wouldn’t need this implementation if we added a “refcount()”
> member to this trait. This lets you abstract away this logic for all
> implementors, which has the massive upside of making sure we hardcode (and thus
> enforce) the refcount == 1 check.

This exist to allow reference counting schemes that are different from
the general implementation. See [1] for an example.

[1] https://github.com/metaspace/linux/blob/3e46e95f0707fa71259b1d241f689144ad61cc62/rust/kernel/block/mq/request.rs#L478

>
>
>> +/// // SAFETY: This implementation of `release()` is safe for any valid `Self`.
>> +/// 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().expect("Failed to allocate a Foo. This shouldn't happen");
>
> All these “expects()” and custom error strings would go away if you
> place this behind a fictional function that returns Result.

I'll do that.

>
>> +/// 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 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>>;
>
> Again, this method can go way if we add a method to expose the refcount.
>
>> +
>> +    /// Converts the [`Owned`] into an [`ARef`].
>> +    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)) }
>> +    }
>> +}
>> +
>> +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)
>> +    }
>> +}
>> diff --git a/rust/kernel/sync/aref.rs b/rust/kernel/sync/aref.rs
>> index 937dcf6ed5de..2dbffe2ed1b8 100644
>> --- a/rust/kernel/sync/aref.rs
>> +++ b/rust/kernel/sync/aref.rs
>> @@ -30,7 +30,10 @@
>> /// Note: Implementing this trait allows types to be wrapped in an [`ARef<Self>`]. It requires an
>> /// internal reference count and provides only shared references. If unique references are required
>> /// [`Ownable`](crate::types::Ownable) should be implemented which allows types to be wrapped in an
>> -/// [`Owned<Self>`](crate::types::Owned).
>> +/// [`Owned<Self>`](crate::types::Owned). Implementing the trait
>> +/// [`OwnableRefCounted`](crate::types::OwnableRefCounted) allows to convert between unique and
>> +/// shared references (i.e. [`Owned<Self>`](crate::types::Owned) and
>> +/// [`ARef<Self>`](crate::types::Owned)).
>> ///
>> /// # Safety
>> ///
>> @@ -180,6 +183,12 @@ fn from(b: &T) -> Self {
>>     }
>> }
>> 
>> +impl<T: crate::types::OwnableRefCounted> From<crate::types::Owned<T>> for ARef<T> {
>> +    fn from(b: crate::types::Owned<T>) -> Self {
>> +        T::into_shared(b)
>> +    }
>> +}
>> +
>
> Not sure why we’re using fully-qualified names here if we can import them, but your call.

I'll import them.


Best regards,
Andreas Hindborg