Re: [PATCH] io_uring/poll: use regular CQE posting for multishot termination

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

On 5/7/25 19:08, Jens Axboe wrote:
> A previous patch avoided reordering of multiple multishot requests
> getting their CQEs potentiall reordered when one of them terminates, as
> that last termination CQE is posted as a deferred completion rather than
> directly as a CQE. This can reduce the efficiency of the batched
> posting, hence was not ideal.
> 
> Provide a basic helper that poll can use for this kind of termination,
> which does a normal CQE posting rather than a deferred one. With that,
> the work-around where io_req_post_cqe() needs to flush deferred
> completions can be removed.
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> ---
> 
> This removes the io_req_post_cqe() flushing, and instead puts the honus
> on the poll side to provide the ordering. I've verified that this also
> fixes the reported issue. The previous patch can be easily backported to
> stable, so makes sense to keep that one.

It still gives a bad feeling tbh, it's not a polling problem,
we're working around shortcomings of the incremental / bundled
uapi and/or design. Patching it in semi unrelated places will
defitely bite back.

Can it be fixed in relevant opcodes? So it stays close to
those who actually use it. And let me ask since I'm lost in
new features, can the uapi be fixed so that it doesn't
depend on request ordering?

> 
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index 541e65a1eebf..505959fc2de0 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -848,14 +848,6 @@ bool io_req_post_cqe(struct io_kiocb *req, s32 res, u32 cflags)
>   	struct io_ring_ctx *ctx = req->ctx;
>   	bool posted;
>   
> -	/*
> -	 * If multishot has already posted deferred completions, ensure that
> -	 * those are flushed first before posting this one. If not, CQEs
> -	 * could get reordered.
> -	 */
> -	if (!wq_list_empty(&ctx->submit_state.compl_reqs))
> -		__io_submit_flush_completions(ctx);
> -
>   	lockdep_assert(!io_wq_current_is_worker());
>   	lockdep_assert_held(&ctx->uring_lock);
>   
> @@ -871,6 +863,23 @@ bool io_req_post_cqe(struct io_kiocb *req, s32 res, u32 cflags)
>   	return posted;
>   }
>   
> +bool io_req_post_cqe_overflow(struct io_kiocb *req)

"overflow" here is rather confusing, it could mean lots of things.
Maybe some *_post_poll_complete for now?

> +{
> +	bool filled;
> +
> +	filled = io_req_post_cqe(req, req->cqe.res, req->cqe.flags);

posting and overflow must be under the same CQ critical section,
like io_cq_lock(). Just copy io_post_aux_cqe() and add
ctx->cq_extra--? Hopefully we'll remove the cq_extra ugliness
later and combine them after.

> +	if (unlikely(!filled)) {
> +		struct io_ring_ctx *ctx = req->ctx;
> +
> +		spin_lock(&ctx->completion_lock);
> +		filled = io_cqring_event_overflow(ctx, req->cqe.user_data,
> +					req->cqe.res, req->cqe.flags, 0, 0);
> +		spin_unlock(&ctx->completion_lock);
> +	}
> +
> +	return filled;
> +}
> +
>   static void io_req_complete_post(struct io_kiocb *req, unsigned issue_flags)
>   {
>   	struct io_ring_ctx *ctx = req->ctx;
> diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
> index e4050b2d0821..d2d4bf7c3b29 100644
> --- a/io_uring/io_uring.h
> +++ b/io_uring/io_uring.h
> @@ -82,6 +82,7 @@ void io_req_defer_failed(struct io_kiocb *req, s32 res);
>   bool io_post_aux_cqe(struct io_ring_ctx *ctx, u64 user_data, s32 res, u32 cflags);
>   void io_add_aux_cqe(struct io_ring_ctx *ctx, u64 user_data, s32 res, u32 cflags);
>   bool io_req_post_cqe(struct io_kiocb *req, s32 res, u32 cflags);
> +bool io_req_post_cqe_overflow(struct io_kiocb *req);
>   void __io_commit_cqring_flush(struct io_ring_ctx *ctx);
>   
>   struct file *io_file_get_normal(struct io_kiocb *req, int fd);
> diff --git a/io_uring/poll.c b/io_uring/poll.c
> index 8eb744eb9f4c..af8e3d4f6f1f 100644
> --- a/io_uring/poll.c
> +++ b/io_uring/poll.c
> @@ -312,6 +312,13 @@ static int io_poll_check_events(struct io_kiocb *req, io_tw_token_t tw)
>   	return IOU_POLL_NO_ACTION;
>   }
>   
> +static void io_poll_req_complete(struct io_kiocb *req, io_tw_token_t tw)
> +{
> +	if (io_req_post_cqe_overflow(req))
> +		req->flags |= REQ_F_CQE_SKIP;

Unconditional would be better. It'd still end up in attempting
to post, likely failing and reattemptng allocation just one
extra time, not like it gives any reliability. And if I'd be
choosing b/w dropping a completion or potentially getting a
botched completion as per the problem you tried, I say the
former is better.

> +	io_req_task_complete(req, tw);
> +}
> +
>   void io_poll_task_func(struct io_kiocb *req, io_tw_token_t tw)
>   {
>   	int ret;
> @@ -349,7 +356,7 @@ void io_poll_task_func(struct io_kiocb *req, io_tw_token_t tw)
>   		io_tw_lock(req->ctx, tw);
>   
>   		if (ret == IOU_POLL_REMOVE_POLL_USE_RES)
> -			io_req_task_complete(req, tw);
> +			io_poll_req_complete(req, tw);
>   		else if (ret == IOU_POLL_DONE || ret == IOU_POLL_REISSUE)
>   			io_req_task_submit(req, tw);
>   		else

-- 
Pavel Begunkov