* [PATCH] io_uring: use TWA_SIGNAL for task_work related to eventfd
@ 2020-08-06 23:57 Jens Axboe
2020-08-07 1:00 ` Jann Horn
0 siblings, 1 reply; 3+ messages in thread
From: Jens Axboe @ 2020-08-06 23:57 UTC (permalink / raw)
To: io-uring
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]>
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] io_uring: use TWA_SIGNAL for task_work related to eventfd
2020-08-06 23:57 [PATCH] io_uring: use TWA_SIGNAL for task_work related to eventfd Jens Axboe
@ 2020-08-07 1:00 ` Jann Horn
2020-08-07 1:10 ` Jens Axboe
0 siblings, 1 reply; 3+ messages in thread
From: Jann Horn @ 2020-08-07 1:00 UTC (permalink / raw)
To: Jens Axboe; +Cc: io-uring
On Fri, Aug 7, 2020 at 1:57 AM Jens Axboe <[email protected]> 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?
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] io_uring: use TWA_SIGNAL for task_work related to eventfd
2020-08-07 1:00 ` Jann Horn
@ 2020-08-07 1:10 ` Jens Axboe
0 siblings, 0 replies; 3+ messages in thread
From: Jens Axboe @ 2020-08-07 1:10 UTC (permalink / raw)
To: Jann Horn; +Cc: io-uring
On 8/6/20 7:00 PM, Jann Horn wrote:
> On Fri, Aug 7, 2020 at 1:57 AM Jens Axboe <[email protected]> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-08-07 1:10 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-08-06 23:57 [PATCH] io_uring: use TWA_SIGNAL for task_work related to eventfd Jens Axboe
2020-08-07 1:00 ` Jann Horn
2020-08-07 1:10 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox