Re: [PATCH 09/11] io_uring/epoll: add support for IORING_OP_EPOLL_WAIT

[Pavel Begunkov <asml.silence@gmail.com>]

On 2/4/25 19:46, Jens Axboe wrote:
> For existing epoll event loops that can't fully convert to io_uring,
> the used approach is usually to add the io_uring fd to the epoll
> instance and use epoll_wait() to wait on both "legacy" and io_uring
> events. While this work, it isn't optimal as:
> 
> 1) epoll_wait() is pretty limited in what it can do. It does not support
>     partial reaping of events, or waiting on a batch of events.
> 
> 2) When an io_uring ring is added to an epoll instance, it activates the
>     io_uring "I'm being polled" logic which slows things down.
> 
> Rather than use this approach, with EPOLL_WAIT support added to io_uring,
> event loops can use the normal io_uring wait logic for everything, as
> long as an epoll wait request has been armed with io_uring.
> 
> Note that IORING_OP_EPOLL_WAIT does NOT take a timeout value, as this
> is an async request. Waiting on io_uring events in general has various
> timeout parameters, and those are the ones that should be used when
> waiting on any kind of request. If events are immediately available for
> reaping, then This opcode will return those immediately. If none are
> available, then it will post an async completion when they become
> available.
> 
> cqe->res will contain either an error code (< 0 value) for a malformed
> request, invalid epoll instance, etc. It will return a positive result
> indicating how many events were reaped.
> 
> IORING_OP_EPOLL_WAIT requests may be canceled using the normal io_uring
> cancelation infrastructure. The poll logic for managing ownership is
> adopted to guard the epoll side too.
...
> diff --git a/io_uring/epoll.c b/io_uring/epoll.c
> index 7848d9cc073d..5a47f0cce647 100644
> --- a/io_uring/epoll.c
> +++ b/io_uring/epoll.c
...
> +static void io_epoll_retry(struct io_kiocb *req, struct io_tw_state *ts)
> +{
> +	int v;
> +
> +	do {
> +		v = atomic_read(&req->poll_refs);
> +		if (unlikely(v != 1)) {
> +			if (WARN_ON_ONCE(!(v & IO_POLL_REF_MASK)))
> +				return;
> +			if (v & IO_POLL_CANCEL_FLAG) {
> +				__io_epoll_cancel(req);
> +				return;
> +			}
> +			if (v & IO_POLL_FINISH_FLAG)
> +				return;
> +		}
> +		v &= IO_POLL_REF_MASK;
> +	} while (atomic_sub_return(v, &req->poll_refs) & IO_POLL_REF_MASK);

I haven't looked deep into the set, but this loop looks very
suspicious. The entire purpose of the twin loop in poll.c is
not to lose events while doing processing, which is why the
processing happens before the decrement...

> +	io_req_task_submit(req, ts);

Maybe the issue is supposed to handle that, but this one is
not allowed unless you fully unhash all the polling. Once you
dropped refs the poll wait entry feels free to claim the request,
and, for example, queue a task work, and io_req_task_submit()
would decide to queue it as well. It's likely not the only
race that can happen.

> +}
> +
> +static int io_epoll_execute(struct io_kiocb *req)
> +{
> +	struct io_epoll_wait *iew = io_kiocb_to_cmd(req, struct io_epoll_wait);
> +
> +	list_del_init_careful(&iew->wait.entry);
> +	if (io_poll_get_ownership(req)) {
> +		req->io_task_work.func = io_epoll_retry;
> +		io_req_task_work_add(req);
> +	}
> +
> +	return 1;
> +}
...
> +int io_epoll_wait(struct io_kiocb *req, unsigned int issue_flags)
> +{
> +	struct io_epoll_wait *iew = io_kiocb_to_cmd(req, struct io_epoll_wait);
> +	struct io_ring_ctx *ctx = req->ctx;
> +	int ret;
> +
> +	io_ring_submit_lock(ctx, issue_flags);
> +
> +	ret = epoll_wait(req->file, iew->events, iew->maxevents, NULL, &iew->wait);
> +	if (ret == -EIOCBQUEUED) {
> +		if (hlist_unhashed(&req->hash_node))
> +			hlist_add_head(&req->hash_node, &ctx->epoll_list);
> +		io_ring_submit_unlock(ctx, issue_flags);
> +		return IOU_ISSUE_SKIP_COMPLETE;
> +	} else if (ret < 0) {
> +		req_set_fail(req);
> +	}
> +	hlist_del_init(&req->hash_node);
> +	io_ring_submit_unlock(ctx, issue_flags);
> +	io_req_set_res(req, ret, 0);
> +	return IOU_OK;
> +}
...
-- 
Pavel Begunkov