[Qemu-devel] [PATCH 0/7] Win32 queue, part 1

[Qemu-devel] [PATCH 0/7] Win32 queue, part 1

[Paolo Bonzini]

Here is the first part of my Win32 queue.  With these patches, VNC
threads should work and only IPI is missing for iothread to work
as well.

Paolo Bonzini (7):
  unlock iothread during WaitForMultipleObjects
  implement win32 dynticks timer
  use win32 timer queues
  add win32 qemu-thread implementation
  include qemu-thread.h early
  add assertions on the owner of a QemuMutex
  remove CONFIG_THREAD

 Makefile.objs                        |    4 +-
 configure                            |    2 -
 cpus.c                               |    3 +-
 os-win32.c                           |    2 +
 qemu-thread.c => qemu-thread-posix.c |   20 +++-
 qemu-thread-posix.h                  |   19 +++
 qemu-thread-win32.c                  |  272 ++++++++++++++++++++++++++++++++++
 qemu-thread-win32.h                  |   22 +++
 qemu-thread.h                        |   27 ++--
 qemu-timer.c                         |   88 +++++------
 10 files changed, 387 insertions(+), 72 deletions(-)
 rename qemu-thread.c => qemu-thread-posix.c (86%)
 create mode 100644 qemu-thread-posix.h
 create mode 100644 qemu-thread-win32.c
 create mode 100644 qemu-thread-win32.h

-- 
1.7.3.5

[Qemu-devel] [PATCH 1/7] unlock iothread during WaitForMultipleObjects

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: Stefan Weil <weil@mail.berlios.de>
Cc: Blue Swirl <blauwirbel@gmail.com>
---
 os-win32.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/os-win32.c b/os-win32.c
index 566d5e9..d907c59 100644
--- a/os-win32.c
+++ b/os-win32.c
@@ -140,7 +140,9 @@ void os_host_main_loop_wait(int *timeout)
         int err;
         WaitObjects *w = &wait_objects;
 
+        qemu_mutex_unlock_iothread();
         ret = WaitForMultipleObjects(w->num, w->events, FALSE, *timeout);
+        qemu_mutex_lock_iothread();
         if (WAIT_OBJECT_0 + 0 <= ret && ret <= WAIT_OBJECT_0 + w->num - 1) {
             if (w->func[ret - WAIT_OBJECT_0])
                 w->func[ret - WAIT_OBJECT_0](w->opaque[ret - WAIT_OBJECT_0]);
-- 
1.7.3.5

[Qemu-devel] [PATCH 2/7] implement win32 dynticks timer

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: Stefan Weil <weil@mail.berlios.de>
Cc: Blue Swirl <blauwirbel@gmail.com>
---
 qemu-timer.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/qemu-timer.c b/qemu-timer.c
index b0db780..42960de 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -1006,6 +1006,7 @@ static void win32_stop_timer(struct qemu_alarm_timer *t)
 static void win32_rearm_timer(struct qemu_alarm_timer *t)
 {
     struct qemu_alarm_win32 *data = t->priv;
+    int nearest_delta_ms;
 
     assert(alarm_has_dynticks(t));
     if (!active_timers[QEMU_CLOCK_REALTIME] &&
@@ -1015,7 +1016,10 @@ static void win32_rearm_timer(struct qemu_alarm_timer *t)
 
     timeKillEvent(data->timerId);
 
-    data->timerId = timeSetEvent(1,
+    nearest_delta_ms = (qemu_next_alarm_deadline() + 999999) / 1000000;
+    if (nearest_delta_ms < 1)
+	nearest_delta_ms = 1;
+    data->timerId = timeSetEvent(nearest_delta_ms,
                         data->period,
                         host_alarm_handler,
                         (DWORD)t,
-- 
1.7.3.5

[Qemu-devel] [PATCH 3/7] use win32 timer queues

[Paolo Bonzini]

Multimedia timers are only useful for compatibility with Windows NT 4.0
and earlier.  Plus, the implementation in Wine is extremely heavyweight.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: Stefan Weil <weil@mail.berlios.de>
Cc: Blue Swirl <blauwirbel@gmail.com>
---
 qemu-timer.c |   86 +++++++++++++++++++++++----------------------------------
 1 files changed, 35 insertions(+), 51 deletions(-)

diff --git a/qemu-timer.c b/qemu-timer.c
index 42960de..a2bac99 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -202,11 +202,6 @@ static void qemu_rearm_alarm_timer(struct qemu_alarm_timer *t)
 
 #ifdef _WIN32
 
-struct qemu_alarm_win32 {
-    MMRESULT timerId;
-    unsigned int period;
-} alarm_win32_data = {0, 0};
-
 static int win32_start_timer(struct qemu_alarm_timer *t);
 static void win32_stop_timer(struct qemu_alarm_timer *t);
 static void win32_rearm_timer(struct qemu_alarm_timer *t);
@@ -300,9 +295,9 @@ static struct qemu_alarm_timer alarm_timers[] = {
     {"unix", unix_start_timer, unix_stop_timer, NULL, NULL},
 #else
     {"dynticks", win32_start_timer,
-     win32_stop_timer, win32_rearm_timer, &alarm_win32_data},
+     win32_stop_timer, win32_rearm_timer, NULL},
     {"win32", win32_start_timer,
-     win32_stop_timer, NULL, &alarm_win32_data},
+     win32_stop_timer, NULL, NULL},
 #endif
     {NULL, }
 };
@@ -638,9 +633,7 @@ void qemu_run_all_timers(void)
 static int64_t qemu_next_alarm_deadline(void);
 
 #ifdef _WIN32
-static void CALLBACK host_alarm_handler(UINT uTimerID, UINT uMsg,
-                                        DWORD_PTR dwUser, DWORD_PTR dw1,
-                                        DWORD_PTR dw2)
+static void CALLBACK host_alarm_handler(PVOID lpParam, BOOLEAN unused)
 #else
 static void host_alarm_handler(int host_signum)
 #endif
@@ -963,50 +956,45 @@ static void unix_stop_timer(struct qemu_alarm_timer *t)
 
 static int win32_start_timer(struct qemu_alarm_timer *t)
 {
-    TIMECAPS tc;
-    struct qemu_alarm_win32 *data = t->priv;
-    UINT flags;
-
-    memset(&tc, 0, sizeof(tc));
-    timeGetDevCaps(&tc, sizeof(tc));
-
-    data->period = tc.wPeriodMin;
-    timeBeginPeriod(data->period);
-
-    flags = TIME_CALLBACK_FUNCTION;
-    if (alarm_has_dynticks(t))
-        flags |= TIME_ONESHOT;
-    else
-        flags |= TIME_PERIODIC;
-
-    data->timerId = timeSetEvent(1,         // interval (ms)
-                        data->period,       // resolution
-                        host_alarm_handler, // function
-                        (DWORD)t,           // parameter
-                        flags);
-
-    if (!data->timerId) {
+    HANDLE hTimer;
+    BOOLEAN success;
+
+    /* If you call ChangeTimerQueueTimer on a one-shot timer (its period
+       is zero) that has already expired, the timer is not updated.  Since
+       creating a new timer is relatively expensive, set a bogus one-hour
+       interval in the dynticks case.  */
+    success = CreateTimerQueueTimer(&hTimer,
+			  NULL,
+			  host_alarm_handler,
+			  t,
+			  1,
+			  alarm_has_dynticks(t) ? 3600000 : 1,
+			  WT_EXECUTEINTIMERTHREAD);
+
+    if (!success) {
         fprintf(stderr, "Failed to initialize win32 alarm timer: %ld\n",
                 GetLastError());
-        timeEndPeriod(data->period);
         return -1;
     }
 
+    t->priv = (PVOID) hTimer;
     return 0;
 }
 
 static void win32_stop_timer(struct qemu_alarm_timer *t)
 {
-    struct qemu_alarm_win32 *data = t->priv;
+    HANDLE hTimer = t->priv;
 
-    timeKillEvent(data->timerId);
-    timeEndPeriod(data->period);
+    if (hTimer) {
+        DeleteTimerQueueTimer(NULL, hTimer, NULL);
+    }
 }
 
 static void win32_rearm_timer(struct qemu_alarm_timer *t)
 {
-    struct qemu_alarm_win32 *data = t->priv;
+    HANDLE hTimer = t->priv;
     int nearest_delta_ms;
+    BOOLEAN success;
 
     assert(alarm_has_dynticks(t));
     if (!active_timers[QEMU_CLOCK_REALTIME] &&
@@ -1014,24 +1002,20 @@ static void win32_rearm_timer(struct qemu_alarm_timer *t)
         !active_timers[QEMU_CLOCK_HOST])
         return;
 
-    timeKillEvent(data->timerId);
-
     nearest_delta_ms = (qemu_next_alarm_deadline() + 999999) / 1000000;
     if (nearest_delta_ms < 1)
 	nearest_delta_ms = 1;
-    data->timerId = timeSetEvent(nearest_delta_ms,
-                        data->period,
-                        host_alarm_handler,
-                        (DWORD)t,
-                        TIME_ONESHOT | TIME_CALLBACK_FUNCTION);
-
-    if (!data->timerId) {
-        fprintf(stderr, "Failed to re-arm win32 alarm timer %ld\n",
-                GetLastError());
+    success = ChangeTimerQueueTimer(NULL,
+				    hTimer,
+				    nearest_delta_ms,
+				    3600000);
 
-        timeEndPeriod(data->period);
-        exit(1);
+    if (!success) {
+        fprintf(stderr, "Failed to rearm win32 alarm timer: %ld\n",
+                GetLastError());
+        exit(-1);
     }
+
 }
 
 #endif /* _WIN32 */
-- 
1.7.3.5

[Qemu-devel] [PATCH 4/7] add win32 qemu-thread implementation

[Paolo Bonzini]

For now, qemu_cond_timedwait and qemu_mutex_timedlock are left as
POSIX-only functions.  They can be removed later, once the patches
that remove their uses are in.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: Stefan Weil <weil@mail.berlios.de>
Cc: Blue Swirl <blauwirbel@gmail.com>
---
 Makefile.objs                        |    4 +-
 qemu-thread.c => qemu-thread-posix.c |    0
 qemu-thread-posix.h                  |   18 +++
 qemu-thread-win32.c                  |  272 ++++++++++++++++++++++++++++++++++
 qemu-thread-win32.h                  |   22 +++
 qemu-thread.h                        |   27 ++--
 6 files changed, 326 insertions(+), 17 deletions(-)
 rename qemu-thread.c => qemu-thread-posix.c (100%)
 create mode 100644 qemu-thread-posix.h
 create mode 100644 qemu-thread-win32.c
 create mode 100644 qemu-thread-win32.h

diff --git a/Makefile.objs b/Makefile.objs
index 353b1a8..19c31fc 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -140,8 +140,8 @@ endif
 common-obj-y += $(addprefix ui/, $(ui-obj-y))
 
 common-obj-y += iov.o acl.o
-common-obj-$(CONFIG_THREAD) += qemu-thread.o
-common-obj-$(CONFIG_IOTHREAD) += compatfd.o
+common-obj-$(CONFIG_POSIX) += qemu-thread-posix.o compatfd.o
+common-obj-$(CONFIG_WIN32) += qemu-thread-win32.o
 common-obj-y += notify.o event_notifier.o
 common-obj-y += qemu-timer.o qemu-timer-common.o
 
diff --git a/qemu-thread.c b/qemu-thread-posix.c
similarity index 100%
rename from qemu-thread.c
rename to qemu-thread-posix.c
diff --git a/qemu-thread-posix.h b/qemu-thread-posix.h
new file mode 100644
index 0000000..7af371c
--- /dev/null
+++ b/qemu-thread-posix.h
@@ -0,0 +1,18 @@
+#ifndef __QEMU_THREAD_POSIX_H
+#define __QEMU_THREAD_POSIX_H 1
+#include "pthread.h"
+
+struct QemuMutex {
+    pthread_mutex_t lock;
+};
+
+struct QemuCond {
+    pthread_cond_t cond;
+};
+
+struct QemuThread {
+    pthread_t thread;
+};
+
+void qemu_thread_signal(QemuThread *thread, int sig);
+#endif
diff --git a/qemu-thread-win32.c b/qemu-thread-win32.c
new file mode 100644
index 0000000..0465a9a
--- /dev/null
+++ b/qemu-thread-win32.c
@@ -0,0 +1,272 @@
+/*
+ * Win32 implementation for mutex/cond/thread functions
+ *
+ * Copyright Red Hat, Inc. 2010
+ *
+ * Author:
+ *  Paolo Bonzini <pbonzini@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+#include "qemu-common.h"
+#include "qemu-thread.h"
+#include <process.h>
+#include <assert.h>
+#include <limits.h>
+
+static void error_exit(int err, const char *msg)
+{
+    char *pstr;
+
+    FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_ALLOCATE_BUFFER,
+                  NULL, err, 0, (LPTSTR) &pstr, 2, NULL);
+    fprintf(stderr, "qemu: %s: %s\n", msg, pstr);
+    LocalFree(pstr);
+    exit(1);
+}
+
+void qemu_mutex_init(QemuMutex *mutex)
+{
+    mutex->owner = 0;
+    InitializeCriticalSection(&mutex->lock);
+}
+
+void qemu_mutex_lock(QemuMutex *mutex)
+{
+    EnterCriticalSection(&mutex->lock);
+
+    /* Win32 CRITICAL_SECTIONs are recursive.  Assert that we're not
+     * using them as such.
+     */
+    assert (mutex->owner == 0);
+    mutex->owner = GetCurrentThreadId();
+}
+
+int qemu_mutex_trylock(QemuMutex *mutex)
+{
+    int owned;
+
+    owned = TryEnterCriticalSection(&mutex->lock);
+    if (owned) {
+        assert (mutex->owner == 0);
+        mutex->owner = GetCurrentThreadId();
+    }
+    return !owned;
+}
+
+void qemu_mutex_unlock(QemuMutex *mutex)
+{
+    assert (mutex->owner == GetCurrentThreadId());
+    mutex->owner = 0;
+    LeaveCriticalSection(&mutex->lock);
+}
+
+void qemu_cond_init(QemuCond *cond)
+{
+    cond->waiters = 0;
+    cond->was_broadcast = 0;
+
+    cond->sema = CreateSemaphore(NULL, 0, LONG_MAX, NULL);
+    if (!cond->sema) {
+        error_exit(GetLastError(), __func__);
+    }
+    cond->continue_broadcast = CreateEvent(NULL,    /* security */
+                                           FALSE,   /* auto-reset */
+                                           FALSE,   /* not signaled */
+                                           NULL);   /* name */
+    if (!cond->continue_broadcast) {
+        error_exit(GetLastError(), __func__);
+    }
+}
+
+void qemu_cond_signal(QemuCond *cond)
+{
+    /*
+     * Signal only when there are waiters.  cond->waiters is
+     * incremented by pthread_cond_wait under the external lock,
+     * so we are safe about that.
+     *
+     * Waiting threads decrement it outside the external lock, but
+     * only if another thread is executing pthread_cond_broadcast and
+     * has the mutex.  So, it also cannot be decremented concurrently
+     * with this particular access.
+     */
+    if (cond->waiters > 0) {
+        cond->waiters--;
+        if (!ReleaseSemaphore(cond->sema, 1, NULL)) {
+            error_exit(GetLastError(), __func__);
+        }
+    }
+}
+
+void qemu_cond_broadcast(QemuCond *cond)
+{
+    BOOLEAN result;
+    /*
+     * As in pthread_cond_signal, access to cond->waiters and
+     * cond->was_broadcast is locked via the external mutex.
+     */
+    if (cond->waiters == 0) {
+        return;
+    }
+
+    /*
+     * As an optimization, when there is exactly one waiter
+     * broadcast is the same as signal.
+     */
+    if (cond->waiters == 1) {
+        qemu_cond_signal(cond);
+        return;
+    }
+
+    cond->was_broadcast = 1;
+    result = ReleaseSemaphore(cond->sema, cond->waiters, NULL);
+    if (!result) {
+        error_exit(GetLastError(), __func__);
+    }
+
+    /*
+     * At this point all waiters continue. Each one takes its
+     * slice of the semaphore. Now it's our turn to wait: Since
+     * the external mutex is held, no thread can leave cond_wait,
+     * yet. For this reason, we can be sure that no thread gets
+     * a chance to eat *more* than one slice. OTOH, it means
+     * that the last waiter must send us a wake-up.
+     */
+    WaitForSingleObject(cond->continue_broadcast, INFINITE);
+    cond->was_broadcast = 0;
+}
+
+void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
+{
+    /*
+     * This access is protected under the mutex.
+     */
+    cond->waiters++;
+
+    /*
+     * Unlock external mutex and wait for signal.
+     * NOTE: we've held mutex locked long enough to increment
+     * waiters count above, so there's no problem with
+     * leaving mutex unlocked before we wait on semaphore.
+     */
+    qemu_mutex_unlock(mutex);
+    WaitForSingleObject(cond->sema, INFINITE);
+
+    /* If the condvar was broadcast, waiters must rendez-vous with
+     * the broadcasting thread and let it continue.  For cond_signal
+     * we do not have to do that because only one signal was added
+     * to the semaphore (also, cond_signal will decrement num_waiters
+     * itself).
+     *
+     * Yes, this has heavy contention and triggers thundering herd.
+     * So goes life.
+     */
+    if (cond->was_broadcast) {
+        /*
+         * Decrease waiters count.  The mutex is not taken, so we have
+         * to do this atomically.
+         *
+         * cond_broadcast was issued while mutex was held, so all
+         * waiters contend for the mutex at the end of this function
+         * until the broadcasting thread relinquishes it.  To ensure
+         * each waiter consumes exactly one slice of the semaphore,
+         * the broadcasting thread stops until it is told by the last
+         * waiter that it can go on.
+         */
+        if (InterlockedDecrement(&cond->waiters) == 0) {
+            SetEvent(cond->continue_broadcast);
+        }
+    }
+    /* lock external mutex again */
+    qemu_mutex_lock(mutex);
+}
+
+struct qemu_thread_data {
+    QemuThread *thread;
+    void *(*start_routine)(void*);
+    void *arg;
+};
+
+static int qemu_thread_tls_index = TLS_OUT_OF_INDEXES;
+
+static unsigned __stdcall win32_start_routine(void *arg)
+{
+    struct qemu_thread_data data = *(struct qemu_thread_data *) arg;
+    QemuThread *thread = data.thread;
+
+    free(arg);
+    TlsSetValue(qemu_thread_tls_index, thread);
+
+    /*
+     * Use DuplicateHandle instead of assigning thread->thread in the
+     * creating thread to avoid races.  It's simpler this way than with
+     * synchronization.
+     */
+    DuplicateHandle(GetCurrentProcess(), GetCurrentThread(),
+                    GetCurrentProcess(), &thread->thread,
+                    0, FALSE, DUPLICATE_SAME_ACCESS);
+
+    qemu_thread_exit(data.start_routine(data.arg));
+}
+
+void qemu_thread_exit(void *arg)
+{
+    QemuThread *thread = TlsGetValue(qemu_thread_tls_index);
+    thread->ret = arg;
+    CloseHandle(thread->thread);
+    thread->thread = NULL;
+    ExitThread(0);
+}
+
+static inline void qemu_thread_init(void)
+{
+    if (qemu_thread_tls_index == TLS_OUT_OF_INDEXES) {
+        qemu_thread_tls_index = TlsAlloc();
+        if (qemu_thread_tls_index == TLS_OUT_OF_INDEXES) {
+            error_exit(ERROR_NO_SYSTEM_RESOURCES, __func__);
+        }
+    }
+}
+
+
+void qemu_thread_create(QemuThread *thread,
+                       void *(*start_routine)(void*),
+                       void *arg)
+{
+    HANDLE hThread;
+
+    struct qemu_thread_data *data;
+    qemu_thread_init ();
+    data = qemu_malloc(sizeof *data);
+    data->thread = thread;
+    data->start_routine = start_routine;
+    data->arg = arg;
+
+    hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
+                                      data, 0, NULL);
+    if (!hThread) {
+        error_exit(GetLastError(), __func__);
+    }
+    CloseHandle(hThread);
+}
+
+void qemu_thread_self(QemuThread *thread)
+{
+    if (!thread->thread) {
+        /* In the main thread of the process.  Initialize the QemuThread
+           pointer in TLS, and use the dummy GetCurrentThread handle as
+           the identifier for qemu_thread_is_current.  */
+        qemu_thread_init ();
+        TlsSetValue(qemu_thread_tls_index, thread);
+        thread->thread = GetCurrentThread();
+    }
+}
+
+int qemu_thread_is_current(QemuThread *thread)
+{
+    QemuThread *this_thread = TlsGetValue(qemu_thread_tls_index);
+    return this_thread->thread == thread->thread;
+}
diff --git a/qemu-thread-win32.h b/qemu-thread-win32.h
new file mode 100644
index 0000000..d7d7d81
--- /dev/null
+++ b/qemu-thread-win32.h
@@ -0,0 +1,22 @@
+#ifndef __QEMU_THREAD_WIN32_H
+#define __QEMU_THREAD_WIN32_H 1
+#include "windows.h"
+
+struct QemuMutex {
+    CRITICAL_SECTION lock;
+    LONG owner;
+};
+
+struct QemuCond {
+    LONG waiters;
+    int was_broadcast;
+    HANDLE sema;
+    HANDLE continue_broadcast;
+};
+
+struct QemuThread {
+    HANDLE thread;
+    void *ret;
+};
+
+#endif
diff --git a/qemu-thread.h b/qemu-thread.h
index 19bb30c..a088eba 100644
--- a/qemu-thread.h
+++ b/qemu-thread.h
@@ -1,24 +1,16 @@
 #ifndef __QEMU_THREAD_H
 #define __QEMU_THREAD_H 1
-#include "semaphore.h"
-#include "pthread.h"
-
-struct QemuMutex {
-    pthread_mutex_t lock;
-};
-
-struct QemuCond {
-    pthread_cond_t cond;
-};
-
-struct QemuThread {
-    pthread_t thread;
-};
 
 typedef struct QemuMutex QemuMutex;
 typedef struct QemuCond QemuCond;
 typedef struct QemuThread QemuThread;
 
+#ifdef _WIN32
+#include "qemu-thread-win32.h"
+#else
+#include "qemu-thread-posix.h"
+#endif
+
 void qemu_mutex_init(QemuMutex *mutex);
 void qemu_mutex_destroy(QemuMutex *mutex);
 void qemu_mutex_lock(QemuMutex *mutex);
@@ -28,6 +20,12 @@ void qemu_mutex_unlock(QemuMutex *mutex);
 
 void qemu_cond_init(QemuCond *cond);
 void qemu_cond_destroy(QemuCond *cond);
+
+/*
+ * IMPORTANT: The implementation does not guarantee that qemu_cond_signal
+ * and qemu_cond_broadcast can be called except while the same mutex is
+ * held as in the corresponding qemu_cond_wait calls!
+ */
 void qemu_cond_signal(QemuCond *cond);
 void qemu_cond_broadcast(QemuCond *cond);
 void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex);
@@ -36,7 +34,6 @@ int qemu_cond_timedwait(QemuCond *cond, QemuMutex *mutex, uint64_t msecs);
 void qemu_thread_create(QemuThread *thread,
                        void *(*start_routine)(void*),
                        void *arg);
-void qemu_thread_signal(QemuThread *thread, int sig);
 void qemu_thread_self(QemuThread *thread);
 int qemu_thread_equal(QemuThread *thread1, QemuThread *thread2);
 void qemu_thread_exit(void *retval);
-- 
1.7.3.5

[Qemu-devel] [PATCH 5/7] include qemu-thread.h early

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: Stefan Weil <weil@mail.berlios.de>
Cc: Blue Swirl <blauwirbel@gmail.com>
---
 cpus.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/cpus.c b/cpus.c
index 4c9928e..68b3fcb 100644
--- a/cpus.c
+++ b/cpus.c
@@ -32,6 +32,7 @@
 #include "kvm.h"
 #include "exec-all.h"
 
+#include "qemu-thread.h"
 #include "cpus.h"
 #include "compatfd.h"
 #ifdef CONFIG_LINUX
@@ -319,8 +320,6 @@ void vm_stop(int reason)
 
 #else /* CONFIG_IOTHREAD */
 
-#include "qemu-thread.h"
-
 QemuMutex qemu_global_mutex;
 static QemuMutex qemu_fair_mutex;
 
-- 
1.7.3.5

[Qemu-devel] [PATCH 6/7] add assertions on the owner of a QemuMutex

[Paolo Bonzini]

These are already present in the Win32 implementation, add them to
the pthread wrappers as well.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: Stefan Weil <weil@mail.berlios.de>
Cc: Blue Swirl <blauwirbel@gmail.com>
---
 qemu-thread-posix.c |   20 +++++++++++++++++++-
 qemu-thread-posix.h |    1 +
 2 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/qemu-thread-posix.c b/qemu-thread-posix.c
index fbc78fe..e6cafd9 100644
--- a/qemu-thread-posix.c
+++ b/qemu-thread-posix.c
@@ -16,9 +16,12 @@
 #include <time.h>
 #include <signal.h>
 #include <stdint.h>
+#include <assert.h>
 #include <string.h>
 #include "qemu-thread.h"
 
+static pthread_t pthread_null;
+
 static void error_exit(int err, const char *msg)
 {
     fprintf(stderr, "qemu: %s: %s\n", msg, strerror(err));
@@ -29,6 +32,7 @@ void qemu_mutex_init(QemuMutex *mutex)
 {
     int err;
 
+    mutex->owner = pthread_null;
     err = pthread_mutex_init(&mutex->lock, NULL);
     if (err)
         error_exit(err, __func__);
@@ -48,13 +52,22 @@ void qemu_mutex_lock(QemuMutex *mutex)
     int err;
 
     err = pthread_mutex_lock(&mutex->lock);
+    assert (pthread_equal(mutex->owner, pthread_null));
+    mutex->owner = pthread_self();
     if (err)
         error_exit(err, __func__);
 }
 
 int qemu_mutex_trylock(QemuMutex *mutex)
 {
-    return pthread_mutex_trylock(&mutex->lock);
+    int err;
+    err = pthread_mutex_trylock(&mutex->lock);
+    if (err == 0) {
+        assert (pthread_equal(mutex->owner, pthread_null));
+        mutex->owner = pthread_self();
+    }
+
+    return !!err;
 }
 
 static void timespec_add_ms(struct timespec *ts, uint64_t msecs)
@@ -85,6 +98,8 @@ void qemu_mutex_unlock(QemuMutex *mutex)
 {
     int err;
 
+    assert (pthread_equal(mutex->owner, pthread_self()));
+    mutex->owner = pthread_null;
     err = pthread_mutex_unlock(&mutex->lock);
     if (err)
         error_exit(err, __func__);
@@ -130,7 +145,10 @@ void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
 {
     int err;
 
+    assert (pthread_equal(mutex->owner, pthread_self()));
+    mutex->owner = pthread_null;
     err = pthread_cond_wait(&cond->cond, &mutex->lock);
+    mutex->owner = pthread_self();
     if (err)
         error_exit(err, __func__);
 }
diff --git a/qemu-thread-posix.h b/qemu-thread-posix.h
index 7af371c..11978db 100644
--- a/qemu-thread-posix.h
+++ b/qemu-thread-posix.h
@@ -4,6 +4,7 @@
 
 struct QemuMutex {
     pthread_mutex_t lock;
+    pthread_t owner;
 };
 
 struct QemuCond {
-- 
1.7.3.5

[Qemu-devel] [PATCH 7/7] remove CONFIG_THREAD

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: Stefan Weil <weil@mail.berlios.de>
Cc: Blue Swirl <blauwirbel@gmail.com>
---
 configure |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 598e8e1..46a6389 100755
--- a/configure
+++ b/configure
@@ -2609,7 +2609,6 @@ if test "$vnc_png" != "no" ; then
 fi
 if test "$vnc_thread" != "no" ; then
   echo "CONFIG_VNC_THREAD=y" >> $config_host_mak
-  echo "CONFIG_THREAD=y" >> $config_host_mak
 fi
 if test "$fnmatch" = "yes" ; then
   echo "CONFIG_FNMATCH=y" >> $config_host_mak
@@ -2696,7 +2695,6 @@ if test "$xen" = "yes" ; then
 fi
 if test "$io_thread" = "yes" ; then
   echo "CONFIG_IOTHREAD=y" >> $config_host_mak
-  echo "CONFIG_THREAD=y" >> $config_host_mak
 fi
 if test "$linux_aio" = "yes" ; then
   echo "CONFIG_LINUX_AIO=y" >> $config_host_mak
-- 
1.7.3.5

[Qemu-devel] Re: [PATCH 6/7] add assertions on the owner of a QemuMutex

[Jan Kiszka]

On 2011-02-10 18:37, Paolo Bonzini wrote:
> These are already present in the Win32 implementation, add them to
> the pthread wrappers as well.

Better use PTHREAD_MUTEX_ERRORCHECK.

Jan

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

[Qemu-devel] Re: [PATCH 4/7] add win32 qemu-thread implementation

[Stefan Weil]

Am 10.02.2011 18:37, schrieb Paolo Bonzini:
> For now, qemu_cond_timedwait and qemu_mutex_timedlock are left as
> POSIX-only functions. They can be removed later, once the patches
> that remove their uses are in.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Stefan Weil <weil@mail.berlios.de>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> ---
> Makefile.objs | 4 +-
> qemu-thread.c => qemu-thread-posix.c | 0
> qemu-thread-posix.h | 18 +++
> qemu-thread-win32.c | 272 ++++++++++++++++++++++++++++++++++
> qemu-thread-win32.h | 22 +++
> qemu-thread.h | 27 ++--
> 6 files changed, 326 insertions(+), 17 deletions(-)
> rename qemu-thread.c => qemu-thread-posix.c (100%)
> create mode 100644 qemu-thread-posix.h
> create mode 100644 qemu-thread-win32.c
> create mode 100644 qemu-thread-win32.h
>

What about using MinGW's pthread support?
http://sourceforge.net/projects/mingw/files/MinGW/pthreads-w32/

I used it for the threaded vnc server in QEMU, but I don't
have more practical experience with it.

Regards,
Stefan

[Qemu-devel] Re: [PATCH 6/7] add assertions on the owner of a QemuMutex

[Paolo Bonzini]

On 02/10/2011 07:25 PM, Jan Kiszka wrote:
> On 2011-02-10 18:37, Paolo Bonzini wrote:
>> These are already present in the Win32 implementation, add them to
>> the pthread wrappers as well.
>
> Better use PTHREAD_MUTEX_ERRORCHECK.

True.  However, later I'd like to include tests that the mutex is held 
during cond_signal/cond_broadcast, so I would have to track the owner 
manually anyway.

Paolo

[Qemu-devel] Re: [PATCH 4/7] add win32 qemu-thread implementation

[Paolo Bonzini]

On 02/10/2011 08:46 PM, Stefan Weil wrote:
> Am 10.02.2011 18:37, schrieb Paolo Bonzini:
>> For now, qemu_cond_timedwait and qemu_mutex_timedlock are left as
>> POSIX-only functions. They can be removed later, once the patches
>> that remove their uses are in.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Stefan Weil <weil@mail.berlios.de>
>> Cc: Blue Swirl <blauwirbel@gmail.com>
>> ---
>> Makefile.objs | 4 +-
>> qemu-thread.c => qemu-thread-posix.c | 0
>> qemu-thread-posix.h | 18 +++
>> qemu-thread-win32.c | 272 ++++++++++++++++++++++++++++++++++
>> qemu-thread-win32.h | 22 +++
>> qemu-thread.h | 27 ++--
>> 6 files changed, 326 insertions(+), 17 deletions(-)
>> rename qemu-thread.c => qemu-thread-posix.c (100%)
>> create mode 100644 qemu-thread-posix.h
>> create mode 100644 qemu-thread-win32.c
>> create mode 100644 qemu-thread-win32.h
>>
>
> What about using MinGW's pthread support?
> http://sourceforge.net/projects/mingw/files/MinGW/pthreads-w32/
>
> I used it for the threaded vnc server in QEMU, but I don't
> have more practical experience with it.

1) For threads, we'd need to do the same in order to keep the HANDLE for 
iothread IPIs (SuspendThread/ResumeThread).  We could use nonportable 
functions from pthread-w32, but if for example in the future QEMU 
switched to glib these wouldn't be available.  So QEMU would need a 
separate Win32 implementation anyway sooner or later.

2) For condvars, we can make some more assumptions and make it 
simpler/faster.

3) It has some namespace pollution issues.  These wouldn't be a problem 
for QEMU at the moment, however.

Paolo

[Qemu-devel] Re: [PATCH 6/7] add assertions on the owner of a QemuMutex

[Jan Kiszka]

On 2011-02-11 13:14, Paolo Bonzini wrote:
> On 02/10/2011 07:25 PM, Jan Kiszka wrote:
>> On 2011-02-10 18:37, Paolo Bonzini wrote:
>>> These are already present in the Win32 implementation, add them to
>>> the pthread wrappers as well.
>>
>> Better use PTHREAD_MUTEX_ERRORCHECK.
> 
> True.  However, later I'd like to include tests that the mutex is held 
> during cond_signal/cond_broadcast, so I would have to track the owner 
> manually anyway.

You could still save the assertions where pthread does the work already.

Jan

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

[Qemu-devel] Re: [PATCH 4/7] add win32 qemu-thread implementation

[Blue Swirl]

On Fri, Feb 11, 2011 at 2:28 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 02/10/2011 08:46 PM, Stefan Weil wrote:
>>
>> Am 10.02.2011 18:37, schrieb Paolo Bonzini:
>>>
>>> For now, qemu_cond_timedwait and qemu_mutex_timedlock are left as
>>> POSIX-only functions. They can be removed later, once the patches
>>> that remove their uses are in.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Stefan Weil <weil@mail.berlios.de>
>>> Cc: Blue Swirl <blauwirbel@gmail.com>
>>> ---
>>> Makefile.objs | 4 +-
>>> qemu-thread.c => qemu-thread-posix.c | 0
>>> qemu-thread-posix.h | 18 +++
>>> qemu-thread-win32.c | 272 ++++++++++++++++++++++++++++++++++
>>> qemu-thread-win32.h | 22 +++
>>> qemu-thread.h | 27 ++--
>>> 6 files changed, 326 insertions(+), 17 deletions(-)
>>> rename qemu-thread.c => qemu-thread-posix.c (100%)
>>> create mode 100644 qemu-thread-posix.h
>>> create mode 100644 qemu-thread-win32.c
>>> create mode 100644 qemu-thread-win32.h
>>>
>>
>> What about using MinGW's pthread support?
>> http://sourceforge.net/projects/mingw/files/MinGW/pthreads-w32/
>>
>> I used it for the threaded vnc server in QEMU, but I don't
>> have more practical experience with it.
>
> 1) For threads, we'd need to do the same in order to keep the HANDLE for
> iothread IPIs (SuspendThread/ResumeThread).  We could use nonportable
> functions from pthread-w32, but if for example in the future QEMU switched
> to glib these wouldn't be available.  So QEMU would need a separate Win32
> implementation anyway sooner or later.
>
> 2) For condvars, we can make some more assumptions and make it
> simpler/faster.
>
> 3) It has some namespace pollution issues.  These wouldn't be a problem for
> QEMU at the moment, however.

This patch no longer applies, please rebase.

[Qemu-devel] Re: [PATCH 2/7] implement win32 dynticks timer

[Blue Swirl]

On Thu, Feb 10, 2011 at 7:37 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Stefan Weil <weil@mail.berlios.de>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> ---
>  qemu-timer.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/qemu-timer.c b/qemu-timer.c
> index b0db780..42960de 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -1006,6 +1006,7 @@ static void win32_stop_timer(struct qemu_alarm_timer *t)
>  static void win32_rearm_timer(struct qemu_alarm_timer *t)
>  {
>     struct qemu_alarm_win32 *data = t->priv;
> +    int nearest_delta_ms;
>
>     assert(alarm_has_dynticks(t));
>     if (!active_timers[QEMU_CLOCK_REALTIME] &&
> @@ -1015,7 +1016,10 @@ static void win32_rearm_timer(struct qemu_alarm_timer *t)
>
>     timeKillEvent(data->timerId);
>
> -    data->timerId = timeSetEvent(1,
> +    nearest_delta_ms = (qemu_next_alarm_deadline() + 999999) / 1000000;
> +    if (nearest_delta_ms < 1)
> +       nearest_delta_ms = 1;

Braces, please.

[Qemu-devel] Re: [PATCH 2/7] implement win32 dynticks timer

[Paolo Bonzini]

On 02/20/2011 10:15 PM, Blue Swirl wrote:
> On Thu, Feb 10, 2011 at 7:37 PM, Paolo Bonzini<pbonzini@redhat.com>  wrote:
>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>> Cc: Stefan Weil<weil@mail.berlios.de>
>> Cc: Blue Swirl<blauwirbel@gmail.com>
>> ---
>>   qemu-timer.c |    6 +++++-
>>   1 files changed, 5 insertions(+), 1 deletions(-)
>>
>> diff --git a/qemu-timer.c b/qemu-timer.c
>> index b0db780..42960de 100644
>> --- a/qemu-timer.c
>> +++ b/qemu-timer.c
>> @@ -1006,6 +1006,7 @@ static void win32_stop_timer(struct qemu_alarm_timer *t)
>>   static void win32_rearm_timer(struct qemu_alarm_timer *t)
>>   {
>>      struct qemu_alarm_win32 *data = t->priv;
>> +    int nearest_delta_ms;
>>
>>      assert(alarm_has_dynticks(t));
>>      if (!active_timers[QEMU_CLOCK_REALTIME]&&
>> @@ -1015,7 +1016,10 @@ static void win32_rearm_timer(struct qemu_alarm_timer *t)
>>
>>      timeKillEvent(data->timerId);
>>
>> -    data->timerId = timeSetEvent(1,
>> +    nearest_delta_ms = (qemu_next_alarm_deadline() + 999999) / 1000000;
>> +    if (nearest_delta_ms<  1)
>> +       nearest_delta_ms = 1;
>
> Braces, please.

Oops, checkpatch missed it.  Will fix.

Paolo