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 9:28 PM, Alex Bligh <address@hidden> wrote:
> 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)
>
Seem that Stefanha had been involved in this, and sent out his patches :)
>
>> 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