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

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

On Mon Jul 7, 2025 at 10:07 AM CEST, Oliver Mangold wrote:
> On 250702 1524, Benno Lossin wrote:
>> On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
>> > @@ -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.

It's not needed as a safety requirement for implementing the trait. If
the implementation only contains sound code, then `Owned::from_raw`
should already ensure that `Owned<Self>` is only created if there is
exactly one reference to it.

>> > +/// - [`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()`.

I don't understand this paragraph.

> 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>`.

Ah right, I forgot about this. What was the refcount characteristics of
this again?

*  1 = in flight, owned by C
*  2 = in flight, owned by Rust
* >2 = in flight, owned by Rust + additional references used by Rust
       code

Correct? Maybe @Andreas can check.

>> > +///
>> > +/// # 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?

Wait which problems on `ARef` do you mean? I disagree that `Owned` and
`ARef` have the same example. `Owned` should expose operations that
`ARef` can't otherwise there would be no value in using `Owned`.

>> > +///
>> > +/// 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?

Just hide those parts behind `#` lines in the example.

---
Cheers,
Benno