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
