Hi Oleg,
On 1/2/25 5:33 PM, Oleg Nesterov wrote:
I was going to send a one-liner patch which adds mb() into pipe_poll()
but then I decided to make even more spam and ask some questions first.
static void wakeup_pipe_readers(struct pipe_inode_info *pipe)
{
smp_mb();
if (waitqueue_active(&pipe->rd_wait))
wake_up_interruptible(&pipe->rd_wait);
kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
}
I think that wq_has_sleeper() + wake_up_interruptible_poll(POLLIN) make more
sense but this is minor.
Either way the waitqueue_active() check is only correct if the waiter has a
barrier between __add_wait_queue() and "check the condition". wait_event()
is fine, but pipe_poll() does:
// poll_wait()
__pollwait() -> add_wait_queue(pipe->rd_wait) -> list_add()
READ_ONCE(pipe->head);
READ_ONCE(pipe->tail);
In theory these LOAD's can leak into the critical section in add_wait_queue()
and they can happen before list_add(entry, rd_wait.head).
So I think we need the trivial
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -680,6 +680,7 @@ pipe_poll(struct file *filp, poll_table *wait)
* if something changes and you got it wrong, the poll
* table entry will wake you up and fix it.
*/
+ smp_mb();
head = READ_ONCE(pipe->head);
tail = READ_ONCE(pipe->tail);
and after that pipe_read/pipe_write can use the wq_has_sleeper() check too
(this is what the patch from WangYuli did).
Would it be possible to create a perf probe to get some statistics?
I see at least 4 options:
- do nothing
- add the smp_mb() into pipe_poll, and convert pipe to wq_has_sleepers()
- add the smp_mb() into poll_wait(), convert pipe and potentially
further poll users to wq_has_sleepers()
- add the smp_mb() into __add_wait_queue(), and merge wq_has_sleepers()
into wake_up().
The tricky part is probably to differentiate wake_up on empty wait
queues vs. wake_up on wait queues with entries.
--
Manfred
