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 <pbonz...@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
