[Qemu-devel] [PATCH 0/3] really fix -icount with iothread

[Qemu-devel] [PATCH 0/3] really fix -icount with iothread

[Paolo Bonzini]

This is a "real" fix for -icount, real in the sense that it
works in all cases including those that weren't fixed by my
first attempt.

Patch 1 is a sequel to my reorganization of qemu_next_deadline
vs. qemu_next_alarm_deadline.  Patch 2 reverts the past attempts,
patch 3 is the three-line fix.

Paolo Bonzini (3):
  qemu_next_deadline should not consider host-time timers
  Revert wrong fix for -icount in the iothread case
  really fix -icount in the iothread case

 cpus.c       |    3 ++
 qemu-timer.c |   73 +++++++++++++++++++++++++++++----------------------------
 2 files changed, 40 insertions(+), 36 deletions(-)

-- 
1.7.4

[Qemu-devel] [PATCH 1/3] qemu_next_deadline should not consider host-time timers

[Paolo Bonzini]

It is purely for icount-based virtual timers.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-timer.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/qemu-timer.c b/qemu-timer.c
index 88c7b28..06fa507 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -689,16 +689,11 @@ int64_t qemu_next_deadline(void)
     /* To avoid problems with overflow limit this to 2^32.  */
     int64_t delta = INT32_MAX;
 
+    assert(use_icount);
     if (active_timers[QEMU_CLOCK_VIRTUAL]) {
         delta = active_timers[QEMU_CLOCK_VIRTUAL]->expire_time -
                      qemu_get_clock_ns(vm_clock);
     }
-    if (active_timers[QEMU_CLOCK_HOST]) {
-        int64_t hdelta = active_timers[QEMU_CLOCK_HOST]->expire_time -
-                 qemu_get_clock_ns(host_clock);
-        if (hdelta < delta)
-            delta = hdelta;
-    }
 
     if (delta < 0)
         delta = 0;
-- 
1.7.4

[Qemu-devel] [PATCH 2/3] Revert wrong fix for -icount in the iothread case

[Paolo Bonzini]

This reverts commits 225d02cd and c9f7383c.  While some parts of
the latter could be saved, I preferred a smooth, complete revert.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-timer.c |   66 +++++++++++++++++++++++++++++++--------------------------
 1 files changed, 36 insertions(+), 30 deletions(-)

diff --git a/qemu-timer.c b/qemu-timer.c
index 06fa507..4499e0c 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -110,9 +110,12 @@ static int64_t cpu_get_clock(void)
     }
 }
 
+#ifndef CONFIG_IOTHREAD
 static int64_t qemu_icount_delta(void)
 {
-    if (use_icount == 1) {
+    if (!use_icount) {
+        return 5000 * (int64_t) 1000000;
+    } else if (use_icount == 1) {
         /* When not using an adaptive execution frequency
            we tend to get badly out of sync with real time,
            so just delay for a reasonable amount of time.  */
@@ -121,6 +124,7 @@ static int64_t qemu_icount_delta(void)
         return cpu_get_icount() - cpu_get_clock();
     }
 }
+#endif
 
 /* enable cpu_get_ticks() */
 void cpu_enable_ticks(void)
@@ -1069,39 +1073,43 @@ void quit_timers(void)
 
 int qemu_calculate_timeout(void)
 {
+#ifndef CONFIG_IOTHREAD
     int timeout;
-    int64_t add;
-    int64_t delta;
 
-    /* When using icount, making forward progress with qemu_icount when the
-       guest CPU is idle is critical. We only use the static io-thread timeout
-       for non icount runs.  */
-    if (!use_icount || !vm_running) {
-        return 5000;
-    }
-
-    /* Advance virtual time to the next event.  */
-    delta = qemu_icount_delta();
-    if (delta > 0) {
-        /* If virtual time is ahead of real time then just
-           wait for IO.  */
-        timeout = (delta + 999999) / 1000000;
-    } else {
-        /* Wait for either IO to occur or the next
-           timer event.  */
-        add = qemu_next_deadline();
-        /* We advance the timer before checking for IO.
-           Limit the amount we advance so that early IO
-           activity won't get the guest too far ahead.  */
-        if (add > 10000000)
-            add = 10000000;
-        delta += add;
-        qemu_icount += qemu_icount_round (add);
-        timeout = delta / 1000000;
-        if (timeout < 0)
-            timeout = 0;
+    if (!vm_running) {
+        timeout = 5000;
+    } else {
+     /* XXX: use timeout computed from timers */
+        int64_t add;
+        int64_t delta;
+        /* Advance virtual time to the next event.  */
+        delta = qemu_icount_delta();
+        if (delta > 0) {
+            /* If virtual time is ahead of real time then just
+               wait for IO.  */
+            timeout = (delta + 999999) / 1000000;
+        } else {
+            /* Wait for either IO to occur or the next
+               timer event.  */
+            add = qemu_next_deadline();
+            /* We advance the timer before checking for IO.
+               Limit the amount we advance so that early IO
+               activity won't get the guest too far ahead.  */
+            if (add > 10000000) {
+                add = 10000000;
+            }
+            delta += add;
+            qemu_icount += qemu_icount_round(add);
+            timeout = delta / 1000000;
+            if (timeout < 0) {
+                timeout = 0;
+            }
+        }
     }
 
     return timeout;
+#else /* CONFIG_IOTHREAD */
+    return 1000;
+#endif
 }
 
-- 
1.7.4

[Qemu-devel] [PATCH 3/3] really fix -icount in the iothread case

[Paolo Bonzini]

The correct fix for -icount is obvious once you consider the biggest
difference between iothread and non-iothread modes.  In the traditional
model, CPUs run _before_ the iothread calls select.  In the iothread
model, CPUs run while the iothread isn't holding the mutex, i.e. _during_
those same calls.

So, the iothread should always block as long as possible to let
the CPUs run smoothly---the timeout might as well be infinite---and
either the OS or the CPU thread itself will let the iothread know
when something happens.  At this point, the iothread will wake up and
interrupt execution of the emulated CPU(s).

This is exactly the approach that this patch takes.  When a vm_clock
deadline is met, tcg_cpu_exec will stop executing instructions
(count == 0) so that cpu_exec_all will return very soon.  The TCG
thread should then check if this is the case and, if so, wake up
the iothread to process the timers.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpus.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/cpus.c b/cpus.c
index 0f33945..a953bac 100644
--- a/cpus.c
+++ b/cpus.c
@@ -861,6 +861,9 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
 
     while (1) {
         cpu_exec_all();
+        if (use_icount && qemu_next_deadline() <= 0) {
+            qemu_notify_event();
+        }
         qemu_tcg_wait_io_event();
     }
 
-- 
1.7.4

[Qemu-devel] Re: [PATCH 1/3] qemu_next_deadline should not consider host-time timers

[Jan Kiszka]

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

On 2011-03-05 18:14, Paolo Bonzini wrote:
> It is purely for icount-based virtual timers.

How about renaming the function to clarify its scope?

Jan

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qemu-timer.c |    7 +------
>  1 files changed, 1 insertions(+), 6 deletions(-)
> 
> diff --git a/qemu-timer.c b/qemu-timer.c
> index 88c7b28..06fa507 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -689,16 +689,11 @@ int64_t qemu_next_deadline(void)
>      /* To avoid problems with overflow limit this to 2^32.  */
>      int64_t delta = INT32_MAX;
>  
> +    assert(use_icount);
>      if (active_timers[QEMU_CLOCK_VIRTUAL]) {
>          delta = active_timers[QEMU_CLOCK_VIRTUAL]->expire_time -
>                       qemu_get_clock_ns(vm_clock);
>      }
> -    if (active_timers[QEMU_CLOCK_HOST]) {
> -        int64_t hdelta = active_timers[QEMU_CLOCK_HOST]->expire_time -
> -                 qemu_get_clock_ns(host_clock);
> -        if (hdelta < delta)
> -            delta = hdelta;
> -    }
>  
>      if (delta < 0)
>          delta = 0;



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

[Qemu-devel] Re: [PATCH 1/3] qemu_next_deadline should not consider host-time timers

[Paolo Bonzini]

On 03/05/2011 07:07 PM, Jan Kiszka wrote:
> On 2011-03-05 18:14, Paolo Bonzini wrote:
>> It is purely for icount-based virtual timers.
>
> How about renaming the function to clarify its scope?

Will do.

Paolo