* [PATCH v2 0/2] async hybrid for pollable requests
@ 2021-10-18 11:29 Hao Xu
2021-10-18 11:29 ` [PATCH 1/2] io_uring: split logic of force_nonblock Hao Xu
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Hao Xu @ 2021-10-18 11:29 UTC (permalink / raw)
To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi
1/2 is a prep patch. 2/2 is the main one.
The good thing: see commit message.
the side effect: for normal io-worker path, added two if and two local
variables. for FORCE_ASYNC path, added three if and several dereferences
I think it is fine since the io-worker path is not the fast path, and
the benefit of this patchset is worth it.
Btw, we need to tweak the io-cancel.c a bit, not a big problem.
liburing tests will come later.
v1-->v2:
- split logic of force_nonblock
- tweak added code in io_wq_submit_work to reduce overhead
from Pavel's commments.
Hao Xu (2):
io_uring: split logic of force_nonblock
io_uring: implement async hybrid mode for pollable requests
fs/io_uring.c | 85 ++++++++++++++++++++++++++---------
include/uapi/linux/io_uring.h | 4 +-
2 files changed, 66 insertions(+), 23 deletions(-)
--
2.24.4
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/2] io_uring: split logic of force_nonblock 2021-10-18 11:29 [PATCH v2 0/2] async hybrid for pollable requests Hao Xu @ 2021-10-18 11:29 ` Hao Xu 2021-10-18 12:13 ` Pavel Begunkov 2021-10-18 12:27 ` Pavel Begunkov 2021-10-18 11:29 ` [PATCH 2/2] io_uring: implement async hybrid mode for pollable requests Hao Xu 2021-10-18 12:31 ` [PATCH v2 0/2] async hybrid " Pavel Begunkov 2 siblings, 2 replies; 12+ messages in thread From: Hao Xu @ 2021-10-18 11:29 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi Currently force_nonblock stands for three meanings: - nowait or not - in an io-worker or not(hold uring_lock or not) Let's split the logic to two flags, IO_URING_F_NONBLOCK and IO_URING_F_UNLOCKED for convenience of the next patch. Signed-off-by: Hao Xu <haoxu@linux.alibaba.com> --- fs/io_uring.c | 50 ++++++++++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 22 deletions(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index b6da03c26122..727cad6c36fc 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -199,6 +199,7 @@ struct io_rings { enum io_uring_cmd_flags { IO_URING_F_COMPLETE_DEFER = 1, + IO_URING_F_UNLOCKED = 2, /* int's last bit, sign checks are usually faster than a bit test */ IO_URING_F_NONBLOCK = INT_MIN, }; @@ -2926,7 +2927,7 @@ static void kiocb_done(struct kiocb *kiocb, ssize_t ret, struct io_ring_ctx *ctx = req->ctx; req_set_fail(req); - if (!(issue_flags & IO_URING_F_NONBLOCK)) { + if (issue_flags & IO_URING_F_UNLOCKED) { mutex_lock(&ctx->uring_lock); __io_req_complete(req, issue_flags, ret, cflags); mutex_unlock(&ctx->uring_lock); @@ -3036,7 +3037,7 @@ static struct io_buffer *io_buffer_select(struct io_kiocb *req, size_t *len, { struct io_buffer *kbuf = req->kbuf; struct io_buffer *head; - bool needs_lock = !(issue_flags & IO_URING_F_NONBLOCK); + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; if (req->flags & REQ_F_BUFFER_SELECTED) return kbuf; @@ -3341,7 +3342,7 @@ static inline int io_rw_prep_async(struct io_kiocb *req, int rw) int ret; /* submission path, ->uring_lock should already be taken */ - ret = io_import_iovec(rw, req, &iov, &iorw->s, IO_URING_F_NONBLOCK); + ret = io_import_iovec(rw, req, &iov, &iorw->s, 0); if (unlikely(ret < 0)) return ret; @@ -3452,6 +3453,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags) struct iovec *iovec; struct kiocb *kiocb = &req->rw.kiocb; bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK; + bool in_worker = issue_flags & IO_URING_F_UNLOCKED; struct io_async_rw *rw; ssize_t ret, ret2; @@ -3495,7 +3497,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags) if (ret == -EAGAIN || (req->flags & REQ_F_REISSUE)) { req->flags &= ~REQ_F_REISSUE; /* IOPOLL retry should happen for io-wq threads */ - if (!force_nonblock && !(req->ctx->flags & IORING_SETUP_IOPOLL)) + if (in_worker && !(req->ctx->flags & IORING_SETUP_IOPOLL)) goto done; /* no retry on NONBLOCK nor RWF_NOWAIT */ if (req->flags & REQ_F_NOWAIT) @@ -3503,7 +3505,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags) ret = 0; } else if (ret == -EIOCBQUEUED) { goto out_free; - } else if (ret == req->result || ret <= 0 || !force_nonblock || + } else if (ret == req->result || ret <= 0 || in_worker || (req->flags & REQ_F_NOWAIT) || !need_read_all(req)) { /* read all, failed, already did sync or don't want to retry */ goto done; @@ -3581,6 +3583,7 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags) struct iovec *iovec; struct kiocb *kiocb = &req->rw.kiocb; bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK; + bool in_worker = issue_flags & IO_URING_F_UNLOCKED; ssize_t ret, ret2; if (!req_has_async_data(req)) { @@ -3651,7 +3654,7 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags) /* no retry on NONBLOCK nor RWF_NOWAIT */ if (ret2 == -EAGAIN && (req->flags & REQ_F_NOWAIT)) goto done; - if (!force_nonblock || ret2 != -EAGAIN) { + if (in_worker || ret2 != -EAGAIN) { /* IOPOLL retry should happen for io-wq threads */ if ((req->ctx->flags & IORING_SETUP_IOPOLL) && ret2 == -EAGAIN) goto copy_iov; @@ -4314,9 +4317,9 @@ static int io_remove_buffers(struct io_kiocb *req, unsigned int issue_flags) struct io_ring_ctx *ctx = req->ctx; struct io_buffer *head; int ret = 0; - bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK; + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; - io_ring_submit_lock(ctx, !force_nonblock); + io_ring_submit_lock(ctx, needs_lock); lockdep_assert_held(&ctx->uring_lock); @@ -4329,7 +4332,7 @@ static int io_remove_buffers(struct io_kiocb *req, unsigned int issue_flags) /* complete before unlock, IOPOLL may need the lock */ __io_req_complete(req, issue_flags, ret, 0); - io_ring_submit_unlock(ctx, !force_nonblock); + io_ring_submit_unlock(ctx, needs_lock); return 0; } @@ -4401,9 +4404,9 @@ static int io_provide_buffers(struct io_kiocb *req, unsigned int issue_flags) struct io_ring_ctx *ctx = req->ctx; struct io_buffer *head, *list; int ret = 0; - bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK; + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; - io_ring_submit_lock(ctx, !force_nonblock); + io_ring_submit_lock(ctx, needs_lock); lockdep_assert_held(&ctx->uring_lock); @@ -4419,7 +4422,7 @@ static int io_provide_buffers(struct io_kiocb *req, unsigned int issue_flags) req_set_fail(req); /* complete before unlock, IOPOLL may need the lock */ __io_req_complete(req, issue_flags, ret, 0); - io_ring_submit_unlock(ctx, !force_nonblock); + io_ring_submit_unlock(ctx, needs_lock); return 0; } @@ -6279,6 +6282,7 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags) { struct io_ring_ctx *ctx = req->ctx; u64 sqe_addr = req->cancel.addr; + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; struct io_tctx_node *node; int ret; @@ -6287,7 +6291,7 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags) goto done; /* slow path, try all io-wq's */ - io_ring_submit_lock(ctx, !(issue_flags & IO_URING_F_NONBLOCK)); + io_ring_submit_lock(ctx, needs_lock); ret = -ENOENT; list_for_each_entry(node, &ctx->tctx_list, ctx_node) { struct io_uring_task *tctx = node->task->io_uring; @@ -6296,7 +6300,7 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags) if (ret != -ENOENT) break; } - io_ring_submit_unlock(ctx, !(issue_flags & IO_URING_F_NONBLOCK)); + io_ring_submit_unlock(ctx, needs_lock); done: if (ret < 0) req_set_fail(req); @@ -6323,6 +6327,7 @@ static int io_rsrc_update_prep(struct io_kiocb *req, static int io_files_update(struct io_kiocb *req, unsigned int issue_flags) { struct io_ring_ctx *ctx = req->ctx; + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; struct io_uring_rsrc_update2 up; int ret; @@ -6332,10 +6337,10 @@ static int io_files_update(struct io_kiocb *req, unsigned int issue_flags) up.tags = 0; up.resv = 0; - io_ring_submit_lock(ctx, !(issue_flags & IO_URING_F_NONBLOCK)); + io_ring_submit_lock(ctx, needs_lock); ret = __io_register_rsrc_update(ctx, IORING_RSRC_FILE, &up, req->rsrc_update.nr_args); - io_ring_submit_unlock(ctx, !(issue_flags & IO_URING_F_NONBLOCK)); + io_ring_submit_unlock(ctx, needs_lock); if (ret < 0) req_set_fail(req); @@ -6745,7 +6750,7 @@ static void io_wq_submit_work(struct io_wq_work *work) if (!ret) { do { - ret = io_issue_sqe(req, 0); + ret = io_issue_sqe(req, IO_URING_F_UNLOCKED); /* * We can get EAGAIN for polled IO even though we're * forcing a sync submission from here, since we can't @@ -8333,12 +8338,12 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file, unsigned int issue_flags, u32 slot_index) { struct io_ring_ctx *ctx = req->ctx; - bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK; + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; bool needs_switch = false; struct io_fixed_file *file_slot; int ret = -EBADF; - io_ring_submit_lock(ctx, !force_nonblock); + io_ring_submit_lock(ctx, needs_lock); if (file->f_op == &io_uring_fops) goto err; ret = -ENXIO; @@ -8379,7 +8384,7 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file, err: if (needs_switch) io_rsrc_node_switch(ctx, ctx->file_data); - io_ring_submit_unlock(ctx, !force_nonblock); + io_ring_submit_unlock(ctx, needs_lock); if (ret) fput(file); return ret; @@ -8389,11 +8394,12 @@ static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags) { unsigned int offset = req->close.file_slot - 1; struct io_ring_ctx *ctx = req->ctx; + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; struct io_fixed_file *file_slot; struct file *file; int ret, i; - io_ring_submit_lock(ctx, !(issue_flags & IO_URING_F_NONBLOCK)); + io_ring_submit_lock(ctx, needs_lock); ret = -ENXIO; if (unlikely(!ctx->file_data)) goto out; @@ -8419,7 +8425,7 @@ static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags) io_rsrc_node_switch(ctx, ctx->file_data); ret = 0; out: - io_ring_submit_unlock(ctx, !(issue_flags & IO_URING_F_NONBLOCK)); + io_ring_submit_unlock(ctx, needs_lock); return ret; } -- 2.24.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] io_uring: split logic of force_nonblock 2021-10-18 11:29 ` [PATCH 1/2] io_uring: split logic of force_nonblock Hao Xu @ 2021-10-18 12:13 ` Pavel Begunkov 2021-10-18 12:27 ` Pavel Begunkov 1 sibling, 0 replies; 12+ messages in thread From: Pavel Begunkov @ 2021-10-18 12:13 UTC (permalink / raw) To: Hao Xu, Jens Axboe; +Cc: io-uring, Joseph Qi On 10/18/21 11:29, Hao Xu wrote: > Currently force_nonblock stands for three meanings: > - nowait or not > - in an io-worker or not(hold uring_lock or not) We should have done it long ago. You can send it separately if it'd help. One more recently added spot is missing: io_iopoll_req_issued() > Let's split the logic to two flags, IO_URING_F_NONBLOCK and > IO_URING_F_UNLOCKED for convenience of the next patch. > > Signed-off-by: Hao Xu <haoxu@linux.alibaba.com> > --- > fs/io_uring.c | 50 ++++++++++++++++++++++++++++---------------------- > 1 file changed, 28 insertions(+), 22 deletions(-) > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index b6da03c26122..727cad6c36fc 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -199,6 +199,7 @@ struct io_rings { > > enum io_uring_cmd_flags { > IO_URING_F_COMPLETE_DEFER = 1, > + IO_URING_F_UNLOCKED = 2, > /* int's last bit, sign checks are usually faster than a bit test */ > IO_URING_F_NONBLOCK = INT_MIN, > }; > @@ -2926,7 +2927,7 @@ static void kiocb_done(struct kiocb *kiocb, ssize_t ret, > struct io_ring_ctx *ctx = req->ctx; > > req_set_fail(req); > - if (!(issue_flags & IO_URING_F_NONBLOCK)) { > + if (issue_flags & IO_URING_F_UNLOCKED) { > mutex_lock(&ctx->uring_lock); > __io_req_complete(req, issue_flags, ret, cflags); > mutex_unlock(&ctx->uring_lock); > @@ -3036,7 +3037,7 @@ static struct io_buffer *io_buffer_select(struct io_kiocb *req, size_t *len, > { > struct io_buffer *kbuf = req->kbuf; > struct io_buffer *head; > - bool needs_lock = !(issue_flags & IO_URING_F_NONBLOCK); > + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; > > if (req->flags & REQ_F_BUFFER_SELECTED) > return kbuf; > @@ -3341,7 +3342,7 @@ static inline int io_rw_prep_async(struct io_kiocb *req, int rw) > int ret; > > /* submission path, ->uring_lock should already be taken */ > - ret = io_import_iovec(rw, req, &iov, &iorw->s, IO_URING_F_NONBLOCK); > + ret = io_import_iovec(rw, req, &iov, &iorw->s, 0); > if (unlikely(ret < 0)) > return ret; > > @@ -3452,6 +3453,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags) > struct iovec *iovec; > struct kiocb *kiocb = &req->rw.kiocb; > bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK; > + bool in_worker = issue_flags & IO_URING_F_UNLOCKED; > struct io_async_rw *rw; > ssize_t ret, ret2; > > @@ -3495,7 +3497,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags) > if (ret == -EAGAIN || (req->flags & REQ_F_REISSUE)) { > req->flags &= ~REQ_F_REISSUE; > /* IOPOLL retry should happen for io-wq threads */ > - if (!force_nonblock && !(req->ctx->flags & IORING_SETUP_IOPOLL)) > + if (in_worker && !(req->ctx->flags & IORING_SETUP_IOPOLL)) > goto done; > /* no retry on NONBLOCK nor RWF_NOWAIT */ > if (req->flags & REQ_F_NOWAIT) > @@ -3503,7 +3505,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags) > ret = 0; > } else if (ret == -EIOCBQUEUED) { > goto out_free; > - } else if (ret == req->result || ret <= 0 || !force_nonblock || > + } else if (ret == req->result || ret <= 0 || in_worker || > (req->flags & REQ_F_NOWAIT) || !need_read_all(req)) { > /* read all, failed, already did sync or don't want to retry */ > goto done; > @@ -3581,6 +3583,7 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags) > struct iovec *iovec; > struct kiocb *kiocb = &req->rw.kiocb; > bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK; > + bool in_worker = issue_flags & IO_URING_F_UNLOCKED; > ssize_t ret, ret2; > > if (!req_has_async_data(req)) { > @@ -3651,7 +3654,7 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags) > /* no retry on NONBLOCK nor RWF_NOWAIT */ > if (ret2 == -EAGAIN && (req->flags & REQ_F_NOWAIT)) > goto done; > - if (!force_nonblock || ret2 != -EAGAIN) { > + if (in_worker || ret2 != -EAGAIN) { > /* IOPOLL retry should happen for io-wq threads */ > if ((req->ctx->flags & IORING_SETUP_IOPOLL) && ret2 == -EAGAIN) > goto copy_iov; > @@ -4314,9 +4317,9 @@ static int io_remove_buffers(struct io_kiocb *req, unsigned int issue_flags) > struct io_ring_ctx *ctx = req->ctx; > struct io_buffer *head; > int ret = 0; > - bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK; > + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; > > - io_ring_submit_lock(ctx, !force_nonblock); > + io_ring_submit_lock(ctx, needs_lock); > > lockdep_assert_held(&ctx->uring_lock); > > @@ -4329,7 +4332,7 @@ static int io_remove_buffers(struct io_kiocb *req, unsigned int issue_flags) > > /* complete before unlock, IOPOLL may need the lock */ > __io_req_complete(req, issue_flags, ret, 0); > - io_ring_submit_unlock(ctx, !force_nonblock); > + io_ring_submit_unlock(ctx, needs_lock); > return 0; > } > > @@ -4401,9 +4404,9 @@ static int io_provide_buffers(struct io_kiocb *req, unsigned int issue_flags) > struct io_ring_ctx *ctx = req->ctx; > struct io_buffer *head, *list; > int ret = 0; > - bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK; > + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; > > - io_ring_submit_lock(ctx, !force_nonblock); > + io_ring_submit_lock(ctx, needs_lock); > > lockdep_assert_held(&ctx->uring_lock); > > @@ -4419,7 +4422,7 @@ static int io_provide_buffers(struct io_kiocb *req, unsigned int issue_flags) > req_set_fail(req); > /* complete before unlock, IOPOLL may need the lock */ > __io_req_complete(req, issue_flags, ret, 0); > - io_ring_submit_unlock(ctx, !force_nonblock); > + io_ring_submit_unlock(ctx, needs_lock); > return 0; > } > > @@ -6279,6 +6282,7 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags) > { > struct io_ring_ctx *ctx = req->ctx; > u64 sqe_addr = req->cancel.addr; > + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; > struct io_tctx_node *node; > int ret; > > @@ -6287,7 +6291,7 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags) > goto done; > > /* slow path, try all io-wq's */ > - io_ring_submit_lock(ctx, !(issue_flags & IO_URING_F_NONBLOCK)); > + io_ring_submit_lock(ctx, needs_lock); > ret = -ENOENT; > list_for_each_entry(node, &ctx->tctx_list, ctx_node) { > struct io_uring_task *tctx = node->task->io_uring; > @@ -6296,7 +6300,7 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags) > if (ret != -ENOENT) > break; > } > - io_ring_submit_unlock(ctx, !(issue_flags & IO_URING_F_NONBLOCK)); > + io_ring_submit_unlock(ctx, needs_lock); > done: > if (ret < 0) > req_set_fail(req); > @@ -6323,6 +6327,7 @@ static int io_rsrc_update_prep(struct io_kiocb *req, > static int io_files_update(struct io_kiocb *req, unsigned int issue_flags) > { > struct io_ring_ctx *ctx = req->ctx; > + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; > struct io_uring_rsrc_update2 up; > int ret; > > @@ -6332,10 +6337,10 @@ static int io_files_update(struct io_kiocb *req, unsigned int issue_flags) > up.tags = 0; > up.resv = 0; > > - io_ring_submit_lock(ctx, !(issue_flags & IO_URING_F_NONBLOCK)); > + io_ring_submit_lock(ctx, needs_lock); > ret = __io_register_rsrc_update(ctx, IORING_RSRC_FILE, > &up, req->rsrc_update.nr_args); > - io_ring_submit_unlock(ctx, !(issue_flags & IO_URING_F_NONBLOCK)); > + io_ring_submit_unlock(ctx, needs_lock); > > if (ret < 0) > req_set_fail(req); > @@ -6745,7 +6750,7 @@ static void io_wq_submit_work(struct io_wq_work *work) > > if (!ret) { > do { > - ret = io_issue_sqe(req, 0); > + ret = io_issue_sqe(req, IO_URING_F_UNLOCKED); > /* > * We can get EAGAIN for polled IO even though we're > * forcing a sync submission from here, since we can't > @@ -8333,12 +8338,12 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file, > unsigned int issue_flags, u32 slot_index) > { > struct io_ring_ctx *ctx = req->ctx; > - bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK; > + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; > bool needs_switch = false; > struct io_fixed_file *file_slot; > int ret = -EBADF; > > - io_ring_submit_lock(ctx, !force_nonblock); > + io_ring_submit_lock(ctx, needs_lock); > if (file->f_op == &io_uring_fops) > goto err; > ret = -ENXIO; > @@ -8379,7 +8384,7 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file, > err: > if (needs_switch) > io_rsrc_node_switch(ctx, ctx->file_data); > - io_ring_submit_unlock(ctx, !force_nonblock); > + io_ring_submit_unlock(ctx, needs_lock); > if (ret) > fput(file); > return ret; > @@ -8389,11 +8394,12 @@ static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags) > { > unsigned int offset = req->close.file_slot - 1; > struct io_ring_ctx *ctx = req->ctx; > + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; > struct io_fixed_file *file_slot; > struct file *file; > int ret, i; > > - io_ring_submit_lock(ctx, !(issue_flags & IO_URING_F_NONBLOCK)); > + io_ring_submit_lock(ctx, needs_lock); > ret = -ENXIO; > if (unlikely(!ctx->file_data)) > goto out; > @@ -8419,7 +8425,7 @@ static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags) > io_rsrc_node_switch(ctx, ctx->file_data); > ret = 0; > out: > - io_ring_submit_unlock(ctx, !(issue_flags & IO_URING_F_NONBLOCK)); > + io_ring_submit_unlock(ctx, needs_lock); > return ret; > } > > -- Pavel Begunkov ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] io_uring: split logic of force_nonblock 2021-10-18 11:29 ` [PATCH 1/2] io_uring: split logic of force_nonblock Hao Xu 2021-10-18 12:13 ` Pavel Begunkov @ 2021-10-18 12:27 ` Pavel Begunkov 2021-10-18 13:00 ` Hao Xu 1 sibling, 1 reply; 12+ messages in thread From: Pavel Begunkov @ 2021-10-18 12:27 UTC (permalink / raw) To: Hao Xu, Jens Axboe; +Cc: io-uring, Joseph Qi On 10/18/21 11:29, Hao Xu wrote: > Currently force_nonblock stands for three meanings: > - nowait or not > - in an io-worker or not(hold uring_lock or not) > > Let's split the logic to two flags, IO_URING_F_NONBLOCK and > IO_URING_F_UNLOCKED for convenience of the next patch. > > Signed-off-by: Hao Xu <haoxu@linux.alibaba.com> > --- > fs/io_uring.c | 50 ++++++++++++++++++++++++++++---------------------- > 1 file changed, 28 insertions(+), 22 deletions(-) > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index b6da03c26122..727cad6c36fc 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -199,6 +199,7 @@ struct io_rings { > > enum io_uring_cmd_flags { > IO_URING_F_COMPLETE_DEFER = 1, > + IO_URING_F_UNLOCKED = 2, > /* int's last bit, sign checks are usually faster than a bit test */ > IO_URING_F_NONBLOCK = INT_MIN, > }; > @@ -2926,7 +2927,7 @@ static void kiocb_done(struct kiocb *kiocb, ssize_t ret, > struct io_ring_ctx *ctx = req->ctx; > > req_set_fail(req); > - if (!(issue_flags & IO_URING_F_NONBLOCK)) { > + if (issue_flags & IO_URING_F_UNLOCKED) { > mutex_lock(&ctx->uring_lock); > __io_req_complete(req, issue_flags, ret, cflags); > mutex_unlock(&ctx->uring_lock); > @@ -3036,7 +3037,7 @@ static struct io_buffer *io_buffer_select(struct io_kiocb *req, size_t *len, > { > struct io_buffer *kbuf = req->kbuf; > struct io_buffer *head; > - bool needs_lock = !(issue_flags & IO_URING_F_NONBLOCK); > + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; > > if (req->flags & REQ_F_BUFFER_SELECTED) > return kbuf; > @@ -3341,7 +3342,7 @@ static inline int io_rw_prep_async(struct io_kiocb *req, int rw) > int ret; > > /* submission path, ->uring_lock should already be taken */ > - ret = io_import_iovec(rw, req, &iov, &iorw->s, IO_URING_F_NONBLOCK); > + ret = io_import_iovec(rw, req, &iov, &iorw->s, 0); > if (unlikely(ret < 0)) > return ret; > > @@ -3452,6 +3453,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags) > struct iovec *iovec; > struct kiocb *kiocb = &req->rw.kiocb; > bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK; > + bool in_worker = issue_flags & IO_URING_F_UNLOCKED; io_read shouldn't have notion of worker or whatever. I'd say let's leave only force_nonblock here. I assume 2/2 relies ot it, but if so you can make sure it ends up in sync (!force_nonblock) at some point if all other ways fail. > struct io_async_rw *rw; > ssize_t ret, ret2; > > @@ -3495,7 +3497,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags) > if (ret == -EAGAIN || (req->flags & REQ_F_REISSUE)) { > req->flags &= ~REQ_F_REISSUE; > /* IOPOLL retry should happen for io-wq threads */ > - if (!force_nonblock && !(req->ctx->flags & IORING_SETUP_IOPOLL)) > + if (in_worker && !(req->ctx->flags & IORING_SETUP_IOPOLL)) > goto done; > /* no retry on NONBLOCK nor RWF_NOWAIT */ > if (req->flags & REQ_F_NOWAIT) > @@ -3503,7 +3505,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags) > ret = 0; > } else if (ret == -EIOCBQUEUED) { > goto out_free; > - } else if (ret == req->result || ret <= 0 || !force_nonblock || > + } else if (ret == req->result || ret <= 0 || in_worker || > (req->flags & REQ_F_NOWAIT) || !need_read_all(req)) { > /* read all, failed, already did sync or don't want to retry */ > goto done; > @@ -3581,6 +3583,7 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags) > struct iovec *iovec; > struct kiocb *kiocb = &req->rw.kiocb; > bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK; > + bool in_worker = issue_flags & IO_URING_F_UNLOCKED; > ssize_t ret, ret2; > > if (!req_has_async_data(req)) { > @@ -3651,7 +3654,7 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags) > /* no retry on NONBLOCK nor RWF_NOWAIT */ > if (ret2 == -EAGAIN && (req->flags & REQ_F_NOWAIT)) > goto done; > - if (!force_nonblock || ret2 != -EAGAIN) { > + if (in_worker || ret2 != -EAGAIN) { > /* IOPOLL retry should happen for io-wq threads */ > if ((req->ctx->flags & IORING_SETUP_IOPOLL) && ret2 == -EAGAIN) > goto copy_iov; > @@ -4314,9 +4317,9 @@ static int io_remove_buffers(struct io_kiocb *req, unsigned int issue_flags) > struct io_ring_ctx *ctx = req->ctx; > struct io_buffer *head; > int ret = 0; > - bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK; > + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; > > - io_ring_submit_lock(ctx, !force_nonblock); > + io_ring_submit_lock(ctx, needs_lock); > > lockdep_assert_held(&ctx->uring_lock); > > @@ -4329,7 +4332,7 @@ static int io_remove_buffers(struct io_kiocb *req, unsigned int issue_flags) > > /* complete before unlock, IOPOLL may need the lock */ > __io_req_complete(req, issue_flags, ret, 0); > - io_ring_submit_unlock(ctx, !force_nonblock); > + io_ring_submit_unlock(ctx, needs_lock); > return 0; > } > > @@ -4401,9 +4404,9 @@ static int io_provide_buffers(struct io_kiocb *req, unsigned int issue_flags) > struct io_ring_ctx *ctx = req->ctx; > struct io_buffer *head, *list; > int ret = 0; > - bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK; > + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; > > - io_ring_submit_lock(ctx, !force_nonblock); > + io_ring_submit_lock(ctx, needs_lock); > > lockdep_assert_held(&ctx->uring_lock); > > @@ -4419,7 +4422,7 @@ static int io_provide_buffers(struct io_kiocb *req, unsigned int issue_flags) > req_set_fail(req); > /* complete before unlock, IOPOLL may need the lock */ > __io_req_complete(req, issue_flags, ret, 0); > - io_ring_submit_unlock(ctx, !force_nonblock); > + io_ring_submit_unlock(ctx, needs_lock); > return 0; > } > > @@ -6279,6 +6282,7 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags) > { > struct io_ring_ctx *ctx = req->ctx; > u64 sqe_addr = req->cancel.addr; > + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; > struct io_tctx_node *node; > int ret; > > @@ -6287,7 +6291,7 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags) > goto done; > > /* slow path, try all io-wq's */ > - io_ring_submit_lock(ctx, !(issue_flags & IO_URING_F_NONBLOCK)); > + io_ring_submit_lock(ctx, needs_lock); > ret = -ENOENT; > list_for_each_entry(node, &ctx->tctx_list, ctx_node) { > struct io_uring_task *tctx = node->task->io_uring; > @@ -6296,7 +6300,7 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags) > if (ret != -ENOENT) > break; > } > - io_ring_submit_unlock(ctx, !(issue_flags & IO_URING_F_NONBLOCK)); > + io_ring_submit_unlock(ctx, needs_lock); > done: > if (ret < 0) > req_set_fail(req); > @@ -6323,6 +6327,7 @@ static int io_rsrc_update_prep(struct io_kiocb *req, > static int io_files_update(struct io_kiocb *req, unsigned int issue_flags) > { > struct io_ring_ctx *ctx = req->ctx; > + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; > struct io_uring_rsrc_update2 up; > int ret; > > @@ -6332,10 +6337,10 @@ static int io_files_update(struct io_kiocb *req, unsigned int issue_flags) > up.tags = 0; > up.resv = 0; > > - io_ring_submit_lock(ctx, !(issue_flags & IO_URING_F_NONBLOCK)); > + io_ring_submit_lock(ctx, needs_lock); > ret = __io_register_rsrc_update(ctx, IORING_RSRC_FILE, > &up, req->rsrc_update.nr_args); > - io_ring_submit_unlock(ctx, !(issue_flags & IO_URING_F_NONBLOCK)); > + io_ring_submit_unlock(ctx, needs_lock); > > if (ret < 0) > req_set_fail(req); > @@ -6745,7 +6750,7 @@ static void io_wq_submit_work(struct io_wq_work *work) > > if (!ret) { > do { > - ret = io_issue_sqe(req, 0); > + ret = io_issue_sqe(req, IO_URING_F_UNLOCKED); > /* > * We can get EAGAIN for polled IO even though we're > * forcing a sync submission from here, since we can't > @@ -8333,12 +8338,12 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file, > unsigned int issue_flags, u32 slot_index) > { > struct io_ring_ctx *ctx = req->ctx; > - bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK; > + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; > bool needs_switch = false; > struct io_fixed_file *file_slot; > int ret = -EBADF; > > - io_ring_submit_lock(ctx, !force_nonblock); > + io_ring_submit_lock(ctx, needs_lock); > if (file->f_op == &io_uring_fops) > goto err; > ret = -ENXIO; > @@ -8379,7 +8384,7 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file, > err: > if (needs_switch) > io_rsrc_node_switch(ctx, ctx->file_data); > - io_ring_submit_unlock(ctx, !force_nonblock); > + io_ring_submit_unlock(ctx, needs_lock); > if (ret) > fput(file); > return ret; > @@ -8389,11 +8394,12 @@ static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags) > { > unsigned int offset = req->close.file_slot - 1; > struct io_ring_ctx *ctx = req->ctx; > + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; > struct io_fixed_file *file_slot; > struct file *file; > int ret, i; > > - io_ring_submit_lock(ctx, !(issue_flags & IO_URING_F_NONBLOCK)); > + io_ring_submit_lock(ctx, needs_lock); > ret = -ENXIO; > if (unlikely(!ctx->file_data)) > goto out; > @@ -8419,7 +8425,7 @@ static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags) > io_rsrc_node_switch(ctx, ctx->file_data); > ret = 0; > out: > - io_ring_submit_unlock(ctx, !(issue_flags & IO_URING_F_NONBLOCK)); > + io_ring_submit_unlock(ctx, needs_lock); > return ret; > } > > -- Pavel Begunkov ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] io_uring: split logic of force_nonblock 2021-10-18 12:27 ` Pavel Begunkov @ 2021-10-18 13:00 ` Hao Xu 0 siblings, 0 replies; 12+ messages in thread From: Hao Xu @ 2021-10-18 13:00 UTC (permalink / raw) To: Pavel Begunkov, Jens Axboe; +Cc: io-uring, Joseph Qi 在 2021/10/18 下午8:27, Pavel Begunkov 写道: > On 10/18/21 11:29, Hao Xu wrote: >> Currently force_nonblock stands for three meanings: >> - nowait or not >> - in an io-worker or not(hold uring_lock or not) >> >> Let's split the logic to two flags, IO_URING_F_NONBLOCK and >> IO_URING_F_UNLOCKED for convenience of the next patch. >> >> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com> >> --- >> fs/io_uring.c | 50 ++++++++++++++++++++++++++++---------------------- >> 1 file changed, 28 insertions(+), 22 deletions(-) >> >> diff --git a/fs/io_uring.c b/fs/io_uring.c >> index b6da03c26122..727cad6c36fc 100644 >> --- a/fs/io_uring.c >> +++ b/fs/io_uring.c >> @@ -199,6 +199,7 @@ struct io_rings { >> enum io_uring_cmd_flags { >> IO_URING_F_COMPLETE_DEFER = 1, >> + IO_URING_F_UNLOCKED = 2, >> /* int's last bit, sign checks are usually faster than a bit >> test */ >> IO_URING_F_NONBLOCK = INT_MIN, >> }; >> @@ -2926,7 +2927,7 @@ static void kiocb_done(struct kiocb *kiocb, >> ssize_t ret, >> struct io_ring_ctx *ctx = req->ctx; >> req_set_fail(req); >> - if (!(issue_flags & IO_URING_F_NONBLOCK)) { >> + if (issue_flags & IO_URING_F_UNLOCKED) { >> mutex_lock(&ctx->uring_lock); >> __io_req_complete(req, issue_flags, ret, cflags); >> mutex_unlock(&ctx->uring_lock); >> @@ -3036,7 +3037,7 @@ static struct io_buffer *io_buffer_select(struct >> io_kiocb *req, size_t *len, >> { >> struct io_buffer *kbuf = req->kbuf; >> struct io_buffer *head; >> - bool needs_lock = !(issue_flags & IO_URING_F_NONBLOCK); >> + bool needs_lock = issue_flags & IO_URING_F_UNLOCKED; >> if (req->flags & REQ_F_BUFFER_SELECTED) >> return kbuf; >> @@ -3341,7 +3342,7 @@ static inline int io_rw_prep_async(struct >> io_kiocb *req, int rw) >> int ret; >> /* submission path, ->uring_lock should already be taken */ >> - ret = io_import_iovec(rw, req, &iov, &iorw->s, IO_URING_F_NONBLOCK); >> + ret = io_import_iovec(rw, req, &iov, &iorw->s, 0); >> if (unlikely(ret < 0)) >> return ret; >> @@ -3452,6 +3453,7 @@ static int io_read(struct io_kiocb *req, >> unsigned int issue_flags) >> struct iovec *iovec; >> struct kiocb *kiocb = &req->rw.kiocb; >> bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK; >> + bool in_worker = issue_flags & IO_URING_F_UNLOCKED; > > io_read shouldn't have notion of worker or whatever. I'd say let's > leave only force_nonblock here. > > I assume 2/2 relies ot it, but if so you can make sure it ends up > in sync (!force_nonblock) at some point if all other ways fail. I re-read the code, found you're right, will send v3. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/2] io_uring: implement async hybrid mode for pollable requests 2021-10-18 11:29 [PATCH v2 0/2] async hybrid for pollable requests Hao Xu 2021-10-18 11:29 ` [PATCH 1/2] io_uring: split logic of force_nonblock Hao Xu @ 2021-10-18 11:29 ` Hao Xu 2021-10-18 11:34 ` Hao Xu 2021-10-18 12:31 ` [PATCH v2 0/2] async hybrid " Pavel Begunkov 2 siblings, 1 reply; 12+ messages in thread From: Hao Xu @ 2021-10-18 11:29 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi The current logic of requests with IOSQE_ASYNC is first queueing it to io-worker, then execute it in a synchronous way. For unbound works like pollable requests(e.g. read/write a socketfd), the io-worker may stuck there waiting for events for a long time. And thus other works wait in the list for a long time too. Let's introduce a new way for unbound works (currently pollable requests), with this a request will first be queued to io-worker, then executed in a nonblock try rather than a synchronous way. Failure of that leads it to arm poll stuff and then the worker can begin to handle other works. The detail process of this kind of requests is: step1: original context: queue it to io-worker step2: io-worker context: nonblock try(the old logic is a synchronous try here) | |--fail--> arm poll | |--(fail/ready)-->synchronous issue | |--(succeed)-->worker finish it's job, tw take over the req This works much better than the old IOSQE_ASYNC logic in cases where unbound max_worker is relatively small. In this case, number of io-worker eazily increments to max_worker, new worker cannot be created and running workers stuck there handling old works in IOSQE_ASYNC mode. In my 64-core machine, set unbound max_worker to 20, run echo-server, turns out: (arguments: register_file, connetion number is 1000, message size is 12 Byte) original IOSQE_ASYNC: 76664.151 tps after this patch: 166934.985 tps Suggested-by: Jens Axboe <axboe@kernel.dk> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com> --- fs/io_uring.c | 39 +++++++++++++++++++++++++++++++++-- include/uapi/linux/io_uring.h | 4 +++- 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index 727cad6c36fc..a3247a5dafc9 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -3497,7 +3497,8 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags) if (ret == -EAGAIN || (req->flags & REQ_F_REISSUE)) { req->flags &= ~REQ_F_REISSUE; /* IOPOLL retry should happen for io-wq threads */ - if (in_worker && !(req->ctx->flags & IORING_SETUP_IOPOLL)) + if (in_worker && !force_nonblock && + !(req->ctx->flags & IORING_SETUP_IOPOLL)) goto done; /* no retry on NONBLOCK nor RWF_NOWAIT */ if (req->flags & REQ_F_NOWAIT) @@ -6749,8 +6750,18 @@ static void io_wq_submit_work(struct io_wq_work *work) ret = -ECANCELED; if (!ret) { + bool needs_poll = false; + unsigned int issue_flags = IO_URING_F_UNLOCKED; + + if (req->flags & REQ_F_FORCE_ASYNC) { + needs_poll = req->file && file_can_poll(req->file); + if (needs_poll) + issue_flags |= IO_URING_F_NONBLOCK; + } + do { - ret = io_issue_sqe(req, IO_URING_F_UNLOCKED); +issue_sqe: + ret = io_issue_sqe(req, issue_flags); /* * We can get EAGAIN for polled IO even though we're * forcing a sync submission from here, since we can't @@ -6758,6 +6769,30 @@ static void io_wq_submit_work(struct io_wq_work *work) */ if (ret != -EAGAIN) break; + if (needs_poll) { + bool armed = false; + + ret = 0; + needs_poll = false; + issue_flags &= ~IO_URING_F_NONBLOCK; + + switch (io_arm_poll_handler(req)) { + case IO_APOLL_READY: + goto issue_sqe; + case IO_APOLL_ABORTED: + /* + * somehow we failed to arm the poll infra, + * fallback it to a normal async worker try. + */ + break; + case IO_APOLL_OK: + armed = true; + break; + } + + if (armed) + break; + } cond_resched(); } while (1); } diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h index c45b5e9a9387..3e49a7dbe636 100644 --- a/include/uapi/linux/io_uring.h +++ b/include/uapi/linux/io_uring.h @@ -70,6 +70,7 @@ enum { IOSQE_IO_HARDLINK_BIT, IOSQE_ASYNC_BIT, IOSQE_BUFFER_SELECT_BIT, + IOSQE_ASYNC_HYBRID_BIT, }; /* @@ -87,7 +88,8 @@ enum { #define IOSQE_ASYNC (1U << IOSQE_ASYNC_BIT) /* select buffer from sqe->buf_group */ #define IOSQE_BUFFER_SELECT (1U << IOSQE_BUFFER_SELECT_BIT) - +/* first force async then arm poll */ +#define IOSQE_ASYNC_HYBRID (1U << IOSQE_ASYNC_HYBRID_BIT) /* * io_uring_setup() flags */ -- 2.24.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] io_uring: implement async hybrid mode for pollable requests 2021-10-18 11:29 ` [PATCH 2/2] io_uring: implement async hybrid mode for pollable requests Hao Xu @ 2021-10-18 11:34 ` Hao Xu 2021-10-18 12:10 ` Pavel Begunkov 0 siblings, 1 reply; 12+ messages in thread From: Hao Xu @ 2021-10-18 11:34 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi 在 2021/10/18 下午7:29, Hao Xu 写道: > The current logic of requests with IOSQE_ASYNC is first queueing it to > io-worker, then execute it in a synchronous way. For unbound works like > pollable requests(e.g. read/write a socketfd), the io-worker may stuck > there waiting for events for a long time. And thus other works wait in > the list for a long time too. > Let's introduce a new way for unbound works (currently pollable > requests), with this a request will first be queued to io-worker, then > executed in a nonblock try rather than a synchronous way. Failure of > that leads it to arm poll stuff and then the worker can begin to handle > other works. > The detail process of this kind of requests is: > > step1: original context: > queue it to io-worker > step2: io-worker context: > nonblock try(the old logic is a synchronous try here) > | > |--fail--> arm poll > | > |--(fail/ready)-->synchronous issue > | > |--(succeed)-->worker finish it's job, tw > take over the req > > This works much better than the old IOSQE_ASYNC logic in cases where > unbound max_worker is relatively small. In this case, number of > io-worker eazily increments to max_worker, new worker cannot be created > and running workers stuck there handling old works in IOSQE_ASYNC mode. > > In my 64-core machine, set unbound max_worker to 20, run echo-server, > turns out: > (arguments: register_file, connetion number is 1000, message size is 12 > Byte) > original IOSQE_ASYNC: 76664.151 tps > after this patch: 166934.985 tps > > Suggested-by: Jens Axboe <axboe@kernel.dk> > Signed-off-by: Hao Xu <haoxu@linux.alibaba.com> An irrelevant question: why do we do linked timeout logic in io_wq_submit_work() again regarding that we've already done it in io_queue_async_work(). ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] io_uring: implement async hybrid mode for pollable requests 2021-10-18 11:34 ` Hao Xu @ 2021-10-18 12:10 ` Pavel Begunkov 2021-10-18 12:20 ` Hao Xu 0 siblings, 1 reply; 12+ messages in thread From: Pavel Begunkov @ 2021-10-18 12:10 UTC (permalink / raw) To: Hao Xu, Jens Axboe; +Cc: io-uring, Joseph Qi On 10/18/21 11:34, Hao Xu wrote: > 在 2021/10/18 下午7:29, Hao Xu 写道: >> The current logic of requests with IOSQE_ASYNC is first queueing it to >> io-worker, then execute it in a synchronous way. For unbound works like >> pollable requests(e.g. read/write a socketfd), the io-worker may stuck >> there waiting for events for a long time. And thus other works wait in >> the list for a long time too. >> Let's introduce a new way for unbound works (currently pollable >> requests), with this a request will first be queued to io-worker, then >> executed in a nonblock try rather than a synchronous way. Failure of >> that leads it to arm poll stuff and then the worker can begin to handle >> other works. >> The detail process of this kind of requests is: >> >> step1: original context: >> queue it to io-worker >> step2: io-worker context: >> nonblock try(the old logic is a synchronous try here) >> | >> |--fail--> arm poll >> | >> |--(fail/ready)-->synchronous issue >> | >> |--(succeed)-->worker finish it's job, tw >> take over the req >> >> This works much better than the old IOSQE_ASYNC logic in cases where >> unbound max_worker is relatively small. In this case, number of >> io-worker eazily increments to max_worker, new worker cannot be created >> and running workers stuck there handling old works in IOSQE_ASYNC mode. >> >> In my 64-core machine, set unbound max_worker to 20, run echo-server, >> turns out: >> (arguments: register_file, connetion number is 1000, message size is 12 >> Byte) >> original IOSQE_ASYNC: 76664.151 tps >> after this patch: 166934.985 tps >> >> Suggested-by: Jens Axboe <axboe@kernel.dk> >> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com> > An irrelevant question: why do we do linked timeout logic in > io_wq_submit_work() again regarding that we've already done it in > io_queue_async_work(). Because io_wq_free_work() may enqueue new work (by returning it) without going through io_queue_async_work(), and we don't care enough to split those cases. -- Pavel Begunkov ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] io_uring: implement async hybrid mode for pollable requests 2021-10-18 12:10 ` Pavel Begunkov @ 2021-10-18 12:20 ` Hao Xu 0 siblings, 0 replies; 12+ messages in thread From: Hao Xu @ 2021-10-18 12:20 UTC (permalink / raw) To: Pavel Begunkov, Jens Axboe; +Cc: io-uring, Joseph Qi 在 2021/10/18 下午8:10, Pavel Begunkov 写道: > On 10/18/21 11:34, Hao Xu wrote: >> 在 2021/10/18 下午7:29, Hao Xu 写道: >>> The current logic of requests with IOSQE_ASYNC is first queueing it to >>> io-worker, then execute it in a synchronous way. For unbound works like >>> pollable requests(e.g. read/write a socketfd), the io-worker may stuck >>> there waiting for events for a long time. And thus other works wait in >>> the list for a long time too. >>> Let's introduce a new way for unbound works (currently pollable >>> requests), with this a request will first be queued to io-worker, then >>> executed in a nonblock try rather than a synchronous way. Failure of >>> that leads it to arm poll stuff and then the worker can begin to handle >>> other works. >>> The detail process of this kind of requests is: >>> >>> step1: original context: >>> queue it to io-worker >>> step2: io-worker context: >>> nonblock try(the old logic is a synchronous try here) >>> | >>> |--fail--> arm poll >>> | >>> |--(fail/ready)-->synchronous issue >>> | >>> |--(succeed)-->worker finish it's job, tw >>> take over the req >>> >>> This works much better than the old IOSQE_ASYNC logic in cases where >>> unbound max_worker is relatively small. In this case, number of >>> io-worker eazily increments to max_worker, new worker cannot be created >>> and running workers stuck there handling old works in IOSQE_ASYNC mode. >>> >>> In my 64-core machine, set unbound max_worker to 20, run echo-server, >>> turns out: >>> (arguments: register_file, connetion number is 1000, message size is 12 >>> Byte) >>> original IOSQE_ASYNC: 76664.151 tps >>> after this patch: 166934.985 tps >>> >>> Suggested-by: Jens Axboe <axboe@kernel.dk> >>> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com> >> An irrelevant question: why do we do linked timeout logic in >> io_wq_submit_work() again regarding that we've already done it in >> io_queue_async_work(). > > Because io_wq_free_work() may enqueue new work (by returning it) > without going through io_queue_async_work(), and we don't care > enough to split those cases. Make sense. Thanks. > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/2] async hybrid for pollable requests 2021-10-18 11:29 [PATCH v2 0/2] async hybrid for pollable requests Hao Xu 2021-10-18 11:29 ` [PATCH 1/2] io_uring: split logic of force_nonblock Hao Xu 2021-10-18 11:29 ` [PATCH 2/2] io_uring: implement async hybrid mode for pollable requests Hao Xu @ 2021-10-18 12:31 ` Pavel Begunkov 2021-10-18 12:35 ` Hao Xu 2021-10-18 13:17 ` Hao Xu 2 siblings, 2 replies; 12+ messages in thread From: Pavel Begunkov @ 2021-10-18 12:31 UTC (permalink / raw) To: Hao Xu, Jens Axboe; +Cc: io-uring, Joseph Qi On 10/18/21 11:29, Hao Xu wrote: > 1/2 is a prep patch. 2/2 is the main one. > The good thing: see commit message. > the side effect: for normal io-worker path, added two if and two local > variables. for FORCE_ASYNC path, added three if and several dereferences > > I think it is fine since the io-worker path is not the fast path, and > the benefit of this patchset is worth it. We don't care about overhead in iowq, but would be better to get rid of the in_worker in io_read(). See comments to 1/2. Btw, you told that it performs better comparing to normal IOSQE_ASYNC. I'm confused, didn't we agree that it can be merged into IOSQE_ASYNC without extra flags? > > Btw, we need to tweak the io-cancel.c a bit, not a big problem. > liburing tests will come later. > > v1-->v2: > - split logic of force_nonblock > - tweak added code in io_wq_submit_work to reduce overhead > from Pavel's commments. > > Hao Xu (2): > io_uring: split logic of force_nonblock > io_uring: implement async hybrid mode for pollable requests > > fs/io_uring.c | 85 ++++++++++++++++++++++++++--------- > include/uapi/linux/io_uring.h | 4 +- > 2 files changed, 66 insertions(+), 23 deletions(-) > -- Pavel Begunkov ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/2] async hybrid for pollable requests 2021-10-18 12:31 ` [PATCH v2 0/2] async hybrid " Pavel Begunkov @ 2021-10-18 12:35 ` Hao Xu 2021-10-18 13:17 ` Hao Xu 1 sibling, 0 replies; 12+ messages in thread From: Hao Xu @ 2021-10-18 12:35 UTC (permalink / raw) To: Pavel Begunkov, Jens Axboe; +Cc: io-uring, Joseph Qi 在 2021/10/18 下午8:31, Pavel Begunkov 写道: > On 10/18/21 11:29, Hao Xu wrote: >> 1/2 is a prep patch. 2/2 is the main one. >> The good thing: see commit message. >> the side effect: for normal io-worker path, added two if and two local >> variables. for FORCE_ASYNC path, added three if and several dereferences >> >> I think it is fine since the io-worker path is not the fast path, and >> the benefit of this patchset is worth it. > > We don't care about overhead in iowq, but would be better to get rid > of the in_worker in io_read(). See comments to 1/2. > > Btw, you told that it performs better comparing to normal > IOSQE_ASYNC. I'm confused, didn't we agree that it can be > merged into IOSQE_ASYNC without extra flags? I said 'better than the old logic IOSQE_ASYNC logic for pollable cases'.. > >> >> Btw, we need to tweak the io-cancel.c a bit, not a big problem. >> liburing tests will come later. >> >> v1-->v2: >> - split logic of force_nonblock >> - tweak added code in io_wq_submit_work to reduce overhead >> from Pavel's commments. >> >> Hao Xu (2): >> io_uring: split logic of force_nonblock >> io_uring: implement async hybrid mode for pollable requests >> >> fs/io_uring.c | 85 ++++++++++++++++++++++++++--------- >> include/uapi/linux/io_uring.h | 4 +- >> 2 files changed, 66 insertions(+), 23 deletions(-) >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/2] async hybrid for pollable requests 2021-10-18 12:31 ` [PATCH v2 0/2] async hybrid " Pavel Begunkov 2021-10-18 12:35 ` Hao Xu @ 2021-10-18 13:17 ` Hao Xu 1 sibling, 0 replies; 12+ messages in thread From: Hao Xu @ 2021-10-18 13:17 UTC (permalink / raw) To: Pavel Begunkov, Jens Axboe; +Cc: io-uring, Joseph Qi 在 2021/10/18 下午8:31, Pavel Begunkov 写道: > On 10/18/21 11:29, Hao Xu wrote: >> 1/2 is a prep patch. 2/2 is the main one. >> The good thing: see commit message. >> the side effect: for normal io-worker path, added two if and two local >> variables. for FORCE_ASYNC path, added three if and several dereferences >> >> I think it is fine since the io-worker path is not the fast path, and >> the benefit of this patchset is worth it. > > We don't care about overhead in iowq, but would be better to get rid > of the in_worker in io_read(). See comments to 1/2. > > Btw, you told that it performs better comparing to normal > IOSQE_ASYNC. I'm confused, didn't we agree that it can be > merged into IOSQE_ASYNC without extra flags? I see what you are saying, forgot to remove the flag stuff..will be more careful. > >> >> Btw, we need to tweak the io-cancel.c a bit, not a big problem. >> liburing tests will come later. >> >> v1-->v2: >> - split logic of force_nonblock >> - tweak added code in io_wq_submit_work to reduce overhead >> from Pavel's commments. >> >> Hao Xu (2): >> io_uring: split logic of force_nonblock >> io_uring: implement async hybrid mode for pollable requests >> >> fs/io_uring.c | 85 ++++++++++++++++++++++++++--------- >> include/uapi/linux/io_uring.h | 4 +- >> 2 files changed, 66 insertions(+), 23 deletions(-) >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-10-18 13:17 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-18 11:29 [PATCH v2 0/2] async hybrid for pollable requests Hao Xu 2021-10-18 11:29 ` [PATCH 1/2] io_uring: split logic of force_nonblock Hao Xu 2021-10-18 12:13 ` Pavel Begunkov 2021-10-18 12:27 ` Pavel Begunkov 2021-10-18 13:00 ` Hao Xu 2021-10-18 11:29 ` [PATCH 2/2] io_uring: implement async hybrid mode for pollable requests Hao Xu 2021-10-18 11:34 ` Hao Xu 2021-10-18 12:10 ` Pavel Begunkov 2021-10-18 12:20 ` Hao Xu 2021-10-18 12:31 ` [PATCH v2 0/2] async hybrid " Pavel Begunkov 2021-10-18 12:35 ` Hao Xu 2021-10-18 13:17 ` Hao Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).