> > True, qemu_event basically works only when a single thread resets it. But > > there is no race condition here because qemu_run_timers cannot be executed > > concurrently by multiple threads (like aio_poll in your bottom half > > patches). > > ... or, if rebasing on top of my patches, qemu_run_timers *can* be > executed concurrently by mulitple threads, but in respect of any given > QEMUTimerList, it will only be executed by one thread.
... so the event would be placed in QEMUTimerList, not QEMUClock. qemu_clock_enable then will have to visit all timer lists. That's not hard to do, but as locks proliferate we need to have a clear policy (e.g. BQL -> clock -> timerlist). So actually there is another problem with this patch (both the condvar and the event approach are equally buggy). If a timer on clock X disables clock X, qemu_clock_enable will deadlock. I suppose it's easier to ask each user of qemu_clock_enable to provide its own synchronization, and drop this patch. The simplest kind of synchronization is to delay some work to a bottom half in the clock's AioContext. If you do that, you know that the timers are not running. Ping Fan, this teaches once more the same lesson: let's not invent complex concurrency mechanisms unless really necessary. So far they almost never survived (there are some exceptions, but we have always taken them from somewhere else: QemuEvent is an abstraction of liburcu code to make it portable, RCU and seqlock are from Linux, and I cannot think of anything else). Paolo