[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [7241] qemu: refactor main_loop (Marcelo Tosatti)
From: |
Jan Kiszka |
Subject: |
[Qemu-devel] Re: [7241] qemu: refactor main_loop (Marcelo Tosatti) |
Date: |
Wed, 29 Apr 2009 00:55:07 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 |
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