From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NSFVV-0002uM-N7 for qemu-devel@nongnu.org; Tue, 05 Jan 2010 14:55:58 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NSFVM-0002oY-I5 for qemu-devel@nongnu.org; Tue, 05 Jan 2010 14:55:54 -0500 Received: from [199.232.76.173] (port=43066 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NSFVM-0002oV-6j for qemu-devel@nongnu.org; Tue, 05 Jan 2010 14:55:48 -0500 Received: from mx20.gnu.org ([199.232.41.8]:36473) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1NSFVH-0005HS-Is for qemu-devel@nongnu.org; Tue, 05 Jan 2010 14:55:47 -0500 Received: from qw-out-1920.google.com ([74.125.92.145]) by mx20.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NRwrG-0004bQ-4Z for qemu-devel@nongnu.org; Mon, 04 Jan 2010 19:01:10 -0500 Received: by qw-out-1920.google.com with SMTP id 5so2582929qwc.4 for ; Mon, 04 Jan 2010 16:00:08 -0800 (PST) Message-ID: <4B4280FC.4020602@codemonkey.ws> Date: Mon, 04 Jan 2010 17:59:56 -0600 From: Anthony Liguori MIME-Version: 1.0 References: <1261382970-23251-1-git-send-email-pbonzini@redhat.com> <1261382970-23251-12-git-send-email-pbonzini@redhat.com> <4B424E95.4040806@codemonkey.ws> <4B42491C.1050709@redhat.com> In-Reply-To: <4B42491C.1050709@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 11/19] use a bottom half to run timers List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org On 01/04/2010 02:01 PM, Paolo Bonzini wrote: > On 01/04/2010 09:24 PM, Anthony Liguori wrote: >> >> I'm not a huge fan of this for a couple reasons. The first is that >> it introduces a subtle semantic change. Previously, timers always >> ran before bottom halves whereas after this change, timers may run >> after some bottoms halves but before others. > > I see what you mean, and you are right: qemu_bh_new adds a > bottom half at the beginning of the queue, so it's pretty much > guaranteed that a ptimer's bottom half will run _before_ the alarm > timer's. > > There are three possible fixes: > > 1) make async.c use a tail queue. Fixes the bug, but it is too clever > IMHO. > > 2) in tcg_exec, where there is > > if (timer_alarm_pending) { > timer_alarm_pending = 0; > break; > } > > instead check if any bottom half is scheduled. With this change, > after the timers run, if the ptimer's bottom half hadn't run TCG would > not execute code, qemu_bh_calculate_timeout would make main_loop_wait > nonblocking, and the ptimer's bottom half would execute right away. > > BTW after my series the above check will test whether the timer bottom > half is scheduled, so in some sense this could be considered a bugfix > that would be placed _very early_ in the series or could even go in > independently. > > 3) Both of the above. 2 would provide the fix and 1 would provide a > performance improvement by avoiding the useless looping. > >> But more importantly, I think timer dispatch needs to be part of the >> select loop. malc has a git tree that replaces host alarm timers >> with select() timeouts. This has a lot of really nice properties >> like it eliminates the need for signals and EINTR handling. A move >> like this would likely make this more difficult. > > Not necessarily, or at least, splitting qemu-timer.c may make it > marginally more difficult but not having a bottom half for timers. When I think of a main loop, I think of something that provides the following three services 1) fd based events 2) time based events 3) an idle callback The problem we have is that bottom halves aren't quite idle callbacks. They have some really weird properties about the guarantees of when they're executed. At any rate, you really cannot express a time based event as an idle callback because you need to setup the select() timeout based on the next deadline and then dispatch timer event based on selects return. idle functions have none of this ability. > With qemu-timer.c split you'd need something like > > if (rc == 0) host_alarm_handler (); > > after the select loop. I suppose you could basically revert this > patch and move timer handling into host_alarm_handler, but the code > should work independent of this patch. This patch (modulo your other > objection) just adds a level of indirection but doesn't change the > overall structure of the main loop. Yeah, I'm not at all opposed to the qemu-timer.c split. What I would suggest is just to continue calling timers explicitly instead of trying to make them bottoms halves. I've played with this in the past and the regressions are really nasty to track down. If we're going to make such a change, I'd rather spend that regression-busting effort on mtloop or completely changing the bottom half semantics to be idle callbacks. Regards, Anthony Liguori > Paolo