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

[Jens Axboe <axboe@kernel.dk>]

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.

-- 
Jens Axboe