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

[Jan Kiszka <jan.kiszka@siemens.com>]

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