[Qemu-devel] Re: [7241] qemu: refactor main_loop (Marcelo Tosatti)

[Marcelo Tosatti <mtosatti@redhat.com>]

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 <mtosatti@redhat.com>
> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > 
> > 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?