io_uring: use TWA_SIGNAL for task_work if the task isn't running

PATCH In review Jens Axboe 2 participants 4 messages Aug 7, 2020

Subsystems (2)

VFS io_uring

lore

Share & tools

lore

Jens Axboe

author maintainer

2020-08-07 16:56 UTC #0 lore raw

PATCH v2 1/1 io_uring: use TWA_SIGNAL for task_work if the task isn't running

An earlier commit:

b7db41c9e03b ("io_uring: fix regression with always ignoring signals in io_cqring_wait()")

ensured that we didn't get stuck waiting for eventfd reads when it's registered with the io_uring ring for event notification, but we still have a gap where the task can be waiting on other events in the kernel and need a bigger nudge to make forward progress.

Ensure that we use signaled notifications for a task that isn't currently running, to be certain the work is seen and processed immediately.

Cc [email protected] # v5.7+

Reported-by Josef <[email protected]>

Signed-off-by Jens Axboe <[email protected]>

---

This isn't perfect, as it'll use TWA_SIGNAL even for cases where we don't absolutely need it (like task waiting for completions in io_cqring_wait()), but we don't have a good way to tell right now. We can probably improve on this in the future, for now I think this is the best solution.

fs/io_uring.c +1 -1

index e9b27cdaa735..b4300a61f231 100644
@@ -1720,7 +1720,7 @@ static int io_req_task_work_add(struct io_kiocb *req, struct callback_head *cb)
 	 */
 	if (ctx->flags & IORING_SETUP_SQPOLL)
 		notify = 0;
-	else if (ctx->cq_ev_fd)
+	else if (ctx->cq_ev_fd || (tsk->state != TASK_RUNNING))
 		notify = TWA_SIGNAL;
 
 	ret = task_work_add(tsk, cb, notify);

Signature · 1 line

Jens Axboe

Jann Horn

2020-08-07 18:01 UTC #1 lore raw

Quoted 30 lines · Jens Axboe

An earlier commit:
b7db41c9e03b ("io_uring: fix regression with always ignoring signals in io_cqring_wait()")
ensured that we didn't get stuck waiting for eventfd reads when it's registered with the io_uring ring for event notification, but we still have a gap where the task can be waiting on other events in the kernel and need a bigger nudge to make forward progress.
Ensure that we use signaled notifications for a task that isn't currently running, to be certain the work is seen and processed immediately.
Cc [email protected] # v5.7+
Reported-by Josef <[email protected]>
Signed-off-by Jens Axboe <[email protected]>
---
This isn't perfect, as it'll use TWA_SIGNAL even for cases where we don't absolutely need it (like task waiting for completions in io_cqring_wait()), but we don't have a good way to tell right now. We can probably improve on this in the future, for now I think this is the best solution.
fs/io_uring.c +0 -0
index e9b27cdaa735..b4300a61f231 100644

fs/io_uring.c static int io_req_task_work_add(struct io_kiocb *req, struct callback_head *cb) · line 1726

Quoted 8 lines

fs/io_uring.c +1 -1

@@ -1720,7 +1720,7 @@ static int io_req_task_work_add(struct io_kiocb *req, struct callback_head *cb)
         */
        if (ctx->flags & IORING_SETUP_SQPOLL)
                notify = 0;
-       else if (ctx->cq_ev_fd)
+       else if (ctx->cq_ev_fd || (tsk->state != TASK_RUNNING))
                notify = TWA_SIGNAL;
 
        ret = task_work_add(tsk, cb, notify);

I don't get it. Apart from still not understanding the big picture:

What guarantees that the lockless read of tsk->state is in any way related to the state of the remote process by the time we reach task_work_add()? And why do we not need to signal in TASK_RUNNING state (e.g. directly before the remote process switches to TASK_INTERRUPTIBLE or something like that)?

Even if this is correct, it would still be nice if you could add a big comment that explains the precise semantics this is attempting to provide. As far as I understand so far, the goal is to trigger -EINTR returns from certain syscalls, or something like that? But I don't understand whether that's indeed what's going on, or which syscalls precisely this is attempting to make return -EINTR.

(Also, lockless reads of concurrently changing variables should be written with READ_ONCE().)

Jens Axboe

author maintainer

2020-08-07 21:50 UTC #2 lore raw

Quoted 42 lines · Jann Horn

Quoted 30 lines · Jens Axboe
An earlier commit:
b7db41c9e03b ("io_uring: fix regression with always ignoring signals in io_cqring_wait()")
ensured that we didn't get stuck waiting for eventfd reads when it's registered with the io_uring ring for event notification, but we still have a gap where the task can be waiting on other events in the kernel and need a bigger nudge to make forward progress.
Ensure that we use signaled notifications for a task that isn't currently running, to be certain the work is seen and processed immediately.
Cc [email protected] # v5.7+
Reported-by Josef <[email protected]>
Signed-off-by Jens Axboe <[email protected]>
---
This isn't perfect, as it'll use TWA_SIGNAL even for cases where we don't absolutely need it (like task waiting for completions in io_cqring_wait()), but we don't have a good way to tell right now. We can probably improve on this in the future, for now I think this is the best solution.
fs/io_uring.c +0 -0
index e9b27cdaa735..b4300a61f231 100644
fs/io_uring.c static int io_req_task_work_add(struct io_kiocb *req, struct callback_head *cb) · line 1726
Quoted 8 lines
fs/io_uring.c +1 -1
@@ -1720,7 +1720,7 @@ static int io_req_task_work_add(struct io_kiocb *req, struct callback_head *cb)
         */
        if (ctx->flags & IORING_SETUP_SQPOLL)
                notify = 0;
-       else if (ctx->cq_ev_fd)
+       else if (ctx->cq_ev_fd || (tsk->state != TASK_RUNNING))
                notify = TWA_SIGNAL;
 
        ret = task_work_add(tsk, cb, notify);
I don't get it. Apart from still not understanding the big picture:

Quoted 5 lines

What guarantees that the lockless read of tsk->state is in any way related to the state of the remote process by the time we reach task_work_add()? And why do we not need to signal in TASK_RUNNING state (e.g. directly before the remote process switches to TASK_INTERRUPTIBLE or something like that)?

Yeah it doesn't, the patch doesn't cover the racy case. As far as I can tell, we've got two ways to do it:

1) We split the task_work_add() into two parts, one adding the work and one doing the signaling. Then we could do:

int notify = TWA_RESUME;

__task_work_add(tsk, cb);

if (ctx->flags & IORING_SETUP_SQPOLL)

	notify = 0;
else if (ctx->cq_ev_fd || (tsk->state != TASK_RUNNING))
	notify = TWA_SIGNAL;

__task_work_signal(tsk, notify);

2) We imply that behavior in task_work_add() itself, if TWA_SIGNAL is used, making TWA_SIGNAL imply "use signal wakeup IFF task is not running". Or add a TWA_SIGNAL_NOT_RUNNING for that behavior.

I kind of like the first approach.

Quoted 6 lines

Even if this is correct, it would still be nice if you could add a big comment that explains the precise semantics this is attempting to provide. As far as I understand so far, the goal is to trigger -EINTR returns from certain syscalls, or something like that? But I don't understand whether that's indeed what's going on, or which syscalls precisely this is attempting to make return -EINTR.

The point is if the original task is currently looping (or just waiting) in the kernel on another event, it still gets a chance to process the task work. The completion it's waiting for may be dependent on getting that task work run.

The test case for this one is kicking off a thread that waits on the completion event, while the main task is waiting for the thread to exit.

Agree it needs a comment, I'll add one.

Quoted 2 lines

(Also, lockless reads of concurrently changing variables should be written with READ_ONCE().)

Good point.

Signature · 1 line

Jens Axboe

Jens Axboe

author maintainer

2020-08-07 22:11 UTC #3 lore raw

Quoted 57 lines · Jens Axboe

Quoted 42 lines · Jann Horn
Quoted 30 lines · Jens Axboe
An earlier commit:
b7db41c9e03b ("io_uring: fix regression with always ignoring signals in io_cqring_wait()")
ensured that we didn't get stuck waiting for eventfd reads when it's registered with the io_uring ring for event notification, but we still have a gap where the task can be waiting on other events in the kernel and need a bigger nudge to make forward progress.
Ensure that we use signaled notifications for a task that isn't currently running, to be certain the work is seen and processed immediately.
Cc [email protected] # v5.7+
Reported-by Josef <[email protected]>
Signed-off-by Jens Axboe <[email protected]>
---
This isn't perfect, as it'll use TWA_SIGNAL even for cases where we don't absolutely need it (like task waiting for completions in io_cqring_wait()), but we don't have a good way to tell right now. We can probably improve on this in the future, for now I think this is the best solution.
fs/io_uring.c +0 -0
index e9b27cdaa735..b4300a61f231 100644
fs/io_uring.c static int io_req_task_work_add(struct io_kiocb *req, struct callback_head *cb) · line 1726
Quoted 8 lines
fs/io_uring.c +1 -1
@@ -1720,7 +1720,7 @@ static int io_req_task_work_add(struct io_kiocb *req, struct callback_head *cb)
         */
        if (ctx->flags & IORING_SETUP_SQPOLL)
                notify = 0;
-       else if (ctx->cq_ev_fd)
+       else if (ctx->cq_ev_fd || (tsk->state != TASK_RUNNING))
                notify = TWA_SIGNAL;
 
        ret = task_work_add(tsk, cb, notify);
I don't get it. Apart from still not understanding the big picture:
Quoted 5 lines
What guarantees that the lockless read of tsk->state is in any way related to the state of the remote process by the time we reach task_work_add()? And why do we not need to signal in TASK_RUNNING state (e.g. directly before the remote process switches to TASK_INTERRUPTIBLE or something like that)?
Yeah it doesn't, the patch doesn't cover the racy case. As far as I can tell, we've got two ways to do it:
1) We split the task_work_add() into two parts, one adding the work and one doing the signaling. Then we could do:
int notify = TWA_RESUME;

Quoted 8 lines

__task_work_add(tsk, cb);
if (ctx->flags & IORING_SETUP_SQPOLL)
	notify = 0;
else if (ctx->cq_ev_fd || (tsk->state != TASK_RUNNING))
	notify = TWA_SIGNAL;
__task_work_signal(tsk, notify);

Something like the attached - totally untested so far, but it implements that idea.

Signature · 1 line

Jens Axboe

From ec67af3b1e0c9325a04d5d1c12311086349d3a79 Mon Sep 17 00:00:00 2001
From: Jens Axboe <[email protected]>
Date: Thu, 6 Aug 2020 19:41:50 -0600
Subject: [PATCH 2/2] io_uring: use TWA_SIGNAL for task_work if the task isn't
running

An earlier commit:

b7db41c9e03b ("io_uring: fix regression with always ignoring signals in io_cqring_wait()")

Ensure that we use signaled notifications for a task that isn't currently running, to be certain the work is seen and processed immediately.

Cc [email protected] # v5.7+

Reported-by Josef <[email protected]>

Signed-off-by Jens Axboe <[email protected]>

---
 fs/io_uring.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

fs/io_uring.c +14 -8

index e9b27cdaa735..443eecdfeda9 100644
@@ -1712,21 +1712,27 @@ static int io_req_task_work_add(struct io_kiocb *req, struct callback_head *cb)
 	struct io_ring_ctx *ctx = req->ctx;
 	int ret, notify = TWA_RESUME;
 
+	ret = __task_work_add(tsk, cb);
+	if (unlikely(ret))
+		return ret;
+
 	/*
 	 * SQPOLL kernel thread doesn't need notification, just a wakeup.
-	 * If we're not using an eventfd, then TWA_RESUME is always fine,
-	 * as we won't have dependencies between request completions for
-	 * other kernel wait conditions.
+	 * For any other work, use signaled wakeups if the task isn't
+	 * running to avoid dependencies between tasks or threads. If
+	 * the issuing task is currently waiting in the kernel on a thread,
+	 * and same thread is waiting for a completion event, then we need
+	 * to ensure that the issuing task processes task_work. TWA_SIGNAL
+	 * is needed for that.
 	 */
 	if (ctx->flags & IORING_SETUP_SQPOLL)
 		notify = 0;
-	else if (ctx->cq_ev_fd)
+	else if (READ_ONCE(tsk->state) != TASK_RUNNING)
 		notify = TWA_SIGNAL;
 
-	ret = task_work_add(tsk, cb, notify);
-	if (!ret)
-		wake_up_process(tsk);
-	return ret;
+	__task_work_notify(tsk, notify);
+	wake_up_process(tsk);
+	return 0;
 }
 
 static void __io_req_task_cancel(struct io_kiocb *req, int error)

Signature · 1 line

2.28.0

From 802b09d10bdd2f90e510049b1c52b81edc2ae0a3 Mon Sep 17 00:00:00 2001
From: Jens Axboe <[email protected]>
Date: Fri, 7 Aug 2020 16:00:53 -0600
Subject: [PATCH 1/2] kernel: split task_work_add() into two separate helpers

Some callers may need to make signaling decisions based on the state of the targeted task, and that can only safely be done post adding the task_work to the task. Split task_work_add() into:

__task_work_add() - adds the work item
__task_work_notify() - sends the notification

No functional changes in this patch.

Signed-off-by Jens Axboe <[email protected]>

---
 include/linux/task_work.h | 19 ++++++++++++++++
 kernel/task_work.c        | 48 +++++++++++++++++++++------------------
 2 files changed, 45 insertions(+), 22 deletions(-)

include/linux/task_work.h +19 -0

index 0fb93aafa478..7abbd8df5e13 100644
@@ -5,6 +5,8 @@
 #include <linux/list.h>
 #include <linux/sched.h>
 
+extern struct callback_head work_exited;
+
 typedef void (*task_work_func_t)(struct callback_head *);
 
 static inline void

@@ -13,6 +15,21 @@ init_task_work(struct callback_head *twork, task_work_func_t func)
 	twork->func = func;
 }
 
+static inline int __task_work_add(struct task_struct *task,
+				  struct callback_head *work)
+{
+	struct callback_head *head;
+
+	do {
+		head = READ_ONCE(task->task_works);
+		if (unlikely(head == &work_exited))
+			return -ESRCH;
+		work->next = head;
+	} while (cmpxchg(&task->task_works, head, work) != head);
+
+	return 0;
+}
+
 #define TWA_RESUME	1
 #define TWA_SIGNAL	2
 int task_work_add(struct task_struct *task, struct callback_head *twork, int);

@@ -20,6 +37,8 @@ int task_work_add(struct task_struct *task, struct callback_head *twork, int);
 struct callback_head *task_work_cancel(struct task_struct *, task_work_func_t);
 void task_work_run(void);
 
+void __task_work_notify(struct task_struct *task, int notify);
+
 static inline void exit_task_work(struct task_struct *task)
 {
 	task_work_run();

kernel/task_work.c +26 ~~-22~~

62 lines not on this page · raw message

Signature · 1 line

2.28.0