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

[Oleg Nesterov <oleg@redhat.com>]

On 07/24, Oleg Nesterov wrote:
>
> On 07/23, Oleg Nesterov wrote:
> >
> > OK, sashiko has some concerns
> >
> > 	https://sashiko.dev/#/patchset/amIqmbbZx3NlzUsX%40redhat.com
>
> Let me quote:
>
> 	Does skipping this wakeup for non-epoll consumers break io_uring?
>
> 	Applications polling pipes via io_uring do not attach an eventpoll context,
> 	so pipe->epoll_usage will be false. If a writer writes to an empty pipe,
> 	io_uring receives the wakeup.
>
> 	If the writer then writes a second chunk before the first is drained,
> 	anon_pipe_write() observes was_empty == false and
> 	pipe_get_epoll_usage() == false, skipping the waitqueue wakeup.
>
> 	Could this cause io_uring to miss events and hang permanently, waiting
> 	for a CQE that will never be emitted for the new data?
>
> and I am starting to think sashiko is right (damn as always ;) and this
> patch does affect/break io_uring.
>
> Jens, could you confirm? If yes, we need to update the comments in pipe.c
> (I've attached 1/1 at the end, so that you can see what this patch does)
...
> It seems that IORING_OP_POLL_ADD / IORING_POLL_ADD_MULTI is edge-triggered
> by default! Like EPOLL_CTL_ADD / EPOLLET.

Yes, sashiko is right. With some help from AI I wrote the simple test-case

	#include <unistd.h>
	#include <sys/mman.h>
	#include <sys/epoll.h>
	#include <sys/syscall.h>
	#include <linux/io_uring.h>
	#include <assert.h>

	int main(void)
	{
		struct io_uring_params p = {};
		int fd, pfd[2];

		pipe(pfd);

		fd = syscall(SYS_io_uring_setup, 2, &p);
		assert(fd >= 0);

		void *ring = mmap(0, p.cq_off.cqes + p.cq_entries * sizeof(struct io_uring_cqe),
				  PROT_READ | PROT_WRITE, MAP_SHARED, fd, IORING_OFF_SQ_RING);
		assert(ring != MAP_FAILED);
		*(unsigned *)(ring + p.sq_off.array) = 0;
		*(unsigned *)(ring + p.sq_off.tail) = 1;

		struct io_uring_sqe *sqes = mmap(0, p.sq_entries * sizeof(*sqes),
				  PROT_READ | PROT_WRITE, MAP_SHARED, fd, IORING_OFF_SQES);
		assert(sqes != MAP_FAILED);
		sqes[0].opcode = IORING_OP_POLL_ADD;
		sqes[0].fd = pfd[0];
		sqes[0].len = IORING_POLL_ADD_MULTI;
		sqes[0].poll32_events = EPOLLIN;

		syscall(SYS_io_uring_enter, fd, 1, 0, 0, 0, 0);

		unsigned *cq_head = ring + p.cq_off.head;
		unsigned *cq_tail = ring + p.cq_off.tail;
		for (int i = 0; i < 2; ++i) {
			write(pfd[1], "", 1);
			syscall(SYS_io_uring_enter, fd, 0, 0, IORING_ENTER_GETEVENTS, 0, 0);
			assert(*cq_tail == ++*cq_head);
		}

		return 0;
	}

with this patch the 2nd assert(*cq_tail == ++*cq_head) fails.

And just for the record, another one for epoll

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

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

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

		for (int i = 0; i < 2; ++i) {
			write(pfd[1], "", 1);
			assert(epoll_wait(efd, &evt, 1, 0) == 1);
		}

		return 0;
	}

I'll try to make V2 later.

> And. io_poll_parse_events() doesn't set EPOLLET if IORING_POLL_ADD_LEVEL, but
> how is it possible to use IORING_POLL_ADD_LEVEL? io_poll_add_prep() only allows
> IORING_POLL_ADD_MULTI in flags? OK, I don't understand this code anyway...

OK, I see: d59bd748db0a9 ("io_uring/poll: disable level triggered poll")

Oleg.