Re: [Qemu-devel] [RFC v2 3/5] timer: make qemu_clock_enable sync between disable and timer's cb

[Alex Bligh]

Paolo,

--On 1 August 2013 08:19:34 -0400 Paolo Bonzini <address@hidden> wrote:

> > > 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.

Correct

> qemu_clock_enable then will have to visit all timer lists. That's
> not hard to do,

Correct, v4 of my patch does this.

> but as locks proliferate we need to have a clear
> policy (e.g. BQL -> clock -> timerlist).

But doesn't do the locking bit yet (Pingfan is going to do that I hope)

> 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.

Yes. I believe there will be a similar problem if a timer
created or deleted an AioContext (clearly less likely!)

> 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.

I'm not sure that's true. If two AioContexts run in different
threads, would their BH's and timers not also run in those two
different threads?

--
Alex Bligh