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

[Paolo Bonzini <pbonzini@redhat.com>]

 On Aug 01 2013, Alex Bligh wrote:
> Paolo,
> 
> --On 1 August 2013 15:51:11 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> >>> 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?
> >
> >Suppose a thread wants to do qemu_clock_enable(foo, false), and the
> >code after qemu_clock_enable assumes that no timers are running.
> >Then you should move that code to a bottom half in foo's AioContext.
> 
> foo is a QEMUClock here.
> 
> A QEMUClock may not have just one AioContext. It could have several
> each operated by a different thread.

Oops, you're right.

Checking the code for callers of qemu_clock_enable() it seems like there
shouldn't be any deadlocks.  So it should work if qemu_clock_enable
walks all of the timerlists and waits on each event.

But we should document the expectations since they are different from
the current code.

Paolo