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
