[Qemu-devel] [PATCH 0/2] iothread improvements for Mac OS X

[Qemu-devel] [PATCH 0/2] iothread improvements for Mac OS X

[Paolo Bonzini]

These are two old patches that I never submitted because I didn't really
think they were useful except as cleanups.  Recently, however, Alex Graf
mentioned some problems that Mac OS X has with iothread, and they sounded
to me like they were related to these patches.  And quite surprisingly,
both of them were fixing bugs!

Mac OS X still has problems with iothread according to Alex's testing
(Linux times out in libata, and reactos likewise hangs early on I/O),
but at least the patches fix deadlocks and keep a responsive UI.

Paolo Bonzini (2):
  iothread: replace fair_mutex with a condition variable
  qemu-timer: change unix timer to dynticks

 cpus.c       |   24 +++++++++---------------
 qemu-timer.c |   40 ++++++++++++++++++++++++++++------------
 2 files changed, 37 insertions(+), 27 deletions(-)

-- 
1.7.4.4

[Qemu-devel] [PATCH 1/2] iothread: replace fair_mutex with a condition variable

[Paolo Bonzini]

This conveys the intention better, and scales to more than >1
threads contending the mutex with the iothread (as long as all
of them have a "quiescent point" like the TCG thread has).

Also, on Mac OS X the fair_mutex somehow didn't work as intended
and deadlocked.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Tested-by: Alexander Graf <agraf@suse.de>
---
 cpus.c |   24 +++++++++---------------
 1 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/cpus.c b/cpus.c
index 1fc34b7..d2ba933 100644
--- a/cpus.c
+++ b/cpus.c
@@ -626,7 +626,8 @@ void vm_stop(int reason)
 #else /* CONFIG_IOTHREAD */
 
 QemuMutex qemu_global_mutex;
-static QemuMutex qemu_fair_mutex;
+static QemuCond qemu_io_proceeded_cond;
+static bool iothread_requesting_mutex;
 
 static QemuThread io_thread;
 
@@ -662,7 +663,7 @@ int qemu_init_main_loop(void)
     qemu_cond_init(&qemu_system_cond);
     qemu_cond_init(&qemu_pause_cond);
     qemu_cond_init(&qemu_work_cond);
-    qemu_mutex_init(&qemu_fair_mutex);
+    qemu_cond_init(&qemu_io_proceeded_cond);
     qemu_mutex_init(&qemu_global_mutex);
     qemu_mutex_lock(&qemu_global_mutex);
 
@@ -745,17 +746,9 @@ static void qemu_tcg_wait_io_event(void)
         qemu_cond_wait(tcg_halt_cond, &qemu_global_mutex);
     }
 
-    qemu_mutex_unlock(&qemu_global_mutex);
-
-    /*
-     * Users of qemu_global_mutex can be starved, having no chance
-     * to acquire it since this path will get to it first.
-     * So use another lock to provide fairness.
-     */
-    qemu_mutex_lock(&qemu_fair_mutex);
-    qemu_mutex_unlock(&qemu_fair_mutex);
-
-    qemu_mutex_lock(&qemu_global_mutex);
+    while (iothread_requesting_mutex) {
+        qemu_cond_wait(&qemu_io_proceeded_cond, &qemu_global_mutex);
+    }
 
     for (env = first_cpu; env != NULL; env = env->next_cpu) {
         qemu_wait_io_event_common(env);
@@ -898,12 +891,13 @@ void qemu_mutex_lock_iothread(void)
     if (kvm_enabled()) {
         qemu_mutex_lock(&qemu_global_mutex);
     } else {
-        qemu_mutex_lock(&qemu_fair_mutex);
+        iothread_requesting_mutex = true;
         if (qemu_mutex_trylock(&qemu_global_mutex)) {
             qemu_cpu_kick_thread(first_cpu);
             qemu_mutex_lock(&qemu_global_mutex);
         }
-        qemu_mutex_unlock(&qemu_fair_mutex);
+        iothread_requesting_mutex = false;
+        qemu_cond_broadcast(&qemu_io_proceeded_cond);
     }
 }
 
-- 
1.7.4.4

[Qemu-devel] [PATCH 2/2] qemu-timer: change unix timer to dynticks

[Paolo Bonzini]

A timer that wakes up every millisecond puts a lot of stress on the
iothread.  The large amount of IPIs causes very high context switch
activity, making emulation slow and the UI unusable.  This is by the
way the same reason why the Windows timers were switched to dynticks.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Tested-by: Alexander Graf <agraf@suse.de>
---
 qemu-timer.c |   40 ++++++++++++++++++++++++++++------------
 1 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/qemu-timer.c b/qemu-timer.c
index 4141b6e..5d78cc4 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -227,6 +227,7 @@ static void win32_rearm_timer(struct qemu_alarm_timer *t);
 
 static int unix_start_timer(struct qemu_alarm_timer *t);
 static void unix_stop_timer(struct qemu_alarm_timer *t);
+static void unix_rearm_timer(struct qemu_alarm_timer *t);
 
 #ifdef __linux__
 
@@ -309,7 +310,7 @@ static struct qemu_alarm_timer alarm_timers[] = {
     /* ...otherwise try RTC */
     {"rtc", rtc_start_timer, rtc_stop_timer, NULL},
 #endif
-    {"unix", unix_start_timer, unix_stop_timer, NULL},
+    {"unix", unix_start_timer, unix_stop_timer, unix_rearm_timer},
 #else
     {"mmtimer", mm_start_timer, mm_stop_timer, NULL},
     {"mmtimer2", mm_start_timer, mm_stop_timer, mm_rearm_timer},
@@ -1010,8 +1011,6 @@ static void dynticks_rearm_timer(struct qemu_alarm_timer *t)
 static int unix_start_timer(struct qemu_alarm_timer *t)
 {
     struct sigaction act;
-    struct itimerval itv;
-    int err;
 
     /* timer signal */
     sigfillset(&act.sa_mask);
@@ -1019,18 +1018,35 @@ static int unix_start_timer(struct qemu_alarm_timer *t)
     act.sa_handler = host_alarm_handler;
 
     sigaction(SIGALRM, &act, NULL);
+    return 0;
+}
 
-    itv.it_interval.tv_sec = 0;
-    /* for i386 kernel 2.6 to get 1 ms */
-    itv.it_interval.tv_usec = 999;
-    itv.it_value.tv_sec = 0;
-    itv.it_value.tv_usec = 10 * 1000;
+static void unix_rearm_timer(struct qemu_alarm_timer *t)
+{
+    struct itimerval itv;
+    int64_t nearest_delta_ns = INT64_MAX;
+    int err;
 
-    err = setitimer(ITIMER_REAL, &itv, NULL);
-    if (err)
-        return -1;
+    assert(alarm_has_dynticks(t));
+    if (!active_timers[QEMU_CLOCK_REALTIME] &&
+        !active_timers[QEMU_CLOCK_VIRTUAL] &&
+        !active_timers[QEMU_CLOCK_HOST])
+        return;
 
-    return 0;
+    nearest_delta_ns = qemu_next_alarm_deadline();
+    if (nearest_delta_ns < MIN_TIMER_REARM_NS)
+        nearest_delta_ns = MIN_TIMER_REARM_NS;
+
+    itv.it_interval.tv_sec = 0;
+    itv.it_interval.tv_usec = 0; /* 0 for one-shot timer */
+    itv.it_value.tv_sec =  nearest_delta_ns / 1000000000;
+    itv.it_value.tv_usec = (nearest_delta_ns % 1000000000) / 1000;
+    err = setitimer(ITIMER_REAL, &itv, NULL);
+    if (err) {
+        perror("setitimer");
+        fprintf(stderr, "Internal timer error: aborting\n");
+        exit(1);
+    }
 }
 
 static void unix_stop_timer(struct qemu_alarm_timer *t)
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH 0/2] iothread improvements for Mac OS X

[Paolo Bonzini]

On 06/09/2011 01:10 PM, Paolo Bonzini wrote:
> These are two old patches that I never submitted because I didn't really
> think they were useful except as cleanups.  Recently, however, Alex Graf
> mentioned some problems that Mac OS X has with iothread, and they sounded
> to me like they were related to these patches.  And quite surprisingly,
> both of them were fixing bugs!
>
> Mac OS X still has problems with iothread according to Alex's testing
> (Linux times out in libata, and reactos likewise hangs early on I/O),
> but at least the patches fix deadlocks and keep a responsive UI.
>
> Paolo Bonzini (2):
>    iothread: replace fair_mutex with a condition variable
>    qemu-timer: change unix timer to dynticks
>
>   cpus.c       |   24 +++++++++---------------
>   qemu-timer.c |   40 ++++++++++++++++++++++++++++------------
>   2 files changed, 37 insertions(+), 27 deletions(-)
>

Ping? 1/2 is probably somehow working around the sigmask problem fixed 
by Alexandre (Mac people, can you check?), but it is way more readable 
than the fair_mutex IMNSHO.  I would be surprised if 2/2 also turned out 
to be a workaround, but even if this were the case, it makes CPU usage 
lower.

Paolo

Re: [Qemu-devel] [PATCH 0/2] iothread improvements for Mac OS X

[Alexandre Raymond]

Hi Paolo,

> Ping? 1/2 is probably somehow working around the sigmask problem fixed by
> Alexandre (Mac people, can you check?), but it is way more readable than the
> fair_mutex IMNSHO.  I would be surprised if 2/2 also turned out to be a
> workaround, but even if this were the case, it makes CPU usage lower.

Those patches (I tested them together) do indeed appear to work around
the freezes I was encountering with io-thread enabled on OS X.

I get dma timeout errors when trying to boot Linux, but they don't
seem related to this patch as I got the same errors with the patches I
submitted a couple of weeks ago.

Alexandre

--

Tested-by: Alexandre Raymond <cerbere@gmail.com>

Re: [Qemu-devel] [PATCH 0/2] iothread improvements for Mac OS X

[Anthony Liguori]

On 06/09/2011 06:10 AM, Paolo Bonzini wrote:
> These are two old patches that I never submitted because I didn't really
> think they were useful except as cleanups.  Recently, however, Alex Graf
> mentioned some problems that Mac OS X has with iothread, and they sounded
> to me like they were related to these patches.  And quite surprisingly,
> both of them were fixing bugs!
>
> Mac OS X still has problems with iothread according to Alex's testing
> (Linux times out in libata, and reactos likewise hangs early on I/O),
> but at least the patches fix deadlocks and keep a responsive UI.
>
> Paolo Bonzini (2):
>    iothread: replace fair_mutex with a condition variable
>    qemu-timer: change unix timer to dynticks
>

Applied.  Thanks.

Regards,

Anthony Liguori

>   cpus.c       |   24 +++++++++---------------
>   qemu-timer.c |   40 ++++++++++++++++++++++++++++------------
>   2 files changed, 37 insertions(+), 27 deletions(-)
>