[Qemu-devel] [PATCH v4 0/4] Fix -icount with iothread

[Qemu-devel] [PATCH v4 0/4] Fix -icount with iothread

[Paolo Bonzini]

This series finally fixes -icount with iothread and avoids deadlocks
due to the vm_clock not making progress when the VM is stopped.
The crux of the fix is in patch 1, while patch 2 implements the
"clock warping" that fixes deadlocks in v2.  Clock warping uses
the nanosecond resolution rt_clock timers introduced by my previous
series.

With this in place, patch 3 can revert the previous attempt(s).
Finally, patch 4 makes the icount code clearer by finishing the
bugfix/reorganization of qemu_next_deadline vs. qemu_next_alarm_deadline.

v1->v2:
        reordered patches, renamed qemu_next_deadline

v2->v3:
        introduced warp timer

v3->v4:
        added comments to the code

Paolo Bonzini (4):
  really fix -icount in the iothread case
  enable vm_clock to "warp" in the iothread+icount case
  Revert wrong fixes for -icount in the iothread case
  qemu_next_deadline should not consider host-time timers

 cpus.c        |   13 ++++-
 qemu-common.h |    1 +
 qemu-timer.c  |  169 ++++++++++++++++++++++++++++++++++++++++++++-------------
 qemu-timer.h  |    3 +-
 roms/seabios  |    2 +-
 5 files changed, 146 insertions(+), 42 deletions(-)

-- 
1.7.4

[Qemu-devel] [PATCH v4 1/4] really fix -icount in the iothread case

[Paolo Bonzini]

The correct fix for -icount is to consider the biggest difference
between iothread and non-iothread modes.  In the traditional model,
CPUs run _before_ the iothread calls select (or WaitForMultipleObjects
for Win32).  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 wakes up and
interrupts the CPU.

This is exactly the approach that this patch takes: when cpu_exec_all
returns in -icount mode, and it is because a vm_clock deadline has
been met, it wakes up the iothread to process the timers.  This is
really the "bulk" of fixing icount.

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

diff --git a/cpus.c b/cpus.c
index 41bec7c..cbeac7a 100644
--- a/cpus.c
+++ b/cpus.c
@@ -830,6 +830,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] [PATCH v4 2/4] enable vm_clock to "warp" in the iothread+icount case

[Paolo Bonzini]

The previous patch however is not enough, because if the virtual CPU
goes to sleep waiting for a future timer interrupt to wake it up, qemu
deadlocks.  The timer interrupt never comes because time is driven by
icount, but the vCPU doesn't run any insns.

You could say that VCPUs should never go to sleep in icount
mode if there is a pending vm_clock timer; rather time should
just warp to the next vm_clock event with no sleep ever taking place.
Even better, you can sleep for some time related to the
time left until the next event, to avoid that the warps are too visible
externally; for example, you could be sending network packets continously
instead of every 100ms.

This is what this patch implements.  qemu_clock_warp is called: 1)
whenever a vm_clock timer is adjusted, to ensure the warp_timer is
synchronized; 2) at strategic points in the CPU thread, to make sure
the insn counter is synchronized before the CPU starts running.
In any case, the warp_timer is disabled while the CPU is running,
because the insn counter will then be making progress on its own.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Tested-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
---
 cpus.c        |    8 ++++-
 qemu-common.h |    1 +
 qemu-timer.c  |   94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 qemu-timer.h  |    1 +
 roms/seabios  |    2 +-
 5 files changed, 103 insertions(+), 3 deletions(-)

diff --git a/cpus.c b/cpus.c
index cbeac7a..6a50199 100644
--- a/cpus.c
+++ b/cpus.c
@@ -155,7 +155,7 @@ static bool cpu_thread_is_idle(CPUState *env)
     return true;
 }
 
-static bool all_cpu_threads_idle(void)
+bool all_cpu_threads_idle(void)
 {
     CPUState *env;
 
@@ -739,6 +739,9 @@ static void qemu_tcg_wait_io_event(void)
     CPUState *env;
 
     while (all_cpu_threads_idle()) {
+       /* Start accounting real time to the virtual clock if the CPUs
+          are idle.  */
+        qemu_clock_warp(vm_clock);
         qemu_cond_wait(tcg_halt_cond, &qemu_global_mutex);
     }
 
@@ -1073,6 +1076,9 @@ bool cpu_exec_all(void)
 {
     int r;
 
+    /* Account partial waits to the vm_clock.  */
+    qemu_clock_warp(vm_clock);
+
     if (next_cpu == NULL) {
         next_cpu = first_cpu;
     }
diff --git a/qemu-common.h b/qemu-common.h
index 82e27c1..4f6037b 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -298,6 +298,7 @@ void qemu_notify_event(void);
 void qemu_cpu_kick(void *env);
 void qemu_cpu_kick_self(void);
 int qemu_cpu_is_self(void *env);
+bool all_cpu_threads_idle(void);
 
 /* work queue */
 struct qemu_work_item {
diff --git a/qemu-timer.c b/qemu-timer.c
index 50f1943..4959688 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -153,6 +153,8 @@ void cpu_disable_ticks(void)
 struct QEMUClock {
     int type;
     int enabled;
+
+    QEMUTimer *warp_timer;
 };
 
 struct QEMUTimer {
@@ -386,6 +388,90 @@ void qemu_clock_enable(QEMUClock *clock, int enabled)
     clock->enabled = enabled;
 }
 
+static int64_t vm_clock_warp_start;
+
+static void icount_warp_rt(void *opaque)
+{
+    if (vm_clock_warp_start == -1) {
+        return;
+    }
+
+    if (vm_running) {
+        int64_t clock = qemu_get_clock_ns(rt_clock);
+        int64_t warp_delta = clock - vm_clock_warp_start;
+        if (use_icount == 1) {
+            qemu_icount_bias += warp_delta;
+        } else {
+            /*
+             * In adaptive mode, do not let the vm_clock run too
+             * far ahead of real time.
+             */
+            int64_t cur_time = cpu_get_clock();
+            int64_t cur_icount = qemu_get_clock_ns(vm_clock);
+            int64_t delta = cur_time - cur_icount;
+            qemu_icount_bias += MIN(warp_delta, delta);
+        }
+        if (qemu_timer_expired(active_timers[QEMU_CLOCK_VIRTUAL],
+                               qemu_get_clock_ns(vm_clock))) {
+            qemu_notify_event();
+        }
+    }
+    vm_clock_warp_start = -1;
+}
+
+void qemu_clock_warp(QEMUClock *clock)
+{
+    int64_t deadline;
+
+    if (!clock->warp_timer) {
+        return;
+    }
+
+    /*
+     * There are too many global variables to make the "warp" behavior
+     * applicable to other clocks.  But a clock argument removes the
+     * need for if statements all over the place.
+     */
+    assert(clock == vm_clock);
+
+    /*
+     * If the CPUs have been sleeping, advance the vm_clock timer now.  This
+     * ensures that the deadline for the timer is computed correctly below.
+     * This also makes sure that the insn counter is synchronized before the
+     * CPU starts running, in case the CPU is woken by an event other than
+     * the earliest vm_clock timer.
+     */
+    icount_warp_rt(NULL);
+    if (!all_cpu_threads_idle() || !active_timers[clock->type]) {
+        qemu_del_timer(clock->warp_timer);
+        return;
+    }
+
+    vm_clock_warp_start = qemu_get_clock_ns(rt_clock);
+    deadline = qemu_next_deadline();
+    if (deadline > 0) {
+        /*
+         * Ensure the vm_clock proceeds even when the virtual CPU goes to
+         * sleep.  Otherwise, the CPU might be waiting for a future timer
+         * interrupt to wake it up, but the interrupt never comes because
+         * the vCPU isn't running any insns and thus doesn't advance the
+         * vm_clock.
+         *
+         * An extreme solution for this problem would be to never let VCPUs
+         * sleep in icount mode if there is a pending vm_clock timer; rather
+         * time could just advance to the next vm_clock event.  Instead, we
+         * do stop VCPUs and only advance vm_clock after some "real" time,
+         * (related to the time left until the next event) has passed.  This
+         * rt_clock timer will do this.  This avoids that the warps are too
+         * visible externally---for example, you will not be sending network
+         * packets continously instead of every 100ms.
+         */
+        qemu_mod_timer(clock->warp_timer, vm_clock_warp_start + deadline);
+    } else {
+        qemu_notify_event();
+    }
+}
+
 QEMUTimer *qemu_new_timer(QEMUClock *clock, int scale,
                           QEMUTimerCB *cb, void *opaque)
 {
@@ -454,8 +540,10 @@ static void qemu_mod_timer_ns(QEMUTimer *ts, int64_t expire_time)
             qemu_rearm_alarm_timer(alarm_timer);
         }
         /* Interrupt execution to force deadline recalculation.  */
-        if (use_icount)
+        qemu_clock_warp(ts->clock);
+        if (use_icount) {
             qemu_notify_event();
+        }
     }
 }
 
@@ -576,6 +664,10 @@ void configure_icount(const char *option)
     if (!option)
         return;
 
+#ifdef CONFIG_IOTHREAD
+    vm_clock->warp_timer = qemu_new_timer_ns(rt_clock, icount_warp_rt, NULL);
+#endif
+
     if (strcmp(option, "auto") != 0) {
         icount_time_shift = strtol(option, NULL, 0);
         use_icount = 1;
diff --git a/qemu-timer.h b/qemu-timer.h
index 75d5675..c01bcab 100644
--- a/qemu-timer.h
+++ b/qemu-timer.h
@@ -39,6 +39,7 @@ extern QEMUClock *host_clock;
 
 int64_t qemu_get_clock_ns(QEMUClock *clock);
 void qemu_clock_enable(QEMUClock *clock, int enabled);
+void qemu_clock_warp(QEMUClock *clock);
 
 QEMUTimer *qemu_new_timer(QEMUClock *clock, int scale,
                           QEMUTimerCB *cb, void *opaque);
diff --git a/roms/seabios b/roms/seabios
index cc97564..06d0bdd 160000
--- a/roms/seabios
+++ b/roms/seabios
@@ -1 +1 @@
-Subproject commit cc975646af69f279396d4d5e1379ac6af80ee637
+Subproject commit 06d0bdd9e2e20377b3180e4986b14c8549b393e4
-- 
1.7.4

[Qemu-devel] [PATCH v4 3/4] Revert wrong fixes 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>
Tested-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
---
 qemu-timer.c |   66 +++++++++++++++++++++++++++++++--------------------------
 1 files changed, 36 insertions(+), 30 deletions(-)

diff --git a/qemu-timer.c b/qemu-timer.c
index 4959688..7998f37 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)
@@ -1147,39 +1151,41 @@ 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 v4 4/4] qemu_next_deadline should not consider host-time timers

[Paolo Bonzini]

It is purely for icount-based virtual timers.  And now that we got the
code right, rename the function to clarify the intended scope.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Tested-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
---
 cpus.c       |    4 ++--
 qemu-timer.c |   13 ++++---------
 qemu-timer.h |    2 +-
 3 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/cpus.c b/cpus.c
index 6a50199..1fc34b7 100644
--- a/cpus.c
+++ b/cpus.c
@@ -833,7 +833,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
 
     while (1) {
         cpu_exec_all();
-        if (use_icount && qemu_next_deadline() <= 0) {
+        if (use_icount && qemu_next_icount_deadline() <= 0) {
             qemu_notify_event();
         }
         qemu_tcg_wait_io_event();
@@ -1050,7 +1050,7 @@ static int tcg_cpu_exec(CPUState *env)
         qemu_icount -= (env->icount_decr.u16.low + env->icount_extra);
         env->icount_decr.u16.low = 0;
         env->icount_extra = 0;
-        count = qemu_icount_round (qemu_next_deadline());
+        count = qemu_icount_round(qemu_next_icount_deadline());
         qemu_icount += count;
         decr = (count > 0xffff) ? 0xffff : count;
         count -= decr;
diff --git a/qemu-timer.c b/qemu-timer.c
index 7998f37..b8c0c88 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -452,7 +452,7 @@ void qemu_clock_warp(QEMUClock *clock)
     }
 
     vm_clock_warp_start = qemu_get_clock_ns(rt_clock);
-    deadline = qemu_next_deadline();
+    deadline = qemu_next_icount_deadline();
     if (deadline > 0) {
         /*
          * Ensure the vm_clock proceeds even when the virtual CPU goes to
@@ -765,21 +765,16 @@ static void host_alarm_handler(int host_signum)
     }
 }
 
-int64_t qemu_next_deadline(void)
+int64_t qemu_next_icount_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;
@@ -1169,7 +1164,7 @@ int qemu_calculate_timeout(void)
         } else {
             /* Wait for either IO to occur or the next
                timer event.  */
-            add = qemu_next_deadline();
+            add = qemu_next_icount_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.  */
diff --git a/qemu-timer.h b/qemu-timer.h
index c01bcab..3a9228f 100644
--- a/qemu-timer.h
+++ b/qemu-timer.h
@@ -51,7 +51,7 @@ int qemu_timer_expired(QEMUTimer *timer_head, int64_t current_time);
 
 void qemu_run_all_timers(void);
 int qemu_alarm_pending(void);
-int64_t qemu_next_deadline(void);
+int64_t qemu_next_icount_deadline(void);
 void configure_alarms(char const *opt);
 void configure_icount(const char *option);
 int qemu_calculate_timeout(void);
-- 
1.7.4

Re: [Qemu-devel] [PATCH v4 0/4] Fix -icount with iothread

[Edgar E. Iglesias]

On Wed, Apr 13, 2011 at 10:03:42AM +0200, Paolo Bonzini wrote:
> This series finally fixes -icount with iothread and avoids deadlocks
> due to the vm_clock not making progress when the VM is stopped.
> The crux of the fix is in patch 1, while patch 2 implements the
> "clock warping" that fixes deadlocks in v2.  Clock warping uses
> the nanosecond resolution rt_clock timers introduced by my previous
> series.
> 
> With this in place, patch 3 can revert the previous attempt(s).
> Finally, patch 4 makes the icount code clearer by finishing the
> bugfix/reorganization of qemu_next_deadline vs. qemu_next_alarm_deadline.
> 
> v1->v2:
>         reordered patches, renamed qemu_next_deadline
> 
> v2->v3:
>         introduced warp timer
> 
> v3->v4:
>         added comments to the code
> 
> Paolo Bonzini (4):
>   really fix -icount in the iothread case
>   enable vm_clock to "warp" in the iothread+icount case
>   Revert wrong fixes for -icount in the iothread case
>   qemu_next_deadline should not consider host-time timers
> 
>  cpus.c        |   13 ++++-
>  qemu-common.h |    1 +
>  qemu-timer.c  |  169 ++++++++++++++++++++++++++++++++++++++++++++-------------
>  qemu-timer.h  |    3 +-
>  roms/seabios  |    2 +-
>  5 files changed, 146 insertions(+), 42 deletions(-)

I've applied this series, thanks alot for working on it.

BTW, I removed this hunk from patch nr 2:
diff --git a/roms/seabios b/roms/seabios
index cc97564..06d0bdd 160000
--- a/roms/seabios
+++ b/roms/seabios
@@ -1 +1 @@
-Subproject commit cc975646af69f279396d4d5e1379ac6af80ee637
+Subproject commit 06d0bdd9e2e20377b3180e4986b14c8549b393e4
--

Cheers

Re: [Qemu-devel] [PATCH v4 0/4] Fix -icount with iothread

[Paolo Bonzini]

On 04/15/2011 09:49 AM, Edgar E. Iglesias wrote:
> BTW, I removed this hunk from patch nr 2:
> diff --git a/roms/seabios b/roms/seabios
> index cc97564..06d0bdd 160000
> --- a/roms/seabios
> +++ b/roms/seabios
> @@ -1 +1 @@
> -Subproject commit cc975646af69f279396d4d5e1379ac6af80ee637
> +Subproject commit 06d0bdd9e2e20377b3180e4986b14c8549b393e4

That was correct, thanks.  I updated my submodules and will add a commit 
hook to prevent this from happening in the future.

Paolo