From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40601) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V463n-00059l-Sa for qemu-devel@nongnu.org; Tue, 30 Jul 2013 05:17:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V463h-0006Nx-Eh for qemu-devel@nongnu.org; Tue, 30 Jul 2013 05:17:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50945) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V463h-0006Nn-7B for qemu-devel@nongnu.org; Tue, 30 Jul 2013 05:17:33 -0400 Message-ID: <51F78491.708@redhat.com> Date: Tue, 30 Jul 2013 11:17:05 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1375067768-11342-1-git-send-email-pingfank@linux.vnet.ibm.com> <1375067768-11342-4-git-send-email-pingfank@linux.vnet.ibm.com> <51F60C02.6020008@redhat.com> <51F65044.4040706@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC v2 3/5] timer: make qemu_clock_enable sync between disable and timer's cb List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: liu ping fan Cc: Kevin Wolf , Stefan Hajnoczi , Jan Kiszka , qemu-devel@nongnu.org, Alex Bligh , Anthony Liguori Il 30/07/2013 04:42, liu ping fan ha scritto: > On Mon, Jul 29, 2013 at 7:21 PM, Paolo Bonzini wrote: >> Il 29/07/2013 10:10, liu ping fan ha scritto: >>> On Mon, Jul 29, 2013 at 2:30 PM, Paolo Bonzini 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 >>>> >>>> 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