From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52115) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4swN-0004Pp-U6 for qemu-devel@nongnu.org; Thu, 01 Aug 2013 09:29:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V4swK-0008Mc-IZ for qemu-devel@nongnu.org; Thu, 01 Aug 2013 09:29:15 -0400 Received: from mail.avalus.com ([2001:41c8:10:1dd::10]:41432) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4swK-0008M5-C4 for qemu-devel@nongnu.org; Thu, 01 Aug 2013 09:29:12 -0400 Date: Thu, 01 Aug 2013 14:28:58 +0100 From: Alex Bligh Message-ID: <97E9EFDF2EC4B9CE77638619@Ximines.local> In-Reply-To: <1277844563.8104892.1375359574399.JavaMail.root@redhat.com> 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> <1277844563.8104892.1375359574399.JavaMail.root@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Content-Disposition: inline Subject: Re: [Qemu-devel] [RFC v2 3/5] timer: make qemu_clock_enable sync between disable and timer's cb Reply-To: Alex Bligh List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Kevin Wolf , Stefan Hajnoczi , Alex Bligh , Jan Kiszka , liu ping fan , qemu-devel@nongnu.org, Anthony Liguori Paolo, --On 1 August 2013 08:19:34 -0400 Paolo Bonzini wrote: >> > 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. Correct > qemu_clock_enable then will have to visit all timer lists. That's > not hard to do, Correct, v4 of my patch does this. > but as locks proliferate we need to have a clear > policy (e.g. BQL -> clock -> timerlist). But doesn't do the locking bit yet (Pingfan is going to do that I hope) > 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. Yes. I believe there will be a similar problem if a timer created or deleted an AioContext (clearly less likely!) > 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. I'm not sure that's true. If two AioContexts run in different threads, would their BH's and timers not also run in those two different threads? -- Alex Bligh