On Mon, Jul 29, 2013 at 7:21 PM, Paolo Bonzini <pbonz...@redhat.com> wrote:
> Il 29/07/2013 10:10, liu ping fan ha scritto:
>> On Mon, Jul 29, 2013 at 2:30 PM, Paolo Bonzini <pbonz...@redhat.com> wrote:
>>> Il 29/07/2013 05:16, Liu Ping Fan ha scritto:
>>>> After disabling the QemuClock, we should make sure that no QemuTimers
>>>> are still in flight. To implement that, the caller of disabling will
>>>> wait until the last user's exit.
>>>>
>>>> Note, the callers of qemu_clock_enable() should be sync by themselves,
>>>> not protected by this patch.
>>>>
>>>> Signed-off-by: Liu Ping Fan <pingf...@linux.vnet.ibm.com>
>>>
>>> This is an interesting approach.
>>>
>>>> - if (!clock->enabled)
>>>> - return;
>>>> + atomic_inc(&clock->using);
>>>> + if (unlikely(!clock->enabled)) {
>>>> + goto exit;
>>>> + }
>>>
>>> This can return directly, it doesn't need to increment and decrement
>>> clock->using.
>>>
>> Here is a race condition like the following
>
> Ah, I see.
>
> Still this seems a bit backwards. Most of the time you will have no one
> on the wait_using condvar, but you are almost always signaling the
> condvar (should be broadcast BTW)...
>
I have tried to filter out the normal case by
+ qemu_mutex_lock(&clock->lock);
+ if (atomic_fetch_dec(&clock->using) == 1) { ---------> 1st place
+ if (unlikely(!clock->enabled)) { -------> 2nd place
+ qemu_cond_signal(&clock->wait_using);
+ }
+ }
+ qemu_mutex_unlock(&clock->lock);
Could I do it better?
Thanks,
Pingfan
