From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59457) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4lqV-0006nZ-6b for qemu-devel@nongnu.org; Thu, 01 Aug 2013 01:54:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V4lqT-0002Iq-LB for qemu-devel@nongnu.org; Thu, 01 Aug 2013 01:54:43 -0400 Received: from mail-la0-x230.google.com ([2a00:1450:4010:c03::230]:38773) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4lqT-0002IP-Dm for qemu-devel@nongnu.org; Thu, 01 Aug 2013 01:54:41 -0400 Received: by mail-la0-f48.google.com with SMTP id hi8so1115612lab.35 for ; Wed, 31 Jul 2013 22:54:39 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <51F78491.708@redhat.com> 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> <51F78491.708@redhat.com> From: liu ping fan Date: Thu, 1 Aug 2013 13:54:18 +0800 Message-ID: Content-Type: text/plain; charset=ISO-8859-1 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: Paolo Bonzini Cc: Kevin Wolf , Stefan Hajnoczi , Jan Kiszka , qemu-devel@nongnu.org, Alex Bligh , Anthony Liguori On Tue, Jul 30, 2013 at 5:17 PM, Paolo Bonzini wrote: > 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). > 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