[PATCH] io_uring: use TWA_SIGNAL for task_work related to eventfd

[PATCH] io_uring: use TWA_SIGNAL for task_work related to eventfd

[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.

Signed-off-by: Jens Axboe <axboe@kernel.dk>

---

diff --git a/fs/eventfd.c b/fs/eventfd.c
index df466ef81ddd..4eb7ae838f61 100644
--- a/fs/eventfd.c
+++ b/fs/eventfd.c
@@ -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
diff --git a/fs/io_uring.c b/fs/io_uring.c
index e9b27cdaa735..c76062be9c4c 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -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);
diff --git a/include/linux/eventfd.h b/include/linux/eventfd.h
index dc4fd8a6644d..2e5eeb3a813c 100644
--- a/include/linux/eventfd.h
+++ b/include/linux/eventfd.h
@@ -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 */

-- 
Jens Axboe

Re: [PATCH] io_uring: use TWA_SIGNAL for task_work related to eventfd

[Jann Horn]

On Fri, Aug 7, 2020 at 1:57 AM Jens Axboe <axboe@kernel.dk> wrote:
> 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.
[...]
> @@ -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?

Re: [PATCH] io_uring: use TWA_SIGNAL for task_work related to eventfd

[Jens Axboe]

On 8/6/20 7:00 PM, Jann Horn wrote:
> On Fri, Aug 7, 2020 at 1:57 AM Jens Axboe <axboe@kernel.dk> wrote:
>> 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.
> [...]
>> @@ -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).

-- 
Jens Axboe