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

[Oliver Mangold <oliver.mangold@pm.me>]

On 250702 1524, Benno Lossin wrote:
> On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
> > diff --git a/rust/kernel/types/ownable.rs b/rust/kernel/types/ownable.rs
> > index 80cd990f6601767aa5a742a6c0997f4f67d06453..b5626dead6bb25ea76a0ae577db1b130308d98b1 100644
> > --- a/rust/kernel/types/ownable.rs
> > +++ b/rust/kernel/types/ownable.rs
> > @@ -2,6 +2,7 @@
> >
> >  //! Owned reference types.
> >
> > +use crate::types::{ARef, RefCounted};
> >  use core::{
> >      marker::PhantomData,
> >      mem::ManuallyDrop,
> > @@ -18,8 +19,9 @@
> >  ///
> >  /// Note: Implementing this trait allows types to be wrapped in an [`Owned<Self>`]. This does not
> >  /// provide reference counting but represents a unique, owned reference. If reference counting is
> > -/// required [`RefCounted`](crate::types::RefCounted) should be implemented which allows types to be
> > -/// wrapped in an [`ARef<Self>`](crate::types::ARef).
> > +/// required [`RefCounted`] should be implemented which allows types to be wrapped in an
> > +/// [`ARef<Self>`]. Implementing the trait [`OwnableRefCounted`] allows to convert between unique
> > +/// and shared references (i.e. [`Owned<Self>`] and [`ARef<Self>`]).
> 
> This change should probably be in the earlier patch.

Ah, yes. Must have happened while splitting the patches.

> >  ///
> >  /// # Safety
> >  ///
> > @@ -132,3 +134,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`].
> > +///
> > +/// # Safety
> > +///
> > +/// Implementers must ensure that:
> > +///
> > +/// - [`try_from_shared()`](OwnableRefCounted::into_shared) only returns an [`Owned<Self>`] if
> > +///   exactly one [`ARef<Self>`] exists.
> 
> This shouldn't be required?

Ehm, why not? `Owned<T>` is supposed to be unique.

> > +/// - [`into_shared()`](OwnableRefCounted::into_shared) set the reference count to the value which
> > +///   the returned [`ARef<Self>`] 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 an [`Owned<Self>`] to an [`ARef<Self>`].
> 
> This also seems pretty weird...
> 
> I feel like `OwnableRefCounted` is essentially just a compatibility
> condition between `Ownable` and `RefCounted`. It ensures that the
> ownership declared in `Ownable` corresponds to exactly one refcount
> declared in `RefCounted`.
> 
> That being said, I think a `RefCounted` *always* canonically is
> `Ownable` by the following impl:
> 
>     unsafe impl<T: RefCounted> Ownable for T {
>         unsafe fn release(this: NonNull<Self>) {
>             T::dec_ref(this)
>         }
>     }
> 
> So I don't think that we need this trait at all?

No. For an `ARef<T>` to be converted to an `Owned<T>` it requires a
`try_from_shared()` implementation. It is not even a given that the
function can implemented, if all the kernel exposes are some kind of
`inc_ref()` and `dec_ref()`.

Also there are more complicated cases like with `Mq::Request`, where the
existence of an `Owned<T>` cannot be represented by the same refcount value
as the existence of exactly one `ARef<T>`.

> > +///
> > +/// # Examples
> 
> If we're having an example here, then we should also have on on `Owned`.

Yes, maybe. I mostly felt the need to create one for `OwnableRefCounted`
because it is a more complex idea than `Ownable`.

If I remember correctly, I didn't create one for `Owned`, as it should
probably more or less the same as for `ARef` and the one there has even
more problems of the kind you are pointing out. So maybe it would be best
to wait until someone fixes that and have the fixed version copied over to
`Owned` in the process?

> > +///
> > +/// 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>,
> > +/// }
> > +///
> > +/// 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.");
> 
> I'm not really convinced that an example using `KBox` is a good one...
> Maybe we should just have a local invisible `bindings` module that
> exposes a `-> *mut foo`. (internally it can just create a KBox`)

The example would become quite a bit more complicated then, no?

> > +///         // SAFETY: We just allocated the `Foo`, thus it is valid.
> 
> This isn't going through all the requirements on `from_raw`...

Yes, this comment should probably be detailed.

Best,

Oliver