On 6/19/22 16:52, Jens Axboe wrote:
On 6/19/22 8:52 AM, Pavel Begunkov wrote:
On 6/19/22 14:31, Jens Axboe wrote:
On 6/19/22 5:26 AM, Pavel Begunkov wrote:
It's not clear how widely used IOSQE_CQE_SKIP_SUCCESS is, and how often
->flush_cqes flag prevents from completion being flushed. Sometimes it's
high level of concurrency that enables it at least for one CQE, but
sometimes it doesn't save much because nobody waiting on the CQ.
Remove ->flush_cqes flag and the optimisation, it should benefit the
normal use case. Note, that there is no spurious eventfd problem with
that as checks for spuriousness were incorporated into
io_eventfd_signal().
Would be note to quantify, which should be pretty easy. Eg run a nop
workload, then run the same but with CQE_SKIP_SUCCESS set. That'd take
it to the extreme, and I do think it'd be nice to have an understanding
of how big the gap could potentially be.
With luck, it doesn't really matter. Always nice to kill stuff like
this, if it isn't that impactful.
Trying without this patch nops32 (submit 32 nops, complete all, repeat).
1) all CQE_SKIP:
~51 Mreqs/s
2) all CQE_SKIP but last, so it triggers locking + *ev_posted()
~49 Mreq/s
3) same as 2) but another task waits on CQ (so we call wake_up_all)
~36 Mreq/s
And that's more or less expected. What is more interesting for me
is how often for those using CQE_SKIP it helps to avoid this
ev_posted()/etc. They obviously can't just mark all requests
with it, and most probably helping only some quite niche cases.
That's not too bad. But I think we disagree on CQE_SKIP being niche,
I wasn't talking about CQE_SKIP but rather cases where that
->flush_cqes actually does anything. Consider that when at least
one of the requests queued for inline completion is not CQE_SKIP
->flush_cqes is effectively disabled.
there are several standard cases where it makes sense. Provide buffers
is one, though that one we have a better solution for now. But also eg
OP_CLOSE is something that I'd personally use CQE_SKIP with always.
Hence I don't think it's fair or reasonable to call it "quite niche" in
terms of general usability.
But if this helps in terms of SINGLE_ISSUER, then I think it's worth it
as we'll likely see more broad appeal from that.
It neither conflicts with the SINGLE_ISSUER locking optimisations
nor with the meantioned mb() optimisation. So, if there is a good
reason to leave ->flush_cqes alone we can drop the patch.
--
Pavel Begunkov
