Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

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

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Sat, Oct 5, 2024 at 11:49 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Hi Greg,
>>
>> "Greg KH" <gregkh@linuxfoundation.org> writes:
>>
>> > On Fri, Oct 04, 2024 at 04:52:24PM +0100, Gary Guo wrote:
>> >> There is an operation needed by `block::mq`, atomically decreasing
>> >> refcount from 2 to 0, which is not available through refcount.h, so
>> >> I exposed `Refcount::as_atomic` which allows accessing the refcount
>> >> directly.
>> >
>> > That's scary, and of course feels wrong on many levels, but:
>> >
>> >
>> >> @@ -91,13 +95,17 @@ pub(crate) unsafe fn start_unchecked(this: &ARef<Self>) {
>> >>      /// C `struct request`. If the operation fails, `this` is returned in the
>> >>      /// `Err` variant.
>> >>      fn try_set_end(this: ARef<Self>) -> Result<*mut bindings::request, ARef<Self>> {
>> >> -        // We can race with `TagSet::tag_to_rq`
>> >> -        if let Err(_old) = this.wrapper_ref().refcount().compare_exchange(
>> >> -            2,
>> >> -            0,
>> >> -            Ordering::Relaxed,
>> >> -            Ordering::Relaxed,
>> >> -        ) {
>> >> +        // To hand back the ownership, we need the current refcount to be 2.
>> >> +        // Since we can race with `TagSet::tag_to_rq`, this needs to atomically reduce
>> >> +        // refcount to 0. `Refcount` does not provide a way to do this, so use the underlying
>> >> +        // atomics directly.
>> >> +        if this
>> >> +            .wrapper_ref()
>> >> +            .refcount()
>> >> +            .as_atomic()
>> >> +            .compare_exchange(2, 0, Ordering::Relaxed, Ordering::Relaxed)
>> >> +            .is_err()
>> >
>> > Why not just call rust_helper_refcount_set()?  Or is the issue that you
>> > think you might not be 2 here?  And if you HAVE to be 2, why that magic
>> > value (i.e. why not just always be 1 and rely on normal
>> > increment/decrement?)
>> >
>> > I know some refcounts are odd in the kernel, but I don't see where the
>> > block layer is caring about 2 as a refcount anywhere, what am I missing?
>>
>> It is in the documentation, rendered version available here [1]. Let me
>> know if it is still unclear, then I guess we need to update the docs.
>>
>> Also, my session from Recipes has a little bit of discussion regarding
>> this refcount and it's use [2].
>>
>> Best regards,
>> Andreas
>>
>>
>> [1] https://rust.docs.kernel.org/kernel/block/mq/struct.Request.html#implementation-details
>> [2] https://youtu.be/1LEvgkhU-t4?si=B1XnJhzCCNnUtRsI&t=1685
>
> So it sounds like there is one refcount from the C side, and some
> number of references from the Rust side.

C side uses a different refcount field. That refcount never read by
Rust, but it is guaranteed to be greater or equal to one while the
driver owns the request.

Rust uses a different refcount field to keep track of how many Rust
references there is to a request. There is an implicit count while the
driver owns the request. This count is not materialized as an `ARef`
instance.

> The function checks whether there's only one Rust reference left, and
> if so, takes ownership of the value, correct?

It checks if the `ARef` passed to the function is the last one in
existence. If it is, it takes ownership of the `Request` object.

> In that case, the CAS should have an acquire ordering to synchronize
> with dropping the refcount 3->2 on another thread. Otherwise, you
> might have a data race with the operations that happened just before
> the 3->2 refcount drop.

I am not sure. I don't think that the thread that does the CAS 2 -> 0
has any data dependencies to any thread that does the atomic decrement 3
-> 2. Any data dependencies between operations on the underlying C
`struct request` would be synchronized by operations on the `ref` field
of `struct request`, which is entirely managed block layer C code and
the C functions called by the Rust abstractions.


BR Andreas