[Qemu-devel] [PATCH] fix halt emulation with icount and CONFIG_IOTHREAD

[Qemu-devel] [PATCH] fix halt emulation with icount and CONFIG_IOTHREAD

[Marcelo Tosatti]

Note: to be applied to uq/master.

In icount mode, halt emulation should take into account the nearest event when sleeping.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Reported-and-tested-by: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>

diff --git a/cpus.c b/cpus.c
index 468544c..21c3eba 100644
--- a/cpus.c
+++ b/cpus.c
@@ -770,7 +770,7 @@ static void qemu_tcg_wait_io_event(void)
     CPUState *env;
 
     while (all_cpu_threads_idle()) {
-        qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, 1000);
+        qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, qemu_calculate_timeout());
     }
 
     qemu_mutex_unlock(&qemu_global_mutex);
diff --git a/vl.c b/vl.c
index b436952..8ba7e9d 100644
--- a/vl.c
+++ b/vl.c
@@ -1335,7 +1335,7 @@ void main_loop_wait(int nonblocking)
     if (nonblocking)
         timeout = 0;
     else {
-        timeout = qemu_calculate_timeout();
+        timeout = 1000;
         qemu_bh_update_timeout(&timeout);
     }
 

[Qemu-devel] Re: [PATCH] fix halt emulation with icount and CONFIG_IOTHREAD

[Jan Kiszka]

On 2011-02-15 18:54, Marcelo Tosatti wrote:
> 
> Note: to be applied to uq/master.
> 
> In icount mode, halt emulation should take into account the nearest event when sleeping.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> Reported-and-tested-by: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> 
> diff --git a/cpus.c b/cpus.c
> index 468544c..21c3eba 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -770,7 +770,7 @@ static void qemu_tcg_wait_io_event(void)
>      CPUState *env;
>  
>      while (all_cpu_threads_idle()) {
> -        qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, 1000);
> +        qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, qemu_calculate_timeout());

checkpatch.pl would complain here.

More important: Paolo was proposing patches to eliminate all those fishy
cond_wait timeouts. That's probably the better way to go. The timeouts
only paper over missing signaling.

>      }
>  
>      qemu_mutex_unlock(&qemu_global_mutex);
> diff --git a/vl.c b/vl.c
> index b436952..8ba7e9d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1335,7 +1335,7 @@ void main_loop_wait(int nonblocking)
>      if (nonblocking)
>          timeout = 0;
>      else {
> -        timeout = qemu_calculate_timeout();
> +        timeout = 1000;
>          qemu_bh_update_timeout(&timeout);
>      }
>  

Isn't this path also relevant for !IOTHREAD? What's the impact of this
change for that configuration?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH] fix halt emulation with icount and CONFIG_IOTHREAD

[Marcelo Tosatti]

On Tue, Feb 15, 2011 at 07:58:53PM +0100, Jan Kiszka wrote:
> On 2011-02-15 18:54, Marcelo Tosatti wrote:
> > 
> > Note: to be applied to uq/master.
> > 
> > In icount mode, halt emulation should take into account the nearest event when sleeping.
> > 
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> > Reported-and-tested-by: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> > 
> > diff --git a/cpus.c b/cpus.c
> > index 468544c..21c3eba 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -770,7 +770,7 @@ static void qemu_tcg_wait_io_event(void)
> >      CPUState *env;
> >  
> >      while (all_cpu_threads_idle()) {
> > -        qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, 1000);
> > +        qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, qemu_calculate_timeout());
> 
> checkpatch.pl would complain here.
> 
> More important: Paolo was proposing patches to eliminate all those fishy
> cond_wait timeouts. That's probably the better way to go. The timeouts
> only paper over missing signaling.
>
> >      }
> >  
> >      qemu_mutex_unlock(&qemu_global_mutex);
> > diff --git a/vl.c b/vl.c
> > index b436952..8ba7e9d 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1335,7 +1335,7 @@ void main_loop_wait(int nonblocking)
> >      if (nonblocking)
> >          timeout = 0;
> >      else {
> > -        timeout = qemu_calculate_timeout();
> > +        timeout = 1000;
> >          qemu_bh_update_timeout(&timeout);
> >      }
> >  
> 
> Isn't this path also relevant for !IOTHREAD? What's the impact of this
> change for that configuration?

Timeout changes from 5s to 1s.

[Qemu-devel] Re: [PATCH] fix halt emulation with icount and CONFIG_IOTHREAD

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1892 bytes --]

On 2011-02-15 21:04, Marcelo Tosatti wrote:
> On Tue, Feb 15, 2011 at 07:58:53PM +0100, Jan Kiszka wrote:
>> On 2011-02-15 18:54, Marcelo Tosatti wrote:
>>>
>>> Note: to be applied to uq/master.
>>>
>>> In icount mode, halt emulation should take into account the nearest event when sleeping.
>>>
>>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>> Reported-and-tested-by: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
>>>
>>> diff --git a/cpus.c b/cpus.c
>>> index 468544c..21c3eba 100644
>>> --- a/cpus.c
>>> +++ b/cpus.c
>>> @@ -770,7 +770,7 @@ static void qemu_tcg_wait_io_event(void)
>>>      CPUState *env;
>>>  
>>>      while (all_cpu_threads_idle()) {
>>> -        qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, 1000);
>>> +        qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, qemu_calculate_timeout());
>>
>> checkpatch.pl would complain here.
>>
>> More important: Paolo was proposing patches to eliminate all those fishy
>> cond_wait timeouts. That's probably the better way to go. The timeouts
>> only paper over missing signaling.
>>
>>>      }
>>>  
>>>      qemu_mutex_unlock(&qemu_global_mutex);
>>> diff --git a/vl.c b/vl.c
>>> index b436952..8ba7e9d 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -1335,7 +1335,7 @@ void main_loop_wait(int nonblocking)
>>>      if (nonblocking)
>>>          timeout = 0;
>>>      else {
>>> -        timeout = qemu_calculate_timeout();
>>> +        timeout = 1000;
>>>          qemu_bh_update_timeout(&timeout);
>>>      }
>>>  
>>
>> Isn't this path also relevant for !IOTHREAD? What's the impact of this
>> change for that configuration?
> 
> Timeout changes from 5s to 1s.
> 

... if (!vm_running).

This patch does have side effects on !IOTHREAD. I doubt the above hunk
can be correct.

What kind of timeout is qemu_calculate_timeout returning?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[Qemu-devel] Re: [PATCH] fix halt emulation with icount and CONFIG_IOTHREAD

[Marcelo Tosatti]

On Tue, Feb 15, 2011 at 09:33:00PM +0100, Jan Kiszka wrote:
> On 2011-02-15 21:04, Marcelo Tosatti wrote:
> > On Tue, Feb 15, 2011 at 07:58:53PM +0100, Jan Kiszka wrote:
> >> On 2011-02-15 18:54, Marcelo Tosatti wrote:
> >>>
> >>> Note: to be applied to uq/master.
> >>>
> >>> In icount mode, halt emulation should take into account the nearest event when sleeping.
> >>>
> >>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> >>> Reported-and-tested-by: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> >>>
> >>> diff --git a/cpus.c b/cpus.c
> >>> index 468544c..21c3eba 100644
> >>> --- a/cpus.c
> >>> +++ b/cpus.c
> >>> @@ -770,7 +770,7 @@ static void qemu_tcg_wait_io_event(void)
> >>>      CPUState *env;
> >>>  
> >>>      while (all_cpu_threads_idle()) {
> >>> -        qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, 1000);
> >>> +        qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, qemu_calculate_timeout());
> >>
> >> checkpatch.pl would complain here.
> >>
> >> More important: Paolo was proposing patches to eliminate all those fishy
> >> cond_wait timeouts. That's probably the better way to go. The timeouts
> >> only paper over missing signaling.

With icount VM_TIMER timeouts are converted to realtime. This is what i
understand qemu_calculate_timeout does.

Otherwise, yes, timeouts are papering over missing signaling.

> >>
> >>>      }
> >>>  
> >>>      qemu_mutex_unlock(&qemu_global_mutex);
> >>> diff --git a/vl.c b/vl.c
> >>> index b436952..8ba7e9d 100644
> >>> --- a/vl.c
> >>> +++ b/vl.c
> >>> @@ -1335,7 +1335,7 @@ void main_loop_wait(int nonblocking)
> >>>      if (nonblocking)
> >>>          timeout = 0;
> >>>      else {
> >>> -        timeout = qemu_calculate_timeout();
> >>> +        timeout = 1000;
> >>>          qemu_bh_update_timeout(&timeout);
> >>>      }
> >>>  
> >>
> >> Isn't this path also relevant for !IOTHREAD? What's the impact of this
> >> change for that configuration?
> > 
> > Timeout changes from 5s to 1s.
> > 
> 
> ... if (!vm_running).
> 
> This patch does have side effects on !IOTHREAD. I doubt the above hunk
> can be correct.
> 
> What kind of timeout is qemu_calculate_timeout returning?

You're right.

[Qemu-devel] [PATCH] fix halt emulation with icount and CONFIG_IOTHREAD (v2)

[Marcelo Tosatti]

Note: to be applied to uq/master.

In icount mode, halt emulation should take into account the nearest event when sleeping.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Reported-and-tested-by: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>

diff --git a/cpus.c b/cpus.c
index 468544c..21c3eba 100644
--- a/cpus.c
+++ b/cpus.c
@@ -770,7 +770,7 @@ static void qemu_tcg_wait_io_event(void)
     CPUState *env;
 
     while (all_cpu_threads_idle()) {
-        qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, 1000);
+        qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, qemu_calculate_timeout());
     }
 
     qemu_mutex_unlock(&qemu_global_mutex);

[Qemu-devel] Re: [PATCH] fix halt emulation with icount and CONFIG_IOTHREAD (v2)

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1102 bytes --]

On 2011-02-15 21:56, Marcelo Tosatti wrote:
> Note: to be applied to uq/master.
> 
> In icount mode, halt emulation should take into account the nearest event when sleeping.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> Reported-and-tested-by: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> 
> diff --git a/cpus.c b/cpus.c
> index 468544c..21c3eba 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -770,7 +770,7 @@ static void qemu_tcg_wait_io_event(void)
>      CPUState *env;
>  
>      while (all_cpu_threads_idle()) {
> -        qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, 1000);
> +        qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, qemu_calculate_timeout());
>      }
>  
>      qemu_mutex_unlock(&qemu_global_mutex);

As we agree that timeouts are wrong here, let's check if Paolo's patches
make a difference.

If not, it would still be better to understand the actual issue. After
trying too many voodoo patches myself, I've finally found that mutex
ping pong between iothread and vcpus by starring at a trace in kernelshark.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[Qemu-devel] Re: [PATCH] fix halt emulation with icount and CONFIG_IOTHREAD (v2)

[Paolo Bonzini]

On 02/15/2011 09:56 PM, Marcelo Tosatti wrote:
> Note: to be applied to uq/master.
>
> In icount mode, halt emulation should take into account the nearest
> event when sleeping.

I agree with Jan that this patch is not the best solution, if not incorrect.

However, in the iothread, the main loop can kick the VCPU thread instead 
of running cpu_exec_all like it does in non-iothread mode.  Something 
like this:

diff --git a/vl.c b/vl.c
index b436952..7835317 100644
--- a/vl.c
+++ b/vl.c
@@ -1425,7 +1425,9 @@ static void main_loop(void)
      qemu_main_loop_start();

      for (;;) {
-#ifndef CONFIG_IOTHREAD
+#ifdef CONFIG_IOTHREAD
+        qemu_cpu_kick(first_cpu);
+#else
          nonblocking = cpu_exec_all();
          if (vm_request_pending()) {
              nonblocking = true;

I don't like this 100% because it relies on the fact that there is only 
one TCG execution thread.  In a multithreaded world you would:

1) have each CPU register its own instruction counter;

2) have each CPU register its own QEMU_CLOCK_REALTIME timer based on 
qemu_icount_delta() and arm it just before going to sleep; the timer 
kicks the CPU.

3) remove all icount business from qemu_calculate_timeout.

Item (3) is what makes me prefer my patch above (if it works) to 
Marcelo's.  Marcelo's patch is tying even more qemu_calculate_timeout to 
the icount.  So if anything, a patch tweaking the timedwait like 
Marcelo's should use something based on qemu_icount_delta().

Paolo

[Qemu-devel] Re: [PATCH] fix halt emulation with icount and CONFIG_IOTHREAD (v2)

[Jan Kiszka]

On 2011-02-16 10:32, Paolo Bonzini wrote:
> On 02/15/2011 09:56 PM, Marcelo Tosatti wrote:
>> Note: to be applied to uq/master.
>>
>> In icount mode, halt emulation should take into account the nearest
>> event when sleeping.
> 
> I agree with Jan that this patch is not the best solution, if not
> incorrect.
> 
> However, in the iothread, the main loop can kick the VCPU thread instead
> of running cpu_exec_all like it does in non-iothread mode.  Something
> like this:
> 
> diff --git a/vl.c b/vl.c
> index b436952..7835317 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1425,7 +1425,9 @@ static void main_loop(void)
>      qemu_main_loop_start();
> 
>      for (;;) {
> -#ifndef CONFIG_IOTHREAD
> +#ifdef CONFIG_IOTHREAD
> +        qemu_cpu_kick(first_cpu);
> +#else
>          nonblocking = cpu_exec_all();
>          if (vm_request_pending()) {
>              nonblocking = true;

What should this be good for? The iothread already kicks the vcpu if it
wants to acquire the contended global mutex. And when the vcpu thread is
in halt state, kicking it should change no other state.

> 
> I don't like this 100% because it relies on the fact that there is only
> one TCG execution thread.  In a multithreaded world you would:
> 
> 1) have each CPU register its own instruction counter;
> 
> 2) have each CPU register its own QEMU_CLOCK_REALTIME timer based on
> qemu_icount_delta() and arm it just before going to sleep; the timer
> kicks the CPU.
> 
> 3) remove all icount business from qemu_calculate_timeout.
> 
> Item (3) is what makes me prefer my patch above (if it works) to
> Marcelo's.  Marcelo's patch is tying even more qemu_calculate_timeout to
> the icount.  So if anything, a patch tweaking the timedwait like
> Marcelo's should use something based on qemu_icount_delta().

Really, that idle loop apparently does _nothing_ while
all_cpu_threads_idle is true. Or does the IPI signal handler apply some
magic? I still don't get what is supposed to be fixed in
qemu_tcg_wait_io_event.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH] fix halt emulation with icount and CONFIG_IOTHREAD (v2)

[Paolo Bonzini]

On 02/16/2011 10:46 AM, Jan Kiszka wrote:
> What should this be good for? The iothread already kicks the vcpu if it
> wants to acquire the contended global mutex.

Assuming the VCPU is in the timedwait that Marcelo changed, the global 
mutex is free and the iothread will not kick the VCPU.

> And when the vcpu thread is
> in halt state, kicking it should change no other state.

Kicking the VCPU will start running it, if an interrupt request from the 
devices caused cpu_has_work to become true (and hence 
all_cpu_threads_idle to become false).

So, perhaps the correct fix is to kick the cpu in cpu_interrupt, and all 
I wrote about timeouts and timers is wrong.  My patch would band-aid it.

Paolo

[Qemu-devel] Re: [PATCH] fix halt emulation with icount and CONFIG_IOTHREAD (v2)

[Jan Kiszka]

On 2011-02-16 10:57, Paolo Bonzini wrote:
> On 02/16/2011 10:46 AM, Jan Kiszka wrote:
>> What should this be good for? The iothread already kicks the vcpu if it
>> wants to acquire the contended global mutex.
> 
> Assuming the VCPU is in the timedwait that Marcelo changed, the global 
> mutex is free and the iothread will not kick the VCPU.

Then why should it kick it at all?

> 
>> And when the vcpu thread is
>> in halt state, kicking it should change no other state.
> 
> Kicking the VCPU will start running it, if an interrupt request from the 
> devices caused cpu_has_work to become true (and hence 
> all_cpu_threads_idle to become false).

If we change the halt condition, we should not kick the vcpus but only
signal the condition variable. Actually, I've a patch queued that skips
pointless qemu_thread_signal in qemu_cpu_kick for TCG.

> 
> So, perhaps the correct fix is to kick the cpu in cpu_interrupt, and all 
> I wrote about timeouts and timers is wrong.  My patch would band-aid it.

That's my strong suspect. We really need to understand what goes wrong.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH] fix halt emulation with icount and CONFIG_IOTHREAD (v2)

[Paolo Bonzini]

On 02/16/2011 11:04 AM, Jan Kiszka wrote:
> On 2011-02-16 10:57, Paolo Bonzini wrote:
>> On 02/16/2011 10:46 AM, Jan Kiszka wrote:
>>> What should this be good for? The iothread already kicks the vcpu if it
>>> wants to acquire the contended global mutex.
>>
>> Assuming the VCPU is in the timedwait that Marcelo changed, the global
>> mutex is free and the iothread will not kick the VCPU.
>
> Then why should it kick it at all?

To make it notice something changed in all_cpu_threads_idle---but that's 
wrong, it should have been kicked in cpu_interrupt.

> If we change the halt condition, we should not kick the vcpus but only
> signal the condition variable. Actually, I've a patch queued that skips
> pointless qemu_thread_signal in qemu_cpu_kick for TCG.

Yes, I was kicking just because that's the wrapper that is used to 
signal the condition variable---just like I was kicking in my patches to 
eliminate timedwait.

>> So, perhaps the correct fix is to kick the cpu in cpu_interrupt, and all
>> I wrote about timeouts and timers is wrong.  My patch would band-aid it.
>
> That's my strong suspect. We really need to understand what goes wrong.

I agree on both counts.

Paolo

[Qemu-devel] Re: [PATCH] fix halt emulation with icount and CONFIG_IOTHREAD (v2)

[Jan Kiszka]

On 2011-02-16 11:27, Paolo Bonzini wrote:
> On 02/16/2011 11:04 AM, Jan Kiszka wrote:
>> On 2011-02-16 10:57, Paolo Bonzini wrote:
>>> On 02/16/2011 10:46 AM, Jan Kiszka wrote:
>>>> What should this be good for? The iothread already kicks the vcpu if it
>>>> wants to acquire the contended global mutex.
>>>
>>> Assuming the VCPU is in the timedwait that Marcelo changed, the global
>>> mutex is free and the iothread will not kick the VCPU.
>>
>> Then why should it kick it at all?
> 
> To make it notice something changed in all_cpu_threads_idle---but that's 
> wrong, it should have been kicked in cpu_interrupt.
> 
>> If we change the halt condition, we should not kick the vcpus but only
>> signal the condition variable. Actually, I've a patch queued that skips
>> pointless qemu_thread_signal in qemu_cpu_kick for TCG.
> 
> Yes, I was kicking just because that's the wrapper that is used to 
> signal the condition variable---just like I was kicking in my patches to 
> eliminate timedwait.
> 
>>> So, perhaps the correct fix is to kick the cpu in cpu_interrupt, and all
>>> I wrote about timeouts and timers is wrong.  My patch would band-aid it.
>>
>> That's my strong suspect. We really need to understand what goes wrong.
> 
> I agree on both counts.
> 

FWIW, I've rebased most of your patches on top of my outstanding ones
and pushed them to

git://git.kiszka.org/qemu-kvm.git queues/kvm-upstream

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH] fix halt emulation with icount and CONFIG_IOTHREAD (v2)

[Paolo Bonzini]

On 02/16/2011 11:34 AM, Jan Kiszka wrote:
> FWIW, I've rebased most of your patches on top of my outstanding ones
> and pushed them to
>
> git://git.kiszka.org/qemu-kvm.git queues/kvm-upstream

Yep, I am waiting for Anthony to actually push it.  In the meanwhile I 
have it at git://github.com/bonzini/qemu, branch iothread-win32.

Paolo

[Qemu-devel] Re: [PATCH] fix halt emulation with icount and CONFIG_IOTHREAD (v2)

[Marcelo Tosatti]

On Wed, Feb 16, 2011 at 10:32:25AM +0100, Paolo Bonzini wrote:
> On 02/15/2011 09:56 PM, Marcelo Tosatti wrote:
> >Note: to be applied to uq/master.
> >
> >In icount mode, halt emulation should take into account the nearest
> >event when sleeping.
> 
> I agree with Jan that this patch is not the best solution, if not incorrect.
> 
> However, in the iothread, the main loop can kick the VCPU thread
> instead of running cpu_exec_all like it does in non-iothread mode.
> Something like this:
> 
> diff --git a/vl.c b/vl.c
> index b436952..7835317 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1425,7 +1425,9 @@ static void main_loop(void)
>      qemu_main_loop_start();
> 
>      for (;;) {
> -#ifndef CONFIG_IOTHREAD
> +#ifdef CONFIG_IOTHREAD
> +        qemu_cpu_kick(first_cpu);
> +#else
>          nonblocking = cpu_exec_all();
>          if (vm_request_pending()) {
>              nonblocking = true;
> 
> I don't like this 100% because it relies on the fact that there is
> only one TCG execution thread.  In a multithreaded world you would:
> 
> 1) have each CPU register its own instruction counter;
> 
> 2) have each CPU register its own QEMU_CLOCK_REALTIME timer based on
> qemu_icount_delta() and arm it just before going to sleep; the timer
> kicks the CPU.
> 
> 3) remove all icount business from qemu_calculate_timeout.
> 
> Item (3) is what makes me prefer my patch above (if it works) to
> Marcelo's.  Marcelo's patch is tying even more
> qemu_calculate_timeout to the icount.  So if anything, a patch
> tweaking the timedwait like Marcelo's should use something based on
> qemu_icount_delta().

Yes, using qemu_icount_delta directly in tcg_wait_io_event timedwait 
is explicit (partially the reason for confusion with my patch).

So the reasoning for the patch is:

With icount vm_timer timers expire on virtual CPU time. If a CPU halts,
you cannot expect passage of realtime to trigger vm_timers expiration.

So instead vm_timer expiration is converted to realtime, and used as
halt timeout.

[Qemu-devel] Re: [PATCH] fix halt emulation with icount and CONFIG_IOTHREAD (v2)

[Paolo Bonzini]

On 02/17/2011 04:15 AM, Marcelo Tosatti wrote:
> Yes, using qemu_icount_delta directly in tcg_wait_io_event timedwait
> is explicit (partially the reason for confusion with my patch).
>
> So the reasoning for the patch is:
>
> With icount vm_timer timers expire on virtual CPU time. If a CPU halts,
> you cannot expect passage of realtime to trigger vm_timers expiration.

But if a CPU is halted, all_cpu_threads_idle() will still be true even 
if you signal the condition variable, and you'll be looping in the while 
condition.  That's why I say that

    while (x) {
        cond_timedwait (cond, mutex);
    }

(i.e. without checking the return value of cond_timedwait, and without 
polling something else upon return) is a broken idiom that can only work 
around missing signals/broadcasts.

> So instead vm_timer expiration is converted to realtime, and used as
> halt timeout.

But vm_timer timers are only used by "-icount auto", which works in my 
tests [*].  It is "-icount N" which is broken and unfortunately your 
patch does not fix it.  The CRIS image on qemu.org triggers the watchdog 
(and if I eliminate the watchdog I see that the CPU is hung).

     [*] Actually, it works but doesn't calibrate very well.  It shows
         25 bogomips, sometimes 50, compared to 250 without iothread.

Paolo

[Qemu-devel] Re: [PATCH] fix halt emulation with icount and CONFIG_IOTHREAD (v2)

[Paolo Bonzini]

On 02/17/2011 09:27 AM, Paolo Bonzini wrote:
> It is "-icount N" which is broken and unfortunately your patch does not
> fix it.

The problem is that for "use_icount == 1" qemu_icount_delta always 
returns 0, and this makes no sense in the iothread case.  As soon as the 
delta becomes greater than 10 ms (the maximum adjustment in 
qemu_calculate_timeout) you just keep polling but hardly execute any code.

I'll try to post something in the weekend.

Paolo

[Qemu-devel] Re: [PATCH] fix halt emulation with icount and CONFIG_IOTHREAD (v2)

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2370 bytes --]

On 2011-02-17 04:15, Marcelo Tosatti wrote:
> On Wed, Feb 16, 2011 at 10:32:25AM +0100, Paolo Bonzini wrote:
>> On 02/15/2011 09:56 PM, Marcelo Tosatti wrote:
>>> Note: to be applied to uq/master.
>>>
>>> In icount mode, halt emulation should take into account the nearest
>>> event when sleeping.
>>
>> I agree with Jan that this patch is not the best solution, if not incorrect.
>>
>> However, in the iothread, the main loop can kick the VCPU thread
>> instead of running cpu_exec_all like it does in non-iothread mode.
>> Something like this:
>>
>> diff --git a/vl.c b/vl.c
>> index b436952..7835317 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1425,7 +1425,9 @@ static void main_loop(void)
>>      qemu_main_loop_start();
>>
>>      for (;;) {
>> -#ifndef CONFIG_IOTHREAD
>> +#ifdef CONFIG_IOTHREAD
>> +        qemu_cpu_kick(first_cpu);
>> +#else
>>          nonblocking = cpu_exec_all();
>>          if (vm_request_pending()) {
>>              nonblocking = true;
>>
>> I don't like this 100% because it relies on the fact that there is
>> only one TCG execution thread.  In a multithreaded world you would:
>>
>> 1) have each CPU register its own instruction counter;
>>
>> 2) have each CPU register its own QEMU_CLOCK_REALTIME timer based on
>> qemu_icount_delta() and arm it just before going to sleep; the timer
>> kicks the CPU.
>>
>> 3) remove all icount business from qemu_calculate_timeout.
>>
>> Item (3) is what makes me prefer my patch above (if it works) to
>> Marcelo's.  Marcelo's patch is tying even more
>> qemu_calculate_timeout to the icount.  So if anything, a patch
>> tweaking the timedwait like Marcelo's should use something based on
>> qemu_icount_delta().
> 
> Yes, using qemu_icount_delta directly in tcg_wait_io_event timedwait 
> is explicit (partially the reason for confusion with my patch).
> 
> So the reasoning for the patch is:
> 
> With icount vm_timer timers expire on virtual CPU time. If a CPU halts,
> you cannot expect passage of realtime to trigger vm_timers expiration.
> 
> So instead vm_timer expiration is converted to realtime, and used as
> halt timeout.

The changing the calculation is trying to cure a symptom. A halt with
timeout is already broken, but we fortunately have a patch against that.
Let's shake potential remaining bugs out of *that*.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]