From: Anthony PERARD <anthony.perard@citrix.com>
Date: Thu, 13 Jul 2023 16:34:53 +0000 (+0100)
Subject: thread-pool: signal requests while locked
X-Git-Url: http://xenbits.xensource.com/gitweb?a=commitdiff_plain;h=refs%2Fheads%2Fwip.fix-assert-thread-pool;p=people%2Faperard%2Fqemu-dm.git

thread-pool: signal requests while locked

thread_pool_free() might have been called on the `pool`, which would
be a reason for worker_thread() to quit. In this case,
`pool->request_cond` is been destroyed.

If worker_thread() didn't managed to signal `request_cond` before it
been destroyed by thread_pool_free(), we got:
    util/qemu-thread-posix.c:198: qemu_cond_signal: Assertion `cond->initialized' failed.

Reported here:
    https://lore.kernel.org/all/ZJwoK50FcnTSfFZ8@MacBook-Air-de-Roger.local/T/#u

Fixes: 900fa208f506 ("thread-pool: replace semaphore with condition variable")
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---

There's maybe an issue in thread_pool_submit_aio() as well, but maybe
it's much less likely to happen or should never happen?.
---

diff --git a/util/thread-pool.c b/util/thread-pool.c
index 0d97888df0..d1c1652e87 100644
--- a/util/thread-pool.c
+++ b/util/thread-pool.c
@@ -120,13 +120,14 @@ static void *worker_thread(void *opaque)
 
     pool->cur_threads--;
     qemu_cond_signal(&pool->worker_stopped);
-    qemu_mutex_unlock(&pool->lock);
 
     /*
      * Wake up another thread, in case we got a wakeup but decided
      * to exit due to pool->cur_threads > pool->max_threads.
      */
-    qemu_cond_signal(&pool->request_cond);
+    qemu_cond_signal(&pool->request_cond); // We probably need lock on &pool->lock
+    // without a lock, thread_pool_free() could destroy the condition.
+    qemu_mutex_unlock(&pool->lock);
     return NULL;
 }
 
@@ -267,7 +268,7 @@ BlockAIOCB *thread_pool_submit_aio(ThreadPoolFunc *func, void *arg,
     }
     QTAILQ_INSERT_TAIL(&pool->request_list, req, reqs);
     qemu_mutex_unlock(&pool->lock);
-    qemu_cond_signal(&pool->request_cond);
+    qemu_cond_signal(&pool->request_cond); // do we need a lock here as well?
     return &req->common;
 }