On 10.10.24 11:06, Andreas Hindborg wrote: > Andreas Hindborg <a.hindborg@xxxxxxxxxx> writes: > >> Andreas Hindborg <a.hindborg@xxxxxxxxxx> writes: >> >>> "Gary Guo" <gary@xxxxxxxxxxx> writes: >>> >>>> On Sat, 5 Oct 2024 13:59:44 +0200 >>>> Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote: >>>> >>>>> On Sat, Oct 5, 2024 at 11:49 AM Andreas Hindborg <a.hindborg@xxxxxxxxxx> wrote: >>>>>> >>>>>> Hi Greg, >>>>>> >>>>>> "Greg KH" <gregkh@xxxxxxxxxxxxxxxxxxx> 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`. >>> >>> I guess we will have said data dependencies when we are going to run >>> drop for fields in the private data area. Thanks for pointing that out. >>> I will update the ordering when I submit that patch. >>> >>> As I mentioned before, I would rather we do not apply this patch before >>> we get a way to inline helpers. >> >> As discussed offline, the code that suffers the performance regression >> is downstream, and since this change seems to be important, I can apply >> the helper LTO patch downstream as well. >> >> Since the plan for the downstream code _is_ to move upstream, I really >> hope to see the helper LTO patch upstream, so we don't get a performance >> regression because of these refcounts. >> >> If we cannot figure out a way to get the LTO patches (or an alternative >> solution) upstream, we can always revert back to a more performant >> solution in block. > > I forgot to report the result of the benchmarks. Over the usual > benchmark workload that I run for `rnull` I see an average 0.8 percent > performance penalty with this patch. For some configurations > I see 95% CI N=40 [-18%;-5%]. So it is not insignificant. Was the benchmark run together with the LTO helper patches? --- Cheers, Benno