Re: [Qemu-devel] [PATCH 0/4]: timers thread-safe stuff

[liu ping fan]

On Mon, Aug 5, 2013 at 6:00 PM, Alex Bligh <address@hidden> wrote:
> Pingfan,
>
>
> --On 5 August 2013 15:33:22 +0800 Liu Ping Fan <address@hidden>
> wrote:
>
>> The patches has been rebased onto Alex's [RFC] [PATCHv5 00/16] aio /
>> timers: Add AioContext timers and use ppoll
>> permalink.gmane.org/gmane.comp.emulators.qemu/226333
>>
>> For some other complied error issue, I can not finish compiling, will fix
>> it later.
>>
>> Changes since last version:
>>  1. drop the overlap partition and leave only thread-safe stuff
>>  2. For timers_state, since currently, only vm_clock can be read outside
>> BQL.     There is no protection with ticks(since the protection will cost
>> more in read_tsc path).  3. use light weight QemuEvent to re-implement
>> the qemu_clock_enable(foo,false)
>
>
> I think you may need to protect a little more.
>
Yes. There is still race issue left. If Stefanha and you will not do
it, I am pleased to do that.

> For instance, do we need to take a lock whilst traversing a QEMUTimerList?
> I hope the answer to this is no. The design idea of my stuff was that only
> one thread would be manipulating or traversing this list. As the notify
> function is a property of the QEMUTimerList itself, no traversal is
> necessary for that. However, the icount stuff (per my patch 15) does
> look at the deadlines for the vm_clock QEMUTimerLists (which is the
> first entry). Is that always going to be thread safe? Before the icount
> stuff, I was pretty certain this did not need a lock, but perhaps
> it does now. If the soonest deadline on any QEMUTimerList was stored
> in a 64 bit atomic variable, this might remove the need for a lock;
> it's possible that putting some memory barrier operations in might
> be enough.
>
> Also, we maintain a per-clock list of QEMUTimerLists. This list is
> traversed by the icount stuff, by things adding and removing timers
> (e.g. creation/deletion of AioContext) and whenever a QEMUClock is
> reenabled. I think this DOES need a lock. Aside from the icount
> stuff, usage is very infrequent. It's far more frequent with the
> icount stuff, so it should probably be optimised for that case.
>
> --
> Alex Bligh
>