On 6/5/24 1:20 PM, Pavel Begunkov wrote: > On 6/5/24 17:41, Jens Axboe wrote: >> On 6/5/24 9:50 AM, Pavel Begunkov wrote: >>> On 6/4/24 19:57, Jens Axboe wrote: >>>> On 6/3/24 7:53 AM, Pavel Begunkov wrote: >>>>> On 5/30/24 16:23, Jens Axboe wrote: >>>>>> Hi, >>>>>> >>>>>> For v1 and replies to that and tons of perf measurements, go here: >>>>> >>>>> I'd really prefer the task_work version rather than carving >>>>> yet another path specific to msg_ring. Perf might sounds better, >>>>> but it's duplicating wake up paths, not integrated with batch >>>>> waiting, not clear how affects different workloads with target >>>>> locking and would work weird in terms of ordering. >>>> >>>> The duplication is really minor, basically non-existent imho. It's a >>>> wakeup call, it's literally 2 lines of code. I do agree on the batching, >>> >>> Well, v3 tries to add msg_ring/nr_overflow handling to local >>> task work, that what I mean by duplicating paths, and we'll >>> continue gutting the hot path for supporting msg_ring in >>> this way. >> >> No matter how you look at it, there will be changes to the hot path >> regardless of whether we use local task_work like in the original, or do >> the current approach. > > The only downside for !msg_ring paths in the original was > un-inlining of local tw_add(). You're comparing an incomplete RFC to a more complete patchset, that will not be the only downside once you're done with the local task_work approach when the roundtrip is avoided. And that is my comparison base, not some half finished POC that I posted for comments. >>> Does it work with eventfd? I can't find any handling, so next >>> you'd be adding: >>> >>> io_commit_cqring_flush(ctx); >> >> That's merely because the flagging should be done in io_defer_wake(), >> moving that code to the common helper as well. > > Flagging? If you mean io_commit_cqring_flush() then no, > it shouldn't and cannot be there. It's called strictly after > posting a CQE (or queuing an overflow), which is after tw > callback execution. I meant the SQ ring flagging and eventfd signaling, which is currently done in local work adding. That should go in io_defer_wake(). -- Jens Axboe
