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). Paolo