5.20 cleanups and poll optimisations
v3 cover 5.20 cleanups and poll optimisations
1-4 kills REQ_F_COMPLETE_INLINE as we're out of bits.
Patch 5 from Hao should remove some overhead from poll requests
Patch 6 from Hao adds per-bucket spinlocks, and 16-19 do a little bit of cleanup. The downside of per-bucket spinlocks is that it adds additional spinlock/unlock pair in the poll request completion side, which shouldn't matter much with 20/25.
Patch 11 uses inline completion infra for poll requests, this nicely improves perf when there is a good tw batching.
Patch 12 implements the userspace visible side of IORING_SETUP_SINGLE_ISSUER, it'll be used for poll requests and later for spinlock optimisations.
13-16 introduces ->uring_lock protected cancellation hashing. It requires us to grab ->uring_lock in the completion side, but saves two spin lock/unlock pairs. We apply it automatically in cases the mutex is already likely to be held (see 25/25 description), so there is no additional mutex overhead and potential latency problemes.
Numbers:
The used poll benchmark each iteration queues a batch of 32 POLLIN poll requests and triggers all of them with read (+write).
baseline (patches 1-10):
11720 K req/s
base + 11 (+ inline completion infra)
12419 K req/s, ~+6%
base + 11-16 (+ uring_lock hashing):
12804 K req/s, +9.2% from the baseline, or +3.2% relative to patch 19.Note that patch 11 only helps performance of poll-add requests, whenever 16/16 also improves apoll.
v2:
don't move ->cancel_seq out of iowq work struct
fix up single-issuer
v3:
clarify locking expectation around ->uring_lock hashing don't complete by hand in io_read/write (see 1/16)
Hao Xu (2):
io_uring: poll: remove unnecessary req->ref set
io_uring: switch cancel_hash to use per entry spinlock
Pavel Begunkov (14):
io_uring: rw: delegate sync completions to core io_uring
io_uring: kill REQ_F_COMPLETE_INLINE
io_uring: refactor io_req_task_complete()
io_uring: don't inline io_put_kbuf
io_uring: pass poll_find lock back
io_uring: clean up io_try_cancel
io_uring: limit the number of cancellation buckets
io_uring: clean up io_ring_ctx_alloc
io_uring: use state completion infra for poll reqs
io_uring: add IORING_SETUP_SINGLE_ISSUER
io_uring: pass hash table into poll_find
io_uring: introduce a struct for hash table
io_uring: propagate locking state to poll cancel
io_uring: mutex locked poll hashing
include/uapi/linux/io_uring.h | 5 +- io_uring/cancel.c | 23 +++- io_uring/cancel.h | 4 +- io_uring/fdinfo.c | 11 +- io_uring/io_uring.c | 84 ++++++++----- io_uring/io_uring.h | 5 - io_uring/io_uring_types.h | 21 +++- io_uring/kbuf.c | 33 +++++ io_uring/kbuf.h | 38 +----- io_uring/poll.c | 225 +++++++++++++++++++++++++--------- io_uring/poll.h | 3 +- io_uring/rw.c | 41 +++---- io_uring/tctx.c | 27 +++- io_uring/tctx.h | 4 +- io_uring/timeout.c | 3 +- 15 files changed, 353 insertions(+), 174 deletions(-)
Signature · 1 line
2.36.1
PATCH v3 1/16 io_uring: rw: delegate sync completions to core io_uring
io_issue_sqe() from the io_uring core knows how to complete requests based on the returned error code, we can delegate io_read()/io_write() completion to it. Make kiocb_done() to return the right completion code and propagate it.
Signed-off-by Pavel Begunkov <[email protected]>
--- io_uring/rw.c | 41 +++++++++++++++++++---------------------- 1 file changed, 19 insertions(+), 22 deletions(-)
io_uring/rw.c +19 -22
@@ -207,15 +207,6 @@ static bool __io_complete_rw_common(struct io_kiocb *req, long res)return false; } -static void __io_complete_rw (struct io_kiocb *req,long res, -unsigned int issue_flags) -{ -if (__io_complete_rw_common (req, res)) -return ; -io_req_set_res (req, req->cqe.res,io_put_kbuf (req, issue_flags)); -__io_req_complete (req, issue_flags); -} -static void io_complete_rw (struct kiocb *kiocb,long res) { struct io_rw *rw = container_of(kiocb, struct io_rw, kiocb);
@@ -247,7 +238,7 @@ static void io_complete_rw_iopoll(struct kiocb *kiocb, long res)smp_store_release (&req->iopoll_completed,1 ); } -static void kiocb_done (struct io_kiocb *req,ssize_t ret, +static int kiocb_done (struct io_kiocb *req,ssize_t ret,unsigned int issue_flags) {struct io_async_rw *io = req->async_data;
@@ -263,10 +254,15 @@ static void kiocb_done(struct io_kiocb *req, ssize_t ret,if (req->flags &REQ_F_CUR_POS ) req->file->f_pos = rw->kiocb.ki_pos; -if (ret >=0 && (rw->kiocb.ki_complete == io_complete_rw)) -__io_complete_rw (req, ret, issue_flags); -else +if (ret >=0 && (rw->kiocb.ki_complete == io_complete_rw)) { +if (!__io_complete_rw_common (req, ret)) { +io_req_set_res (req, req->cqe.res, +io_put_kbuf (req, issue_flags)); +return IOU_OK ; + } + }else {io_rw_done (&rw->kiocb, ret); + }if (req->flags &REQ_F_REISSUE ) { req->flags &= ~REQ_F_REISSUE ;
@@ -275,6 +271,7 @@ static void kiocb_done(struct io_kiocb *req, ssize_t ret,else io_req_task_queue_fail (req, ret); } +return IOU_ISSUE_SKIP_COMPLETE ; } static int __io_import_fixed(struct io_kiocb *req, int ddir,
@@ -846,7 +843,9 @@ int io_read(struct io_kiocb *req, unsigned int issue_flags) goto done; ret =0 ; }else if (ret == -EIOCBQUEUED ) { - goto out_free; +if (iovec) +kfree (iovec); +return IOU_ISSUE_SKIP_COMPLETE ; }else if (ret == req->cqe.res || ret <=0 || !force_nonblock || (req->flags &REQ_F_NOWAIT ) || !need_read_all (req)) {/* read all, failed, already did sync or don't want to retry */
@@ -904,12 +903,10 @@ int io_read(struct io_kiocb *req, unsigned int issue_flags)iov_iter_restore (&s->iter, &s->iter_state); }while (ret >0 ); done: -kiocb_done (req, ret, issue_flags); -out_free:/* it's faster to check here then delegate to kfree */ if (iovec)kfree (iovec); -return IOU_ISSUE_SKIP_COMPLETE ; +return kiocb_done (req, ret, issue_flags); }int io_write (struct io_kiocb *req,unsigned int issue_flags)
@@ -959,8 +956,10 @@ int io_write(struct io_kiocb *req, unsigned int issue_flags) ppos =io_kiocb_update_pos (req); ret =rw_verify_area (WRITE , req->file, ppos, req->cqe.res); -if (unlikely (ret)) - goto out_free; +if (unlikely (ret)) { +kfree (iovec); +return ret; + }/* * Open-code file_start_write here to grab freeze protection,
@@ -1002,15 +1001,13 @@ int io_write(struct io_kiocb *req, unsigned int issue_flags)if (ret2 == -EAGAIN && (req->ctx->flags &IORING_SETUP_IOPOLL )) goto copy_iov; done: -kiocb_done (req, ret2, issue_flags); - ret =IOU_ISSUE_SKIP_COMPLETE ; + ret =kiocb_done (req, ret2, issue_flags); }else { copy_iov:iov_iter_restore (&s->iter, &s->iter_state); ret =io_setup_async_rw (req, iovec, s, false);return ret ?: -EAGAIN ; } -out_free:/* it's reportedly faster than delegating the null check to kfree() */ if (iovec)kfree (iovec);
Signature · 1 line
2.36.1
PATCH v3 2/16 io_uring: kill REQ_F_COMPLETE_INLINE
In reply to Pavel BegunkovREQ_F_COMPLETE_INLINE is only needed to delay queueing into the completion list to io_queue_sqe() as __io_req_complete() is inlined and we don't want to bloat the kernel.
As now we complete in a more centralised fashion in io_issue_sqe() we can get rid of the flag and queue to the list directly.
Signed-off-by Pavel Begunkov <[email protected]>
--- io_uring/io_uring.c | 18 +++++++----------- io_uring/io_uring.h | 5 ----- io_uring/io_uring_types.h | 3 --- 3 files changed, 7 insertions(+), 19 deletions(-)
io_uring/io_uring.c +7 -11
@@ -742,10 +742,7 @@ void io_req_complete_post(struct io_kiocb *req)inline void __io_req_complete (struct io_kiocb *req,unsigned issue_flags) { -if (issue_flags &IO_URING_F_COMPLETE_DEFER ) - req->flags |=REQ_F_COMPLETE_INLINE ; -else -io_req_complete_post (req); +io_req_complete_post (req); }void io_req_complete_failed (struct io_kiocb *req,s32 res)
@@ -1581,9 +1578,12 @@ static int io_issue_sqe(struct io_kiocb *req, unsigned int issue_flags)if (creds)revert_creds (creds); -if (ret ==IOU_OK ) -__io_req_complete (req, issue_flags); -else if (ret !=IOU_ISSUE_SKIP_COMPLETE ) +if (ret ==IOU_OK ) { +if (issue_flags &IO_URING_F_COMPLETE_DEFER ) +io_req_add_compl_list (req); +else +io_req_complete_post (req); + }else if (ret !=IOU_ISSUE_SKIP_COMPLETE )return ret;/* If the op doesn't have a file, we're not polling for it */
@@ -1748,10 +1748,6 @@ static inline void io_queue_sqe(struct io_kiocb *req) ret =io_issue_sqe (req,IO_URING_F_NONBLOCK |IO_URING_F_COMPLETE_DEFER ); -if (req->flags &REQ_F_COMPLETE_INLINE ) { -io_req_add_compl_list (req); -return ; - }/* * We async punt it if the file wasn't marked NOWAIT, or if the file * doesn't support non-blocking read/write attempts
io_uring/io_uring.h +0 -5
@@ -217,11 +217,6 @@ static inline bool io_run_task_work(void)return false; } -static inline void io_req_complete_state (struct io_kiocb *req) -{ - req->flags |=REQ_F_COMPLETE_INLINE ; -} -static inline void io_tw_lock (struct io_ring_ctx *ctx,bool *locked) {if (!*locked) {
io_uring/io_uring_types.h +0 -3
@@ -301,7 +301,6 @@ enum {REQ_F_POLLED_BIT ,REQ_F_BUFFER_SELECTED_BIT ,REQ_F_BUFFER_RING_BIT , -REQ_F_COMPLETE_INLINE_BIT ,REQ_F_REISSUE_BIT ,REQ_F_CREDS_BIT , REQ_F_REFCOUNT_BIT,
@@ -356,8 +355,6 @@ enum {REQ_F_BUFFER_SELECTED =BIT (REQ_F_BUFFER_SELECTED_BIT ),/* buffer selected from ring, needs commit */ REQ_F_BUFFER_RING =BIT (REQ_F_BUFFER_RING_BIT ), -/* completion is deferred through io_comp_state */ -REQ_F_COMPLETE_INLINE =BIT (REQ_F_COMPLETE_INLINE_BIT ),/* caller should reissue async */ REQ_F_REISSUE = BIT(REQ_F_REISSUE_BIT),/* supports async reads/writes */
Signature · 1 line
2.36.1
PATCH v3 3/16 io_uring: refactor io_req_task_complete()
In reply to Pavel BegunkovClean up io_req_task_complete() and deduplicate io_put_kbuf() calls.
Signed-off-by Pavel Begunkov <[email protected]>
--- io_uring/io_uring.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
io_uring/io_uring.c +10 -6
@@ -1306,15 +1306,19 @@ static int io_iopoll_check(struct io_ring_ctx *ctx, long min)return ret; } -inline void io_req_task_complete (struct io_kiocb *req,bool *locked) + +void io_req_task_complete (struct io_kiocb *req,bool *locked) { -if (*locked) { - req->cqe.flags |=io_put_kbuf (req,0 ); +if (req->flags & (REQ_F_BUFFER_SELECTED |REQ_F_BUFFER_RING )) { +unsigned issue_flags = *locked ?0 :IO_URING_F_UNLOCKED ; + + req->cqe.flags |=io_put_kbuf (req, issue_flags); + } + +if (*locked)io_req_add_compl_list (req); - }else { - req->cqe.flags |=io_put_kbuf (req,IO_URING_F_UNLOCKED ); +else io_req_complete_post (req); - } }/*
Signature · 1 line
2.36.1
PATCH v3 4/16 io_uring: don't inline io_put_kbuf
In reply to Pavel Begunkovio_put_kbuf() is huge, don't bloat the kernel with inlining.
Signed-off-by Pavel Begunkov <[email protected]>
--- io_uring/kbuf.c | 33 +++++++++++++++++++++++++++++++++ io_uring/kbuf.h | 38 ++++++-------------------------------- 2 files changed, 39 insertions(+), 32 deletions(-)
io_uring/kbuf.c +33 -0
@@ -82,6 +82,39 @@ static int io_buffer_add_list(struct io_ring_ctx *ctx,return xa_err (xa_store (&ctx->io_bl_xa, bgid, bl,GFP_KERNEL )); } +unsigned int __io_put_kbuf (struct io_kiocb *req,unsigned issue_flags) +{ +unsigned int cflags; + +/* +* We can add this buffer back to two lists: +* +* 1) The io_buffers_cache list. This one is protected by the +* ctx->uring_lock. If we already hold this lock, add back to this +* list as we can grab it from issue as well. +* 2) The io_buffers_comp list. This one is protected by the +* ctx->completion_lock. +* +* We migrate buffers from the comp_list to the issue cache list +* when we need one. +*/ +if (req->flags &REQ_F_BUFFER_RING ) { +/* no buffers to recycle for this case */ + cflags =__io_put_kbuf_list (req,NULL ); + }else if (issue_flags &IO_URING_F_UNLOCKED ) { +struct io_ring_ctx *ctx = req->ctx; + +spin_lock (&ctx->completion_lock); + cflags =__io_put_kbuf_list (req, &ctx->io_buffers_comp); +spin_unlock (&ctx->completion_lock); + }else { +lockdep_assert_held (&req->ctx->uring_lock); + + cflags =__io_put_kbuf_list (req, &req->ctx->io_buffers_cache); + } +return cflags; +} +static void __user *io_provided_buffer_select (struct io_kiocb *req,size_t *len,struct io_buffer_list *bl) {
io_uring/kbuf.h +6 -32
@@ -47,6 +47,8 @@ int io_provide_buffers(struct io_kiocb *req, unsigned int issue_flags);int io_register_pbuf_ring (struct io_ring_ctx *ctx,void __user *arg);int io_unregister_pbuf_ring (struct io_ring_ctx *ctx,void __user *arg); +unsigned int __io_put_kbuf (struct io_kiocb *req,unsigned issue_flags); +static inline bool io_do_buffer_select (struct io_kiocb *req) {if (!(req->flags &REQ_F_BUFFER_SELECT ))
@@ -70,7 +72,8 @@ static inline void io_kbuf_recycle(struct io_kiocb *req, unsigned issue_flags)__io_kbuf_recycle (req, issue_flags); } -static unsigned int __io_put_kbuf (struct io_kiocb *req,struct list_head *list) +static inline unsigned int __io_put_kbuf_list (struct io_kiocb *req, +struct list_head *list) {if (req->flags &REQ_F_BUFFER_RING ) {if (req->buf_list)
@@ -90,44 +93,15 @@ static inline unsigned int io_put_kbuf_comp(struct io_kiocb *req)if (!(req->flags & (REQ_F_BUFFER_SELECTED |REQ_F_BUFFER_RING )))return 0 ; -return __io_put_kbuf (req, &req->ctx->io_buffers_comp); + return __io_put_kbuf_list(req, &req->ctx->io_buffers_comp); }static inline unsigned int io_put_kbuf (struct io_kiocb *req,unsigned issue_flags) { -unsigned int cflags;if (!(req->flags & (REQ_F_BUFFER_SELECTED |REQ_F_BUFFER_RING )))return 0 ; - -/* -* We can add this buffer back to two lists: -* -* 1) The io_buffers_cache list. This one is protected by the -* ctx->uring_lock. If we already hold this lock, add back to this -* list as we can grab it from issue as well. -* 2) The io_buffers_comp list. This one is protected by the -* ctx->completion_lock. -* -* We migrate buffers from the comp_list to the issue cache list -* when we need one. -*/ -if (req->flags &REQ_F_BUFFER_RING ) { -/* no buffers to recycle for this case */ - cflags =__io_put_kbuf (req,NULL ); - }else if (issue_flags &IO_URING_F_UNLOCKED ) { -struct io_ring_ctx *ctx = req->ctx; - -spin_lock (&ctx->completion_lock); - cflags =__io_put_kbuf (req, &ctx->io_buffers_comp); -spin_unlock (&ctx->completion_lock); - }else { -lockdep_assert_held (&req->ctx->uring_lock); - - cflags =__io_put_kbuf (req, &req->ctx->io_buffers_cache); - } - -return cflags; +return __io_put_kbuf (req, issue_flags); }#endif
Signature · 1 line
2.36.1
PATCH v3 5/16 io_uring: poll: remove unnecessary req->ref set
In reply to Pavel BegunkovFrom: Hao Xu <[email protected]>
We now don't need to set req->refcount for poll requests since the reworked poll code ensures no request release race.
Signed-off-by Hao Xu <[email protected]>
Signed-off-by Pavel Begunkov <[email protected]>
--- io_uring/poll.c | 1 - 1 file changed, 1 deletion(-)
io_uring/poll.c +0 -1
@@ -683,7 +683,6 @@ int io_poll_add_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)if ((flags &IORING_POLL_ADD_MULTI ) && (req->flags &REQ_F_CQE_SKIP ))return -EINVAL ; -io_req_set_refcount (req); req->apoll_events = poll->events =io_poll_parse_events (sqe, flags);return 0 ; }
Signature · 1 line
2.36.1
PATCH v3 6/16 io_uring: switch cancel_hash to use per entry spinlock
In reply to Pavel BegunkovFrom: Hao Xu <[email protected]>
Add a new io_hash_bucket structure so that each bucket in cancel_hash has separate spinlock. Use per entry lock for cancel_hash, this removes some completion lock invocation and remove contension between different cancel_hash entries.
Signed-off-by Hao Xu <[email protected]>
Signed-off-by Pavel Begunkov <[email protected]>
--- io_uring/cancel.c | 14 ++++++- io_uring/cancel.h | 6 +++ io_uring/fdinfo.c | 9 +++-- io_uring/io_uring.c | 9 +++-- io_uring/io_uring_types.h | 2 +- io_uring/poll.c | 80 ++++++++++++++++++++++++--------------- 6 files changed, 80 insertions(+), 40 deletions(-)
io_uring/cancel.c +12 -2
@@ -93,14 +93,14 @@ int io_try_cancel(struct io_kiocb *req, struct io_cancel_data *cd)if (!ret)return 0 ; -spin_lock (&ctx->completion_lock); ret =io_poll_cancel (ctx, cd);if (ret != -ENOENT ) goto out; +spin_lock (&ctx->completion_lock);if (!(cd->flags &IORING_ASYNC_CANCEL_FD )) ret =io_timeout_cancel (ctx, cd); -out:spin_unlock (&ctx->completion_lock); +out:return ret; }
@@ -192,3 +192,13 @@ int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags)io_req_set_res (req, ret,0 );return IOU_OK ; } + +void init_hash_table (struct io_hash_bucket *hash_table,unsigned size) +{ +unsigned int i; + +for (i =0 ; i < size; i++) { +spin_lock_init (&hash_table[i].lock); +INIT_HLIST_HEAD (&hash_table[i].list); + } +}
io_uring/cancel.h +6 -0
@@ -4,3 +4,9 @@ int io_async_cancel_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe);int io_async_cancel (struct io_kiocb *req,unsigned int issue_flags);int io_try_cancel (struct io_kiocb *req,struct io_cancel_data *cd); +void init_hash_table (struct io_hash_bucket *hash_table,unsigned size); + +struct io_hash_bucket { +spinlock_t lock; +struct hlist_head list; +}____cacheline_aligned_in_smp ;
io_uring/fdinfo.c +6 -3
@@ -13,6 +13,7 @@#include "io_uring.h" #include "sqpoll.h" #include "fdinfo.h" +#include "cancel.h" #ifdef CONFIG_PROC_FS static __cold int io_uring_show_cred(struct seq_file *m, unsigned int id,
@@ -157,17 +158,19 @@ static __cold void __io_uring_show_fdinfo(struct io_ring_ctx *ctx,mutex_unlock (&ctx->uring_lock);seq_puts (m,"PollList:\n" ); -spin_lock (&ctx->completion_lock);for (i =0 ; i < (1U << ctx->cancel_hash_bits); i++) { -struct hlist_head *list = &ctx->cancel_hash[i]; +struct io_hash_bucket *hb = &ctx->cancel_hash[i];struct io_kiocb *req; -hlist_for_each_entry (req, list, hash_node) +spin_lock (&hb->lock); +hlist_for_each_entry (req, &hb->list, hash_node)seq_printf (m," op=%d, task_works=%d\n" , req->opcode,task_work_pending (req->task)); +spin_unlock (&hb->lock); }seq_puts (m,"CqOverflowList:\n" ); +spin_lock (&ctx->completion_lock);list_for_each_entry (ocqe, &ctx->cq_overflow_list, list) {struct io_uring_cqe *cqe = &ocqe->cqe;
io_uring/io_uring.c +6 -3
@@ -89,6 +89,7 @@#include "fdinfo.h" #include "kbuf.h" #include "rsrc.h" +#include "cancel.h" #include "timeout.h" #include "poll.h"
@@ -260,11 +261,13 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)if (hash_bits <=0 ) hash_bits =1 ; ctx->cancel_hash_bits = hash_bits; - ctx->cancel_hash =kmalloc ((1U << hash_bits) *sizeof (struct hlist_head ), -GFP_KERNEL ); + ctx->cancel_hash = +kmalloc ((1U << hash_bits) *sizeof (struct io_hash_bucket ), +GFP_KERNEL );if (!ctx->cancel_hash) goto err; -__hash_init (ctx->cancel_hash,1U << hash_bits); + +init_hash_table (ctx->cancel_hash,1U << hash_bits); ctx->dummy_ubuf =kzalloc (sizeof (*ctx->dummy_ubuf),GFP_KERNEL );if (!ctx->dummy_ubuf)
io_uring/io_uring_types.h +1 -1
@@ -224,7 +224,7 @@ struct io_ring_ctx {* manipulate the list, hence no extra locking is needed there. */ struct io_wq_work_list iopoll_list; -struct hlist_head *cancel_hash; +struct io_hash_bucket *cancel_hash;unsigned cancel_hash_bits;bool poll_multi_queue;
io_uring/poll.c +49 -31
198 lines not on this page · raw message
Signature · 1 line
2.36.1
PATCH v3 7/16 io_uring: pass poll_find lock back
In reply to Pavel BegunkovInstead of using implicit knowledge of what is locked or not after io_poll_find() and co returns, pass back a pointer to the locked bucket if any. If set the user must to unlock the spinlock.
Signed-off-by Pavel Begunkov <[email protected]>
--- io_uring/poll.c | 46 ++++++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 20 deletions(-)
io_uring/poll.c +26 -20
@@ -559,12 +559,15 @@ __cold bool io_poll_remove_all(struct io_ring_ctx *ctx, struct task_struct *tsk, }static struct io_kiocb *io_poll_find (struct io_ring_ctx *ctx,bool poll_only, -struct io_cancel_data *cd) +struct io_cancel_data *cd, +struct io_hash_bucket **out_bucket) {struct io_kiocb *req;u32 index =hash_long (cd->data, ctx->cancel_hash_bits);struct io_hash_bucket *hb = &ctx->cancel_hash[index]; + *out_bucket =NULL ; +spin_lock (&hb->lock);hlist_for_each_entry (req, &hb->list, hash_node) {if (cd->data != req->cqe.user_data)
@@ -576,6 +579,7 @@ static struct io_kiocb *io_poll_find(struct io_ring_ctx *ctx, bool poll_only,continue ; req->work.cancel_seq = cd->seq; } + *out_bucket = hb;return req; }spin_unlock (&hb->lock);
@@ -583,11 +587,14 @@ static struct io_kiocb *io_poll_find(struct io_ring_ctx *ctx, bool poll_only, }static struct io_kiocb *io_poll_file_find (struct io_ring_ctx *ctx, -struct io_cancel_data *cd) +struct io_cancel_data *cd, +struct io_hash_bucket **out_bucket) {struct io_kiocb *req;int i; + *out_bucket =NULL ; +for (i =0 ; i < (1U << ctx->cancel_hash_bits); i++) {struct io_hash_bucket *hb = &ctx->cancel_hash[i];
@@ -599,6 +606,7 @@ static struct io_kiocb *io_poll_file_find(struct io_ring_ctx *ctx,if (cd->seq == req->work.cancel_seq)continue ; req->work.cancel_seq = cd->seq; + *out_bucket = hb;return req; }spin_unlock (&hb->lock);
@@ -617,23 +625,19 @@ static bool io_poll_disarm(struct io_kiocb *req)int io_poll_cancel (struct io_ring_ctx *ctx,struct io_cancel_data *cd) { +struct io_hash_bucket *bucket;struct io_kiocb *req; -u32 index; -spinlock_t *lock;if (cd->flags & (IORING_ASYNC_CANCEL_FD |IORING_ASYNC_CANCEL_ANY )) - req =io_poll_file_find (ctx, cd); + req =io_poll_file_find (ctx, cd, &bucket);else - req =io_poll_find (ctx, false, cd); -if (!req) { -return -ENOENT ; - }else { - index =hash_long (req->cqe.user_data, ctx->cancel_hash_bits); - lock = &ctx->cancel_hash[index].lock; - } -io_poll_cancel_req (req); -spin_unlock (lock); -return 0 ; + req =io_poll_find (ctx, false, cd, &bucket); + +if (req) +io_poll_cancel_req (req); +if (bucket) +spin_unlock (&bucket->lock); +return req ?0 : -ENOENT ; } static __poll_t io_poll_parse_events(const struct io_uring_sqe *sqe,
@@ -726,19 +730,21 @@ int io_poll_remove(struct io_kiocb *req, unsigned int issue_flags)struct io_poll_update *poll_update =io_kiocb_to_cmd (req);struct io_cancel_data cd = { .data = poll_update->old_user_data, };struct io_ring_ctx *ctx = req->ctx; -u32 index =hash_long (cd.data, ctx->cancel_hash_bits); -spinlock_t *lock = &ctx->cancel_hash[index].lock; +struct io_hash_bucket *bucket;struct io_kiocb *preq;int ret2, ret =0 ;bool locked; - preq =io_poll_find (ctx, true, &cd); + preq =io_poll_find (ctx, true, &cd, &bucket); +if (preq) + ret2 =io_poll_disarm (preq); +if (bucket) +spin_unlock (&bucket->lock); +if (!preq) { ret = -ENOENT ; goto out; } - ret2 =io_poll_disarm (preq); -spin_unlock (lock);if (!ret2) { ret = -EALREADY ; goto out;
Signature · 1 line
2.36.1
PATCH v3 8/16 io_uring: clean up io_try_cancel
In reply to Pavel BegunkovGet rid of an unnecessary extra goto in io_try_cancel() and simplify the function.
Signed-off-by Pavel Begunkov <[email protected]>
--- io_uring/cancel.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
io_uring/cancel.c +2 -2
@@ -95,12 +95,12 @@ int io_try_cancel(struct io_kiocb *req, struct io_cancel_data *cd) ret =io_poll_cancel (ctx, cd);if (ret != -ENOENT ) - goto out; +return ret; +spin_lock (&ctx->completion_lock);if (!(cd->flags &IORING_ASYNC_CANCEL_FD )) ret =io_timeout_cancel (ctx, cd);spin_unlock (&ctx->completion_lock); -out:return ret; }
Signature · 1 line
2.36.1
PATCH v3 9/16 io_uring: limit the number of cancellation buckets
In reply to Pavel BegunkovDon't allocate to many hash/cancellation buckets, there might be too many, clamp it to 8 bits, or 256 * 64B = 16KB. We don't usually have too many requests, and 256 buckets should be enough, especially since we do hash search only in the cancellation path.
Signed-off-by Pavel Begunkov <[email protected]>
--- io_uring/io_uring.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
io_uring/io_uring.c +5 -5
@@ -254,12 +254,12 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)/* * Use 5 bits less than the max cq entries, that should give us around -* 32 entries per hash list if totally full and uniformly spread. +* 32 entries per hash list if totally full and uniformly spread, but +* don't keep too many buckets to not overconsume memory. */ - hash_bits =ilog2 (p->cq_entries); - hash_bits -=5 ; -if (hash_bits <=0 ) - hash_bits =1 ; + hash_bits =ilog2 (p->cq_entries) -5 ; + hash_bits =clamp (hash_bits,1 ,8 ); + ctx->cancel_hash_bits = hash_bits; ctx->cancel_hash = kmalloc((1U << hash_bits) * sizeof(struct io_hash_bucket),
Signature · 1 line
2.36.1
PATCH v3 10/16 io_uring: clean up io_ring_ctx_alloc
In reply to Pavel BegunkovAdd a variable for the number of hash buckets in io_ring_ctx_alloc(), makes it more readable.
Signed-off-by Pavel Begunkov <[email protected]>
--- io_uring/io_uring.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
io_uring/io_uring.c +6 -4
@@ -244,6 +244,8 @@ static __cold void io_fallback_req_func(struct work_struct *work)static __cold struct io_ring_ctx *io_ring_ctx_alloc (struct io_uring_params *p) {struct io_ring_ctx *ctx; +unsigned hash_buckets; +size_t hash_size;int hash_bits; ctx =kzalloc (sizeof (*ctx),GFP_KERNEL );
@@ -259,15 +261,15 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)*/ hash_bits =ilog2 (p->cq_entries) -5 ; hash_bits =clamp (hash_bits,1 ,8 ); + hash_buckets =1U << hash_bits; + hash_size = hash_buckets *sizeof (struct io_hash_bucket ); ctx->cancel_hash_bits = hash_bits; - ctx->cancel_hash = -kmalloc ((1U << hash_bits) *sizeof (struct io_hash_bucket ), -GFP_KERNEL ); + ctx->cancel_hash =kmalloc (hash_size,GFP_KERNEL );if (!ctx->cancel_hash) goto err; -init_hash_table (ctx->cancel_hash,1U << hash_bits); +init_hash_table (ctx->cancel_hash, hash_buckets); ctx->dummy_ubuf =kzalloc (sizeof (*ctx->dummy_ubuf),GFP_KERNEL );if (!ctx->dummy_ubuf)
Signature · 1 line
2.36.1
PATCH v3 11/16 io_uring: use state completion infra for poll reqs
In reply to Pavel BegunkovUse io_req_task_complete() for poll request completions, so it can utilise state completions and save lots of unnecessary locking.
Signed-off-by Pavel Begunkov <[email protected]>
--- io_uring/poll.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
io_uring/poll.c +2 -6
@@ -234,12 +234,8 @@ static void io_poll_task_func(struct io_kiocb *req, bool *locked)io_poll_remove_entries (req);io_poll_req_delete (req, ctx); -spin_lock (&ctx->completion_lock); - req->cqe.flags =0 ; -__io_req_complete_post (req); -io_commit_cqring (ctx); -spin_unlock (&ctx->completion_lock); -io_cqring_ev_posted (ctx); +io_req_set_res (req, req->cqe.res,0 ); +io_req_task_complete (req, locked); }static void io_apoll_task_func (struct io_kiocb *req,bool *locked)
Signature · 1 line
2.36.1
PATCH v3 12/16 io_uring: add IORING_SETUP_SINGLE_ISSUER
In reply to Pavel BegunkovAdd a new IORING_SETUP_SINGLE_ISSUER flag and the userspace visible part of it, i.e. put limitations of submitters. Also, don't allow it together with IOPOLL as we're not going to put it to good use.
Signed-off-by Pavel Begunkov <[email protected]>
--- include/uapi/linux/io_uring.h | 5 ++++- io_uring/io_uring.c | 7 +++++-- io_uring/io_uring_types.h | 1 + io_uring/tctx.c | 27 ++++++++++++++++++++++++--- io_uring/tctx.h | 4 ++-- 5 files changed, 36 insertions(+), 8 deletions(-)
include/uapi/linux/io_uring.h +4 -1
@@ -137,9 +137,12 @@ enum {* IORING_SQ_TASKRUN in the sq ring flags. Not valid with COOP_TASKRUN. */ #define IORING_SETUP_TASKRUN_FLAG (1U << 9) -#define IORING_SETUP_SQE128 (1U << 10)/* SQEs are 128 byte */ #define IORING_SETUP_CQE32 (1U << 11)/* CQEs are 32 byte */ +/* +* Only one task is allowed to submit requests +*/ +#define IORING_SETUP_SINGLE_ISSUER (1U << 12)enum io_uring_op {IORING_OP_NOP ,
io_uring/io_uring.c +5 -2
@@ -2469,6 +2469,8 @@ static __cold void io_ring_ctx_free(struct io_ring_ctx *ctx)io_destroy_buffers (ctx);if (ctx->sq_creds)put_cred (ctx->sq_creds); +if (ctx->submitter_task) +put_task_struct (ctx->submitter_task);/* there are no registered resources left, nobody uses it */ if (ctx->rsrc_node)
@@ -3201,7 +3203,7 @@ static int io_uring_install_fd(struct io_ring_ctx *ctx, struct file *file)if (fd <0 )return fd; - ret =io_uring_add_tctx_node (ctx); + ret =__io_uring_add_tctx_node (ctx, false);if (ret) {put_unused_fd (fd);return ret;
@@ -3421,7 +3423,8 @@ static long io_uring_setup(u32 entries, struct io_uring_params __user *params)IORING_SETUP_CLAMP |IORING_SETUP_ATTACH_WQ |IORING_SETUP_R_DISABLED |IORING_SETUP_SUBMIT_ALL |IORING_SETUP_COOP_TASKRUN |IORING_SETUP_TASKRUN_FLAG | - IORING_SETUP_SQE128 | IORING_SETUP_CQE32)) +IORING_SETUP_SQE128 |IORING_SETUP_CQE32 | + IORING_SETUP_SINGLE_ISSUER)) return -EINVAL;return io_uring_create (entries, &p, params);
io_uring/io_uring_types.h +1 -0
@@ -243,6 +243,7 @@ struct io_ring_ctx {/* Keep this last, we don't need it for the fast path */ struct io_restriction restrictions; +struct task_struct *submitter_task;/* slow path rsrc auxilary data, used by update/register */ struct io_rsrc_node *rsrc_backup_node;
io_uring/tctx.c +24 -3
@@ -81,12 +81,32 @@ __cold int io_uring_alloc_task_context(struct task_struct *task,return 0 ; } -int __io_uring_add_tctx_node (struct io_ring_ctx *ctx) +static int io_register_submitter (struct io_ring_ctx *ctx) +{ +int ret =0 ; + +mutex_lock (&ctx->uring_lock); +if (!ctx->submitter_task) + ctx->submitter_task =get_task_struct (current); +else if (ctx->submitter_task != current) + ret = -EEXIST ; +mutex_unlock (&ctx->uring_lock); + +return ret; +} + +int __io_uring_add_tctx_node (struct io_ring_ctx *ctx,bool submitter) {struct io_uring_task *tctx = current->io_uring;struct io_tctx_node *node;int ret; +if ((ctx->flags &IORING_SETUP_SINGLE_ISSUER ) && submitter) { + ret =io_register_submitter (ctx); +if (ret) +return ret; + } +if (unlikely (!tctx)) { ret =io_uring_alloc_task_context (current, ctx);if (unlikely (ret))
@@ -120,7 +140,8 @@ int __io_uring_add_tctx_node(struct io_ring_ctx *ctx)list_add (&node->ctx_node, &ctx->tctx_list);mutex_unlock (&ctx->uring_lock); } - tctx->last = ctx; +if (submitter) + tctx->last = ctx;return 0 ; }
@@ -228,7 +249,7 @@ int io_ringfd_register(struct io_ring_ctx *ctx, void __user *__arg,return -EINVAL ;mutex_unlock (&ctx->uring_lock); - ret =io_uring_add_tctx_node (ctx); + ret =__io_uring_add_tctx_node (ctx, false);mutex_lock (&ctx->uring_lock);if (ret)return ret;
io_uring/tctx.h +2 -2
@@ -34,7 +34,7 @@ struct io_tctx_node {int io_uring_alloc_task_context (struct task_struct *task,struct io_ring_ctx *ctx);void io_uring_del_tctx_node (unsigned long index); -int __io_uring_add_tctx_node (struct io_ring_ctx *ctx); +int __io_uring_add_tctx_node (struct io_ring_ctx *ctx,bool submitter);void io_uring_clean_tctx (struct io_uring_task *tctx);void io_uring_unreg_ringfd (void );
@@ -52,5 +52,5 @@ static inline int io_uring_add_tctx_node(struct io_ring_ctx *ctx)if (likely (tctx && tctx->last == ctx))return 0 ; -return __io_uring_add_tctx_node (ctx); +return __io_uring_add_tctx_node (ctx, true); }
Signature · 1 line
2.36.1
PATCH v3 13/16 io_uring: pass hash table into poll_find
In reply to Pavel BegunkovIn preparation for having multiple cancellation hash tables, pass a table pointer into io_poll_find() and other poll cancel functions.
Signed-off-by Pavel Begunkov <[email protected]>
--- io_uring/poll.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
io_uring/poll.c +14 -6
@@ -556,11 +556,12 @@ __cold bool io_poll_remove_all(struct io_ring_ctx *ctx, struct task_struct *tsk,static struct io_kiocb *io_poll_find (struct io_ring_ctx *ctx,bool poll_only,struct io_cancel_data *cd, +struct io_hash_bucket hash_table[],struct io_hash_bucket **out_bucket) {struct io_kiocb *req;u32 index =hash_long (cd->data, ctx->cancel_hash_bits); -struct io_hash_bucket *hb = &ctx->cancel_hash[index]; +struct io_hash_bucket *hb = &hash_table[index]; *out_bucket =NULL ;
@@ -584,6 +585,7 @@ static struct io_kiocb *io_poll_find(struct io_ring_ctx *ctx, bool poll_only,static struct io_kiocb *io_poll_file_find (struct io_ring_ctx *ctx,struct io_cancel_data *cd, +struct io_hash_bucket hash_table[],struct io_hash_bucket **out_bucket) {struct io_kiocb *req;
@@ -592,7 +594,7 @@ static struct io_kiocb *io_poll_file_find(struct io_ring_ctx *ctx, *out_bucket =NULL ;for (i =0 ; i < (1U << ctx->cancel_hash_bits); i++) { -struct io_hash_bucket *hb = &ctx->cancel_hash[i]; +struct io_hash_bucket *hb = &hash_table[i];spin_lock (&hb->lock);hlist_for_each_entry (req, &hb->list, hash_node) {
@@ -619,15 +621,16 @@ static bool io_poll_disarm(struct io_kiocb *req)return true; } -int io_poll_cancel (struct io_ring_ctx *ctx,struct io_cancel_data *cd) +static int __io_poll_cancel (struct io_ring_ctx *ctx,struct io_cancel_data *cd, +struct io_hash_bucket hash_table[]) {struct io_hash_bucket *bucket;struct io_kiocb *req;if (cd->flags & (IORING_ASYNC_CANCEL_FD |IORING_ASYNC_CANCEL_ANY )) - req =io_poll_file_find (ctx, cd, &bucket); + req =io_poll_file_find (ctx, cd, ctx->cancel_hash, &bucket);else - req =io_poll_find (ctx, false, cd, &bucket); + req =io_poll_find (ctx, false, cd, ctx->cancel_hash, &bucket);if (req)io_poll_cancel_req (req);
@@ -636,6 +639,11 @@ int io_poll_cancel(struct io_ring_ctx *ctx, struct io_cancel_data *cd)return req ?0 : -ENOENT ; } +int io_poll_cancel (struct io_ring_ctx *ctx,struct io_cancel_data *cd) +{ +return __io_poll_cancel (ctx, cd, ctx->cancel_hash); +} +static __poll_t io_poll_parse_events (const struct io_uring_sqe *sqe,unsigned int flags) {
@@ -731,7 +739,7 @@ int io_poll_remove(struct io_kiocb *req, unsigned int issue_flags)int ret2, ret =0 ;bool locked; - preq =io_poll_find (ctx, true, &cd, &bucket); + preq =io_poll_find (ctx, true, &cd, ctx->cancel_hash, &bucket);if (preq) ret2 =io_poll_disarm (preq);if (bucket)
Signature · 1 line
2.36.1
PATCH v3 14/16 io_uring: introduce a struct for hash table
In reply to Pavel BegunkovInstead of passing around a pointer to hash buckets, add a bit of type safety and wrap it into a structure.
Signed-off-by Pavel Begunkov <[email protected]>
--- io_uring/cancel.c | 6 +++--- io_uring/cancel.h | 7 +------ io_uring/fdinfo.c | 4 ++-- io_uring/io_uring.c | 29 ++++++++++++++++------------ io_uring/io_uring_types.h | 13 +++++++++++-- io_uring/poll.c | 40 +++++++++++++++++++++------------------ 6 files changed, 56 insertions(+), 43 deletions(-)
io_uring/cancel.c +3 -3
@@ -193,12 +193,12 @@ int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags)return IOU_OK ; } -void init_hash_table (struct io_hash_bucket *hash_table,unsigned size) +void init_hash_table (struct io_hash_table *table,unsigned size) {unsigned int i;for (i =0 ; i < size; i++) { -spin_lock_init (&hash_table[i].lock); -INIT_HLIST_HEAD (&hash_table[i].list); +spin_lock_init (&table->hbs[i].lock); +INIT_HLIST_HEAD (&table->hbs[i].list); } }
io_uring/cancel.h +1 -6
@@ -4,9 +4,4 @@ int io_async_cancel_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe);int io_async_cancel (struct io_kiocb *req,unsigned int issue_flags);int io_try_cancel (struct io_kiocb *req,struct io_cancel_data *cd); -void init_hash_table (struct io_hash_bucket *hash_table,unsigned size); - -struct io_hash_bucket { -spinlock_t lock; -struct hlist_head list; -}____cacheline_aligned_in_smp ; +void init_hash_table (struct io_hash_table *table,unsigned size);
io_uring/fdinfo.c +2 -2
@@ -158,8 +158,8 @@ static __cold void __io_uring_show_fdinfo(struct io_ring_ctx *ctx,mutex_unlock (&ctx->uring_lock);seq_puts (m,"PollList:\n" ); -for (i =0 ; i < (1U << ctx->cancel_hash_bits); i++) { -struct io_hash_bucket *hb = &ctx->cancel_hash[i]; +for (i =0 ; i < (1U << ctx->cancel_table.hash_bits); i++) { +struct io_hash_bucket *hb = &ctx->cancel_table.hbs[i];struct io_kiocb *req;spin_lock (&hb->lock);
io_uring/io_uring.c +17 -12
@@ -241,11 +241,23 @@ static __cold void io_fallback_req_func(struct work_struct *work)percpu_ref_put (&ctx->refs); } +static int io_alloc_hash_table (struct io_hash_table *table,unsigned bits) +{ +unsigned hash_buckets =1U << bits; +size_t hash_size = hash_buckets *sizeof (table->hbs[0 ]); + + table->hbs =kmalloc (hash_size,GFP_KERNEL ); +if (!table->hbs) +return -ENOMEM ; + + table->hash_bits = bits; +init_hash_table (table, hash_buckets); +return 0 ; +} +static __cold struct io_ring_ctx *io_ring_ctx_alloc (struct io_uring_params *p) {struct io_ring_ctx *ctx; -unsigned hash_buckets; -size_t hash_size;int hash_bits; ctx =kzalloc (sizeof (*ctx),GFP_KERNEL );
@@ -261,16 +273,9 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)*/ hash_bits =ilog2 (p->cq_entries) -5 ; hash_bits =clamp (hash_bits,1 ,8 ); - hash_buckets =1U << hash_bits; - hash_size = hash_buckets *sizeof (struct io_hash_bucket ); - - ctx->cancel_hash_bits = hash_bits; - ctx->cancel_hash =kmalloc (hash_size,GFP_KERNEL ); -if (!ctx->cancel_hash) +if (io_alloc_hash_table (&ctx->cancel_table, hash_bits)) goto err; -init_hash_table (ctx->cancel_hash, hash_buckets); - ctx->dummy_ubuf =kzalloc (sizeof (*ctx->dummy_ubuf),GFP_KERNEL );if (!ctx->dummy_ubuf) goto err;
@@ -311,7 +316,7 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)return ctx; err:kfree (ctx->dummy_ubuf); -kfree (ctx->cancel_hash); +kfree (ctx->cancel_table.hbs);kfree (ctx->io_bl);xa_destroy (&ctx->io_bl_xa);kfree (ctx);
@@ -2499,7 +2504,7 @@ static __cold void io_ring_ctx_free(struct io_ring_ctx *ctx)io_req_caches_free (ctx);if (ctx->hash_map)io_wq_put_hash (ctx->hash_map); -kfree (ctx->cancel_hash); +kfree (ctx->cancel_table.hbs);kfree (ctx->dummy_ubuf);kfree (ctx->io_bl);xa_destroy (&ctx->io_bl_xa);
io_uring/io_uring_types.h +11 -2
@@ -9,6 +9,16 @@#include "io-wq.h" #include "filetable.h" +struct io_hash_bucket { +spinlock_t lock; +struct hlist_head list; +}____cacheline_aligned_in_smp ; + +struct io_hash_table { +struct io_hash_bucket *hbs; +unsigned hash_bits; +}; +struct io_uring {u32 head____cacheline_aligned_in_smp ;u32 tail____cacheline_aligned_in_smp ;
@@ -224,8 +234,7 @@ struct io_ring_ctx {* manipulate the list, hence no extra locking is needed there. */ struct io_wq_work_list iopoll_list; -struct io_hash_bucket *cancel_hash; -unsigned cancel_hash_bits; +struct io_hash_table cancel_table;bool poll_multi_queue;struct list_head io_buffers_comp;
io_uring/poll.c +22 -18
@@ -73,9 +73,9 @@ static struct io_poll *io_poll_get_single(struct io_kiocb *req)static void io_poll_req_insert (struct io_kiocb *req) { -struct io_ring_ctx *ctx = req->ctx; -u32 index =hash_long (req->cqe.user_data, ctx->cancel_hash_bits); -struct io_hash_bucket *hb = &ctx->cancel_hash[index]; +struct io_hash_table *table = &req->ctx->cancel_table; +u32 index =hash_long (req->cqe.user_data, table->hash_bits); +struct io_hash_bucket *hb = &table->hbs[index];spin_lock (&hb->lock);hlist_add_head (&req->hash_node, &hb->list);
@@ -84,8 +84,9 @@ static void io_poll_req_insert(struct io_kiocb *req)static void io_poll_req_delete (struct io_kiocb *req,struct io_ring_ctx *ctx) { -u32 index =hash_long (req->cqe.user_data, ctx->cancel_hash_bits); -spinlock_t *lock = &ctx->cancel_hash[index].lock; +struct io_hash_table *table = &req->ctx->cancel_table; +u32 index =hash_long (req->cqe.user_data, table->hash_bits); +spinlock_t *lock = &table->hbs[index].lock;spin_lock (lock);hash_del (&req->hash_node);
@@ -533,13 +534,15 @@ int io_arm_poll_handler(struct io_kiocb *req, unsigned issue_flags)__cold bool io_poll_remove_all (struct io_ring_ctx *ctx,struct task_struct *tsk,bool cancel_all) { +struct io_hash_table *table = &ctx->cancel_table; +unsigned nr_buckets =1U << table->hash_bits;struct hlist_node *tmp;struct io_kiocb *req;bool found = false;int i; -for (i =0 ; i < (1U << ctx->cancel_hash_bits); i++) { -struct io_hash_bucket *hb = &ctx->cancel_hash[i]; +for (i =0 ; i < nr_buckets; i++) { +struct io_hash_bucket *hb = &table->hbs[i];spin_lock (&hb->lock);hlist_for_each_entry_safe (req, tmp, &hb->list, hash_node) {
@@ -556,12 +559,12 @@ __cold bool io_poll_remove_all(struct io_ring_ctx *ctx, struct task_struct *tsk,static struct io_kiocb *io_poll_find (struct io_ring_ctx *ctx,bool poll_only,struct io_cancel_data *cd, -struct io_hash_bucket hash_table[], +struct io_hash_table *table,struct io_hash_bucket **out_bucket) {struct io_kiocb *req; -u32 index =hash_long (cd->data, ctx->cancel_hash_bits); -struct io_hash_bucket *hb = &hash_table[index]; +u32 index =hash_long (cd->data, table->hash_bits); +struct io_hash_bucket *hb = &table->hbs[index]; *out_bucket =NULL ;
@@ -585,16 +588,17 @@ static struct io_kiocb *io_poll_find(struct io_ring_ctx *ctx, bool poll_only,static struct io_kiocb *io_poll_file_find (struct io_ring_ctx *ctx,struct io_cancel_data *cd, -struct io_hash_bucket hash_table[], +struct io_hash_table *table,struct io_hash_bucket **out_bucket) { +unsigned nr_buckets =1U << table->hash_bits;struct io_kiocb *req;int i; *out_bucket =NULL ; -for (i =0 ; i < (1U << ctx->cancel_hash_bits); i++) { -struct io_hash_bucket *hb = &hash_table[i]; +for (i =0 ; i < nr_buckets; i++) { +struct io_hash_bucket *hb = &table->hbs[i];spin_lock (&hb->lock);hlist_for_each_entry (req, &hb->list, hash_node) {
@@ -622,15 +626,15 @@ static bool io_poll_disarm(struct io_kiocb *req) }static int __io_poll_cancel (struct io_ring_ctx *ctx,struct io_cancel_data *cd, -struct io_hash_bucket hash_table[]) +struct io_hash_table *table) {struct io_hash_bucket *bucket;struct io_kiocb *req;if (cd->flags & (IORING_ASYNC_CANCEL_FD |IORING_ASYNC_CANCEL_ANY )) - req =io_poll_file_find (ctx, cd, ctx->cancel_hash, &bucket); + req =io_poll_file_find (ctx, cd, table, &bucket);else - req =io_poll_find (ctx, false, cd, ctx->cancel_hash, &bucket); + req =io_poll_find (ctx, false, cd, table, &bucket);if (req)io_poll_cancel_req (req);
@@ -641,7 +645,7 @@ static int __io_poll_cancel(struct io_ring_ctx *ctx, struct io_cancel_data *cd,int io_poll_cancel (struct io_ring_ctx *ctx,struct io_cancel_data *cd) { -return __io_poll_cancel (ctx, cd, ctx->cancel_hash); +return __io_poll_cancel (ctx, cd, &ctx->cancel_table); } static __poll_t io_poll_parse_events(const struct io_uring_sqe *sqe,
@@ -739,7 +743,7 @@ int io_poll_remove(struct io_kiocb *req, unsigned int issue_flags)int ret2, ret =0 ;bool locked; - preq =io_poll_find (ctx, true, &cd, ctx->cancel_hash, &bucket); + preq =io_poll_find (ctx, true, &cd, &ctx->cancel_table, &bucket);if (preq) ret2 =io_poll_disarm (preq);if (bucket)
Signature · 1 line
2.36.1
PATCH v3 15/16 io_uring: propagate locking state to poll cancel
In reply to Pavel BegunkovPoll cancellation will be soon need to grab ->uring_lock inside, pass the locking state, i.e. issue_flags, inside the cancellation functions.
Signed-off-by Pavel Begunkov <[email protected]>
--- io_uring/cancel.c | 7 ++++--- io_uring/cancel.h | 3 ++- io_uring/poll.c | 3 ++- io_uring/poll.h | 3 ++- io_uring/timeout.c | 3 ++- 5 files changed, 12 insertions(+), 7 deletions(-)
io_uring/cancel.c +4 -3
@@ -78,7 +78,8 @@ static int io_async_cancel_one(struct io_uring_task *tctx,return ret; } -int io_try_cancel (struct io_kiocb *req,struct io_cancel_data *cd) +int io_try_cancel (struct io_kiocb *req,struct io_cancel_data *cd, +unsigned issue_flags) {struct io_ring_ctx *ctx = req->ctx;int ret;
@@ -93,7 +94,7 @@ int io_try_cancel(struct io_kiocb *req, struct io_cancel_data *cd)if (!ret)return 0 ; - ret =io_poll_cancel (ctx, cd); + ret =io_poll_cancel (ctx, cd, issue_flags);if (ret != -ENOENT )return ret;
@@ -136,7 +137,7 @@ static int __io_async_cancel(struct io_cancel_data *cd, struct io_kiocb *req,int ret, nr =0 ; do { - ret = io_try_cancel(req, cd); + ret = io_try_cancel(req, cd, issue_flags); if (ret == -ENOENT) break; if (!all)
io_uring/cancel.h +2 -1
@@ -3,5 +3,6 @@int io_async_cancel_prep (struct io_kiocb *req,const struct io_uring_sqe *sqe);int io_async_cancel (struct io_kiocb *req,unsigned int issue_flags); -int io_try_cancel (struct io_kiocb *req,struct io_cancel_data *cd); +int io_try_cancel (struct io_kiocb *req,struct io_cancel_data *cd, +unsigned int issue_flags);void init_hash_table (struct io_hash_table *table,unsigned size);
io_uring/poll.c +2 -1
@@ -643,7 +643,8 @@ static int __io_poll_cancel(struct io_ring_ctx *ctx, struct io_cancel_data *cd,return req ?0 : -ENOENT ; } -int io_poll_cancel (struct io_ring_ctx *ctx,struct io_cancel_data *cd) +int io_poll_cancel (struct io_ring_ctx *ctx,struct io_cancel_data *cd, +unsigned issue_flags) {return __io_poll_cancel (ctx, cd, &ctx->cancel_table); }
io_uring/poll.h +2 -1
@@ -24,7 +24,8 @@ int io_poll_add(struct io_kiocb *req, unsigned int issue_flags);int io_poll_remove_prep (struct io_kiocb *req,const struct io_uring_sqe *sqe);int io_poll_remove (struct io_kiocb *req,unsigned int issue_flags); -int io_poll_cancel (struct io_ring_ctx *ctx,struct io_cancel_data *cd); +int io_poll_cancel (struct io_ring_ctx *ctx,struct io_cancel_data *cd, +unsigned issue_flags);int io_arm_poll_handler (struct io_kiocb *req,unsigned issue_flags);bool io_poll_remove_all (struct io_ring_ctx *ctx,struct task_struct *tsk,bool cancel_all);
io_uring/timeout.c +2 -1
@@ -262,6 +262,7 @@ int io_timeout_cancel(struct io_ring_ctx *ctx, struct io_cancel_data *cd)static void io_req_task_link_timeout (struct io_kiocb *req,bool *locked) { +unsigned issue_flags = *locked ?0 :IO_URING_F_UNLOCKED ;struct io_timeout *timeout =io_kiocb_to_cmd (req);struct io_kiocb *prev = timeout->prev;int ret = -ENOENT ;
@@ -273,7 +274,7 @@ static void io_req_task_link_timeout(struct io_kiocb *req, bool *locked) .data = prev->cqe.user_data, }; - ret =io_try_cancel (req, &cd); + ret =io_try_cancel (req, &cd, issue_flags); }io_req_set_res (req, ret ?: -ETIME ,0 );io_req_complete_post (req);
Signature · 1 line
2.36.1
PATCH v3 16/16 io_uring: mutex locked poll hashing
In reply to Pavel BegunkovCurrently we do two extra spin lock/unlock pairs to add a poll/apoll request to the cancellation hash table and remove it from there.
On the submission side we often already hold ->uring_lock and tw completion is likely to hold it as well. Add a second cancellation hash table protected by ->uring_lock. In concerns for latency because of a need to have the mutex locked on the completion side, use the new table only in following cases:
1) IORING_SETUP_SINGLE_ISSUER: only one task grabs uring_lock, so there is little to no contention and so the main tw hander will almost always end up grabbing it before calling callbacks.
2) IORING_SETUP_SQPOLL: same as with single issuer, only one task is a major user of ->uring_lock.
3) apoll: we normally grab the lock on the completion side anyway to execute the request, so it's free.
Signed-off-by Pavel Begunkov <[email protected]>
--- io_uring/io_uring.c | 9 ++- io_uring/io_uring_types.h | 4 ++ io_uring/poll.c | 117 +++++++++++++++++++++++++++++++------- 3 files changed, 108 insertions(+), 22 deletions(-)
io_uring/io_uring.c +8 -1
@@ -275,6 +275,8 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p) hash_bits =clamp (hash_bits,1 ,8 );if (io_alloc_hash_table (&ctx->cancel_table, hash_bits)) goto err; +if (io_alloc_hash_table (&ctx->cancel_table_locked, hash_bits)) + goto err; ctx->dummy_ubuf =kzalloc (sizeof (*ctx->dummy_ubuf),GFP_KERNEL );if (!ctx->dummy_ubuf)
@@ -317,6 +319,7 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p) err:kfree (ctx->dummy_ubuf);kfree (ctx->cancel_table.hbs); +kfree (ctx->cancel_table_locked.hbs);kfree (ctx->io_bl);xa_destroy (&ctx->io_bl_xa);kfree (ctx);
@@ -2505,6 +2508,7 @@ static __cold void io_ring_ctx_free(struct io_ring_ctx *ctx)if (ctx->hash_map)io_wq_put_hash (ctx->hash_map);kfree (ctx->cancel_table.hbs); +kfree (ctx->cancel_table_locked.hbs);kfree (ctx->dummy_ubuf);kfree (ctx->io_bl);xa_destroy (&ctx->io_bl_xa);
@@ -2666,12 +2670,13 @@ static __cold void io_ring_ctx_wait_and_kill(struct io_ring_ctx *ctx)__io_cqring_overflow_flush (ctx, true);xa_for_each (&ctx->personalities, index, creds)io_unregister_personality (ctx, index); +if (ctx->rings) +io_poll_remove_all (ctx,NULL , true);mutex_unlock (&ctx->uring_lock);/* failed during ring init, it couldn't have issued any requests */ if (ctx->rings) {io_kill_timeouts (ctx,NULL , true); -io_poll_remove_all (ctx,NULL , true);/* if we failed setting up the ctx, we might not have any rings */ io_iopoll_try_reap_events (ctx); }
@@ -2796,7 +2801,9 @@ static __cold void io_uring_try_cancel_requests(struct io_ring_ctx *ctx, } ret |=io_cancel_defer_files (ctx, task, cancel_all); +mutex_lock (&ctx->uring_lock); ret |=io_poll_remove_all (ctx, task, cancel_all); +mutex_unlock (&ctx->uring_lock); ret |=io_kill_timeouts (ctx, task, cancel_all);if (task) ret |=io_run_task_work ();
io_uring/io_uring_types.h +4 -0
@@ -191,6 +191,7 @@ struct io_ring_ctx {struct xarray io_bl_xa;struct list_head io_buffers_cache; +struct io_hash_table cancel_table_locked;struct list_head cq_overflow_list;struct list_head apoll_cache;struct xarray personalities;
@@ -323,6 +324,7 @@ enum {REQ_F_CQE32_INIT_BIT ,REQ_F_APOLL_MULTISHOT_BIT ,REQ_F_CLEAR_POLLIN_BIT , +REQ_F_HASH_LOCKED_BIT ,/* keep async read/write and isreg together and in order */ REQ_F_SUPPORT_NOWAIT_BIT , REQ_F_ISREG_BIT,
@@ -393,6 +395,8 @@ enum {REQ_F_CQE32_INIT =BIT (REQ_F_CQE32_INIT_BIT ),/* recvmsg special flag, clear EPOLLIN */ REQ_F_CLEAR_POLLIN =BIT (REQ_F_CLEAR_POLLIN_BIT ), +/* hashed into ->cancel_hash_locked, protected by ->uring_lock */ + REQ_F_HASH_LOCKED = BIT(REQ_F_HASH_LOCKED_BIT), };typedef void (*io_req_tw_func_t )(struct io_kiocb *req,bool *locked);
io_uring/poll.c +96 -21
201 lines not on this page · raw message
Signature · 1 line
2.36.1
Re: [PATCH for-next v3 16/16] io_uring: mutex locked poll hashing
Quoted 20 lines · Pavel Begunkov
Currently we do two extra spin lock/unlock pairs to add a poll/apoll request to the cancellation hash table and remove it from there.
On the submission side we often already hold ->uring_lock and tw completion is likely to hold it as well. Add a second cancellation hash table protected by ->uring_lock. In concerns for latency because of a need to have the mutex locked on the completion side, use the new table only in following cases:
1)
IORING_SETUP_SINGLE_ISSUER: only one task grabsuring_lock, so there is little to no contention and so the main tw hander will almost always end up grabbing it before calling callbacks.2)
IORING_SETUP_SQPOLL: same as with single issuer, only one task is a major user of ->uring_lock.3) apoll: we normally grab the lock on the completion side anyway to execute the request, so it's free.
Signed-off-by Pavel Begunkov <[email protected]>
<snip>
Quoted 17 lines
(quoted diff) +3
-6-/* - * Returns true if we found and killed one or more poll requests - */ -__cold bool io_poll_remove_all(struct io_ring_ctx *ctx, struct task_struct *tsk, - bool cancel_all) +static __cold bool io_poll_remove_all_table(struct task_struct *tsk, + struct io_hash_table *table, + bool cancel_all) { - struct io_hash_table *table = &ctx->cancel_table; unsigned nr_buckets = 1U << table->hash_bits; struct hlist_node *tmp; struct io_kiocb *req;@@ -557,6 +592,17 @@ __cold bool io_poll_remove_all(struct io_ring_ctx *ctx, struct task_struct *tsk, return found; }
Review __cold bool io_poll_remove_all(struct io_ring_ctx *ctx, struct task_struct *tsk, · line 604
Quoted 10 lines
(quoted diff) +10
-0+/* + * Returns true if we found and killed one or more poll requests + */ +__cold bool io_poll_remove_all(struct io_ring_ctx *ctx, struct task_struct *tsk, + bool cancel_all) + __must_hold(&ctx->uring_lock) +{ + return io_poll_remove_all_table(tsk, &ctx->cancel_table, cancel_all) | + io_poll_remove_all_table(tsk, &ctx->cancel_table_locked, cancel_all); +}
Clang warns:
io_uring/poll.c:602:9: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
return io_poll_remove_all_table(tsk, &ctx->cancel_table, cancel_all) |
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|| io_uring/poll.c:602:9: note: cast one or both operands to int to silence this warning 1 error generated.
I assume this is intentional so io_poll_remove_all_table() gets called twice every time? If so, would you be opposed to unrolling this a bit to make it clear to the compiler? Alternatively, we could change the return type of io_poll_remove_all_table to be an int or add a cast to int like the note mentioned but that is rather ugly to me. I can send a formal patch depending on your preference.
Cheers,
Nathan
io_uring/poll.c +6 -2
@@ -599,8 +599,12 @@ __cold bool io_poll_remove_all(struct io_ring_ctx *ctx, struct task_struct *tsk, bool cancel_all) __must_hold(&ctx->uring_lock) { -return io_poll_remove_all_table (tsk, &ctx->cancel_table, cancel_all) | -io_poll_remove_all_table (tsk, &ctx->cancel_table_locked, cancel_all); +bool ret; + + ret =io_poll_remove_all_table (tsk, &ctx->cancel_table, cancel_all); + ret |=io_poll_remove_all_table (tsk, &ctx->cancel_table_locked, cancel_all); + +return ret; } static struct io_kiocb *io_poll_find(struct io_ring_ctx *ctx, bool poll_only,
Re: [PATCH for-next v3 16/16] io_uring: mutex locked poll hashing
Quoted 61 lines · Nathan Chancellor
Quoted 20 lines · Pavel Begunkov
Currently we do two extra spin lock/unlock pairs to add a poll/apoll request to the cancellation hash table and remove it from there.
On the submission side we often already hold ->uring_lock and tw completion is likely to hold it as well. Add a second cancellation hash table protected by ->uring_lock. In concerns for latency because of a need to have the mutex locked on the completion side, use the new table only in following cases:
1)
IORING_SETUP_SINGLE_ISSUER: only one task grabsuring_lock, so there is little to no contention and so the main tw hander will almost always end up grabbing it before calling callbacks.2)
IORING_SETUP_SQPOLL: same as with single issuer, only one task isa major user of ->uring_lock.3) apoll: we normally grab the lock on the completion side anyway to execute the request, so it's free.
Signed-off-by Pavel Begunkov <[email protected]><snip>
Quoted 17 lines
(quoted diff) +3
-6-/* - * Returns true if we found and killed one or more poll requests - */ -__cold bool io_poll_remove_all(struct io_ring_ctx *ctx, struct task_struct *tsk, - bool cancel_all) +static __cold bool io_poll_remove_all_table(struct task_struct *tsk, + struct io_hash_table *table, + bool cancel_all) { - struct io_hash_table *table = &ctx->cancel_table; unsigned nr_buckets = 1U << table->hash_bits; struct hlist_node *tmp; struct io_kiocb *req;@@ -557,6 +592,17 @@ __cold bool io_poll_remove_all(struct io_ring_ctx *ctx, struct task_struct *tsk, return found; }Review __cold bool io_poll_remove_all(struct io_ring_ctx *ctx, struct task_struct *tsk, · line 604
Quoted 10 lines
(quoted diff) +10
-0+/* + * Returns true if we found and killed one or more poll requests + */ +__cold bool io_poll_remove_all(struct io_ring_ctx *ctx, struct task_struct *tsk, + bool cancel_all) + __must_hold(&ctx->uring_lock) +{ + return io_poll_remove_all_table(tsk, &ctx->cancel_table, cancel_all) | + io_poll_remove_all_table(tsk, &ctx->cancel_table_locked, cancel_all); +}Clang warns:
io_uring/poll.c:602:9: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]return io_poll_remove_all_table(tsk, &ctx->cancel_table, cancel_all) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ||
io_uring/poll.c:602:9: note: cast one or both operands to int to silence this warning 1 error generated.
Quoted 6 lines
I assume this is intentional so
io_poll_remove_all_table()gets called twice every time? If so, would you be opposed to unrolling this a bit to make it clear to the compiler? Alternatively, we could change the return type ofio_poll_remove_all_tableto be an int or add a cast to int like the note mentioned but that is rather ugly to me. I can send a formal patch depending on your preference.
Thanks for the heads up. Yes, it's intentional, and I'm fine with either version. Are you going to send a patch?
Signature · 1 line
Pavel Begunkov
Quoted 10 lines · Pavel Begunkov
1-4 kills
REQ_F_COMPLETE_INLINEas we're out of bits.Patch 5 from Hao should remove some overhead from poll requests
Patch 6 from Hao adds per-bucket spinlocks, and 16-19 do a little bit of cleanup. The downside of per-bucket spinlocks is that it adds additional spinlock/unlock pair in the poll request completion side, which shouldn't matter much with 20/25.
[...]
Applied, thanks!
[01/16] io_uring: rw: delegate sync completions to core io_uring
commit: 45bfddb605aebe5298b054553ac8daa04bd73c67
[02/16] io_uring: kill REQ_F_COMPLETE_INLINE
commit: c2399c806444c54c8414f2196e43ea22843e51a5
[03/16] io_uring: refactor io_req_task_complete()
commit: a74ba63b8d9ed12fc06d6e122a94ebb53e8ae126
[04/16] io_uring: don't inline io_put_kbuf
commit: 8fffc6537fb8d7e4e8901b0ca982396999c89c09
[05/16] io_uring: poll: remove unnecessary req->ref set
commit: 0e769a4667807d1bb249b4bcd9cc6ac6cbdea3ab
[06/16] io_uring: switch cancel_hash to use per entry spinlock
commit: 6c41fff4b73e393107a867c3259a7ce38e3d7137
[07/16] io_uring: pass poll_find lock back
commit: 4488b60bf5d73e69c6e17f6296f71ab19f290fae
[08/16] io_uring: clean up io_try_cancel
commit: 034c5701e192e9521dae1a60c295a3dea8bd9f07
[09/16] io_uring: limit the number of cancellation buckets
commit: 8a0089110740eb78bb6592b12b73c15096cc5b41
[10/16] io_uring: clean up io_ring_ctx_alloc
commit: 8797b59e7bd7463775690a0ee0de4c2121e39a90
[11/16] io_uring: use state completion infra for poll reqs
commit: 60ad0a221eb26eb7c3babb000c9fe05f5f3f9231
[12/16] io_uring: add IORING_SETUP_SINGLE_ISSUER
commit: d2fbea05a52db51e1939fe3f99fdc5086ff093c4
[13/16] io_uring: pass hash table into poll_find
commit: fbd91877aac264a470b666fbd88a8a31d202993e
[14/16] io_uring: introduce a struct for hash table
commit: 1aa9e1ce9887505ea87aa86128c1e0960e85e9dd
[15/16] io_uring: propagate locking state to poll cancel
commit: 3f301363931da831687160817f4b31fad89b50de
[16/16] io_uring: mutex locked poll hashing
commit: 154d61b44e7eee3b5db68d65d9cb7403c9f58e71Best regards,
Signature · 1 line
Jens Axboe
Quoted 16 lines · Pavel Begunkov
1-4 kills
REQ_F_COMPLETE_INLINEas we're out of bits.Patch 5 from Hao should remove some overhead from poll requests
Patch 6 from Hao adds per-bucket spinlocks, and 16-19 do a little bit of cleanup. The downside of per-bucket spinlocks is that it adds additional spinlock/unlock pair in the poll request completion side, which shouldn't matter much with 20/25.
Patch 11 uses inline completion infra for poll requests, this nicely improves perf when there is a good tw batching.
Patch 12 implements the userspace visible side of
IORING_SETUP_SINGLE_ISSUER, it'll be used for poll requests and later for spinlock optimisations.
Quoted 6 lines
13-16 introduces ->uring_lock protected cancellation hashing. It requires us to grab ->uring_lock in the completion side, but saves two spin lock/unlock pairs. We apply it automatically in cases the mutex is already likely to be held (see 25/25 description), so there is no additional mutex overhead and potential latency problemes.
Reviewed-by Hao Xu <[email protected]>