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

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

[Oleg Nesterov]

Breno, Christian, Mateusz,

To be honest, I am not sure this change makes a lot of sense.
But somehow the extra wakeup in anon_pipe_write() looks annoying to me.

Can you take a look?

Plus I'd like to know what sashiko.dev thinks.

I have no idea how to test it wrt performance...

Oleg.
---

 fs/pipe.c                 | 23 +++++++++++++++++++----
 include/linux/pipe_fs_i.h |  6 ++++--
 2 files changed, 23 insertions(+), 6 deletions(-)

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

[Oleg Nesterov]

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.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 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
+	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.
 	 *
diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
index 7f6a92ac9704..d06da9bad3a8 100644
--- a/include/linux/pipe_fs_i.h
+++ b/include/linux/pipe_fs_i.h
@@ -74,7 +74,7 @@ union pipe_index {
  *	@files: number of struct file referring this pipe (protected by ->i_lock)
  *	@r_counter: reader counter
  *	@w_counter: writer counter
- *	@poll_usage: is this pipe used for epoll, which has crazy wakeups?
+ *	@epoll_usage: is this pipe used for epoll, which has crazy wakeups?
  *	@fasync_readers: reader side fasync
  *	@fasync_writers: writer side fasync
  *	@bufs: the circular array of pipe buffers
@@ -95,7 +95,9 @@ struct pipe_inode_info {
 	unsigned int files;
 	unsigned int r_counter;
 	unsigned int w_counter;
-	bool poll_usage;
+#ifdef CONFIG_EPOLL
+	bool epoll_usage;
+#endif
 #ifdef CONFIG_WATCH_QUEUE
 	bool note_loss;
 #endif
-- 
2.52.0

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@redhat.com> 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@redhat.com>
> ---
>  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

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

[Oleg Nesterov]

OK, sashiko has some concerns

	https://sashiko.dev/#/patchset/amIqmbbZx3NlzUsX%40redhat.com

As for READ_ONCE(), I agree, I just forgot to add them.

But so far I don't understand the note about io_uring...
I'll try to check later.

Anyway, I'd like to know your opinion.

On 07/23, Oleg Nesterov wrote:
>
> Breno, Christian, Mateusz,
>
> To be honest, I am not sure this change makes a lot of sense.
> But somehow the extra wakeup in anon_pipe_write() looks annoying to me.
>
> Can you take a look?
>
> Plus I'd like to know what sashiko.dev thinks.
>
> I have no idea how to test it wrt performance...
>
> Oleg.
> ---
>
>  fs/pipe.c                 | 23 +++++++++++++++++++----
>  include/linux/pipe_fs_i.h |  6 ++++--
>  2 files changed, 23 insertions(+), 6 deletions(-)