| Message ID | c1596f5f-405b-4370-997d-f42c8303c58c@kernel.dk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [for-next] io_uring: ensure io_queue_deferred() is out-of-line | expand |
Hi, > -----Original Message----- > From: Jens Axboe <axboe@kernel.dk> > Sent: Tuesday, December 31, 2024 8:37 AM > To: io-uring <io-uring@vger.kernel.org> > Subject: [PATCH for-next] io_uring: ensure io_queue_deferred() is out-of-line > > This is not the hot path, it's a slow path. Yet the locking for it is in the hot path, > and __cold does not prevent it from being inlined. > > Move the locking to the function itself, and mark it noinline as well to avoid it > polluting the icache of the hot path. > > Signed-off-by: Jens Axboe <axboe@kernel.dk> > > --- > > diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c index > 42d4cc5da73b..db198bd435b5 100644 > --- a/io_uring/io_uring.c > +++ b/io_uring/io_uring.c > @@ -550,8 +550,9 @@ void io_req_queue_iowq(struct io_kiocb *req) > io_req_task_work_add(req); > } > > -static __cold void io_queue_deferred(struct io_ring_ctx *ctx) > +static __cold noinline void io_queue_deferred(struct io_ring_ctx *ctx) > { > + spin_lock(&ctx->completion_lock); Just a digression, whether the io_uring subsystem welcomes scope-based cleanup helpers, this is somewhat controversial in other submodules. > while (!list_empty(&ctx->defer_list)) { > struct io_defer_entry *de = list_first_entry(&ctx->defer_list, > struct io_defer_entry, list); > @@ -562,6 +563,7 @@ static __cold void io_queue_deferred(struct io_ring_ctx > *ctx) > io_req_task_queue(de->req); > kfree(de); > } > + spin_unlock(&ctx->completion_lock); > } > > void __io_commit_cqring_flush(struct io_ring_ctx *ctx) @@ -570,11 +572,8 > @@ void __io_commit_cqring_flush(struct io_ring_ctx *ctx) > io_poll_wq_wake(ctx); > if (ctx->off_timeout_used) > io_flush_timeouts(ctx); > - if (ctx->drain_active) { > - spin_lock(&ctx->completion_lock); > + if (ctx->drain_active) > io_queue_deferred(ctx); > - spin_unlock(&ctx->completion_lock); > - } > if (ctx->has_evfd) > io_eventfd_flush_signal(ctx); > } > > -- > Jens Axboe > Reviewed-by: Li Zetao<lizetao1@huawei.com> --- Li Zetao
On 1/2/25 5:38 AM, lizetao wrote: > Hi, > >> -----Original Message----- >> From: Jens Axboe <axboe@kernel.dk> >> Sent: Tuesday, December 31, 2024 8:37 AM >> To: io-uring <io-uring@vger.kernel.org> >> Subject: [PATCH for-next] io_uring: ensure io_queue_deferred() is out-of-line >> >> This is not the hot path, it's a slow path. Yet the locking for it is in the hot path, >> and __cold does not prevent it from being inlined. >> >> Move the locking to the function itself, and mark it noinline as well to avoid it >> polluting the icache of the hot path. >> >> Signed-off-by: Jens Axboe <axboe@kernel.dk> >> >> --- >> >> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c index >> 42d4cc5da73b..db198bd435b5 100644 >> --- a/io_uring/io_uring.c >> +++ b/io_uring/io_uring.c >> @@ -550,8 +550,9 @@ void io_req_queue_iowq(struct io_kiocb *req) >> io_req_task_work_add(req); >> } >> >> -static __cold void io_queue_deferred(struct io_ring_ctx *ctx) >> +static __cold noinline void io_queue_deferred(struct io_ring_ctx *ctx) >> { >> + spin_lock(&ctx->completion_lock); > Just a digression, whether the io_uring subsystem welcomes scope-based > cleanup helpers, this is somewhat We welcome any change that makes sense :-) For this particular one, no point having io_queue_deferred() marked as cold yet still inlined, and no point having the locking outside of the helper as that gets inlined as well.
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c index 42d4cc5da73b..db198bd435b5 100644 --- a/io_uring/io_uring.c +++ b/io_uring/io_uring.c @@ -550,8 +550,9 @@ void io_req_queue_iowq(struct io_kiocb *req) io_req_task_work_add(req); } -static __cold void io_queue_deferred(struct io_ring_ctx *ctx) +static __cold noinline void io_queue_deferred(struct io_ring_ctx *ctx) { + spin_lock(&ctx->completion_lock); while (!list_empty(&ctx->defer_list)) { struct io_defer_entry *de = list_first_entry(&ctx->defer_list, struct io_defer_entry, list); @@ -562,6 +563,7 @@ static __cold void io_queue_deferred(struct io_ring_ctx *ctx) io_req_task_queue(de->req); kfree(de); } + spin_unlock(&ctx->completion_lock); } void __io_commit_cqring_flush(struct io_ring_ctx *ctx) @@ -570,11 +572,8 @@ void __io_commit_cqring_flush(struct io_ring_ctx *ctx) io_poll_wq_wake(ctx); if (ctx->off_timeout_used) io_flush_timeouts(ctx); - if (ctx->drain_active) { - spin_lock(&ctx->completion_lock); + if (ctx->drain_active) io_queue_deferred(ctx); - spin_unlock(&ctx->completion_lock); - } if (ctx->has_evfd) io_eventfd_flush_signal(ctx); }
This is not the hot path, it's a slow path. Yet the locking for it is in the hot path, and __cold does not prevent it from being inlined. Move the locking to the function itself, and mark it noinline as well to avoid it polluting the icache of the hot path. Signed-off-by: Jens Axboe <axboe@kernel.dk> ---