From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48122) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4tsK-0003Df-QF for qemu-devel@nongnu.org; Thu, 01 Aug 2013 10:29:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V4tsE-0005iy-HE for qemu-devel@nongnu.org; Thu, 01 Aug 2013 10:29:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37547) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4tsE-0005il-7v for qemu-devel@nongnu.org; Thu, 01 Aug 2013 10:29:02 -0400 Date: Thu, 1 Aug 2013 16:28:44 +0200 From: Paolo Bonzini Message-ID: <20130801142844.GB5761@mail.corp.redhat.com> References: <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> <97E9EFDF2EC4B9CE77638619@Ximines.local> <20130801135111.GL5245@mail.corp.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: 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 On Aug 01 2013, Alex Bligh wrote: > Paolo, > > --On 1 August 2013 15:51:11 +0200 Paolo Bonzini wrote: > > >>> 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? > > > >Suppose a thread wants to do qemu_clock_enable(foo, false), and the > >code after qemu_clock_enable assumes that no timers are running. > >Then you should move that code to a bottom half in foo's AioContext. > > foo is a QEMUClock here. > > A QEMUClock may not have just one AioContext. It could have several > each operated by a different thread. Oops, you're right. Checking the code for callers of qemu_clock_enable() it seems like there shouldn't be any deadlocks. So it should work if qemu_clock_enable walks all of the timerlists and waits on each event. But we should document the expectations since they are different from the current code. Paolo