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

[Gary Guo <gary@garyguo.net>]

On Sat, 5 Oct 2024 13:59:44 +0200
Alice Ryhl <aliceryhl@google.com> wrote:

> 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. The function checks whether
> there's only one Rust reference left, and if so, takes ownership of
> the value, correct?
> 
> 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.
> 
> Alice

The code as is is fine since there's no data protected in
`RequestDataWrapper` yet (in fact it's not even generic yet). I know
Andreas does want to introduce driver-specific data into that, so in
the long term the acquire would be necessary.

Andreas, please let me know if you want me to make the change now, or
you'd rather change the ordering when you introduce data to
`RequestDataWrapper`.

Best,
Gary