[PATCH for-next 0/7] completion path optimisations

[PATCH for-next 0/7] completion path optimisations

[Pavel Begunkov]

A small series that for me prepares the code for further work but is
also adds some nice optimisations for completion path, including
removing an extra smp_mb() from the iopoll path.

Pavel Begunkov (7):
  io_uring: normilise naming for fill_cqe*
  io_uring: refactor timeout cancellation cqe posting
  io_uring: extend provided buf return to fails
  io_uring: remove extra barrier for non-sqpoll iopoll
  io_uring: shuffle io_eventfd_signal() bits around
  io_uring: thin down io_commit_cqring()
  io_uring: fold evfd signalling under a slower path

 fs/io_uring.c | 106 +++++++++++++++++++++++++-------------------------
 1 file changed, 54 insertions(+), 52 deletions(-)

-- 
2.35.1

[PATCH 1/7] io_uring: normilise naming for fill_cqe*

[Pavel Begunkov]

Restore consistency in __io_fill_cqe* like helpers, always honouring
"io_" prefix and adding "req" when we're passing in a request.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 299154efcd8a..10fb82f1c8ca 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2011,7 +2011,7 @@ static bool io_cqring_event_overflow(struct io_ring_ctx *ctx, u64 user_data,
 	return true;
 }
 
-static inline bool __fill_cqe(struct io_ring_ctx *ctx, u64 user_data,
+static inline bool __io_fill_cqe(struct io_ring_ctx *ctx, u64 user_data,
 				 s32 res, u32 cflags)
 {
 	struct io_uring_cqe *cqe;
@@ -2031,16 +2031,16 @@ static inline bool __fill_cqe(struct io_ring_ctx *ctx, u64 user_data,
 	return io_cqring_event_overflow(ctx, user_data, res, cflags);
 }
 
-static inline bool __io_fill_cqe(struct io_kiocb *req, s32 res, u32 cflags)
+static inline bool __io_fill_cqe_req(struct io_kiocb *req, s32 res, u32 cflags)
 {
 	trace_io_uring_complete(req->ctx, req, req->user_data, res, cflags);
-	return __fill_cqe(req->ctx, req->user_data, res, cflags);
+	return __io_fill_cqe(req->ctx, req->user_data, res, cflags);
 }
 
 static noinline void io_fill_cqe_req(struct io_kiocb *req, s32 res, u32 cflags)
 {
 	if (!(req->flags & REQ_F_CQE_SKIP))
-		__io_fill_cqe(req, res, cflags);
+		__io_fill_cqe_req(req, res, cflags);
 }
 
 static noinline bool io_fill_cqe_aux(struct io_ring_ctx *ctx, u64 user_data,
@@ -2048,7 +2048,7 @@ static noinline bool io_fill_cqe_aux(struct io_ring_ctx *ctx, u64 user_data,
 {
 	ctx->cq_extra++;
 	trace_io_uring_complete(ctx, NULL, user_data, res, cflags);
-	return __fill_cqe(ctx, user_data, res, cflags);
+	return __io_fill_cqe(ctx, user_data, res, cflags);
 }
 
 static void __io_req_complete_post(struct io_kiocb *req, s32 res,
@@ -2057,7 +2057,7 @@ static void __io_req_complete_post(struct io_kiocb *req, s32 res,
 	struct io_ring_ctx *ctx = req->ctx;
 
 	if (!(req->flags & REQ_F_CQE_SKIP))
-		__io_fill_cqe(req, res, cflags);
+		__io_fill_cqe_req(req, res, cflags);
 	/*
 	 * If we're the last reference to this request, add to our locked
 	 * free_list cache.
@@ -2649,7 +2649,7 @@ static void __io_submit_flush_completions(struct io_ring_ctx *ctx)
 						    comp_list);
 
 			if (!(req->flags & REQ_F_CQE_SKIP))
-				__io_fill_cqe(req, req->result, req->cflags);
+				__io_fill_cqe_req(req, req->result, req->cflags);
 		}
 
 		io_commit_cqring(ctx);
@@ -2771,7 +2771,7 @@ static int io_do_iopoll(struct io_ring_ctx *ctx, bool force_nonspin)
 		if (unlikely(req->flags & REQ_F_CQE_SKIP))
 			continue;
 
-		__io_fill_cqe(req, req->result, io_put_kbuf(req, 0));
+		__io_fill_cqe_req(req, req->result, io_put_kbuf(req, 0));
 		nr_events++;
 	}
 
-- 
2.35.1

[PATCH 2/7] io_uring: refactor timeout cancellation cqe posting

[Pavel Begunkov]

io_fill_cqe*() is not always the best way to post CQEs just because
there is enough of infrastructure on top. Replace a raw call to a
variant of it inside of io_timeout_cancel(), which also saves us some
bloating and might help with batching later.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 10fb82f1c8ca..b4b12aa7d107 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -6432,10 +6432,7 @@ static int io_timeout_cancel(struct io_ring_ctx *ctx, __u64 user_data)
 
 	if (IS_ERR(req))
 		return PTR_ERR(req);
-
-	req_set_fail(req);
-	io_fill_cqe_req(req, -ECANCELED, 0);
-	io_put_req_deferred(req);
+	io_req_task_queue_fail(req, -ECANCELED);
 	return 0;
 }
 
-- 
2.35.1

[PATCH 3/7] io_uring: extend provided buf return to fails

[Pavel Begunkov]

It's never a good idea to put provided buffers without notifying the
userspace, it'll lead to userspace leaks, so add io_put_kbuf() in
io_req_complete_failed(). The fail helper is called by all sorts of
requests, but it's still safe to do as io_put_kbuf() will return 0 in
for all requests that don't support and so don't expect provided buffers.

btw, remove some code duplication from kiocb_done().

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index b4b12aa7d107..bbbbf889dfd8 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2116,7 +2116,7 @@ static inline void io_req_complete(struct io_kiocb *req, s32 res)
 static void io_req_complete_failed(struct io_kiocb *req, s32 res)
 {
 	req_set_fail(req);
-	io_req_complete_post(req, res, 0);
+	io_req_complete_post(req, res, io_put_kbuf(req, 0));
 }
 
 static void io_req_complete_fail_submit(struct io_kiocb *req)
@@ -3225,14 +3225,10 @@ static void kiocb_done(struct io_kiocb *req, ssize_t ret,
 
 	if (req->flags & REQ_F_REISSUE) {
 		req->flags &= ~REQ_F_REISSUE;
-		if (io_resubmit_prep(req)) {
+		if (io_resubmit_prep(req))
 			io_req_task_queue_reissue(req);
-		} else {
-			req_set_fail(req);
-			req->result = ret;
-			req->io_task_work.func = io_req_task_complete;
-			io_req_task_work_add(req, false);
-		}
+		else
+			io_req_task_queue_fail(req, ret);
 	}
 }
 
-- 
2.35.1

Re: [PATCH 3/7] io_uring: extend provided buf return to fails

[Jens Axboe]

On 3/16/22 8:03 PM, Pavel Begunkov wrote:
> It's never a good idea to put provided buffers without notifying the
> userspace, it'll lead to userspace leaks, so add io_put_kbuf() in
> io_req_complete_failed(). The fail helper is called by all sorts of
> requests, but it's still safe to do as io_put_kbuf() will return 0 in
> for all requests that don't support and so don't expect provided buffers.
> 
> btw, remove some code duplication from kiocb_done().

This really would be nicer as two patches...

-- 
Jens Axboe

[PATCH 4/7] io_uring: remove extra barrier for non-sqpoll iopoll

[Pavel Begunkov]

smp_mb() in io_cqring_ev_posted_iopoll() is only there because of
waitqueue_active(). However, non-SQPOLL IOPOLL ring doesn't wake the CQ
and so the barrier there is useless. Kill it, it's usually pretty
expensive.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index bbbbf889dfd8..603cbe687dd2 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1869,11 +1869,8 @@ static void io_cqring_ev_posted(struct io_ring_ctx *ctx)
 
 static void io_cqring_ev_posted_iopoll(struct io_ring_ctx *ctx)
 {
-	/* see waitqueue_active() comment */
-	smp_mb();
-
 	if (ctx->flags & IORING_SETUP_SQPOLL) {
-		if (waitqueue_active(&ctx->cq_wait))
+		if (wq_has_sleeper(&ctx->cq_wait))
 			wake_up_all(&ctx->cq_wait);
 	}
 	io_eventfd_signal(ctx);
-- 
2.35.1

[PATCH 5/7] io_uring: shuffle io_eventfd_signal() bits around

[Pavel Begunkov]

A preparation patch, which moves a fast ->io_ev_fd check out of
io_eventfd_signal() into ev_posted*(). Compilers are smart enough for it
to not change anything, but will need it later.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 603cbe687dd2..5a87e0622ecb 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1820,10 +1820,6 @@ static void io_eventfd_signal(struct io_ring_ctx *ctx)
 {
 	struct io_ev_fd *ev_fd;
 
-	/* Return quickly if ctx->io_ev_fd doesn't exist */
-	if (likely(!rcu_dereference_raw(ctx->io_ev_fd)))
-		return;
-
 	rcu_read_lock();
 	/*
 	 * rcu_dereference ctx->io_ev_fd once and use it for both for checking
@@ -1843,7 +1839,6 @@ static void io_eventfd_signal(struct io_ring_ctx *ctx)
 
 	if (!ev_fd->eventfd_async || io_wq_current_is_worker())
 		eventfd_signal(ev_fd->cq_ev_fd, 1);
-
 out:
 	rcu_read_unlock();
 }
@@ -1855,7 +1850,7 @@ static void io_eventfd_signal(struct io_ring_ctx *ctx)
  * 1:1 relationship between how many times this function is called (and
  * hence the eventfd count) and number of CQEs posted to the CQ ring.
  */
-static void io_cqring_ev_posted(struct io_ring_ctx *ctx)
+static inline void io_cqring_ev_posted(struct io_ring_ctx *ctx)
 {
 	/*
 	 * wake_up_all() may seem excessive, but io_wake_function() and
@@ -1864,7 +1859,8 @@ static void io_cqring_ev_posted(struct io_ring_ctx *ctx)
 	 */
 	if (wq_has_sleeper(&ctx->cq_wait))
 		wake_up_all(&ctx->cq_wait);
-	io_eventfd_signal(ctx);
+	if (unlikely(rcu_dereference_raw(ctx->io_ev_fd)))
+		io_eventfd_signal(ctx);
 }
 
 static void io_cqring_ev_posted_iopoll(struct io_ring_ctx *ctx)
@@ -1873,7 +1869,8 @@ static void io_cqring_ev_posted_iopoll(struct io_ring_ctx *ctx)
 		if (wq_has_sleeper(&ctx->cq_wait))
 			wake_up_all(&ctx->cq_wait);
 	}
-	io_eventfd_signal(ctx);
+	if (unlikely(rcu_dereference_raw(ctx->io_ev_fd)))
+		io_eventfd_signal(ctx);
 }
 
 /* Returns true if there are no backlogged entries after the flush */
-- 
2.35.1

[PATCH 6/7] io_uring: thin down io_commit_cqring()

[Pavel Begunkov]

io_commit_cqring() is currently always under spinlock section, so it's
always better to keep it as slim as possible. Move
__io_commit_cqring_flush() out of it into ev_posted*(). If fast checks
do fail and this post-processing is required, we'll reacquire
->completion_lock, which is fine as we don't care about performance of
draining and offset timeouts.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 5a87e0622ecb..c75a5767f58d 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1771,20 +1771,21 @@ static __cold void io_flush_timeouts(struct io_ring_ctx *ctx)
 	spin_unlock_irq(&ctx->timeout_lock);
 }
 
+static inline void io_commit_cqring(struct io_ring_ctx *ctx)
+{
+	/* order cqe stores with ring update */
+	smp_store_release(&ctx->rings->cq.tail, ctx->cached_cq_tail);
+}
+
 static __cold void __io_commit_cqring_flush(struct io_ring_ctx *ctx)
 {
+	spin_lock(&ctx->completion_lock);
 	if (ctx->off_timeout_used)
 		io_flush_timeouts(ctx);
 	if (ctx->drain_active)
 		io_queue_deferred(ctx);
-}
-
-static inline void io_commit_cqring(struct io_ring_ctx *ctx)
-{
-	if (unlikely(ctx->off_timeout_used || ctx->drain_active))
-		__io_commit_cqring_flush(ctx);
-	/* order cqe stores with ring update */
-	smp_store_release(&ctx->rings->cq.tail, ctx->cached_cq_tail);
+	io_commit_cqring(ctx);
+	spin_unlock(&ctx->completion_lock);
 }
 
 static inline bool io_sqring_full(struct io_ring_ctx *ctx)
@@ -1852,6 +1853,9 @@ static void io_eventfd_signal(struct io_ring_ctx *ctx)
  */
 static inline void io_cqring_ev_posted(struct io_ring_ctx *ctx)
 {
+	if (unlikely(ctx->off_timeout_used || ctx->drain_active))
+		__io_commit_cqring_flush(ctx);
+
 	/*
 	 * wake_up_all() may seem excessive, but io_wake_function() and
 	 * io_should_wake() handle the termination of the loop and only
@@ -1865,6 +1869,9 @@ static inline void io_cqring_ev_posted(struct io_ring_ctx *ctx)
 
 static void io_cqring_ev_posted_iopoll(struct io_ring_ctx *ctx)
 {
+	if (unlikely(ctx->off_timeout_used || ctx->drain_active))
+		__io_commit_cqring_flush(ctx);
+
 	if (ctx->flags & IORING_SETUP_SQPOLL) {
 		if (wq_has_sleeper(&ctx->cq_wait))
 			wake_up_all(&ctx->cq_wait);
-- 
2.35.1

[PATCH 7/7] io_uring: fold evfd signalling under a slower path

[Pavel Begunkov]

Add ->has_evfd flag, which is true IFF there is an eventfd attached, and
use it to hide io_eventfd_signal() into __io_commit_cqring_flush() and
combine fast checks in a single if. Also, gcc 11.2 wasn't inlining
io_cqring_ev_posted() without this change, so helps with that as well.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 60 +++++++++++++++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index c75a5767f58d..c026a90a8bd3 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -347,6 +347,7 @@ struct io_ring_ctx {
 		unsigned int		off_timeout_used: 1;
 		unsigned int		drain_active: 1;
 		unsigned int		drain_disabled: 1;
+		unsigned int		has_evfd: 1;
 	} ____cacheline_aligned_in_smp;
 
 	/* submission data */
@@ -1174,6 +1175,7 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
 static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags);
 
 static enum hrtimer_restart io_link_timeout_fn(struct hrtimer *timer);
+static void io_eventfd_signal(struct io_ring_ctx *ctx);
 
 static struct kmem_cache *req_cachep;
 
@@ -1777,15 +1779,19 @@ static inline void io_commit_cqring(struct io_ring_ctx *ctx)
 	smp_store_release(&ctx->rings->cq.tail, ctx->cached_cq_tail);
 }
 
-static __cold void __io_commit_cqring_flush(struct io_ring_ctx *ctx)
+static void __io_commit_cqring_flush(struct io_ring_ctx *ctx)
 {
-	spin_lock(&ctx->completion_lock);
-	if (ctx->off_timeout_used)
-		io_flush_timeouts(ctx);
-	if (ctx->drain_active)
-		io_queue_deferred(ctx);
-	io_commit_cqring(ctx);
-	spin_unlock(&ctx->completion_lock);
+	if (ctx->off_timeout_used || ctx->drain_active) {
+		spin_lock(&ctx->completion_lock);
+		if (ctx->off_timeout_used)
+			io_flush_timeouts(ctx);
+		if (ctx->drain_active)
+			io_queue_deferred(ctx);
+		io_commit_cqring(ctx);
+		spin_unlock(&ctx->completion_lock);
+	}
+	if (ctx->has_evfd)
+		io_eventfd_signal(ctx);
 }
 
 static inline bool io_sqring_full(struct io_ring_ctx *ctx)
@@ -1844,6 +1850,17 @@ static void io_eventfd_signal(struct io_ring_ctx *ctx)
 	rcu_read_unlock();
 }
 
+static inline void io_cqring_wake(struct io_ring_ctx *ctx)
+{
+	/*
+	 * wake_up_all() may seem excessive, but io_wake_function() and
+	 * io_should_wake() handle the termination of the loop and only
+	 * wake as many waiters as we need to.
+	 */
+	if (wq_has_sleeper(&ctx->cq_wait))
+		wake_up_all(&ctx->cq_wait);
+}
+
 /*
  * This should only get called when at least one event has been posted.
  * Some applications rely on the eventfd notification count only changing
@@ -1853,31 +1870,21 @@ static void io_eventfd_signal(struct io_ring_ctx *ctx)
  */
 static inline void io_cqring_ev_posted(struct io_ring_ctx *ctx)
 {
-	if (unlikely(ctx->off_timeout_used || ctx->drain_active))
+	if (unlikely(ctx->off_timeout_used || ctx->drain_active ||
+		     ctx->has_evfd))
 		__io_commit_cqring_flush(ctx);
 
-	/*
-	 * wake_up_all() may seem excessive, but io_wake_function() and
-	 * io_should_wake() handle the termination of the loop and only
-	 * wake as many waiters as we need to.
-	 */
-	if (wq_has_sleeper(&ctx->cq_wait))
-		wake_up_all(&ctx->cq_wait);
-	if (unlikely(rcu_dereference_raw(ctx->io_ev_fd)))
-		io_eventfd_signal(ctx);
+	io_cqring_wake(ctx);
 }
 
 static void io_cqring_ev_posted_iopoll(struct io_ring_ctx *ctx)
 {
-	if (unlikely(ctx->off_timeout_used || ctx->drain_active))
+	if (unlikely(ctx->off_timeout_used || ctx->drain_active ||
+		     ctx->has_evfd))
 		__io_commit_cqring_flush(ctx);
 
-	if (ctx->flags & IORING_SETUP_SQPOLL) {
-		if (wq_has_sleeper(&ctx->cq_wait))
-			wake_up_all(&ctx->cq_wait);
-	}
-	if (unlikely(rcu_dereference_raw(ctx->io_ev_fd)))
-		io_eventfd_signal(ctx);
+	if (ctx->flags & IORING_SETUP_SQPOLL)
+		io_cqring_wake(ctx);
 }
 
 /* Returns true if there are no backlogged entries after the flush */
@@ -9859,7 +9866,7 @@ static int io_eventfd_register(struct io_ring_ctx *ctx, void __user *arg,
 		return ret;
 	}
 	ev_fd->eventfd_async = eventfd_async;
-
+	ctx->has_evfd = true;
 	rcu_assign_pointer(ctx->io_ev_fd, ev_fd);
 	return 0;
 }
@@ -9879,6 +9886,7 @@ static int io_eventfd_unregister(struct io_ring_ctx *ctx)
 	ev_fd = rcu_dereference_protected(ctx->io_ev_fd,
 					lockdep_is_held(&ctx->uring_lock));
 	if (ev_fd) {
+		ctx->has_evfd = false;
 		rcu_assign_pointer(ctx->io_ev_fd, NULL);
 		call_rcu(&ev_fd->rcu, io_eventfd_put);
 		return 0;
-- 
2.35.1

Re: [PATCH for-next 0/7] completion path optimisations

[Jens Axboe]

On Thu, 17 Mar 2022 02:03:35 +0000, Pavel Begunkov wrote:
> A small series that for me prepares the code for further work but is
> also adds some nice optimisations for completion path, including
> removing an extra smp_mb() from the iopoll path.
> 
> Pavel Begunkov (7):
>   io_uring: normilise naming for fill_cqe*
>   io_uring: refactor timeout cancellation cqe posting
>   io_uring: extend provided buf return to fails
>   io_uring: remove extra barrier for non-sqpoll iopoll
>   io_uring: shuffle io_eventfd_signal() bits around
>   io_uring: thin down io_commit_cqring()
>   io_uring: fold evfd signalling under a slower path
> 
> [...]

Applied, thanks!

[1/7] io_uring: normilise naming for fill_cqe*
      commit: ae4da18941c1c13a9bd6f1d39888ca9a4ff3db91
[2/7] io_uring: refactor timeout cancellation cqe posting
      commit: 6695490dc85781fe98b782f36f27c13710dbc921
[3/7] io_uring: extend provided buf return to fails
      commit: b91ef1872869d99cd42e908eb9754b81115c2c05
[4/7] io_uring: remove extra barrier for non-sqpoll iopoll
      commit: 0f84747177b962c32243a57cb454193bdba4fe8d
[5/7] io_uring: shuffle io_eventfd_signal() bits around
      commit: 66fc25ca6b7ec4124606e0d59c71c6bcf14e05bb
[6/7] io_uring: thin down io_commit_cqring()
      commit: 9333f6b4628c8037a89ed23e1188d4b7dc5d74e4
[7/7] io_uring: fold evfd signalling under a slower path
      commit: 9aa8dfde4869ccdec0a7290b686dbc10e079e163

Best regards,
-- 
Jens Axboe