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

[liu ping fan]

On Mon, Jul 29, 2013 at 7:21 PM, Paolo Bonzini <address@hidden> wrote:
> Il 29/07/2013 10:10, liu ping fan ha scritto:
>> On Mon, Jul 29, 2013 at 2:30 PM, Paolo Bonzini <address@hidden> 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 <address@hidden>
>>>
>>> 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