Re: [PATCH 3/5] io_uring: implement our own schedule timeout handling

[David Wei <dw@davidwei.uk>]

On 2024-08-20 14:34, Jens Axboe wrote:
> On 8/20/24 2:08 PM, David Wei wrote:
>> On 2024-08-19 16:28, Jens Axboe wrote:
>>> In preparation for having two distinct timeouts and avoid waking the
>>> task if we don't need to.
>>>
>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>> ---
>>>  io_uring/io_uring.c | 41 ++++++++++++++++++++++++++++++++++++-----
>>>  io_uring/io_uring.h |  2 ++
>>>  2 files changed, 38 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
>>> index 9e2b8d4c05db..ddfbe04c61ed 100644
>>> --- a/io_uring/io_uring.c
>>> +++ b/io_uring/io_uring.c
>>> @@ -2322,7 +2322,7 @@ static int io_wake_function(struct wait_queue_entry *curr, unsigned int mode,
>>>  	 * Cannot safely flush overflowed CQEs from here, ensure we wake up
>>>  	 * the task, and the next invocation will do it.
>>>  	 */
>>> -	if (io_should_wake(iowq) || io_has_work(iowq->ctx))
>>> +	if (io_should_wake(iowq) || io_has_work(iowq->ctx) || iowq->hit_timeout)
>>
>> iowq->hit_timeout may be modified in a timer softirq context, while this
>> wait_queue_func_t (AIUI) may get called from any context e.g.
>> net_rx_softirq for sockets. Does this need a READ_ONLY()?
> 
> Yes probably not a bad idea to make it READ_ONCE().
> 
>>>  		return autoremove_wake_function(curr, mode, wake_flags, key);
>>>  	return -1;
>>>  }
>>> @@ -2350,6 +2350,38 @@ static bool current_pending_io(void)
>>>  	return percpu_counter_read_positive(&tctx->inflight);
>>>  }
>>>  
>>> +static enum hrtimer_restart io_cqring_timer_wakeup(struct hrtimer *timer)
>>> +{
>>> +	struct io_wait_queue *iowq = container_of(timer, struct io_wait_queue, t);
>>> +	struct io_ring_ctx *ctx = iowq->ctx;
>>> +
>>> +	WRITE_ONCE(iowq->hit_timeout, 1);
>>> +	if (ctx->flags & IORING_SETUP_DEFER_TASKRUN)
>>> +		wake_up_process(ctx->submitter_task);
>>> +	else
>>> +		io_cqring_wake(ctx);
>>
>> This is a bit different to schedule_hrtimeout_range_clock(). Why is
>> io_cqring_wake() needed here for non-DEFER_TASKRUN?
> 
> That's how the wakeups work - for defer taskrun, the task isn't on a
> waitqueue at all. Hence we need to wake the task itself. For any other
> setup, they will be on the waitqueue, and we just call io_cqring_wake()
> to wake up anyone waiting on the waitqueue. That will iterate the wake
> queue and call handlers for each item. Having a separate handler for
> that will allow to NOT wake up the task if we don't need to.
> taskrun, the waker

To rephase the question, why is the original code calling
schedule_hrtimeout_range_clock() not needing to differentiate behaviour
between defer taskrun and not?