* [PATCH for-next 0/4] clean up io_req_complete_post
@ 2024-04-05 15:50 Pavel Begunkov
2024-04-05 15:50 ` [PATCH for-next 1/4] io_uring: kill dead code in io_req_complete_post Pavel Begunkov
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: Pavel Begunkov @ 2024-04-05 15:50 UTC (permalink / raw)
To: io-uring; +Cc: Jens Axboe, asml.silence, Ming Lei
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH for-next 1/4] io_uring: kill dead code in io_req_complete_post
2024-04-05 15:50 [PATCH for-next 0/4] clean up io_req_complete_post Pavel Begunkov
@ 2024-04-05 15:50 ` Pavel Begunkov
2024-04-05 15:50 ` [PATCH for-next 2/4] io_uring: turn implicit assumptions into a warning Pavel Begunkov
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Pavel Begunkov @ 2024-04-05 15:50 UTC (permalink / raw)
To: io-uring; +Cc: Jens Axboe, asml.silence, Ming Lei
From: Ming Lei <[email protected]>
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 <[email protected]>
Signed-off-by: Ming Lei <[email protected]>
Reviewed-by: Pavel Begunkov <[email protected]>
Signed-by: Pavel Begunkov <[email protected]>
Signed-off-by: Pavel Begunkov <[email protected]>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH for-next 2/4] io_uring: turn implicit assumptions into a warning
2024-04-05 15:50 [PATCH for-next 0/4] clean up io_req_complete_post Pavel Begunkov
2024-04-05 15:50 ` [PATCH for-next 1/4] io_uring: kill dead code in io_req_complete_post Pavel Begunkov
@ 2024-04-05 15:50 ` Pavel Begunkov
2024-04-06 13:21 ` Ming Lei
2024-04-05 15:50 ` [PATCH for-next 3/4] io_uring: remove async request cache Pavel Begunkov
` (2 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Pavel Begunkov @ 2024-04-05 15:50 UTC (permalink / raw)
To: io-uring; +Cc: Jens Axboe, asml.silence, Ming Lei
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 <[email protected]>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH for-next 3/4] io_uring: remove async request cache
2024-04-05 15:50 [PATCH for-next 0/4] clean up io_req_complete_post Pavel Begunkov
2024-04-05 15:50 ` [PATCH for-next 1/4] io_uring: kill dead code in io_req_complete_post Pavel Begunkov
2024-04-05 15:50 ` [PATCH for-next 2/4] io_uring: turn implicit assumptions into a warning Pavel Begunkov
@ 2024-04-05 15:50 ` Pavel Begunkov
2024-04-06 13:26 ` Ming Lei
2024-04-05 15:50 ` [PATCH for-next 4/4] io_uring: remove io_req_put_rsrc_locked() Pavel Begunkov
2024-04-06 2:06 ` [PATCH for-next 0/4] clean up io_req_complete_post Jens Axboe
4 siblings, 1 reply; 9+ messages in thread
From: Pavel Begunkov @ 2024-04-05 15:50 UTC (permalink / raw)
To: io-uring; +Cc: Jens Axboe, asml.silence, Ming Lei
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 <[email protected]>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH for-next 4/4] io_uring: remove io_req_put_rsrc_locked()
2024-04-05 15:50 [PATCH for-next 0/4] clean up io_req_complete_post Pavel Begunkov
` (2 preceding siblings ...)
2024-04-05 15:50 ` [PATCH for-next 3/4] io_uring: remove async request cache Pavel Begunkov
@ 2024-04-05 15:50 ` Pavel Begunkov
2024-04-06 13:28 ` Ming Lei
2024-04-06 2:06 ` [PATCH for-next 0/4] clean up io_req_complete_post Jens Axboe
4 siblings, 1 reply; 9+ messages in thread
From: Pavel Begunkov @ 2024-04-05 15:50 UTC (permalink / raw)
To: io-uring; +Cc: Jens Axboe, asml.silence, Ming Lei
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 <[email protected]>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH for-next 0/4] clean up io_req_complete_post
2024-04-05 15:50 [PATCH for-next 0/4] clean up io_req_complete_post Pavel Begunkov
` (3 preceding siblings ...)
2024-04-05 15:50 ` [PATCH for-next 4/4] io_uring: remove io_req_put_rsrc_locked() Pavel Begunkov
@ 2024-04-06 2:06 ` Jens Axboe
4 siblings, 0 replies; 9+ messages in thread
From: Jens Axboe @ 2024-04-06 2:06 UTC (permalink / raw)
To: io-uring, Pavel Begunkov; +Cc: Ming Lei
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH for-next 2/4] io_uring: turn implicit assumptions into a warning
2024-04-05 15:50 ` [PATCH for-next 2/4] io_uring: turn implicit assumptions into a warning Pavel Begunkov
@ 2024-04-06 13:21 ` Ming Lei
0 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2024-04-06 13:21 UTC (permalink / raw)
To: Pavel Begunkov; +Cc: io-uring, Jens Axboe
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 <[email protected]>
Reviewed-by: Ming Lei <[email protected]>
Thanks,
Ming
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH for-next 3/4] io_uring: remove async request cache
2024-04-05 15:50 ` [PATCH for-next 3/4] io_uring: remove async request cache Pavel Begunkov
@ 2024-04-06 13:26 ` Ming Lei
0 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2024-04-06 13:26 UTC (permalink / raw)
To: Pavel Begunkov; +Cc: io-uring, Jens Axboe
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 <[email protected]>
Reviewed-by: Ming Lei <[email protected]>
Thanks,
Ming
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH for-next 4/4] io_uring: remove io_req_put_rsrc_locked()
2024-04-05 15:50 ` [PATCH for-next 4/4] io_uring: remove io_req_put_rsrc_locked() Pavel Begunkov
@ 2024-04-06 13:28 ` Ming Lei
0 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2024-04-06 13:28 UTC (permalink / raw)
To: Pavel Begunkov; +Cc: io-uring, Jens Axboe
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 <[email protected]>
Reviewed-by: Ming Lei <[email protected]>
Thanks,
Ming
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-04-06 13:29 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-05 15:50 [PATCH for-next 0/4] clean up io_req_complete_post Pavel Begunkov
2024-04-05 15:50 ` [PATCH for-next 1/4] io_uring: kill dead code in io_req_complete_post Pavel Begunkov
2024-04-05 15:50 ` [PATCH for-next 2/4] io_uring: turn implicit assumptions into a warning Pavel Begunkov
2024-04-06 13:21 ` Ming Lei
2024-04-05 15:50 ` [PATCH for-next 3/4] io_uring: remove async request cache Pavel Begunkov
2024-04-06 13:26 ` Ming Lei
2024-04-05 15:50 ` [PATCH for-next 4/4] io_uring: remove io_req_put_rsrc_locked() Pavel Begunkov
2024-04-06 13:28 ` Ming Lei
2024-04-06 2:06 ` [PATCH for-next 0/4] clean up io_req_complete_post Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox