From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57697) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4rzU-0005SP-Bi for qemu-devel@nongnu.org; Thu, 01 Aug 2013 08:28:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V4rzO-0003tt-Bq for qemu-devel@nongnu.org; Thu, 01 Aug 2013 08:28:24 -0400 Received: from mx3-phx2.redhat.com ([209.132.183.24]:36995) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4rr5-0000ih-7o for qemu-devel@nongnu.org; Thu, 01 Aug 2013 08:19:43 -0400 Date: Thu, 1 Aug 2013 08:19:34 -0400 (EDT) From: Paolo Bonzini Message-ID: <1277844563.8104892.1375359574399.JavaMail.root@redhat.com> In-Reply-To: <753B6C66578E71C12F9DFD2A@Ximines.local> References: <1375067768-11342-1-git-send-email-pingfank@linux.vnet.ibm.com> <51F65044.4040706@redhat.com> <51F78491.708@redhat.com> <391717867.8023244.1375347472193.JavaMail.root@redhat.com> <753B6C66578E71C12F9DFD2A@Ximines.local> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 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: Alex Bligh Cc: Kevin Wolf , Stefan Hajnoczi , Jan Kiszka , liu ping fan , qemu-devel@nongnu.org, Anthony Liguori > > True, qemu_event basically works only when a single thread resets it. But > > there is no race condition here because qemu_run_timers cannot be executed > > concurrently by multiple threads (like aio_poll in your bottom half > > patches). > > ... or, if rebasing on top of my patches, qemu_run_timers *can* be > executed concurrently by mulitple threads, but in respect of any given > QEMUTimerList, it will only be executed by one thread. ... so the event would be placed in QEMUTimerList, not QEMUClock. qemu_clock_enable then will have to visit all timer lists. That's not hard to do, but as locks proliferate we need to have a clear policy (e.g. BQL -> clock -> timerlist). So actually there is another problem with this patch (both the condvar and the event approach are equally buggy). If a timer on clock X disables clock X, qemu_clock_enable will deadlock. I suppose it's easier to ask each user of qemu_clock_enable to provide its own synchronization, and drop this patch. The simplest kind of synchronization is to delay some work to a bottom half in the clock's AioContext. If you do that, you know that the timers are not running. Ping Fan, this teaches once more the same lesson: let's not invent complex concurrency mechanisms unless really necessary. So far they almost never survived (there are some exceptions, but we have always taken them from somewhere else: QemuEvent is an abstraction of liburcu code to make it portable, RCU and seqlock are from Linux, and I cannot think of anything else). Paolo