On Thu, Aug 1, 2013 at 10:28 PM, Paolo Bonzini <pbonz...@redhat.com> wrote: > On Aug 01 2013, Alex Bligh wrote: >> Paolo, >> >> --On 1 August 2013 15:51:11 +0200 Paolo Bonzini <pbonz...@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
Do you mean the caller of qemu_clock_enable(foo,false) can NOT be timer cb? As for this deadlock issue, making qemu_clock_enabe(foo,false) ASYNC, and forcing the caller to sync explicitly ? > 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 >
