Re: [PATCH 2/3] io_uring/poll: flag request as having gone through poll wake machinery

[Jens Axboe <axboe@kernel.dk>]

On 7/14/25 3:26 AM, Pavel Begunkov wrote:
> On 7/12/25 21:59, Jens Axboe wrote:
>> On 7/12/25 5:39 AM, Pavel Begunkov wrote:
>>> On 7/12/25 00:59, Jens Axboe wrote:
>>>> No functional changes in this patch, just in preparation for being able
>>>> to flag completions as having completed via being triggered from poll.
>>>>
>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>> ---
>>>>    include/linux/io_uring_types.h | 3 +++
>>>>    io_uring/poll.c                | 1 +
>>>>    2 files changed, 4 insertions(+)
>>>>
>>>> diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
>>>> index 80a178f3d896..b56fe2247077 100644
>>>> --- a/include/linux/io_uring_types.h
>>>> +++ b/include/linux/io_uring_types.h
>>>> @@ -505,6 +505,7 @@ enum {
>>>>        REQ_F_HAS_METADATA_BIT,
>>>>        REQ_F_IMPORT_BUFFER_BIT,
>>>>        REQ_F_SQE_COPIED_BIT,
>>>> +    REQ_F_POLL_WAKE_BIT,
>>>>          /* not a real bit, just to check we're not overflowing the space */
>>>>        __REQ_F_LAST_BIT,
>>>> @@ -596,6 +597,8 @@ enum {
>>>>        REQ_F_IMPORT_BUFFER    = IO_REQ_FLAG(REQ_F_IMPORT_BUFFER_BIT),
>>>>        /* ->sqe_copy() has been called, if necessary */
>>>>        REQ_F_SQE_COPIED    = IO_REQ_FLAG(REQ_F_SQE_COPIED_BIT),
>>>> +    /* request went through poll wakeup machinery */
>>>> +    REQ_F_POLL_WAKE        = IO_REQ_FLAG(REQ_F_POLL_WAKE_BIT),
>>>>    };
>>>>      typedef void (*io_req_tw_func_t)(struct io_kiocb *req, io_tw_token_t tw);
>>>> diff --git a/io_uring/poll.c b/io_uring/poll.c
>>>> index c7e9fb34563d..e1950b744f3b 100644
>>>> --- a/io_uring/poll.c
>>>> +++ b/io_uring/poll.c
>>>> @@ -423,6 +423,7 @@ static int io_poll_wake(struct wait_queue_entry *wait, unsigned mode, int sync,
>>>>                else
>>>>                    req->flags &= ~REQ_F_SINGLE_POLL;
>>>>            }
>>>> +        req->flags |= REQ_F_POLL_WAKE;
>>>
>>> Same, it's overhead for all polled requests for a not clear gain.
>>> Just move it to the arming function. It's also not correct to
>>> keep it here, if that's what you care about.
>>
>> Not too worried about overhead, for an unlocked or. The whole poll
> 
> You know, I wrote this machinery and optimised it, I'm not saying it
> to just piss you off, I still need it to work well for zcrx :)

This was not a critique of the code, it's just a generic statement on
the serialization around poll triggering is obviously a lot more
expensive than basic flag checking or setting. Every comment is not a
backhanded attack on someones code.

> Not going into details, but it's not such a simple unlocked or. And
> death by a thousand is never old either.

That's obviously true, I was just trying to set expectations that a
single flag mask is not really a big deal. If the idea and feature was
fully solidified and useful, then arguing that adding a bit or is a
problem is nonsense. By that standard, we could never add anything to
the code, only remove. At the same time, adding frivolous code is of
course always a bad idea.

>> machinery is pretty intense in that regard. But yeah, do agree that just
>> moving it to arming would be better and more appropriate too.
>>
>> I'm still a bit split on whether this makes any sense at all, 2-3 really
> 
> Right, that what I meant by unclear benefit. You're returning
> information from past when it's be already irrelevant, especially
> so for socket tx with how they handle their wait queue wakeups.

Indeed, and that's really the core of it and why I'm not totally sold on
it either. As mentioned in a previous email, I think it's better to let
this concept brew a bit in the background. Maybe something more specific
and useful will come out of this later.

-- 
Jens Axboe