On 2/21/25 19:44, Pavel Begunkov wrote:
On 2/21/25 04:19, Bui Quang Minh wrote: >> Currently, in case we don't use IORING_SETUP_DEFER_TASKRUN, when >>
io worker frees work, it needs to add a task work. This creates >>
contention on tctx->task_list. With this commit, io work queues >> free
work on a local list and batch multiple free work in one call >> when
the number of free work in local list exceeds >> IO_REQ_ALLOC_BATCH. > >
I see no relation to IO_REQ_ALLOC_BATCH, that should be a separate >
macro. > >> Signed-off-by: Bui Quang Minh <minhquangbui99@xxxxxxxxx> ---
>> io_uring/io-wq.c | 62 ++++++++++++++++++++++++++++++++++++++++++ >>
+-- io_uring/io-wq.h | 4 ++- io_uring/io_uring.c | 23 +++++++++ >>
+++++--- io_uring/io_uring.h | 6 ++++- 4 files changed, 87 >>
insertions(+), 8 deletions(-) >> >> diff --git a/io_uring/io-wq.c
b/io_uring/io-wq.c index >> 5d0928f37471..096711707db9 100644 ---
a/io_uring/io-wq.c +++ b/ >> io_uring/io-wq.c > ... >> @@ -601,7 +622,41
@@ static void io_worker_handle_work(struct >> io_wq_acct *acct,
wq->do_work(work); >> io_assign_current_work(worker, NULL); - linked =
wq- >> >free_work(work); + /* + * All requests in >> free list must have
the same + * io_ring_ctx. >> + */ + if (last_added_ctx && >>
last_added_ctx != req->ctx) { + >> flush_req_free_list(&free_list,
tail); + tail = >> NULL; + last_added_ctx = NULL; + >> free_req = 0; + }
+ + /* + * Try >> to batch free work when + * ! >>
IORING_SETUP_DEFER_TASKRUN to reduce contention + * on >>
tctx->task_list. + */ + if (req->ctx->flags >> &
IORING_SETUP_DEFER_TASKRUN) + linked = wq- >> >free_work(work, NULL,
NULL); + else + >> linked = wq->free_work(work, &free_list, &did_free);
> > The problem here is that iowq is blocking and hence you lock up >
resources of already completed request for who knows how long. In > case
of unbound requests (see IO_WQ_ACCT_UNBOUND) it's indefinite, > and it's
absolutely cannot be used without some kind of a timer. But > even in
case of bound work, it can be pretty long.
That's a good point, I've overlooked the fact that work handler might
block indefinitely.
Maybe, for bound requests it can target N like here, but read > jiffies in between each request and flush if it has been too long. >
So in worst case the total delay is the last req execution time + > DT.
But even then it feels wrong, especially with filesystems > sometimes
not even honouring NOWAIT. > > The question is, why do you force it into
the worker pool with the > IOSQE_ASYNC flag? It's generally not
recommended, and the name of > the flag is confusing as it should've
been more like > "WORKER_OFFLOAD".
I launched more workers to parallel the work handler, but as you said,
it seems like an incorrect use case.
However, I think the request free seems heavy, we need to create a task
work so that we can hold the uring_lock to queue the request to
ctx->submit_state->compl_reqs. Let me play around more to see if I can
find an optimization for this.
Thank you,
Quang Minh.
