Re: [PATCH 1/3] pidfd_poll: report POLLHUP when pid_task() == NULL
From: Christian Brauner <brauner@kernel.org>
Date: 2024-02-02 17:24:30
Also in:
lkml
Subsystem:
io_uring, scheduler, the rest · Maintainers:
Jens Axboe, Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot, Linus Torvalds
I think we need a simpler patch. I was going to send it as 4/4, but I'd like to think more, _perhaps_ we can also discriminate the PIDFD_THREAD and non-PIDFD_THREAD waiters. I'll try to make the patch(es) tomorrow or
Right, I didn't go that far.
at least provided more info. 3 notes for now: 1. we can't use wake_up_poll(), it passes nr_exclusive => 1
Bah. So we need the same stuff we did for io_uring and use __wake_up() directly. Or we add wake_up_all_poll() and convert the other three callsites: // uncompiled, untested
diff --git a/include/linux/wait.h b/include/linux/wait.h
index 8aa3372f21a0..210ee0d69b6f 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h@@ -234,6 +234,8 @@ void __wake_up_pollfree(struct wait_queue_head *wq_head); #define key_to_poll(m) ((__force __poll_t)(uintptr_t)(void *)(m)) #define wake_up_poll(x, m) \ __wake_up(x, TASK_NORMAL, 1, poll_to_key(m)) +#define wake_up_all_poll(x, m) \ + __wake_up(x, TASK_NORMAL, 0, poll_to_key(m)) #define wake_up_poll_on_current_cpu(x, m) \ __wake_up_on_current_cpu(x, TASK_NORMAL, poll_to_key(m)) #define wake_up_locked_poll(x, m) \
diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
index 04e33f25919c..65dcd5dc9645 100644
--- a/io_uring/io_uring.h
+++ b/io_uring/io_uring.h@@ -228,8 +228,7 @@ static inline void io_commit_cqring(struct io_ring_ctx *ctx) static inline void io_poll_wq_wake(struct io_ring_ctx *ctx) { if (wq_has_sleeper(&ctx->poll_wq)) - __wake_up(&ctx->poll_wq, TASK_NORMAL, 0, - poll_to_key(EPOLL_URING_WAKE | EPOLLIN)); + wake_up_all_poll(&ctx->poll_wq, EPOLL_URING_WAKE | EPOLLIN); } static inline void io_cqring_wake(struct io_ring_ctx *ctx)
@@ -245,8 +244,7 @@ static inline void io_cqring_wake(struct io_ring_ctx *ctx) * epoll and should terminate multishot poll at that point. */ if (wq_has_sleeper(&ctx->cq_wait)) - __wake_up(&ctx->cq_wait, TASK_NORMAL, 0, - poll_to_key(EPOLL_URING_WAKE | EPOLLIN)); + wake_up_all_poll(&ctx->cq_wait, EPOLL_URING_WAKE | EPOLLIN); } static inline bool io_sqring_full(struct io_ring_ctx *ctx)
diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
index 51e38f5f4701..ee849fb35603 100644
--- a/kernel/sched/wait.c
+++ b/kernel/sched/wait.c@@ -208,7 +208,7 @@ EXPORT_SYMBOL_GPL(__wake_up_sync); /* For internaluse only */
void __wake_up_pollfree(struct wait_queue_head *wq_head)
{
- __wake_up(wq_head, TASK_NORMAL, 0, poll_to_key(EPOLLHUP |
POLLFREE));
+ wake_up_all_poll(wq_head, EPOLLHUP | POLLFREE);
/* POLLFREE must have cleared the queue. */
WARN_ON_ONCE(waitqueue_active(wq_head));
}
2. exit_notify() should not pass EPOLLHUP to wake_up, we do
not want to wake up the { .events = POLLHUP } waiters.Indeed.
3. we do not need to change __change_pid(). Well, _perhaps_ it can/should use __wake_up_pollfree(), but I need to check if fs/select.c use "autoremove" or not.quoted
-static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts) +static __poll_t pidfd_poll(struct file *file, poll_table *wait) { struct pid *pid = file->private_data; bool thread = file->f_flags & PIDFD_THREAD; struct task_struct *task; __poll_t poll_flags = 0; - poll_wait(file, &pid->wait_pidfd, pts); + poll_wait(file, &pid->wait_pidfd, wait);This is correct but only cosemtic and has nothing to do with what we discuss?
No, I just folded all of the changes because it was just a draft. See the updated draft I appended.
Attachments
- 0001-UNTESTED-UNCOMPILED.patch [text/x-diff] 5210 bytes · preview