Re: [PATCH 4/5] io_uring: add support for batch wait timeout

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

On 8/22/24 17:14, Jens Axboe wrote:
> On 8/22/24 10:06 AM, Pavel Begunkov wrote:
>> On 8/22/24 16:37, Jens Axboe wrote:
>>> On 8/22/24 7:46 AM, Pavel Begunkov wrote:
>>>> On 8/21/24 15:16, Jens Axboe wrote:
>> ...
>>>>>           if (ext_arg->sig) {
>>>>> @@ -2484,14 +2544,16 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events, u32 flags,
>>>>>             unsigned long check_cq;
>>>>>               if (ctx->flags & IORING_SETUP_DEFER_TASKRUN) {
>>>>> -            atomic_set(&ctx->cq_wait_nr, nr_wait);
>>>>> +            /* if min timeout has been hit, don't reset wait count */
>>>>> +            if (!READ_ONCE(iowq.hit_timeout))
>>>>
>>>> Why read once? You're out of io_cqring_schedule_timeout(),
>>>> timers are cancelled and everything should've been synchronised
>>>> by this point.
>>>
>>> Just for consistency's sake.
>>
>> Please drop it. Sync primitives tell a story, and this one says
>> that it's racing with something when it's not. It's always hard to
>> work with code with unnecessary protection. If it has to change in
>> the future the first question asked would be why read once is there,
>> what does it try to achieve / protect and if it's safe to kill it.
>> It'll also hide real races from sanitizers.
> 
> Sure I don't disagree, I'll kill it.

Thanks. Personal trauma, especially after tracking down some chunks
of code back to 2.6 with no explanation nor author to ask.

-- 
Pavel Begunkov