[PATCH for-next v3 0/9] io_uring: batch multishot completions

[PATCH for-next v3 0/9] io_uring: batch multishot completions

[Dylan Yudaken]

Multishot completions currently all go through io_post_aux_cqe which will
do a lock/unlock pair of the completion spinlock, and also possibly signal
an eventfd if registered. This can slow down applications that use these
features.

This series allows the posted completions to be batched using the same
IO_URING_F_COMPLETE_DEFER as exists for non multishot completions. A
critical property of this is that all multishot completions must be
flushed to the CQ ring before the non-multishot completion (say an error)
or else ordering will break. This implies that if some completions were
deferred, then the rest must also be to keep that ordering. In order to do
this the first few patches move all the completion code into a simpler
path that defers completions when possible.

The batching is done by keeping an array of 16 CQEs, and adding to it
rather than posting immediately. If it fills up the posting happens then.

A microbenchmark was run ([1]) to test this and showed a 2.3x rps
improvment (8.3 M/s vs 19.3 M/s).

Patches 1-3 clean up the completion paths
Patch 4 introduces the cqe array
Patch 5 allows io_post_aux_cqe to use the cqe array to defer completions
Patches 6-8 are small cleanups
Patch 9 enables defered completions for multishot polled requests

v3:
 - rebase onto recent changes. A lot of duplicate changes so dropped 4 patches

v2:
 - Rebase. This includes having to deal with the allow_overflow flag
 - split io_post_aux_cqe up
 - remove msg_ring changes (as not needed)
 - add the patch 10-12 cleanups

[1]: https://github.com/DylanZA/liburing/commit/9ac66b36bcf4477bfafeff1c5f107896b7ae31cf
Run with $ make -j && ./benchmark/reg.b -s 1 -t 2000 -r 10

Dylan Yudaken (9):
  io_uring: io_req_complete_post should defer if available
  io_uring: always lock in io_apoll_task_func
  io_uring: defer all io_req_complete_failed
  io_uring: allow defer completion for aux posted cqes
  io_uring: add io_aux_cqe which allows deferred completion
  io_uring: make io_fill_cqe_aux static
  io_uring: add lockdep assertion in io_fill_cqe_aux
  io_uring: remove overflow param from io_post_aux_cqe
  io_uring: allow multishot polled reqs to defer completion

 include/linux/io_uring_types.h |   2 +
 io_uring/io_uring.c            | 101 ++++++++++++++++++++++++++-------
 io_uring/io_uring.h            |   9 ++-
 io_uring/msg_ring.c            |   4 +-
 io_uring/net.c                 |   7 ++-
 io_uring/poll.c                |   9 +--
 io_uring/rsrc.c                |   4 +-
 7 files changed, 101 insertions(+), 35 deletions(-)


base-commit: 6321e75e0bc52b0feff9643292acc92494c00e8d
-- 
2.30.2

[PATCH for-next v3 1/9] io_uring: io_req_complete_post should defer if available

[Dylan Yudaken]

For consistency always defer completion if specified in the issue flags.

Signed-off-by: Dylan Yudaken <dylany@meta.com>
---
 io_uring/io_uring.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index cc27413129fc..ec23ebb63489 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -852,7 +852,9 @@ static void __io_req_complete_post(struct io_kiocb *req)
 
 void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags)
 {
-	if (!(issue_flags & IO_URING_F_UNLOCKED) ||
+	if (issue_flags & IO_URING_F_COMPLETE_DEFER) {
+		io_req_complete_defer(req);
+	} else if (!(issue_flags & IO_URING_F_UNLOCKED) ||
 	    !(req->ctx->flags & IORING_SETUP_IOPOLL)) {
 		__io_req_complete_post(req);
 	} else {
-- 
2.30.2

[PATCH for-next v3 2/9] io_uring: always lock in io_apoll_task_func

[Dylan Yudaken]

This is required for the failure case (io_req_complete_failed) and is
missing.

The alternative would be to only lock in the failure path, however all of
the non-error paths in io_poll_check_events that do not do not return
IOU_POLL_NO_ACTION end up locking anyway. The only extraneous lock would
be for the multishot poll overflowing the CQE ring, however multishot poll
would probably benefit from being locked as it will allow completions to
be batched.

So it seems reasonable to lock always.

Signed-off-by: Dylan Yudaken <dylany@meta.com>
---
 io_uring/poll.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/io_uring/poll.c b/io_uring/poll.c
index 4624e5eba63e..42aa10b50f6c 100644
--- a/io_uring/poll.c
+++ b/io_uring/poll.c
@@ -308,11 +308,12 @@ static void io_apoll_task_func(struct io_kiocb *req, bool *locked)
 	if (ret == IOU_POLL_NO_ACTION)
 		return;
 
+	io_tw_lock(req->ctx, locked);
 	io_poll_remove_entries(req);
 	io_poll_tw_hash_eject(req, locked);
 
 	if (ret == IOU_POLL_REMOVE_POLL_USE_RES)
-		io_req_complete_post_tw(req, locked);
+		io_req_task_complete(req, locked);
 	else if (ret == IOU_POLL_DONE)
 		io_req_task_submit(req, locked);
 	else
-- 
2.30.2

[PATCH for-next v3 3/9] io_uring: defer all io_req_complete_failed

[Dylan Yudaken]

All failures happen under lock now, and can be deferred. To be consistent
when the failure has happened after some multishot cqe has been
deferred (and keep ordering), always defer failures.

To make this obvious at the caller (and to help prevent a future bug)
rename io_req_complete_failed to io_req_defer_failed.

Signed-off-by: Dylan Yudaken <dylany@meta.com>
---
 io_uring/io_uring.c | 17 ++++++++---------
 io_uring/io_uring.h |  2 +-
 io_uring/poll.c     |  2 +-
 3 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index ec23ebb63489..cbd271b6188a 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -866,7 +866,7 @@ void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags)
 	}
 }
 
-void io_req_complete_failed(struct io_kiocb *req, s32 res)
+void io_req_defer_failed(struct io_kiocb *req, s32 res)
 	__must_hold(&ctx->uring_lock)
 {
 	const struct io_op_def *def = &io_op_defs[req->opcode];
@@ -877,7 +877,7 @@ void io_req_complete_failed(struct io_kiocb *req, s32 res)
 	io_req_set_res(req, res, io_put_kbuf(req, IO_URING_F_UNLOCKED));
 	if (def->fail)
 		def->fail(req);
-	io_req_complete_post(req, 0);
+	io_req_complete_defer(req);
 }
 
 /*
@@ -1233,9 +1233,8 @@ int io_run_local_work(struct io_ring_ctx *ctx)
 
 static void io_req_task_cancel(struct io_kiocb *req, bool *locked)
 {
-	/* not needed for normal modes, but SQPOLL depends on it */
 	io_tw_lock(req->ctx, locked);
-	io_req_complete_failed(req, req->cqe.res);
+	io_req_defer_failed(req, req->cqe.res);
 }
 
 void io_req_task_submit(struct io_kiocb *req, bool *locked)
@@ -1245,7 +1244,7 @@ void io_req_task_submit(struct io_kiocb *req, bool *locked)
 	if (likely(!(req->task->flags & PF_EXITING)))
 		io_queue_sqe(req);
 	else
-		io_req_complete_failed(req, -EFAULT);
+		io_req_defer_failed(req, -EFAULT);
 }
 
 void io_req_task_queue_fail(struct io_kiocb *req, int ret)
@@ -1632,7 +1631,7 @@ static __cold void io_drain_req(struct io_kiocb *req)
 	ret = io_req_prep_async(req);
 	if (ret) {
 fail:
-		io_req_complete_failed(req, ret);
+		io_req_defer_failed(req, ret);
 		return;
 	}
 	io_prep_async_link(req);
@@ -1862,7 +1861,7 @@ static void io_queue_async(struct io_kiocb *req, int ret)
 	struct io_kiocb *linked_timeout;
 
 	if (ret != -EAGAIN || (req->flags & REQ_F_NOWAIT)) {
-		io_req_complete_failed(req, ret);
+		io_req_defer_failed(req, ret);
 		return;
 	}
 
@@ -1912,14 +1911,14 @@ static void io_queue_sqe_fallback(struct io_kiocb *req)
 		 */
 		req->flags &= ~REQ_F_HARDLINK;
 		req->flags |= REQ_F_LINK;
-		io_req_complete_failed(req, req->cqe.res);
+		io_req_defer_failed(req, req->cqe.res);
 	} else if (unlikely(req->ctx->drain_active)) {
 		io_drain_req(req);
 	} else {
 		int ret = io_req_prep_async(req);
 
 		if (unlikely(ret))
-			io_req_complete_failed(req, ret);
+			io_req_defer_failed(req, ret);
 		else
 			io_queue_iowq(req, NULL);
 	}
diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
index b5b80bf03385..a26d5aa7f3f3 100644
--- a/io_uring/io_uring.h
+++ b/io_uring/io_uring.h
@@ -30,7 +30,7 @@ bool io_req_cqe_overflow(struct io_kiocb *req);
 int io_run_task_work_sig(struct io_ring_ctx *ctx);
 int __io_run_local_work(struct io_ring_ctx *ctx, bool *locked);
 int io_run_local_work(struct io_ring_ctx *ctx);
-void io_req_complete_failed(struct io_kiocb *req, s32 res);
+void io_req_defer_failed(struct io_kiocb *req, s32 res);
 void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags);
 bool io_post_aux_cqe(struct io_ring_ctx *ctx, u64 user_data, s32 res, u32 cflags,
 		     bool allow_overflow);
diff --git a/io_uring/poll.c b/io_uring/poll.c
index 42aa10b50f6c..4bd43e6f5b72 100644
--- a/io_uring/poll.c
+++ b/io_uring/poll.c
@@ -317,7 +317,7 @@ static void io_apoll_task_func(struct io_kiocb *req, bool *locked)
 	else if (ret == IOU_POLL_DONE)
 		io_req_task_submit(req, locked);
 	else
-		io_req_complete_failed(req, ret);
+		io_req_defer_failed(req, ret);
 }
 
 static void __io_poll_execute(struct io_kiocb *req, int mask)
-- 
2.30.2

[PATCH for-next v3 4/9] io_uring: allow defer completion for aux posted cqes

[Dylan Yudaken]

Multishot ops cannot use the compl_reqs list as the request must stay in
the poll list, but that means they need to run each completion without
benefiting from batching.

Here introduce batching infrastructure for only small (ie 16 byte)
CQEs. This restriction is ok because there are no use cases posting 32
byte CQEs.

In the ring keep a batch of up to 16 posted results, and flush in the same
way as compl_reqs.

16 was chosen through experimentation on a microbenchmark ([1]), as well
as trying not to increase the size of the ring too much. This increases
the size to 1472 bytes from 1216.

[1]: https://github.com/DylanZA/liburing/commit/9ac66b36bcf4477bfafeff1c5f107896b7ae31cf
Run with $ make -j && ./benchmark/reg.b -s 1 -t 2000 -r 10
Gives results:
baseline	8309 k/s
8		18807 k/s
16		19338 k/s
32		20134 k/s

Suggested-by: Pavel Begunkov <asml.silence@gmail.com>
Signed-off-by: Dylan Yudaken <dylany@meta.com>
---
 include/linux/io_uring_types.h |  2 ++
 io_uring/io_uring.c            | 27 ++++++++++++++++++++++++---
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
index f5b687a787a3..accdfecee953 100644
--- a/include/linux/io_uring_types.h
+++ b/include/linux/io_uring_types.h
@@ -174,7 +174,9 @@ struct io_submit_state {
 	bool			plug_started;
 	bool			need_plug;
 	unsigned short		submit_nr;
+	unsigned int		cqes_count;
 	struct blk_plug		plug;
+	struct io_uring_cqe	cqes[16];
 };
 
 struct io_ev_fd {
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index cbd271b6188a..53b61b5cde80 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -167,7 +167,8 @@ EXPORT_SYMBOL(io_uring_get_socket);
 
 static inline void io_submit_flush_completions(struct io_ring_ctx *ctx)
 {
-	if (!wq_list_empty(&ctx->submit_state.compl_reqs))
+	if (!wq_list_empty(&ctx->submit_state.compl_reqs) ||
+	    ctx->submit_state.cqes_count)
 		__io_submit_flush_completions(ctx);
 }
 
@@ -802,6 +803,21 @@ bool io_fill_cqe_aux(struct io_ring_ctx *ctx, u64 user_data, s32 res, u32 cflags
 	return false;
 }
 
+static void __io_flush_post_cqes(struct io_ring_ctx *ctx)
+	__must_hold(&ctx->uring_lock)
+{
+	struct io_submit_state *state = &ctx->submit_state;
+	unsigned int i;
+
+	lockdep_assert_held(&ctx->uring_lock);
+	for (i = 0; i < state->cqes_count; i++) {
+		struct io_uring_cqe *cqe = &state->cqes[i];
+
+		io_fill_cqe_aux(ctx, cqe->user_data, cqe->res, cqe->flags, true);
+	}
+	state->cqes_count = 0;
+}
+
 bool io_post_aux_cqe(struct io_ring_ctx *ctx,
 		     u64 user_data, s32 res, u32 cflags,
 		     bool allow_overflow)
@@ -1325,6 +1341,9 @@ static void __io_submit_flush_completions(struct io_ring_ctx *ctx)
 	struct io_submit_state *state = &ctx->submit_state;
 
 	io_cq_lock(ctx);
+	/* must come first to preserve CQE ordering in failure cases */
+	if (state->cqes_count)
+		__io_flush_post_cqes(ctx);
 	wq_list_for_each(node, prev, &state->compl_reqs) {
 		struct io_kiocb *req = container_of(node, struct io_kiocb,
 					    comp_list);
@@ -1334,8 +1353,10 @@ static void __io_submit_flush_completions(struct io_ring_ctx *ctx)
 	}
 	io_cq_unlock_post(ctx);
 
-	io_free_batch_list(ctx, state->compl_reqs.first);
-	INIT_WQ_LIST(&state->compl_reqs);
+	if (!wq_list_empty(&ctx->submit_state.compl_reqs)) {
+		io_free_batch_list(ctx, state->compl_reqs.first);
+		INIT_WQ_LIST(&state->compl_reqs);
+	}
 }
 
 /*
-- 
2.30.2

[PATCH for-next v3 5/9] io_uring: add io_aux_cqe which allows deferred completion

[Dylan Yudaken]

Use the just introduced deferred post cqe completion state when possible
in io_aux_cqe. If not possible fallback to io_post_aux_cqe.

This introduces a complication because of allow_overflow. For deferred
completions we cannot know without locking the completion_lock if it will
overflow (and even if we locked it, another post could sneak in and cause
this cqe to be in overflow).
However since overflow protection is mostly a best effort defence in depth
to prevent infinite loops of CQEs for poll, just checking the overflow bit
is going to be good enough and will result in at most 16 (array size of
deferred cqes) overflows.

Suggested-by: Pavel Begunkov <asml.silence@gmail.com>
Signed-off-by: Dylan Yudaken <dylany@meta.com>
---
 io_uring/io_uring.c | 34 ++++++++++++++++++++++++++++++++++
 io_uring/io_uring.h |  2 ++
 io_uring/net.c      |  7 ++++---
 io_uring/poll.c     |  4 ++--
 4 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 53b61b5cde80..4f48e8a919a2 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -830,6 +830,40 @@ bool io_post_aux_cqe(struct io_ring_ctx *ctx,
 	return filled;
 }
 
+bool io_aux_cqe(struct io_ring_ctx *ctx, bool defer, u64 user_data, s32 res, u32 cflags,
+		bool allow_overflow)
+{
+	struct io_uring_cqe *cqe;
+	unsigned int length;
+
+	if (!defer)
+		return io_post_aux_cqe(ctx, user_data, res, cflags, allow_overflow);
+
+	length = ARRAY_SIZE(ctx->submit_state.cqes);
+
+	lockdep_assert_held(&ctx->uring_lock);
+
+	if (ctx->submit_state.cqes_count == length) {
+		io_cq_lock(ctx);
+		__io_flush_post_cqes(ctx);
+		/* no need to flush - flush is deferred */
+		spin_unlock(&ctx->completion_lock);
+	}
+
+	/* For defered completions this is not as strict as it is otherwise,
+	 * however it's main job is to prevent unbounded posted completions,
+	 * and in that it works just as well.
+	 */
+	if (!allow_overflow && test_bit(IO_CHECK_CQ_OVERFLOW_BIT, &ctx->check_cq))
+		return false;
+
+	cqe = &ctx->submit_state.cqes[ctx->submit_state.cqes_count++];
+	cqe->user_data = user_data;
+	cqe->res = res;
+	cqe->flags = cflags;
+	return true;
+}
+
 static void __io_req_complete_post(struct io_kiocb *req)
 {
 	struct io_ring_ctx *ctx = req->ctx;
diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
index a26d5aa7f3f3..dd02adf3d0df 100644
--- a/io_uring/io_uring.h
+++ b/io_uring/io_uring.h
@@ -36,6 +36,8 @@ bool io_post_aux_cqe(struct io_ring_ctx *ctx, u64 user_data, s32 res, u32 cflags
 		     bool allow_overflow);
 bool io_fill_cqe_aux(struct io_ring_ctx *ctx, u64 user_data, s32 res, u32 cflags,
 		     bool allow_overflow);
+bool io_aux_cqe(struct io_ring_ctx *ctx, bool defer, u64 user_data, s32 res, u32 cflags,
+		bool allow_overflow);
 void __io_commit_cqring_flush(struct io_ring_ctx *ctx);
 
 static inline void io_req_complete_post_tw(struct io_kiocb *req, bool *locked)
diff --git a/io_uring/net.c b/io_uring/net.c
index 0de6f78ad978..90342dcb6b1d 100644
--- a/io_uring/net.c
+++ b/io_uring/net.c
@@ -601,8 +601,8 @@ static inline bool io_recv_finish(struct io_kiocb *req, int *ret,
 	}
 
 	if (!mshot_finished) {
-		if (io_post_aux_cqe(req->ctx, req->cqe.user_data, *ret,
-				    cflags | IORING_CQE_F_MORE, true)) {
+		if (io_aux_cqe(req->ctx, issue_flags & IO_URING_F_COMPLETE_DEFER,
+			       req->cqe.user_data, *ret, cflags | IORING_CQE_F_MORE, true)) {
 			io_recv_prep_retry(req);
 			return false;
 		}
@@ -1320,7 +1320,8 @@ int io_accept(struct io_kiocb *req, unsigned int issue_flags)
 
 	if (ret < 0)
 		return ret;
-	if (io_post_aux_cqe(ctx, req->cqe.user_data, ret, IORING_CQE_F_MORE, true))
+	if (io_aux_cqe(ctx, issue_flags & IO_URING_F_COMPLETE_DEFER,
+		       req->cqe.user_data, ret, IORING_CQE_F_MORE, true))
 		goto retry;
 
 	return -ECANCELED;
diff --git a/io_uring/poll.c b/io_uring/poll.c
index 4bd43e6f5b72..922c1a366c41 100644
--- a/io_uring/poll.c
+++ b/io_uring/poll.c
@@ -252,8 +252,8 @@ static int io_poll_check_events(struct io_kiocb *req, bool *locked)
 			__poll_t mask = mangle_poll(req->cqe.res &
 						    req->apoll_events);
 
-			if (!io_post_aux_cqe(ctx, req->cqe.user_data,
-					     mask, IORING_CQE_F_MORE, false)) {
+			if (!io_aux_cqe(ctx, *locked, req->cqe.user_data,
+					mask, IORING_CQE_F_MORE, false)) {
 				io_req_set_res(req, mask, 0);
 				return IOU_POLL_REMOVE_POLL_USE_RES;
 			}
-- 
2.30.2

[PATCH for-next v3 6/9] io_uring: make io_fill_cqe_aux static

[Dylan Yudaken]

This is only used in io_uring.c

Signed-off-by: Dylan Yudaken <dylany@meta.com>
---
 io_uring/io_uring.c | 4 ++--
 io_uring/io_uring.h | 2 --
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 4f48e8a919a2..92a7d6deacb6 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -770,8 +770,8 @@ struct io_uring_cqe *__io_get_cqe(struct io_ring_ctx *ctx, bool overflow)
 	return &rings->cqes[off];
 }
 
-bool io_fill_cqe_aux(struct io_ring_ctx *ctx, u64 user_data, s32 res, u32 cflags,
-		     bool allow_overflow)
+static bool io_fill_cqe_aux(struct io_ring_ctx *ctx, u64 user_data, s32 res, u32 cflags,
+			    bool allow_overflow)
 {
 	struct io_uring_cqe *cqe;
 
diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
index dd02adf3d0df..46694f40bf72 100644
--- a/io_uring/io_uring.h
+++ b/io_uring/io_uring.h
@@ -34,8 +34,6 @@ void io_req_defer_failed(struct io_kiocb *req, s32 res);
 void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags);
 bool io_post_aux_cqe(struct io_ring_ctx *ctx, u64 user_data, s32 res, u32 cflags,
 		     bool allow_overflow);
-bool io_fill_cqe_aux(struct io_ring_ctx *ctx, u64 user_data, s32 res, u32 cflags,
-		     bool allow_overflow);
 bool io_aux_cqe(struct io_ring_ctx *ctx, bool defer, u64 user_data, s32 res, u32 cflags,
 		bool allow_overflow);
 void __io_commit_cqring_flush(struct io_ring_ctx *ctx);
-- 
2.30.2

[PATCH for-next v3 7/9] io_uring: add lockdep assertion in io_fill_cqe_aux

[Dylan Yudaken]

Add an assertion for the completion lock to io_fill_cqe_aux

Signed-off-by: Dylan Yudaken <dylany@meta.com>
---
 io_uring/io_uring.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 92a7d6deacb6..e937e5d68b22 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -775,6 +775,8 @@ static bool io_fill_cqe_aux(struct io_ring_ctx *ctx, u64 user_data, s32 res, u32
 {
 	struct io_uring_cqe *cqe;
 
+	lockdep_assert_held(&ctx->completion_lock);
+
 	ctx->cq_extra++;
 
 	/*
-- 
2.30.2

[PATCH for-next v3 8/9] io_uring: remove overflow param from io_post_aux_cqe

[Dylan Yudaken]

The only call sites which would not allow overflow are also call sites
which would use the io_aux_cqe as they care about ordering.

So remove this parameter from io_post_aux_cqe.

Signed-off-by: Dylan Yudaken <dylany@meta.com>
---
 io_uring/io_uring.c | 12 ++++++++----
 io_uring/io_uring.h |  3 +--
 io_uring/msg_ring.c |  4 ++--
 io_uring/rsrc.c     |  4 ++--
 4 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index e937e5d68b22..f0e5ae7740cf 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -820,9 +820,8 @@ static void __io_flush_post_cqes(struct io_ring_ctx *ctx)
 	state->cqes_count = 0;
 }
 
-bool io_post_aux_cqe(struct io_ring_ctx *ctx,
-		     u64 user_data, s32 res, u32 cflags,
-		     bool allow_overflow)
+static bool __io_post_aux_cqe(struct io_ring_ctx *ctx, u64 user_data, s32 res, u32 cflags,
+			      bool allow_overflow)
 {
 	bool filled;
 
@@ -832,6 +831,11 @@ bool io_post_aux_cqe(struct io_ring_ctx *ctx,
 	return filled;
 }
 
+bool io_post_aux_cqe(struct io_ring_ctx *ctx, u64 user_data, s32 res, u32 cflags)
+{
+	return __io_post_aux_cqe(ctx, user_data, res, cflags, true);
+}
+
 bool io_aux_cqe(struct io_ring_ctx *ctx, bool defer, u64 user_data, s32 res, u32 cflags,
 		bool allow_overflow)
 {
@@ -839,7 +843,7 @@ bool io_aux_cqe(struct io_ring_ctx *ctx, bool defer, u64 user_data, s32 res, u32
 	unsigned int length;
 
 	if (!defer)
-		return io_post_aux_cqe(ctx, user_data, res, cflags, allow_overflow);
+		return __io_post_aux_cqe(ctx, user_data, res, cflags, allow_overflow);
 
 	length = ARRAY_SIZE(ctx->submit_state.cqes);
 
diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
index 46694f40bf72..dcb8e3468f1d 100644
--- a/io_uring/io_uring.h
+++ b/io_uring/io_uring.h
@@ -32,8 +32,7 @@ int __io_run_local_work(struct io_ring_ctx *ctx, bool *locked);
 int io_run_local_work(struct io_ring_ctx *ctx);
 void io_req_defer_failed(struct io_kiocb *req, s32 res);
 void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags);
-bool io_post_aux_cqe(struct io_ring_ctx *ctx, u64 user_data, s32 res, u32 cflags,
-		     bool allow_overflow);
+bool io_post_aux_cqe(struct io_ring_ctx *ctx, u64 user_data, s32 res, u32 cflags);
 bool io_aux_cqe(struct io_ring_ctx *ctx, bool defer, u64 user_data, s32 res, u32 cflags,
 		bool allow_overflow);
 void __io_commit_cqring_flush(struct io_ring_ctx *ctx);
diff --git a/io_uring/msg_ring.c b/io_uring/msg_ring.c
index 90d2fc6fd80e..afb543aab9f6 100644
--- a/io_uring/msg_ring.c
+++ b/io_uring/msg_ring.c
@@ -31,7 +31,7 @@ static int io_msg_ring_data(struct io_kiocb *req)
 	if (msg->src_fd || msg->dst_fd || msg->flags)
 		return -EINVAL;
 
-	if (io_post_aux_cqe(target_ctx, msg->user_data, msg->len, 0, true))
+	if (io_post_aux_cqe(target_ctx, msg->user_data, msg->len, 0))
 		return 0;
 
 	return -EOVERFLOW;
@@ -116,7 +116,7 @@ static int io_msg_send_fd(struct io_kiocb *req, unsigned int issue_flags)
 	 * completes with -EOVERFLOW, then the sender must ensure that a
 	 * later IORING_OP_MSG_RING delivers the message.
 	 */
-	if (!io_post_aux_cqe(target_ctx, msg->user_data, msg->len, 0, true))
+	if (!io_post_aux_cqe(target_ctx, msg->user_data, msg->len, 0))
 		ret = -EOVERFLOW;
 out_unlock:
 	io_double_unlock_ctx(ctx, target_ctx, issue_flags);
diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
index 187f1c83e779..133608200769 100644
--- a/io_uring/rsrc.c
+++ b/io_uring/rsrc.c
@@ -170,10 +170,10 @@ static void __io_rsrc_put_work(struct io_rsrc_node *ref_node)
 		if (prsrc->tag) {
 			if (ctx->flags & IORING_SETUP_IOPOLL) {
 				mutex_lock(&ctx->uring_lock);
-				io_post_aux_cqe(ctx, prsrc->tag, 0, 0, true);
+				io_post_aux_cqe(ctx, prsrc->tag, 0, 0);
 				mutex_unlock(&ctx->uring_lock);
 			} else {
-				io_post_aux_cqe(ctx, prsrc->tag, 0, 0, true);
+				io_post_aux_cqe(ctx, prsrc->tag, 0, 0);
 			}
 		}
 
-- 
2.30.2

[PATCH for-next v3 9/9] io_uring: allow multishot polled reqs to defer completion

[Dylan Yudaken]

Until now there was no reason for multishot polled requests to defer
completions as there was no functional difference. However now this will
actually defer the completions, for a performance win.

Signed-off-by: Dylan Yudaken <dylany@meta.com>
---
 io_uring/io_uring.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index f0e5ae7740cf..164904e7da25 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -1805,7 +1805,8 @@ int io_poll_issue(struct io_kiocb *req, bool *locked)
 	io_tw_lock(req->ctx, locked);
 	if (unlikely(req->task->flags & PF_EXITING))
 		return -EFAULT;
-	return io_issue_sqe(req, IO_URING_F_NONBLOCK|IO_URING_F_MULTISHOT);
+	return io_issue_sqe(req, IO_URING_F_NONBLOCK|IO_URING_F_MULTISHOT|
+				 IO_URING_F_COMPLETE_DEFER);
 }
 
 struct io_wq_work *io_wq_free_work(struct io_wq_work *work)
-- 
2.30.2

Re: [PATCH for-next v3 0/9] io_uring: batch multishot completions

[Jens Axboe]

On Thu, 24 Nov 2022 01:35:50 -0800, Dylan Yudaken wrote:
> Multishot completions currently all go through io_post_aux_cqe which will
> do a lock/unlock pair of the completion spinlock, and also possibly signal
> an eventfd if registered. This can slow down applications that use these
> features.
> 
> This series allows the posted completions to be batched using the same
> IO_URING_F_COMPLETE_DEFER as exists for non multishot completions. A
> critical property of this is that all multishot completions must be
> flushed to the CQ ring before the non-multishot completion (say an error)
> or else ordering will break. This implies that if some completions were
> deferred, then the rest must also be to keep that ordering. In order to do
> this the first few patches move all the completion code into a simpler
> path that defers completions when possible.
> 
> [...]

Applied, thanks!

[1/9] io_uring: io_req_complete_post should defer if available
      commit: 8fa737e0de7d3c4dc3d7cb9a9d9a6362d872c3f3
[2/9] io_uring: always lock in io_apoll_task_func
      commit: ca23d244ec99cc1a7a1c91f6a25bb074ea00bff1
[3/9] io_uring: defer all io_req_complete_failed
      commit: d62208d1e2d73d9949c7c58518fbd915eacad102
[4/9] io_uring: allow defer completion for aux posted cqes
      commit: 8e003ae8505a7bb728cb158198ca88912818da70
[5/9] io_uring: add io_aux_cqe which allows deferred completion
      commit: 9101a50761cf126399014cbfa518804c75c64157
[6/9] io_uring: make io_fill_cqe_aux static
      commit: 84eca39b41bc03372d647db83319ec0f6c230cda
[7/9] io_uring: add lockdep assertion in io_fill_cqe_aux
      commit: 3827133217eb3cdcb3824b22a31b76bf106a2ae3
[8/9] io_uring: remove overflow param from io_post_aux_cqe
      commit: 48ca51e666ed91f75a548a460faa60d08cfd3af6
[9/9] io_uring: allow multishot polled reqs to defer completion
      commit: 41d52216bb00504be5cf08e2d5ec8a41d16d9a67

Best regards,
-- 
Jens Axboe

Re: [PATCH for-next v3 1/9] io_uring: io_req_complete_post should defer if available

[Pavel Begunkov]

On 11/24/22 09:35, Dylan Yudaken wrote:
> For consistency always defer completion if specified in the issue flags.
> 
> Signed-off-by: Dylan Yudaken <dylany@meta.com>
> ---
>   io_uring/io_uring.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index cc27413129fc..ec23ebb63489 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -852,7 +852,9 @@ static void __io_req_complete_post(struct io_kiocb *req)
>   
>   void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags)
>   {
> -	if (!(issue_flags & IO_URING_F_UNLOCKED) ||
> +	if (issue_flags & IO_URING_F_COMPLETE_DEFER) {
> +		io_req_complete_defer(req);
> +	} else if (!(issue_flags & IO_URING_F_UNLOCKED) ||
>   	    !(req->ctx->flags & IORING_SETUP_IOPOLL)) {
>   		__io_req_complete_post(req);
>   	} else {

I think it's better to leave it and not impose a second meaning
onto it. We can explicitly call io_req_complete_defer() in all
places that require it, maybe with a new helper like io_req_complete()
if needed.

-- 
Pavel Begunkov

Re: [PATCH for-next v3 4/9] io_uring: allow defer completion for aux posted cqes

[Pavel Begunkov]

On 11/24/22 09:35, Dylan Yudaken wrote:
> Multishot ops cannot use the compl_reqs list as the request must stay in
> the poll list, but that means they need to run each completion without
> benefiting from batching.
> 
> Here introduce batching infrastructure for only small (ie 16 byte)
> CQEs. This restriction is ok because there are no use cases posting 32
> byte CQEs.
> 
> In the ring keep a batch of up to 16 posted results, and flush in the same
> way as compl_reqs.
> 
> 16 was chosen through experimentation on a microbenchmark ([1]), as well
> as trying not to increase the size of the ring too much. This increases
> the size to 1472 bytes from 1216.

It might be cleaner to defer io_cqring_ev_posted() instead of caching
cqes and spinlocking is not a big problem, because we can globally
get rid of them.


> [1]: https://github.com/DylanZA/liburing/commit/9ac66b36bcf4477bfafeff1c5f107896b7ae31cf
> Run with $ make -j && ./benchmark/reg.b -s 1 -t 2000 -r 10
> Gives results:
> baseline	8309 k/s
> 8		18807 k/s
> 16		19338 k/s
> 32		20134 k/s
> 
> Suggested-by: Pavel Begunkov <asml.silence@gmail.com>
> Signed-off-by: Dylan Yudaken <dylany@meta.com>
> ---
>   include/linux/io_uring_types.h |  2 ++
>   io_uring/io_uring.c            | 27 ++++++++++++++++++++++++---
>   2 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
> index f5b687a787a3..accdfecee953 100644
> --- a/include/linux/io_uring_types.h
> +++ b/include/linux/io_uring_types.h
> @@ -174,7 +174,9 @@ struct io_submit_state {
>   	bool			plug_started;
>   	bool			need_plug;
>   	unsigned short		submit_nr;
> +	unsigned int		cqes_count;
>   	struct blk_plug		plug;
> +	struct io_uring_cqe	cqes[16];
>   };
>   
>   struct io_ev_fd {
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index cbd271b6188a..53b61b5cde80 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -167,7 +167,8 @@ EXPORT_SYMBOL(io_uring_get_socket);
>   
>   static inline void io_submit_flush_completions(struct io_ring_ctx *ctx)
>   {
> -	if (!wq_list_empty(&ctx->submit_state.compl_reqs))
> +	if (!wq_list_empty(&ctx->submit_state.compl_reqs) ||
> +	    ctx->submit_state.cqes_count)
>   		__io_submit_flush_completions(ctx);
>   }
>   
> @@ -802,6 +803,21 @@ bool io_fill_cqe_aux(struct io_ring_ctx *ctx, u64 user_data, s32 res, u32 cflags
>   	return false;
>   }
>   
> +static void __io_flush_post_cqes(struct io_ring_ctx *ctx)
> +	__must_hold(&ctx->uring_lock)
> +{
> +	struct io_submit_state *state = &ctx->submit_state;
> +	unsigned int i;
> +
> +	lockdep_assert_held(&ctx->uring_lock);
> +	for (i = 0; i < state->cqes_count; i++) {
> +		struct io_uring_cqe *cqe = &state->cqes[i];
> +
> +		io_fill_cqe_aux(ctx, cqe->user_data, cqe->res, cqe->flags, true);
> +	}
> +	state->cqes_count = 0;
> +}
> +
>   bool io_post_aux_cqe(struct io_ring_ctx *ctx,
>   		     u64 user_data, s32 res, u32 cflags,
>   		     bool allow_overflow)
> @@ -1325,6 +1341,9 @@ static void __io_submit_flush_completions(struct io_ring_ctx *ctx)
>   	struct io_submit_state *state = &ctx->submit_state;
>   
>   	io_cq_lock(ctx);
> +	/* must come first to preserve CQE ordering in failure cases */
> +	if (state->cqes_count)
> +		__io_flush_post_cqes(ctx);
>   	wq_list_for_each(node, prev, &state->compl_reqs) {
>   		struct io_kiocb *req = container_of(node, struct io_kiocb,
>   					    comp_list);
> @@ -1334,8 +1353,10 @@ static void __io_submit_flush_completions(struct io_ring_ctx *ctx)
>   	}
>   	io_cq_unlock_post(ctx);
>   
> -	io_free_batch_list(ctx, state->compl_reqs.first);
> -	INIT_WQ_LIST(&state->compl_reqs);
> +	if (!wq_list_empty(&ctx->submit_state.compl_reqs)) {
> +		io_free_batch_list(ctx, state->compl_reqs.first);
> +		INIT_WQ_LIST(&state->compl_reqs);
> +	}
>   }
>   
>   /*

-- 
Pavel Begunkov

Re: [PATCH for-next v3 1/9] io_uring: io_req_complete_post should defer if available

[Dylan Yudaken]

On Thu, 2022-11-24 at 15:56 +0000, Pavel Begunkov wrote:
> On 11/24/22 09:35, Dylan Yudaken wrote:
> > For consistency always defer completion if specified in the issue
> > flags.
> > 
> > Signed-off-by: Dylan Yudaken <dylany@meta.com>
> > ---
> >   io_uring/io_uring.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> > index cc27413129fc..ec23ebb63489 100644
> > --- a/io_uring/io_uring.c
> > +++ b/io_uring/io_uring.c
> > @@ -852,7 +852,9 @@ static void __io_req_complete_post(struct
> > io_kiocb *req)
> >   
> >   void io_req_complete_post(struct io_kiocb *req, unsigned
> > issue_flags)
> >   {
> > -       if (!(issue_flags & IO_URING_F_UNLOCKED) ||
> > +       if (issue_flags & IO_URING_F_COMPLETE_DEFER) {
> > +               io_req_complete_defer(req);
> > +       } else if (!(issue_flags & IO_URING_F_UNLOCKED) ||
> >             !(req->ctx->flags & IORING_SETUP_IOPOLL)) {
> >                 __io_req_complete_post(req);
> >         } else {
> 
> I think it's better to leave it and not impose a second meaning
> onto it. We can explicitly call io_req_complete_defer() in all
> places that require it, maybe with a new helper like
> io_req_complete()
> if needed.
> 

I think you may be right - I'll give it a try and send some clean up
patches.

Dylan