[PATCH for-next 0/4] clean up io_req_complete_post

[PATCH for-next 0/4] clean up io_req_complete_post

[Pavel Begunkov]

Patch 1 from Ming Lei removes a good chunk of unreachable code. Add a
warning in patch 2, and apparently we can develop on the idea and remove
even more dead code in patches 3,4.

Ming Lei (1):
  io_uring: kill dead code in io_req_complete_post

Pavel Begunkov (3):
  io_uring: turn implicit assumptions into a warning
  io_uring: remove async request cache
  io_uring: remove io_req_put_rsrc_locked()

 include/linux/io_uring_types.h |  4 --
 io_uring/io_uring.c            | 72 ++++++----------------------------
 io_uring/refs.h                |  7 ++++
 io_uring/rsrc.h                |  6 ---
 4 files changed, 20 insertions(+), 69 deletions(-)

-- 
2.44.0

[PATCH for-next 1/4] io_uring: kill dead code in io_req_complete_post

[Pavel Begunkov]

From: Ming Lei <ming.lei@redhat.com>

Since commit 8f6c829491fe ("io_uring: remove struct io_tw_state::locked"),
io_req_complete_post() is only called from io-wq submit work, where the
request reference is guaranteed to be grabbed and won't drop to zero
in io_req_complete_post().

Kill the dead code, meantime add req_ref_put() to put the reference.

Cc: Pavel Begunkov <asml.silence@gmail.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>
Signed-by: Pavel Begunkov <asml.silence@gmail.com>
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/io_uring.c | 37 ++-----------------------------------
 io_uring/refs.h     |  7 +++++++
 2 files changed, 9 insertions(+), 35 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 8a9584c5c8ce..b7f742fe9d41 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -930,7 +930,6 @@ bool io_req_post_cqe(struct io_kiocb *req, s32 res, u32 cflags)
 static void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags)
 {
 	struct io_ring_ctx *ctx = req->ctx;
-	struct io_rsrc_node *rsrc_node = NULL;
 
 	/*
 	 * Handle special CQ sync cases via task_work. DEFER_TASKRUN requires
@@ -947,42 +946,10 @@ static void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags)
 		if (!io_fill_cqe_req(ctx, req))
 			io_req_cqe_overflow(req);
 	}
-
-	/*
-	 * If we're the last reference to this request, add to our locked
-	 * free_list cache.
-	 */
-	if (req_ref_put_and_test(req)) {
-		if (req->flags & IO_REQ_LINK_FLAGS) {
-			if (req->flags & IO_DISARM_MASK)
-				io_disarm_next(req);
-			if (req->link) {
-				io_req_task_queue(req->link);
-				req->link = NULL;
-			}
-		}
-		io_put_kbuf_comp(req);
-		if (unlikely(req->flags & IO_REQ_CLEAN_FLAGS))
-			io_clean_op(req);
-		io_put_file(req);
-
-		rsrc_node = req->rsrc_node;
-		/*
-		 * Selected buffer deallocation in io_clean_op() assumes that
-		 * we don't hold ->completion_lock. Clean them here to avoid
-		 * deadlocks.
-		 */
-		io_put_task_remote(req->task);
-		wq_list_add_head(&req->comp_list, &ctx->locked_free_list);
-		ctx->locked_free_nr++;
-	}
 	io_cq_unlock_post(ctx);
 
-	if (rsrc_node) {
-		io_ring_submit_lock(ctx, issue_flags);
-		io_put_rsrc_node(ctx, rsrc_node);
-		io_ring_submit_unlock(ctx, issue_flags);
-	}
+	/* called from io-wq submit work only, the ref won't drop to zero */
+	req_ref_put(req);
 }
 
 void io_req_defer_failed(struct io_kiocb *req, s32 res)
diff --git a/io_uring/refs.h b/io_uring/refs.h
index 1336de3f2a30..63982ead9f7d 100644
--- a/io_uring/refs.h
+++ b/io_uring/refs.h
@@ -33,6 +33,13 @@ static inline void req_ref_get(struct io_kiocb *req)
 	atomic_inc(&req->refs);
 }
 
+static inline void req_ref_put(struct io_kiocb *req)
+{
+	WARN_ON_ONCE(!(req->flags & REQ_F_REFCOUNT));
+	WARN_ON_ONCE(req_ref_zero_or_close_to_overflow(req));
+	atomic_dec(&req->refs);
+}
+
 static inline void __io_req_set_refcount(struct io_kiocb *req, int nr)
 {
 	if (!(req->flags & REQ_F_REFCOUNT)) {
-- 
2.44.0

[PATCH for-next 2/4] io_uring: turn implicit assumptions into a warning

[Pavel Begunkov]

io_req_complete_post() is now io-wq only and shouldn't be used outside
of it, i.e. it relies that io-wq holds a ref for the request as
explained in a comment below. Let's add a warning to enforce the
assumption and make sure nobody would try to do anything weird.

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

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index b7f742fe9d41..c84650b0f7f2 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -931,6 +931,13 @@ static void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags)
 {
 	struct io_ring_ctx *ctx = req->ctx;
 
+	/*
+	 * All execution paths but io-wq use the deferred completions by
+	 * passing IO_URING_F_COMPLETE_DEFER and thus should not end up here.
+	 */
+	if (WARN_ON_ONCE(!(issue_flags & IO_URING_F_IOWQ)))
+		return;
+
 	/*
 	 * Handle special CQ sync cases via task_work. DEFER_TASKRUN requires
 	 * the submitter task context, IOPOLL protects with uring_lock.
@@ -948,7 +955,10 @@ static void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags)
 	}
 	io_cq_unlock_post(ctx);
 
-	/* called from io-wq submit work only, the ref won't drop to zero */
+	/*
+	 * We don't free the request here because we know it's called from
+	 * io-wq only, which holds a reference, so it cannot be the last put.
+	 */
 	req_ref_put(req);
 }
 
-- 
2.44.0

Re: [PATCH for-next 2/4] io_uring: turn implicit assumptions into a warning

[Ming Lei]

On Fri, Apr 05, 2024 at 04:50:03PM +0100, Pavel Begunkov wrote:
> io_req_complete_post() is now io-wq only and shouldn't be used outside
> of it, i.e. it relies that io-wq holds a ref for the request as
> explained in a comment below. Let's add a warning to enforce the
> assumption and make sure nobody would try to do anything weird.
> 
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Thanks,
Ming

[PATCH for-next 3/4] io_uring: remove async request cache

[Pavel Begunkov]

io_req_complete_post() was a sole user of ->locked_free_list, but
since we just gutted the function, the cache is not used anymore and
can be removed.

->locked_free_list served as an asynhronous counterpart of the main
request (i.e. struct io_kiocb) cache for all unlocked cases like io-wq.
Now they're all forced to be completed into the main cache directly,
off of the normal completion path or via io_free_req().

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 include/linux/io_uring_types.h |  4 ----
 io_uring/io_uring.c            | 22 ----------------------
 2 files changed, 26 deletions(-)

diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
index b191710bec4f..9c49aa2dac38 100644
--- a/include/linux/io_uring_types.h
+++ b/include/linux/io_uring_types.h
@@ -347,10 +347,6 @@ struct io_ring_ctx {
 
 	spinlock_t		completion_lock;
 
-	/* IRQ completion list, under ->completion_lock */
-	unsigned int		locked_free_nr;
-	struct io_wq_work_list	locked_free_list;
-
 	struct list_head	io_buffers_comp;
 	struct list_head	cq_overflow_list;
 	struct io_hash_table	cancel_table;
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index c84650b0f7f2..b20ee6a0e32e 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -335,7 +335,6 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
 	init_llist_head(&ctx->work_llist);
 	INIT_LIST_HEAD(&ctx->tctx_list);
 	ctx->submit_state.free_list.next = NULL;
-	INIT_WQ_LIST(&ctx->locked_free_list);
 	INIT_HLIST_HEAD(&ctx->waitid_list);
 #ifdef CONFIG_FUTEX
 	INIT_HLIST_HEAD(&ctx->futex_list);
@@ -990,15 +989,6 @@ static void io_preinit_req(struct io_kiocb *req, struct io_ring_ctx *ctx)
 	memset(&req->big_cqe, 0, sizeof(req->big_cqe));
 }
 
-static void io_flush_cached_locked_reqs(struct io_ring_ctx *ctx,
-					struct io_submit_state *state)
-{
-	spin_lock(&ctx->completion_lock);
-	wq_list_splice(&ctx->locked_free_list, &state->free_list);
-	ctx->locked_free_nr = 0;
-	spin_unlock(&ctx->completion_lock);
-}
-
 /*
  * A request might get retired back into the request caches even before opcode
  * handlers and io_issue_sqe() are done with it, e.g. inline completion path.
@@ -1012,17 +1002,6 @@ __cold bool __io_alloc_req_refill(struct io_ring_ctx *ctx)
 	void *reqs[IO_REQ_ALLOC_BATCH];
 	int ret;
 
-	/*
-	 * If we have more than a batch's worth of requests in our IRQ side
-	 * locked cache, grab the lock and move them over to our submission
-	 * side cache.
-	 */
-	if (data_race(ctx->locked_free_nr) > IO_COMPL_BATCH) {
-		io_flush_cached_locked_reqs(ctx, &ctx->submit_state);
-		if (!io_req_cache_empty(ctx))
-			return true;
-	}
-
 	ret = kmem_cache_alloc_bulk(req_cachep, gfp, ARRAY_SIZE(reqs), reqs);
 
 	/*
@@ -2741,7 +2720,6 @@ static void io_req_caches_free(struct io_ring_ctx *ctx)
 	int nr = 0;
 
 	mutex_lock(&ctx->uring_lock);
-	io_flush_cached_locked_reqs(ctx, &ctx->submit_state);
 
 	while (!io_req_cache_empty(ctx)) {
 		req = io_extract_req(ctx);
-- 
2.44.0

Re: [PATCH for-next 3/4] io_uring: remove async request cache

[Ming Lei]

On Fri, Apr 05, 2024 at 04:50:04PM +0100, Pavel Begunkov wrote:
> io_req_complete_post() was a sole user of ->locked_free_list, but
> since we just gutted the function, the cache is not used anymore and
> can be removed.
> 
> ->locked_free_list served as an asynhronous counterpart of the main
> request (i.e. struct io_kiocb) cache for all unlocked cases like io-wq.
> Now they're all forced to be completed into the main cache directly,
> off of the normal completion path or via io_free_req().
> 
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Thanks,
Ming

[PATCH for-next 4/4] io_uring: remove io_req_put_rsrc_locked()

[Pavel Begunkov]

io_req_put_rsrc_locked() is a weird shim function around
io_req_put_rsrc(). All calls to io_req_put_rsrc() require holding
->uring_lock, so we can just use it directly.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/io_uring.c | 5 ++---
 io_uring/rsrc.h     | 6 ------
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index b20ee6a0e32e..909842cb1436 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -1451,10 +1451,9 @@ static void io_free_batch_list(struct io_ring_ctx *ctx,
 				io_clean_op(req);
 		}
 		io_put_file(req);
-
-		io_req_put_rsrc_locked(req, ctx);
-
+		io_put_rsrc_node(ctx, req->rsrc_node);
 		io_put_task(req->task);
+
 		node = req->comp_list.next;
 		io_req_add_to_cache(req, ctx);
 	} while (node);
diff --git a/io_uring/rsrc.h b/io_uring/rsrc.h
index 83c079a707f8..c032ca3436ca 100644
--- a/io_uring/rsrc.h
+++ b/io_uring/rsrc.h
@@ -83,12 +83,6 @@ static inline void io_put_rsrc_node(struct io_ring_ctx *ctx, struct io_rsrc_node
 		io_rsrc_node_ref_zero(node);
 }
 
-static inline void io_req_put_rsrc_locked(struct io_kiocb *req,
-					  struct io_ring_ctx *ctx)
-{
-	io_put_rsrc_node(ctx, req->rsrc_node);
-}
-
 static inline void io_charge_rsrc_node(struct io_ring_ctx *ctx,
 				       struct io_rsrc_node *node)
 {
-- 
2.44.0

Re: [PATCH for-next 4/4] io_uring: remove io_req_put_rsrc_locked()

[Ming Lei]

On Fri, Apr 05, 2024 at 04:50:05PM +0100, Pavel Begunkov wrote:
> io_req_put_rsrc_locked() is a weird shim function around
> io_req_put_rsrc(). All calls to io_req_put_rsrc() require holding
> ->uring_lock, so we can just use it directly.
> 
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Thanks,
Ming

Re: [PATCH for-next 0/4] clean up io_req_complete_post

[Jens Axboe]

On Fri, 05 Apr 2024 16:50:01 +0100, Pavel Begunkov wrote:
> Patch 1 from Ming Lei removes a good chunk of unreachable code. Add a
> warning in patch 2, and apparently we can develop on the idea and remove
> even more dead code in patches 3,4.
> 
> Ming Lei (1):
>   io_uring: kill dead code in io_req_complete_post
> 
> [...]

Applied, thanks!

[1/4] io_uring: kill dead code in io_req_complete_post
      (no commit info)
[2/4] io_uring: turn implicit assumptions into a warning
      (no commit info)
[3/4] io_uring: remove async request cache
      (no commit info)
[4/4] io_uring: remove io_req_put_rsrc_locked()
      (no commit info)

Best regards,
-- 
Jens Axboe