From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Lyw0u-0006Ap-4e for qemu-devel@nongnu.org; Tue, 28 Apr 2009 18:42:56 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Lyw0p-00068a-NZ for qemu-devel@nongnu.org; Tue, 28 Apr 2009 18:42:55 -0400 Received: from [199.232.76.173] (port=57862 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Lyw0p-00068U-BF for qemu-devel@nongnu.org; Tue, 28 Apr 2009 18:42:51 -0400 Received: from mx2.redhat.com ([66.187.237.31]:35399) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Lyw0o-0001xV-NQ for qemu-devel@nongnu.org; Tue, 28 Apr 2009 18:42:51 -0400 Date: Tue, 28 Apr 2009 19:41:49 -0300 From: Marcelo Tosatti Message-ID: <20090428224149.GA15512@amt.cnet> References: <49F7848F.7020708@web.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <49F7848F.7020708@web.de> Subject: [Qemu-devel] Re: [7241] qemu: refactor main_loop (Marcelo Tosatti) List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: qemu-devel@nongnu.org On Wed, Apr 29, 2009 at 12:34:55AM +0200, Jan Kiszka wrote: > Anthony Liguori wrote: > > Revision: 7241 > > http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=7241 > > Author: aliguori > > Date: 2009-04-24 18:03:33 +0000 (Fri, 24 Apr 2009) > > Log Message: > > ----------- > > qemu: refactor main_loop (Marcelo Tosatti) > > > > Break main loop into 3 main functions. > > I suspect this patch comes with a race between SIGALRM and > qemu_calculate_timeout() so that I see occasional freezes of exactly 5 > seconds if the IO thread is disabled. host_alarm_handler writes to the notification fd (via qemu_event_increment), which should cause the select() to exit immediately, even if a pending timer was not taken into account by qemu_calculate_timeout(). But 5 seconds is a good clue :) > I do not yet understand what > happens precisely or if this patch only widens an already existing race > window in the old code, but I'm on it. > > Besides that... > > > > > Signed-off-by: Marcelo Tosatti > > Signed-off-by: Anthony Liguori > > > > Modified Paths: > > -------------- > > trunk/vl.c > > > > Modified: trunk/vl.c > > =================================================================== > > --- trunk/vl.c 2009-04-24 18:03:29 UTC (rev 7240) > > +++ trunk/vl.c 2009-04-24 18:03:33 UTC (rev 7241) > > @@ -273,7 +273,7 @@ > > > > static CPUState *cur_cpu; > > static CPUState *next_cpu; > > -static int event_pending = 1; > > +static int timer_alarm_pending = 1; > > /* Conversion factor from emulated instructions to virtual clock ticks. */ > > static int icount_time_shift; > > /* Arbitrarily pick 1MIPS as the minimum allowable speed. */ > > @@ -1360,7 +1360,7 @@ > > } > > #endif > > } > > - event_pending = 1; > > + timer_alarm_pending = 1; > > qemu_notify_event(); > > } > > } > > @@ -3879,153 +3879,175 @@ > > > > } > > > > -static int main_loop(void) > > +static int qemu_cpu_exec(CPUState *env) > > { > > - int ret, timeout; > > + int ret; > > #ifdef CONFIG_PROFILER > > int64_t ti; > > #endif > > + > > +#ifdef CONFIG_PROFILER > > + ti = profile_getclock(); > > +#endif > > + if (use_icount) { > > + int64_t count; > > + int decr; > > + qemu_icount -= (env->icount_decr.u16.low + env->icount_extra); > > + env->icount_decr.u16.low = 0; > > + env->icount_extra = 0; > > + count = qemu_next_deadline(); > > + count = (count + (1 << icount_time_shift) - 1) > > + >> icount_time_shift; > > + qemu_icount += count; > > + decr = (count > 0xffff) ? 0xffff : count; > > + count -= decr; > > + env->icount_decr.u16.low = decr; > > + env->icount_extra = count; > > + } > > + ret = cpu_exec(env); > > +#ifdef CONFIG_PROFILER > > + qemu_time += profile_getclock() - ti; > > +#endif > > + if (use_icount) { > > + /* Fold pending instructions back into the > > + instruction counter, and clear the interrupt flag. */ > > + qemu_icount -= (env->icount_decr.u16.low > > + + env->icount_extra); > > + env->icount_decr.u32 = 0; > > + env->icount_extra = 0; > > + } > > + return ret; > > +} > > + > > +static int cpu_has_work(CPUState *env) > > ...this naming is suboptimal. There is already cpu_has_work() in > target-*/exec.h which is at least confusing. Please rename. Well its static. What name do you prefer (can't find a better name really). do_cpu_exec?