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

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

"Benno Lossin" <lossin@kernel.org> writes:

> 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:

[...]

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

We have been a bit back and forth on this. This is how we would like it
going forward:


/// There are three states for a request that the Rust bindings care about:
///
/// - 0: The request is owned by C block layer or is uniquely referenced (by [`Owned<_>`]).
/// - 1: The request is owned by Rust abstractions but is not referenced.
/// - 2+: There is one or more [`ARef`] instances referencing the request.


Best regards,
Andreas Hindborg