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

[liu ping fan]

On Thu, Aug 1, 2013 at 10:28 PM, Paolo Bonzini <address@hidden> wrote:
>  On Aug 01 2013, Alex Bligh wrote:
>> Paolo,
>>
>> --On 1 August 2013 15:51:11 +0200 Paolo Bonzini <address@hidden> 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
>