* [Qemu-devel] [PATCH 1/4] qemu-thread: add API for joinable threads
2011-12-06 17:05 [Qemu-devel] [PATCH 0/4] add qemu_thread_join, use it to fix bug in ccid Paolo Bonzini
@ 2011-12-06 17:05 ` Paolo Bonzini
2011-12-06 17:40 ` Jan Kiszka
2011-12-06 17:05 ` [Qemu-devel] [PATCH 2/4] qemu-thread: implement joinable threads for POSIX Paolo Bonzini
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Paolo Bonzini @ 2011-12-06 17:05 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka, alevy
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH 1/4] qemu-thread: add API for joinable threads
2011-12-06 17:05 ` [Qemu-devel] [PATCH 1/4] qemu-thread: add API for joinable threads Paolo Bonzini
@ 2011-12-06 17:40 ` Jan Kiszka
0 siblings, 0 replies; 11+ messages in thread
From: Jan Kiszka @ 2011-12-06 17:40 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: alevy@redhat.com, qemu-devel@nongnu.org
On 2011-12-06 18:05, Paolo Bonzini wrote:
> 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 */
> }
>
Acked - but I already signed it anyway.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 2/4] qemu-thread: implement joinable threads for POSIX
2011-12-06 17:05 [Qemu-devel] [PATCH 0/4] add qemu_thread_join, use it to fix bug in ccid Paolo Bonzini
2011-12-06 17:05 ` [Qemu-devel] [PATCH 1/4] qemu-thread: add API for joinable threads Paolo Bonzini
@ 2011-12-06 17:05 ` Paolo Bonzini
2011-12-06 17:37 ` Jan Kiszka
2011-12-06 17:05 ` [Qemu-devel] [PATCH 3/4] qemu-thread: implement joinable threads for Win32 Paolo Bonzini
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Paolo Bonzini @ 2011-12-06 17:05 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka, alevy
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,16 +122,28 @@ void qemu_thread_create(QemuThread *thread,
sigset_t set, oldset;
int err;
- assert(mode == QEMU_THREAD_DETACHED);
+ pthread_attr_t attr;
+ 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH 2/4] qemu-thread: implement joinable threads for POSIX
2011-12-06 17:05 ` [Qemu-devel] [PATCH 2/4] qemu-thread: implement joinable threads for POSIX Paolo Bonzini
@ 2011-12-06 17:37 ` Jan Kiszka
0 siblings, 0 replies; 11+ messages in thread
From: Jan Kiszka @ 2011-12-06 17:37 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: alevy@redhat.com, qemu-devel@nongnu.org
On 2011-12-06 18:05, Paolo Bonzini wrote:
> 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,16 +122,28 @@ void qemu_thread_create(QemuThread *thread,
> sigset_t set, oldset;
> int err;
>
> - assert(mode == QEMU_THREAD_DETACHED);
> + pthread_attr_t attr;
Minor nit: blank line here instead. Fine otherwise.
Jan
> + 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;
> +}
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 3/4] qemu-thread: implement joinable threads for Win32
2011-12-06 17:05 [Qemu-devel] [PATCH 0/4] add qemu_thread_join, use it to fix bug in ccid Paolo Bonzini
2011-12-06 17:05 ` [Qemu-devel] [PATCH 1/4] qemu-thread: add API for joinable threads Paolo Bonzini
2011-12-06 17:05 ` [Qemu-devel] [PATCH 2/4] qemu-thread: implement joinable threads for POSIX Paolo Bonzini
@ 2011-12-06 17:05 ` Paolo Bonzini
2011-12-06 17:39 ` Jan Kiszka
2011-12-06 17:05 ` [Qemu-devel] [PATCH 4/4] ccid: make threads joinable Paolo Bonzini
2011-12-06 17:37 ` [Qemu-devel] [PATCH 0/4] add qemu_thread_join, use it to fix bug in ccid Jan Kiszka
4 siblings, 1 reply; 11+ messages in thread
From: Paolo Bonzini @ 2011-12-06 17:05 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka, alevy
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 +-
roms/seabios | 2 +-
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
diff --git a/roms/seabios b/roms/seabios
index 8e30147..cc97564 160000
--- a/roms/seabios
+++ b/roms/seabios
@@ -1 +1 @@
-Subproject commit 8e301472e324b6d6496d8b4ffc66863e99d7a505
+Subproject commit cc975646af69f279396d4d5e1379ac6af80ee637
--
1.7.7.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH 3/4] qemu-thread: implement joinable threads for Win32
2011-12-06 17:05 ` [Qemu-devel] [PATCH 3/4] qemu-thread: implement joinable threads for Win32 Paolo Bonzini
@ 2011-12-06 17:39 ` Jan Kiszka
2011-12-06 18:10 ` Paolo Bonzini
0 siblings, 1 reply; 11+ messages in thread
From: Jan Kiszka @ 2011-12-06 17:39 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: alevy@redhat.com, qemu-devel@nongnu.org
On 2011-12-06 18:05, Paolo Bonzini wrote:
> 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 +-
> roms/seabios | 2 +-
> 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
> diff --git a/roms/seabios b/roms/seabios
> index 8e30147..cc97564 160000
> --- a/roms/seabios
> +++ b/roms/seabios
> @@ -1 +1 @@
> -Subproject commit 8e301472e324b6d6496d8b4ffc66863e99d7a505
> +Subproject commit cc975646af69f279396d4d5e1379ac6af80ee637
Spurious change, I suppose. :)
Locking looks ok from the distance.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH 3/4] qemu-thread: implement joinable threads for Win32
2011-12-06 17:39 ` Jan Kiszka
@ 2011-12-06 18:10 ` Paolo Bonzini
0 siblings, 0 replies; 11+ messages in thread
From: Paolo Bonzini @ 2011-12-06 18:10 UTC (permalink / raw)
To: Jan Kiszka; +Cc: alevy@redhat.com, qemu-devel@nongnu.org
On 12/06/2011 06:39 PM, Jan Kiszka wrote:
>> > diff --git a/roms/seabios b/roms/seabios
>> > index 8e30147..cc97564 160000
>> > --- a/roms/seabios
>> > +++ b/roms/seabios
>> > @@ -1 +1 @@
>> > -Subproject commit 8e301472e324b6d6496d8b4ffc66863e99d7a505
>> > +Subproject commit cc975646af69f279396d4d5e1379ac6af80ee637
> Spurious change, I suppose.:)
>
> Locking looks ok from the distance.
Uff, yeah. Will resubmit on Monday.
Paolo
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 4/4] ccid: make threads joinable
2011-12-06 17:05 [Qemu-devel] [PATCH 0/4] add qemu_thread_join, use it to fix bug in ccid Paolo Bonzini
` (2 preceding siblings ...)
2011-12-06 17:05 ` [Qemu-devel] [PATCH 3/4] qemu-thread: implement joinable threads for Win32 Paolo Bonzini
@ 2011-12-06 17:05 ` Paolo Bonzini
2011-12-07 11:35 ` Alon Levy
2011-12-06 17:37 ` [Qemu-devel] [PATCH 0/4] add qemu_thread_join, use it to fix bug in ccid Jan Kiszka
4 siblings, 1 reply; 11+ messages in thread
From: Paolo Bonzini @ 2011-12-06 17:05 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka, alevy
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH 4/4] ccid: make threads joinable
2011-12-06 17:05 ` [Qemu-devel] [PATCH 4/4] ccid: make threads joinable Paolo Bonzini
@ 2011-12-07 11:35 ` Alon Levy
0 siblings, 0 replies; 11+ messages in thread
From: Alon Levy @ 2011-12-07 11:35 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: jan.kiszka, qemu-devel
On Tue, Dec 06, 2011 at 06:05:55PM +0100, Paolo Bonzini wrote:
> 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.
>
Looks good to me.
Reviewed-by: Alon Levy <alevy@redhat.com>
> (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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 0/4] add qemu_thread_join, use it to fix bug in ccid
2011-12-06 17:05 [Qemu-devel] [PATCH 0/4] add qemu_thread_join, use it to fix bug in ccid Paolo Bonzini
` (3 preceding siblings ...)
2011-12-06 17:05 ` [Qemu-devel] [PATCH 4/4] ccid: make threads joinable Paolo Bonzini
@ 2011-12-06 17:37 ` Jan Kiszka
4 siblings, 0 replies; 11+ messages in thread
From: Jan Kiszka @ 2011-12-06 17:37 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: alevy@redhat.com, qemu-devel@nongnu.org
On 2011-12-06 18:05, 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.
>
> 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 +++-
> roms/seabios | 2 +-
> ui/vnc-jobs-async.c | 2 +-
> 8 files changed, 127 insertions(+), 63 deletions(-)
>
Oh, cool! I was digging in that pile today as well, wondering what
happened to my patches.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 11+ messages in thread