[Qemu-devel] [PATCH v2 0/4] add qemu_thread_join, use it to fix bug in ccid

[Qemu-devel] [PATCH v2 0/4] add qemu_thread_join, use it to fix bug in ccid

[Paolo Bonzini]

Patches introducing qemu_thread_join have floated around multiple times.
Now I found a bug that requires it to be fixed, so perhaps this time
it will be more successful.

For the actual bug, see patch 4.

v1->v2: remove spurious submodule change, fix blank lines

Jan Kiszka (2):
  qemu-thread: add API for joinable threads
  qemu-thread: implement joinable threads for POSIX

Paolo Bonzini (2):
  qemu-thread: implement joinable threads for Win32
  ccid: make threads joinable

 cpus.c                  |    6 ++-
 hw/ccid-card-emulated.c |   25 +++++------
 qemu-thread-posix.c     |   35 ++++++++++++++--
 qemu-thread-win32.c     |  107 ++++++++++++++++++++++++++++++----------------
 qemu-thread-win32.h     |    5 +-
 qemu-thread.h           |    8 +++-
 ui/vnc-jobs-async.c     |    2 +-
 8 files changed, 127 insertions(+), 63 deletions(-)

-- 
1.7.7.1

[Qemu-devel] [PATCH v2 1/4] qemu-thread: add API for joinable threads

[Paolo Bonzini]

From: Jan Kiszka <jan.kiszka@siemens.com>

Split from Jan's original qemu-thread-posix.c patch.  No semantic change,
just introduce the new API that POSIX and Win32 implementations will
conform to.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpus.c                  |    6 ++++--
 hw/ccid-card-emulated.c |    5 +++--
 qemu-thread-posix.c     |    7 ++++---
 qemu-thread-win32.c     |    4 +++-
 qemu-thread.h           |    8 ++++++--
 ui/vnc-jobs-async.c     |    2 +-
 6 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/cpus.c b/cpus.c
index ca46ec6..cbb4617 100644
--- a/cpus.c
+++ b/cpus.c
@@ -910,7 +910,8 @@ static void qemu_tcg_init_vcpu(void *_env)
         env->halt_cond = g_malloc0(sizeof(QemuCond));
         qemu_cond_init(env->halt_cond);
         tcg_halt_cond = env->halt_cond;
-        qemu_thread_create(env->thread, qemu_tcg_cpu_thread_fn, env);
+        qemu_thread_create(env->thread, qemu_tcg_cpu_thread_fn, env,
+                           QEMU_THREAD_DETACHED);
         while (env->created == 0) {
             qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
         }
@@ -926,7 +927,8 @@ static void qemu_kvm_start_vcpu(CPUState *env)
     env->thread = g_malloc0(sizeof(QemuThread));
     env->halt_cond = g_malloc0(sizeof(QemuCond));
     qemu_cond_init(env->halt_cond);
-    qemu_thread_create(env->thread, qemu_kvm_cpu_thread_fn, env);
+    qemu_thread_create(env->thread, qemu_kvm_cpu_thread_fn, env,
+                       QEMU_THREAD_DETACHED);
     while (env->created == 0) {
         qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
     }
diff --git a/hw/ccid-card-emulated.c b/hw/ccid-card-emulated.c
index 092301b..9fe9db5 100644
--- a/hw/ccid-card-emulated.c
+++ b/hw/ccid-card-emulated.c
@@ -541,8 +541,9 @@ static int emulated_initfn(CCIDCardState *base)
         printf("%s: failed to initialize vcard\n", EMULATED_DEV_NAME);
         return -1;
     }
-    qemu_thread_create(&thread_id, event_thread, card);
-    qemu_thread_create(&thread_id, handle_apdu_thread, card);
+    qemu_thread_create(&thread_id, event_thread, card, QEMU_THREAD_DETACHED);
+    qemu_thread_create(&thread_id, handle_apdu_thread, card,
+                       QEMU_THREAD_DETACHED);
     return 0;
 }
 
diff --git a/qemu-thread-posix.c b/qemu-thread-posix.c
index ac3c0c9..49d3388 100644
--- a/qemu-thread-posix.c
+++ b/qemu-thread-posix.c
@@ -117,13 +117,14 @@ void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
 
 void qemu_thread_create(QemuThread *thread,
                        void *(*start_routine)(void*),
-                       void *arg)
+                       void *arg, int mode)
 {
+    sigset_t set, oldset;
     int err;
 
-    /* Leave signal handling to the iothread.  */
-    sigset_t set, oldset;
+    assert(mode == QEMU_THREAD_DETACHED);
 
+    /* Leave signal handling to the iothread.  */
     sigfillset(&set);
     pthread_sigmask(SIG_SETMASK, &set, &oldset);
     err = pthread_create(&thread->thread, NULL, start_routine, arg);
diff --git a/qemu-thread-win32.c b/qemu-thread-win32.c
index db8e744..ff80e84 100644
--- a/qemu-thread-win32.c
+++ b/qemu-thread-win32.c
@@ -243,10 +243,12 @@ static inline void qemu_thread_init(void)
 
 void qemu_thread_create(QemuThread *thread,
                        void *(*start_routine)(void *),
-                       void *arg)
+                       void *arg, int mode)
 {
     HANDLE hThread;
 
+    assert(mode == QEMU_THREAD_DETACHED);
+
     struct QemuThreadData *data;
     qemu_thread_init();
     data = g_malloc(sizeof *data);
diff --git a/qemu-thread.h b/qemu-thread.h
index e008b60..a78a8f2 100644
--- a/qemu-thread.h
+++ b/qemu-thread.h
@@ -13,6 +13,9 @@ typedef struct QemuThread QemuThread;
 #include "qemu-thread-posix.h"
 #endif
 
+#define QEMU_THREAD_JOINABLE 0
+#define QEMU_THREAD_DETACHED 1
+
 void qemu_mutex_init(QemuMutex *mutex);
 void qemu_mutex_destroy(QemuMutex *mutex);
 void qemu_mutex_lock(QemuMutex *mutex);
@@ -35,8 +38,9 @@ void qemu_cond_broadcast(QemuCond *cond);
 void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex);
 
 void qemu_thread_create(QemuThread *thread,
-                       void *(*start_routine)(void*),
-                       void *arg);
+                        void *(*start_routine)(void *),
+                        void *arg, int mode);
+void *qemu_thread_join(QemuThread *thread);
 void qemu_thread_get_self(QemuThread *thread);
 int qemu_thread_is_self(QemuThread *thread);
 void qemu_thread_exit(void *retval);
diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
index de5ea6b..9b3016c 100644
--- a/ui/vnc-jobs-async.c
+++ b/ui/vnc-jobs-async.c
@@ -318,7 +318,7 @@ void vnc_start_worker_thread(void)
         return ;
 
     q = vnc_queue_init();
-    qemu_thread_create(&q->thread, vnc_worker_thread, q);
+    qemu_thread_create(&q->thread, vnc_worker_thread, q, QEMU_THREAD_DETACHED);
     queue = q; /* Set global queue */
 }
 
-- 
1.7.7.1

[Qemu-devel] [PATCH v2 2/4] qemu-thread: implement joinable threads for POSIX

[Paolo Bonzini]

From: Jan Kiszka <jan.kiszka@siemens.com>

Allow to control if a QEMU thread is created joinable or not. Make it
not joinable by default to avoid that we keep the associated resources
around when terminating a thread without joining it (what we couldn't do
so far for obvious reasons).

The audio subsystem will need the join feature when converting it to
QEMU threading/locking abstractions, so provide that service.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-thread-posix.c |   28 ++++++++++++++++++++++++++++--
 1 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/qemu-thread-posix.c b/qemu-thread-posix.c
index 49d3388..603ff3d 100644
--- a/qemu-thread-posix.c
+++ b/qemu-thread-posix.c
@@ -122,17 +122,29 @@ void qemu_thread_create(QemuThread *thread,
 {
     sigset_t set, oldset;
     int err;
+    pthread_attr_t attr;
 
-    assert(mode == QEMU_THREAD_DETACHED);
+    err = pthread_attr_init(&attr);
+    if (err) {
+        error_exit(err, __func__);
+    }
+    if (mode == QEMU_THREAD_DETACHED) {
+        err = pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
+        if (err) {
+            error_exit(err, __func__);
+        }
+    }
 
     /* Leave signal handling to the iothread.  */
     sigfillset(&set);
     pthread_sigmask(SIG_SETMASK, &set, &oldset);
-    err = pthread_create(&thread->thread, NULL, start_routine, arg);
+    err = pthread_create(&thread->thread, &attr, start_routine, arg);
     if (err)
         error_exit(err, __func__);
 
     pthread_sigmask(SIG_SETMASK, &oldset, NULL);
+
+    pthread_attr_destroy(&attr);
 }
 
 void qemu_thread_get_self(QemuThread *thread)
@@ -148,3 +162,15 @@ void qemu_thread_exit(void *retval)
 {
     pthread_exit(retval);
 }
+
+void *qemu_thread_join(QemuThread *thread)
+{
+    int err;
+    void *ret;
+
+    err = pthread_join(thread->thread, &ret);
+    if (err) {
+        error_exit(err, __func__);
+    }
+    return ret;
+}
-- 
1.7.7.1

[Qemu-devel] [PATCH v2 3/4] qemu-thread: implement joinable threads for Win32

[Paolo Bonzini]

Rewrite the handshaking between qemu_thread_create and the
win32_start_routine, so that the thread can be joined without races.
Similar handshaking is done now between qemu_thread_exit and
qemu_thread_join.

This also simplifies how QemuThreads are initialized.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-thread-win32.c |  107 +++++++++++++++++++++++++++++++++------------------
 qemu-thread-win32.h |    5 +-
 3 files changed, 73 insertions(+), 41 deletions(-)

diff --git a/qemu-thread-win32.c b/qemu-thread-win32.c
index ff80e84..a13ffcc 100644
--- a/qemu-thread-win32.c
+++ b/qemu-thread-win32.c
@@ -193,41 +193,78 @@ void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
 }
 
 struct QemuThreadData {
-    QemuThread *thread;
-    void *(*start_routine)(void *);
-    void *arg;
+    /* Passed to win32_start_routine.  */
+    void             *(*start_routine)(void *);
+    void             *arg;
+    short             mode;
+
+    /* Only used for joinable threads. */
+    bool              exited;
+    void             *ret;
+    CRITICAL_SECTION  cs;
 };
 
 static int qemu_thread_tls_index = TLS_OUT_OF_INDEXES;
 
 static unsigned __stdcall win32_start_routine(void *arg)
 {
-    struct QemuThreadData data = *(struct QemuThreadData *) 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));
+    QemuThreadData *data = (QemuThreadData *) arg;
+    void *(*start_routine)(void *) = data->start_routine;
+    void *thread_arg = data->arg;
+
+    if (data->mode == QEMU_THREAD_DETACHED) {
+        g_free(data);
+        data = NULL;
+    } else {
+        InitializeCriticalSection(&data->cs);
+    }
+    TlsSetValue(qemu_thread_tls_index, data);
+    qemu_thread_exit(start_routine(thread_arg));
     abort();
 }
 
 void qemu_thread_exit(void *arg)
 {
-    QemuThread *thread = TlsGetValue(qemu_thread_tls_index);
-    thread->ret = arg;
-    CloseHandle(thread->thread);
-    thread->thread = NULL;
-    ExitThread(0);
+    QemuThreadData *data = TlsGetValue(qemu_thread_tls_index);
+    if (data) {
+        data->ret = arg;
+        EnterCriticalSection(&data->cs);
+        data->exited = true;
+        LeaveCriticalSection(&data->cs);
+    }
+    _endthreadex(0);
+}
+
+void *qemu_thread_join(QemuThread *thread)
+{
+    QemuThreadData *data;
+    void *ret;
+    HANDLE handle;
+
+    data = thread->data;
+    if (!data) {
+        return NULL;
+    }
+    /*
+     * Because multiple copies of the QemuThread can exist via
+     * qemu_thread_get_self, we need to store a value that cannot
+     * leak there.  The simplest, non racy way is to store the TID,
+     * discard the handle that _beginthreadex gives back, and
+     * get another copy of the handle here.
+     */
+    EnterCriticalSection(&data->cs);
+    if (!data->exited) {
+        handle = OpenThread(SYNCHRONIZE, FALSE, thread->tid);
+        LeaveCriticalSection(&data->cs);
+        WaitForSingleObject(handle, INFINITE);
+        CloseHandle(handle);
+    } else {
+        LeaveCriticalSection(&data->cs);
+    }
+    ret = data->ret;
+    DeleteCriticalSection(&data->cs);
+    g_free(data);
+    return ret;
 }
 
 static inline void qemu_thread_init(void)
@@ -247,37 +284,31 @@ void qemu_thread_create(QemuThread *thread,
 {
     HANDLE hThread;
 
-    assert(mode == QEMU_THREAD_DETACHED);
-
     struct QemuThreadData *data;
     qemu_thread_init();
     data = g_malloc(sizeof *data);
-    data->thread = thread;
     data->start_routine = start_routine;
     data->arg = arg;
+    data->mode = mode;
+    data->exited = false;
 
     hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
-                                      data, 0, NULL);
+                                      data, 0, &thread->tid);
     if (!hThread) {
         error_exit(GetLastError(), __func__);
     }
     CloseHandle(hThread);
+    thread->data = (mode == QEMU_THREAD_DETACHED) ? NULL : data;
 }
 
 void qemu_thread_get_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_self.  */
-        qemu_thread_init();
-        TlsSetValue(qemu_thread_tls_index, thread);
-        thread->thread = GetCurrentThread();
-    }
+    qemu_thread_init();
+    thread->data = TlsGetValue(qemu_thread_tls_index);
+    thread->tid = GetCurrentThreadId();
 }
 
 int qemu_thread_is_self(QemuThread *thread)
 {
-    QemuThread *this_thread = TlsGetValue(qemu_thread_tls_index);
-    return this_thread->thread == thread->thread;
+    return GetCurrentThreadId() == thread->tid;
 }
diff --git a/qemu-thread-win32.h b/qemu-thread-win32.h
index 878f86a..2983490 100644
--- a/qemu-thread-win32.h
+++ b/qemu-thread-win32.h
@@ -13,9 +13,10 @@ struct QemuCond {
     HANDLE continue_event;
 };
 
+typedef struct QemuThreadData QemuThreadData;
 struct QemuThread {
-    HANDLE thread;
-    void *ret;
+    QemuThreadData *data;
+    unsigned tid;
 };
 
 #endif
-- 
1.7.7.1

[Qemu-devel] [PATCH v2 4/4] ccid: make threads joinable

[Paolo Bonzini]

Destroying a mutex that another thread might have just unlocked
is racy.  It usually works, but you cannot do that in general and
can lead to deadlocks or segfaults.  Change ccid to use joinable
threads instead.

(Also, qemu_mutex_init/qemu_cond_init were missing).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ccid-card-emulated.c |   26 +++++++++++---------------
 1 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/hw/ccid-card-emulated.c b/hw/ccid-card-emulated.c
index 9fe9db5..2d2ebce 100644
--- a/hw/ccid-card-emulated.c
+++ b/hw/ccid-card-emulated.c
@@ -120,6 +120,7 @@ struct EmulatedState {
     uint8_t  atr_length;
     QSIMPLEQ_HEAD(event_list, EmulEvent) event_list;
     QemuMutex event_list_mutex;
+    QemuThread event_thread_id;
     VReader *reader;
     QSIMPLEQ_HEAD(guest_apdu_list, EmulEvent) guest_apdu_list;
     QemuMutex vreader_mutex; /* and guest_apdu_list mutex */
@@ -127,8 +128,7 @@ struct EmulatedState {
     QemuCond handle_apdu_cond;
     int      pipe[2];
     int      quit_apdu_thread;
-    QemuMutex apdu_thread_quit_mutex;
-    QemuCond apdu_thread_quit_cond;
+    QemuThread apdu_thread_id;
 };
 
 static void emulated_apdu_from_guest(CCIDCardState *base,
@@ -271,9 +271,6 @@ static void *handle_apdu_thread(void* arg)
         }
         qemu_mutex_unlock(&card->vreader_mutex);
     }
-    qemu_mutex_lock(&card->apdu_thread_quit_mutex);
-    qemu_cond_signal(&card->apdu_thread_quit_cond);
-    qemu_mutex_unlock(&card->apdu_thread_quit_mutex);
     return NULL;
 }
 
@@ -489,7 +486,6 @@ static uint32_t parse_enumeration(char *str,
 static int emulated_initfn(CCIDCardState *base)
 {
     EmulatedState *card = DO_UPCAST(EmulatedState, base, base);
-    QemuThread thread_id;
     VCardEmulError ret;
     EnumTable *ptable;
 
@@ -541,9 +537,10 @@ static int emulated_initfn(CCIDCardState *base)
         printf("%s: failed to initialize vcard\n", EMULATED_DEV_NAME);
         return -1;
     }
-    qemu_thread_create(&thread_id, event_thread, card, QEMU_THREAD_DETACHED);
-    qemu_thread_create(&thread_id, handle_apdu_thread, card,
-                       QEMU_THREAD_DETACHED);
+    qemu_thread_create(&card->event_thread_id, event_thread, card,
+                       QEMU_THREAD_JOINABLE);
+    qemu_thread_create(&card->apdu_thread_id, handle_apdu_thread, card,
+                       QEMU_THREAD_JOINABLE);
     return 0;
 }
 
@@ -553,15 +550,14 @@ static int emulated_exitfn(CCIDCardState *base)
     VEvent *vevent = vevent_new(VEVENT_LAST, NULL, NULL);
 
     vevent_queue_vevent(vevent); /* stop vevent thread */
-    qemu_mutex_lock(&card->apdu_thread_quit_mutex);
+    qemu_thread_join(&card->event_thread_id);
+
     card->quit_apdu_thread = 1; /* stop handle_apdu thread */
     qemu_cond_signal(&card->handle_apdu_cond);
-    qemu_cond_wait(&card->apdu_thread_quit_cond,
-                      &card->apdu_thread_quit_mutex);
-    /* handle_apdu thread stopped, can destroy all of it's mutexes */
+    qemu_thread_join(&card->apdu_thread_id);
+
+    /* threads exited, can destroy all condvars/mutexes */
     qemu_cond_destroy(&card->handle_apdu_cond);
-    qemu_cond_destroy(&card->apdu_thread_quit_cond);
-    qemu_mutex_destroy(&card->apdu_thread_quit_mutex);
     qemu_mutex_destroy(&card->handle_apdu_mutex);
     qemu_mutex_destroy(&card->vreader_mutex);
     qemu_mutex_destroy(&card->event_list_mutex);
-- 
1.7.7.1

Re: [Qemu-devel] [PATCH v2 0/4] add qemu_thread_join, use it to fix bug in ccid

[Anthony Liguori]

On 12/12/2011 10:21 AM, Paolo Bonzini wrote:
> Patches introducing qemu_thread_join have floated around multiple times.
> Now I found a bug that requires it to be fixed, so perhaps this time
> it will be more successful.
>
> For the actual bug, see patch 4.
>
> v1->v2: remove spurious submodule change, fix blank lines

Applied all.  Thanks.

Regards,

Anthony Liguori

>
> Jan Kiszka (2):
>    qemu-thread: add API for joinable threads
>    qemu-thread: implement joinable threads for POSIX
>
> Paolo Bonzini (2):
>    qemu-thread: implement joinable threads for Win32
>    ccid: make threads joinable
>
>   cpus.c                  |    6 ++-
>   hw/ccid-card-emulated.c |   25 +++++------
>   qemu-thread-posix.c     |   35 ++++++++++++++--
>   qemu-thread-win32.c     |  107 ++++++++++++++++++++++++++++++----------------
>   qemu-thread-win32.h     |    5 +-
>   qemu-thread.h           |    8 +++-
>   ui/vnc-jobs-async.c     |    2 +-
>   8 files changed, 127 insertions(+), 63 deletions(-)
>