On Thu, Jun 27, 2024 at 2:32 AM Roman Kiryanov <r...@google.com> wrote:
> + if (qatomic_read(&ts->cb_running)) {
> + qemu_event_wait(&timer_list->timers_done_ev);
> + }
qemu_event_wait() already has the right atomic magic, and
ts->cb_running is both redundant (in general), and I think racy (as
implemented in this patch). This stuff is really hard to get right. At
the very least you need a store-release when clearing the flag, and I
think it also has to be read under the mutex (I'm not sure if there's
anything else, I haven't done a full analysis).
But especially, you haven't justified in the commit message _why_ you
need this. Since this problem is not timer-specific, using
aio_bh_schedule_oneshot() or aio_wait_bh_oneshot() to synchronize
everything with the AioContext thread seems like a superior solution
to me.
Paolo
> }
> }
>
> @@ -571,9 +576,15 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
> cb = ts->cb;
> opaque = ts->opaque;
>
> + /* prevent timer_del from returning while cb(opaque)
> + * is still running (by waiting for timers_done_ev).
> + */
> + qatomic_set(&ts->cb_running, true);
> +
> /* run the callback (the timer list can be modified) */
> qemu_mutex_unlock(&timer_list->active_timers_lock);
> cb(opaque);
> + qatomic_set(&ts->cb_running, false);
> qemu_mutex_lock(&timer_list->active_timers_lock);
>
> progress = true;
> --
> 2.45.2.741.gdbec12cfda-goog
>
