io_uring: use TWA_SIGNAL for task_work related to eventfd

PATCH In review Jens Axboe 2 participants 3 messages Aug 6, 2020 – Aug 7, 2020

Subsystems (2)

VFS io_uring

lore

Share & tools

lore

Jens Axboe

author maintainer

2020-08-06 23:57 UTC #0 lore raw

PATCH 1/1 io_uring: use TWA_SIGNAL for task_work related to eventfd

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 that didn't cover the general case of waiting on eventfd and having that dependency between io_uring and eventfd.

Ensure that we use signaled notification for anything related to eventfd.

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

---

fs/eventfd.c +1 -1

index df466ef81ddd..4eb7ae838f61 100644
@@ -321,7 +321,7 @@ static void eventfd_show_fdinfo(struct seq_file *m, struct file *f)
 }
 #endif
 
-static const struct file_operations eventfd_fops = {
+const struct file_operations eventfd_fops = {
 #ifdef CONFIG_PROC_FS
 	.show_fdinfo	= eventfd_show_fdinfo,
 #endif

fs/io_uring.c +1 -1

index e9b27cdaa735..c76062be9c4c 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 || (req->file && eventfd_file(req->file)))
 		notify = TWA_SIGNAL;
 
 	ret = task_work_add(tsk, cb, notify);

include/linux/eventfd.h +13 -0

index dc4fd8a6644d..2e5eeb3a813c 100644
@@ -14,6 +14,7 @@
 #include <linux/err.h>
 #include <linux/percpu-defs.h>
 #include <linux/percpu.h>
+#include <linux/fs.h>
 
 /*
  * CAREFUL: Check include/uapi/asm-generic/fcntl.h when defining

@@ -49,6 +50,13 @@ static inline bool eventfd_signal_count(void)
 	return this_cpu_read(eventfd_wake_count);
 }
 
+extern const struct file_operations eventfd_fops;
+
+static inline bool evenfd_file(struct file *file)
+{
+	return file->f_op == &eventfd_fops;
+}
+
 #else /* CONFIG_EVENTFD */
 
 /*

@@ -82,6 +90,11 @@ static inline bool eventfd_signal_count(void)
 	return false;
 }
 
+static inline bool eventfd_file(struct file *file)
+{
+	return false;
+}
+
 #endif
 
 #endif /* _LINUX_EVENTFD_H */

Signature · 1 line

Jens Axboe

Jann Horn

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

Quoted 10 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 that didn't cover the general case of waiting on eventfd and having that dependency between io_uring and eventfd.
Ensure that we use signaled notification for anything related to eventfd.

[...]

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

Quoted 7 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 || (req->file && eventfd_file(req->file)))
                notify = TWA_SIGNAL;

Is the idea here that you want "polling an eventfd" to have different UAPI semantics compared to e.g. "polling a pipe"? Or is there something in-kernel that makes eventfds special?

Jens Axboe

author maintainer

2020-08-07 01:10 UTC #2 lore raw

Quoted 12 lines · Jann Horn

Quoted 10 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 that didn't cover the general case of waiting on eventfd and having that dependency between io_uring and eventfd.
Ensure that we use signaled notification for anything related to eventfd.
[...]

Quoted 11 lines

fs/io_uring.c static int io_req_task_work_add(struct io_kiocb *req, struct callback_head *cb) · line 1724
Quoted 7 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 || (req->file && eventfd_file(req->file)))
                notify = TWA_SIGNAL;
Is the idea here that you want "polling an eventfd" to have different UAPI semantics compared to e.g. "polling a pipe"? Or is there something in-kernel that makes eventfds special?

I looked more at this after sending it out, and I actually think we want the logic to be something ala:

else if (ctx->cq_ev_fd || (tsk->state != TASK_RUNNING))

        notify = TWA_SIGNAL;

instead, or something like that. Basically if the task is currently waiting in the kernel, then the wakeup logic isn't enough, while TWA_SIGNAL will do the right thing.

Looking into if we have a better way to tell if that's the case outside of checking the targeted task state (and even if TASK_RUNNING is the right one, vs checking if it's TASK_INTERRUPTIBLE/TASK_UNINTERRUPTIBLE specifically).

Signature · 1 line

Jens Axboe