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

[Jan Kiszka]

Marcelo Tosatti wrote:
> 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().

Yeah, now I remember. And I always wondered why my strace logs reported
that writing to that file descriptor failed. I should have looked closer
into this immediately... (patch will follow 8) )

> 
> 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 <address@hidden>
>>> Signed-off-by: Anthony Liguori <address@hidden>
>>>
>>> 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? 
> 

Yes, it works, but it doesn't help reading the code. Also: tcg_has_work
- isn't kvm also running through this?

Jan

signature.asc
Description: OpenPGP digital signature