Re: [PATCH] pipe: keep per-write wakeups for EPOLLEXCLUSIVE consumers

[Oleg Nesterov <oleg@redhat.com>]

This probably needs more discussion... I will write another email.
But so far I think we don't really care.

On 11/11, Mani Tofigh via B4 Relay wrote:
>
> A task blocked in read() on a pipe can sleep while the pipe holds data,
> when an epoll(EPOLLEXCLUSIVE) watcher of the read end was added before
> it.  The watcher's entry on ->rd_wait is exclusive and sits ahead of the
> reader, so a write into an empty pipe wakes only the watcher.  The
> watcher reads without sleeping in anon_pipe_read() and doesn't pass the
> wakeup on.  Since commit f1cebdec9739 ("pipe: only enable the extra
> wake_up(rd_wait) for EPOLLET consumers") the next writes into the
> non-empty pipe don't wake anyone anymore:
>
>   Task E                   Task A                 Task C
>   epoll_wait(ep, -1)
>     /* EPOLLEXCLUSIVE */
>                            read(p[0])
>                                                   write(p[1], "1")
>                                                     /* wakes E only */
>                                                   write(p[1], "2")
>                                                     /* no wakeup */
>   read(p[0], 1 byte)
>   exit()
>                            /* still asleep, "2" is in the pipe */

Yes, in this case f1cebdec9739 ("pipe: only enable the extra wake_up(rd_wait)
for EPOLLET consumers") makes a difference.

However, this trivial test-case

	#include <unistd.h>
	#include <sys/epoll.h>
	#include <sys/wait.h>
	#include <assert.h>
	#include <stdio.h>

	int main(void)
	{
		int pfd[2], efd;
		struct epoll_event evt = { .events = EPOLLIN | EPOLLEXCLUSIVE };
		char c;

		pipe(pfd);
		efd = epoll_create1(0);
		epoll_ctl(efd, EPOLL_CTL_ADD, pfd[0], &evt);

		if (!fork()) {
			epoll_wait(efd, &evt, 1, -1);
			read(pfd[0], &c, 1);
			printf("1st: %c\n", c);
			return 0;
		}

		sleep(1);

		if (!fork()) {
			read(pfd[0], &c, 1);
			printf("2nd: %c\n", c);
			return 0;
		}

		sleep(1);

		write(pfd[1], "12", 2);
		assert(wait(NULL) > 0);
		assert(wait(NULL) > 0);

		return 0;
	}

hangs even with this patch

	+++ b/fs/pipe.c
	@@ -749,6 +749,7 @@ pipe_poll(struct file *filp, poll_table *wait)
		    unlikely(!READ_ONCE(pipe->pseudo_edgetrigger)))
			WRITE_ONCE(pipe->pseudo_edgetrigger, true);
	 
	+WRITE_ONCE(pipe->pseudo_edgetrigger, true);
		/*
		 * Reading pipe state only -- no need for acquiring the semaphore.
		 *

which reverts f1cebdec9739.

The 2nd child sleeps in read(pfd[0]) forever even if the pipe is not empty.
So I am not sure we want to enforce another oddity in pipe_poll/read ...

Oleg.