[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87zfniop6i.fsf@kernel.org>
Date: Sat, 05 Oct 2024 11:48:53 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Greg KH" <gregkh@...uxfoundation.org>
Cc: "Gary Guo" <gary@...yguo.net>, "Boqun Feng" <boqun.feng@...il.com>,
"Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
<alex.gaynor@...il.com>, Björn Roy Baron
<bjorn3_gh@...tonmail.com>,
"Benno Lossin" <benno.lossin@...ton.me>, "Alice Ryhl"
<aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>, "Jens Axboe"
<axboe@...nel.dk>, "Will Deacon" <will@...nel.org>, "Peter Zijlstra"
<peterz@...radead.org>, "Mark Rutland" <mark.rutland@....com>,
<linux-block@...r.kernel.org>, <rust-for-linux@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`
Hi Greg,
"Greg KH" <gregkh@...uxfoundation.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
Powered by blists - more mailing lists