Message-ID: <87jz4iomc9.fsf@kernel.org>
Date: Tue, 08 Jul 2025 15:15:18 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Benno Lossin" <lossin@...nel.org>
Cc: "Oliver Mangold" <oliver.mangold@...me>,  "Miguel Ojeda"
 <ojeda@...nel.org>,  "Alex Gaynor" <alex.gaynor@...il.com>,  "Boqun Feng"
 <boqun.feng@...il.com>,  "Gary Guo" <gary@...yguo.net>,  Björn Roy Baron
 <bjorn3_gh@...tonmail.com>,  "Alice Ryhl" <aliceryhl@...gle.com>,  "Trevor
 Gross" <tmgross@...ch.edu>,  "Asahi Lina" <lina+kernel@...hilina.net>,
  <rust-for-linux@...r.kernel.org>,  <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v11 4/4] rust: Add `OwnableRefCounted`

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

> On Mon Jul 7, 2025 at 3:21 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@...nel.org> writes:
>>
>>> On Mon Jul 7, 2025 at 1:12 PM CEST, Andreas Hindborg wrote:
>>>> "Benno Lossin" <lossin@...nel.org> writes:
>>>>> 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.
>>>
>>> Huh, now I'm more confused... Could you go into the details again?
>>
>> Well, there is not much to it. We found out we can alias "unique" and
>> "owned by C".
>>
>> We initialize the refcount to 0 when we initialize the request
>> structure. This happens at queue creation time.
>
> And IIRC this refcount is only on the Rust side, since you store it in
> some private data, right?

Yes.

>
>> When C block layer hands over a request for processing to a Rust driver,
>> we `debug_assert!` that the refcount is 0. We unsafely invent an
>> `Owned<Request<_>>` and pass that to the driver.
>
> And you don't increment the refcount?

No, we leave it at 0.

>
>> The driver has the option of `into_shared` to obtain an
>> `ARef<Request<_>>`. We use this for remote completion and timer
>> completion in rnull.
>
> This operation will set the refcount to 2?

Yes.

>
>> In most drivers, when the driver hands off the request to the hardware,
>> the driver will stop accounting for the request. The `Owned<Request<_>>`
>> is consumed when issuing to the driver, or the driver could simply drop
>> it. Refcount goes to 1. An ID is passed along to hardware.
>
> And with the current API design it must let go of all `ARef<Request<_>>`
> because otherwise it can't call `end_ok` (or some other method?).

Exactly. That one will take an `Owned<Request<_>>` (previously `Unique`).

>
>> When a completion comes back from hardware, it carries the ID. We use
>> the C block layer `tag_to_rq` machinery to turn this ID back into an
>> `Owned<Request<_>>`. In that process, we check that `refcount == 1` and
>> if so, we set refcount to 0 and invent an `Owned<Request<_>>`.
>
> (because if not, this would be wrong)

I hope it is not wrong 😆 You can see the latest and greatest here [1].

>
> I have some idea what we should do for the safety requirements of
> `Ownable`, but still need some time to write it down.

Great!


Best regards,
Andreas Hindborg


[1] https://git.kernel.org/pub/scm/linux/kernel/git/a.hindborg/linux.git/tree/rust/kernel/block/mq/request.rs?h=rnull-v6.15#n398

Powered by blists - more mailing lists