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

[liu ping fan]

On Tue, Jul 30, 2013 at 5:17 PM, Paolo Bonzini <address@hidden> wrote:
> Il 30/07/2013 04:42, liu ping fan ha scritto:
>> 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?
>
> Hmm, do we even need clock->using at this point?  For example:
>
>    qemu_clock_enable()
>    {
>        clock->enabled = enabled;
>        ...
>        if (!enabled) {
>            /* If another thread is within qemu_run_timers,
>             * wait for it to finish.
>             */
>            qemu_event_wait(&clock->callbacks_done_event);
>        }
>    }
>
>    qemu_run_timers()
>    {
>        qemu_event_reset(&clock->callbacks_done_event);
>        if (!clock->enabled) {
>            goto out;
>        }
>        ...
>    out:
>        qemu_event_set(&eclock->callbacks_done_event);
>    }
>
> In the fast path this only does two atomic operations (an OR for reset,
> and XCHG for set).
>
There is race condition, suppose the following scenario with A/B thread
A: qemu_event_reset()
B: qemu_event_reset()
A: qemu_event_set() ----> B is still in flight when
qemu_clock_enable() is notified
B: qemu_event_set()

I had tried to build something around futex(2) like qemu_event, but failed.

Thanks and regards,
Pingfan

> Paolo