Skip to contentSkip to conversation

sqpoll cancellation

PATCH Applied Pavel Begunkov 2 participants 4 messages Apr 25, 2021 – Apr 26, 2021

Subsystems (3)
Stable branch VFS io_uring
lore
Share & tools
lore

Pavel Begunkov

author
#0 lore raw

cover sqpoll cancellation

1/2 is yet another SQPOLL cancellation fix (not critical), 2/2 cleans up after it and sqpoll recent fixes

p.s. can't locate without long bisecting which commit 1/2 fixes

Pavel Begunkov (2):
io_uring: fix work_exit sqpoll cancellations
io_uring: simplify SQPOLL cancellations

 fs/io_uring.c | 70 ++++++++++++++++-----------------------------------
 1 file changed, 21 insertions(+), 49 deletions(-)
Signature · 1 line
2.31.1
author
#1 lore raw

PATCH 1/2 io_uring: fix work_exit sqpoll cancellations

After closing an SQPOLL ring, io_ring_exit_work() kicks in and starts doing cancellations via io_uring_try_cancel_requests(). It will go through io_uring_try_cancel_iowq(), which uses ctx->tctx_list, but as SQPOLL task don't have a ctx note, its io-wq won't be reachable and so is left not cancelled.

It will eventually cancelled when one of the tasks dies, but if a thread group survives for long and changes rings, it will spawn lots of unreclaimed resources and live locked works.

Cancel SQPOLL task's io-wq separately in io_ring_exit_work().

Cc [email protected]
Signed-off-by Pavel Begunkov <[email protected]>
---
 fs/io_uring.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)
fs/io_uring.c +18 -7
index eef373394ee9..640a4bc77aa6 100644
@@ -8678,6 +8678,13 @@ static void io_tctx_exit_cb(struct callback_head *cb)
 	complete(&work->completion);
 }
 
+static bool io_cancel_ctx_cb(struct io_wq_work *work, void *data)
+{
+	struct io_kiocb *req = container_of(work, struct io_kiocb, work);
+
+	return req->ctx == data;
+}
+
 static void io_ring_exit_work(struct work_struct *work)
 {
 	struct io_ring_ctx *ctx = container_of(work, struct io_ring_ctx, exit_work);

@@ -8694,6 +8701,17 @@ static void io_ring_exit_work(struct work_struct *work)
 	 */
 	do {
 		io_uring_try_cancel_requests(ctx, NULL, NULL);
+		if (ctx->sq_data) {
+			struct io_sq_data *sqd = ctx->sq_data;
+			struct task_struct *tsk;
+
+			io_sq_thread_park(sqd);
+			tsk = sqd->thread;
+			if (tsk && tsk->io_uring && tsk->io_uring->io_wq)
+				io_wq_cancel_cb(tsk->io_uring->io_wq,
+						io_cancel_ctx_cb, ctx, true);
+			io_sq_thread_unpark(sqd);
+		}
 
 		WARN_ON_ONCE(time_after(jiffies, timeout));
 	} while (!wait_for_completion_timeout(&ctx->ref_comp, HZ/20));

@@ -8843,13 +8861,6 @@ static bool io_cancel_defer_files(struct io_ring_ctx *ctx,
 	return true;
 }
 
-static bool io_cancel_ctx_cb(struct io_wq_work *work, void *data)
-{
-	struct io_kiocb *req = container_of(work, struct io_kiocb, work);
-
-	return req->ctx == data;
-}
-
 static bool io_uring_try_cancel_iowq(struct io_ring_ctx *ctx)
 {
 	struct io_tctx_node *node;
Signature · 1 line
2.31.1
author
#2 lore raw

PATCH 2/2 io_uring: simplify SQPOLL cancellations

In reply to Pavel Begunkov

All sqpoll rings (even sharing sqpoll task) are currently dead bound to the task that created them, iow when owner task dies it kills all its SQPOLL rings and their inflight requests via task_work infra. It's neither the nicist way nor the most convenient as adds extra locking/waiting and dependencies.

Leave it alone and rely on SIGKILL being delivered on its thread group exit, so there are only two cases left:

1) thread group is dying, so sqpoll task gets a signal and exit itself cancelling all requests.

2) an sqpoll ring is dying. Because refs_kill() is called the sqpoll not going to submit any new request, and that's what we need. And io_ring_exit_work() will do all the cancellation itself before actually killing ctx, so sqpoll doesn't need to worry about it.

Signed-off-by Pavel Begunkov <[email protected]>
---
 fs/io_uring.c | 45 +++------------------------------------------
 1 file changed, 3 insertions(+), 42 deletions(-)
fs/io_uring.c +3 -42
index 640a4bc77aa6..1cdc9b7c5c8c 100644
@@ -9020,41 +9020,6 @@ static s64 tctx_inflight(struct io_uring_task *tctx, bool tracked)
 	return percpu_counter_sum(&tctx->inflight);
 }
 
-static void io_sqpoll_cancel_cb(struct callback_head *cb)
-{
-	struct io_tctx_exit *work = container_of(cb, struct io_tctx_exit, task_work);
-	struct io_sq_data *sqd = work->ctx->sq_data;
-
-	if (sqd->thread)
-		io_uring_cancel_sqpoll(sqd);
-	list_del_init(&work->ctx->sqd_list);
-	io_sqd_update_thread_idle(sqd);
-	complete(&work->completion);
-}
-
-static void io_sqpoll_cancel_sync(struct io_ring_ctx *ctx)
-{
-	struct io_sq_data *sqd = ctx->sq_data;
-	struct io_tctx_exit work = { .ctx = ctx, };
-	struct task_struct *task;
-
-	io_sq_thread_park(sqd);
-	task = sqd->thread;
-	if (task) {
-		init_completion(&work.completion);
-		init_task_work(&work.task_work, io_sqpoll_cancel_cb);
-		io_task_work_add_head(&sqd->park_task_work, &work.task_work);
-		wake_up_process(task);
-	} else {
-		list_del_init(&ctx->sqd_list);
-		io_sqd_update_thread_idle(sqd);
-	}
-	io_sq_thread_unpark(sqd);
-
-	if (task)
-		wait_for_completion(&work.completion);
-}
-
 static void io_uring_try_cancel(struct files_struct *files)
 {
 	struct io_uring_task *tctx = current->io_uring;

@@ -9064,11 +9029,9 @@ static void io_uring_try_cancel(struct files_struct *files)
 	xa_for_each(&tctx->xa, index, node) {
 		struct io_ring_ctx *ctx = node->ctx;
 
-		if (ctx->sq_data) {
-			io_sqpoll_cancel_sync(ctx);
-			continue;
-		}
-		io_uring_try_cancel_requests(ctx, current, files);
+		/* sqpoll task will cancel all its requests */
+		if (!ctx->sq_data)
+			io_uring_try_cancel_requests(ctx, current, files);
 	}
 }
 

@@ -9116,8 +9079,6 @@ void __io_uring_cancel(struct files_struct *files)
 
 	/* make sure overflow events are dropped */
 	atomic_inc(&tctx->in_idle);
-	io_uring_try_cancel(files);
-
 	do {
 		/* read completions before cancelations */
 		inflight = tctx_inflight(tctx, !!files);
Signature · 1 line
2.31.1
maintainer Applied
#3 lore raw
In reply to Pavel Begunkov
Quoted 2 lines · Pavel Begunkov

1/2 is yet another SQPOLL cancellation fix (not critical), 2/2 cleans up after it and sqpoll recent fixes

Applied, thanks.

Signature · 1 line
Jens Axboe