Re: [Qemu-devel] [PATCH 1/2] iothread: stash thread ID away

[Stefan Hajnoczi <stefanha@gmail.com>]

On Fri, Feb 21, 2014 at 04:18:30PM +0100, Paolo Bonzini wrote:
> Il 21/02/2014 15:51, Stefan Hajnoczi ha scritto:
> >Keep the thread ID around so we can report it via QMP.
> >
> >There's only one problem: qemu_get_thread_id() (gettid() wrapper on
> >Linux) must be called from the thread itself.  There is no way to get
> >the thread ID outside the thread.
> >
> >This patch uses a condvar to wait for iothread_run() to populate the
> >thread_id inside the thread.
> >
> >Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >---
> > iothread.c | 34 ++++++++++++++++++++++++++++++++--
> > 1 file changed, 32 insertions(+), 2 deletions(-)
> >
> >diff --git a/iothread.c b/iothread.c
> >index 033de7f..1a0d1ec 100644
> >--- a/iothread.c
> >+++ b/iothread.c
> >@@ -26,8 +26,15 @@ struct IOThread {
> >     QemuThread thread;
> >     AioContext *ctx;
> >     bool stopping;
> >+    int thread_id;
> > };
> >
> >+typedef struct {
> >+    QemuMutex init_done_lock;
> >+    QemuCond init_done_cond;    /* is thread initialization done? */
> >+    IOThread *iothread;
> >+} ThreadInitInfo;
> >+
> > #define IOTHREAD_GET_CLASS(obj) \
> >    OBJECT_GET_CLASS(IOThreadClass, obj, TYPE_IOTHREAD)
> > #define IOTHREAD_CLASS(klass) \
> >@@ -35,7 +42,15 @@ struct IOThread {
> >
> > static void *iothread_run(void *opaque)
> > {
> >-    IOThread *iothread = opaque;
> >+    ThreadInitInfo *init_info = opaque;
> >+    IOThread *iothread = init_info->iothread;
> >+
> >+    iothread->thread_id = qemu_get_thread_id();
> >+
> >+    /* Signal that initialization is done */
> >+    qemu_mutex_lock(&init_info->init_done_lock);
> >+    qemu_cond_signal(&init_info->init_done_cond);
> >+    qemu_mutex_unlock(&init_info->init_done_lock);
> >
> >     while (!iothread->stopping) {
> >         aio_context_acquire(iothread->ctx);
> >@@ -60,15 +75,30 @@ static void iothread_instance_finalize(Object *obj)
> > static void iothread_complete(UserCreatable *obj, Error **errp)
> > {
> >     IOThread *iothread = IOTHREAD(obj);
> >+    ThreadInitInfo init_info = {
> >+        .iothread = iothread,
> >+    };
> >
> >     iothread->stopping = false;
> >     iothread->ctx = aio_context_new();
> >
> >+    qemu_mutex_init(&init_info.init_done_lock);
> >+    qemu_cond_init(&init_info.init_done_cond);
> >+
> >     /* This assumes we are called from a thread with useful CPU affinity for us
> >      * to inherit.
> >      */
> >     qemu_thread_create(&iothread->thread, iothread_run,
> >-                       iothread, QEMU_THREAD_JOINABLE);
> >+                       &init_info, QEMU_THREAD_JOINABLE);
> >+
> >+    /* Wait for initialization to complete */
> >+    qemu_mutex_lock(&init_info.init_done_lock);
> >+    qemu_cond_wait(&init_info.init_done_cond,
> >+                   &init_info.init_done_lock);
> >+    qemu_mutex_unlock(&init_info.init_done_lock);
> >+
> >+    qemu_cond_destroy(&init_info.init_done_cond);
> >+    qemu_mutex_destroy(&init_info.init_done_lock);
> 
> Destroying the mutex here is racy.  You need to keep it until the
> iothread is destroyed.

I don't think so:

qemu_cond_signal() is called with the mutex held.  Therefore, our
qemu_cond_wait() followed by qemu_mutex_unlock() will only complete once
the thread has released the mutex.

The thread will never touch the mutex again so it is safe to destroy it.
There is no race condition.

Stefan