fix halt emulation with icount and CONFIG_IOTHREAD

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 <[email protected]>

Reported-and-tested-by "Edgar E. Iglesias" <[email protected]>

cpus.c +0 -0

index 468544c..21c3eba 100644

cpus.c +0 -0

     }
 
     qemu_mutex_unlock(&qemu_global_mutex);

vl.c +0 -0

index b436952..8ba7e9d 100644

Quoted 13 lines · 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 <[email protected]>

Reported-and-tested-by "Edgar E. Iglesias" <[email protected]>

cpus.c +0 -0

index 468544c..21c3eba 100644

cpus.c static void qemu_tcg_wait_io_event(void) · line 773

Quoted 5 lines

cpus.c +1 -1

@@ -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.

Quoted 11 lines

cpus.c +0 -0

     }
 
     qemu_mutex_unlock(&qemu_global_mutex);

vl.c +0 -0

index b436952..8ba7e9d 100644
@@ -1335,7 +1335,7 @@ void main_loop_wait(int nonblocking)
     if (nonblocking)
         timeout = 0;
     else {

vl.c void main_loop_wait(int nonblocking) · line 1341

Quoted 5 lines

vl.c +1 -1

-        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?

Quoted 37 lines · Jan Kiszka

Quoted 13 lines · 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 <[email protected]>

Reported-and-tested-by "Edgar E. Iglesias" <[email protected]>

cpus.c +0 -0

index 468544c..21c3eba 100644

cpus.c static void qemu_tcg_wait_io_event(void) · line 773

Quoted 5 lines

cpus.c +1 -1

@@ -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.

Quoted 11 lines

cpus.c +0 -0

     }
 
     qemu_mutex_unlock(&qemu_global_mutex);

vl.c +0 -0

index b436952..8ba7e9d 100644
@@ -1335,7 +1335,7 @@ void main_loop_wait(int nonblocking)
     if (nonblocking)
         timeout = 0;
     else {

Quoted 8 lines

vl.c void main_loop_wait(int nonblocking) · line 1341

Quoted 5 lines

vl.c +1 -1

-        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.

Quoted 23 lines · Marcelo Tosatti

Quoted 22 lines · Jan Kiszka

Quoted 13 lines · 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 <[email protected]>

Reported-and-tested-by "Edgar E. Iglesias" <[email protected]>

cpus.c +0 -0

index 468544c..21c3eba 100644

cpus.c static void qemu_tcg_wait_io_event(void) · line 773

Quoted 5 lines

cpus.c +1 -1

@@ -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.

Quoted 3 lines

Quoted 3 lines

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.

Quoted 21 lines

Quoted 12 lines

Quoted 12 lines

cpus.c +0 -0

     }
 
     qemu_mutex_unlock(&qemu_global_mutex);

vl.c +0 -0

index b436952..8ba7e9d 100644
@@ -1335,7 +1335,7 @@ void main_loop_wait(int nonblocking)
     if (nonblocking)
         timeout = 0;
     else {

Quoted 8 lines

vl.c void main_loop_wait(int nonblocking) · line 1341

Quoted 5 lines

vl.c +1 -1

-        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?

Quoted 2 lines

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?

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 <[email protected]>

Reported-and-tested-by "Edgar E. Iglesias" <[email protected]>

cpus.c +0 -0

index 468544c..21c3eba 100644

Note: to be applied to uq/master.

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

Quoted 4 lines · Marcelo Tosatti

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:

vl.c +0 -0

index b436952..7835317 100644

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().

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.

Quoted 2 lines · Jan Kiszka

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.

Quoted 3 lines

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.

Quoted 6 lines · Paolo Bonzini

Quoted 2 lines · Jan Kiszka

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?

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.

Quoted 2 lines

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.

Quoted 9 lines · Jan Kiszka

Quoted 6 lines · Paolo Bonzini

Quoted 2 lines · Jan Kiszka

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.

Quoted 3 lines

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.

Quoted 4 lines

Quoted 2 lines

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

Quoted 4 lines · Marcelo Tosatti

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:

vl.c +3 -1

index b436952..7835317 100644
@@ -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.

It is "-icount N" which is broken and unfortunately your patch does not fix it.

Quoted 37 lines · Paolo Bonzini

Quoted 4 lines · Marcelo Tosatti

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:

vl.c +3 -1

index b436952..7835317 100644
@@ -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.

Quoted 7 lines

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.