On Tue, Jul 30, 2013 at 5:17 PM, Paolo Bonzini <pbonz...@redhat.com> wrote:
> Il 30/07/2013 04:42, liu ping fan ha scritto:
>> 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?
>
> 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
