Re: [PATCH 1/1] pipe: only enable the extra wake_up(rd_wait) when epoll is actually used

[Mateusz Guzik]

On Thu, Jul 23, 2026 at 4:52 PM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
>
> pipe_poll() unconditionally sets poll_usage on the first call, forcing
> anon_pipe_write() to wake up readers on every write even if the pipe was
> not empty. But this is only needed for epoll's "nasty semantics"; poll()
> and select() users pay for it for no reason.
>
> Rename it to epoll_usage, and only set it when the caller is actually
> using epoll on the read side of the pipe.
>

I am worried in that the poll_usage thing showed up in 2021 in commit
3b844826b6c6affa ("pipe: avoid unnecessary EPOLLET wakeups under
normal loads"), fixing up "missing" wakeups after some rework. Even
then it went ahead and as you noted it also did it for poll/select
users. So by Hyrum's law there is plenty of potential for someone to
depend on it.

However, I think this makes sense to try out and there is history of
committing possibly breaking changes, so I think the idea is fine to
go in.

I does rub me the wrong way that epoll lands in the same handler
though, but that's not something I'm going to do anything about.

The routine pretends to operate in a lockless manner, possibly
returning only a partial result. But it guarantees not blocking when
it should not by queueing up the caller unconditionally, which later
has to be undone. Have you considered patching that up? (as in, do the
work. if there are no events, queue up and do the work again -- should
reduce work if there was stuff already there)

> Signed-off-by: Oleg Nesterov <oleg@xxxxxxxxxx>
> ---
>  fs/pipe.c                 | 23 +++++++++++++++++++----
>  include/linux/pipe_fs_i.h |  6 ++++--
>  2 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 32140cb00d7e..b45b4b8d0b0b 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -357,6 +357,23 @@ static inline unsigned int pipe_update_tail(struct pipe_inode_info *pipe,
>         return tail;
>  }
>
> +static void pipe_set_epoll_usage(struct file *filp, struct pipe_inode_info *pipe)
> +{
> +#ifdef CONFIG_EPOLL
> +       if ((filp->f_mode & FMODE_READ) && filp->f_ep &&
> +           unlikely(!READ_ONCE(pipe->epoll_usage)))
> +               WRITE_ONCE(pipe->epoll_usage, true);
> +#endif
> +}
> +
> +static bool pipe_get_epoll_usage(struct pipe_inode_info *pipe)
> +{
> +#ifdef CONFIG_EPOLL
> +       return pipe->epoll_usage;
> +#endif

If patching this up I think a comment explaining what's going on would be nice.

I think you could steal parts of
3a34b13a88caeb2800ab44a4918f230041b37dd9 for that purpose.

> +       return false;
> +}
> +
>  static ssize_t
>  anon_pipe_read(struct kiocb *iocb, struct iov_iter *to)
>  {
> @@ -689,7 +706,7 @@ anon_pipe_write(struct kiocb *iocb, struct iov_iter *from)
>          * Epoll nonsensically wants a wakeup whether the pipe
>          * was already empty or not.
>          */
> -       if (was_empty || pipe->poll_usage)
> +       if (was_empty || pipe_get_epoll_usage(pipe))
>                 wake_up_interruptible_sync_poll(&pipe->rd_wait, EPOLLIN | EPOLLRDNORM);
>         kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
>         if (wake_next_writer)
> @@ -761,9 +778,7 @@ pipe_poll(struct file *filp, poll_table *wait)
>         union pipe_index idx;
>
>         /* Epoll has some historical nasty semantics, this enables them */
> -       if (unlikely(!READ_ONCE(pipe->poll_usage)))
> -               WRITE_ONCE(pipe->poll_usage, true);
> -
> +       pipe_set_epoll_usage(filp, pipe);
>         /*
>          * Reading pipe state only -- no need for acquiring the semaphore.
>          *

nit: I would add a new line between this and the following comment