Fix a data race occurred between `worker_thread()` writing and `thread_pool_completion_bh()` reading shared data in `util/thread-pool.c`. Signed-off-by: Marc Morcos --- util/thread-pool.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/util/thread-pool.c b/util/thread-pool.c index d2ead6b728..1ced3bd371 100644 --- a/util/thread-pool.c +++ b/util/thread-pool.c @@ -18,6 +18,7 @@ #include "qemu/defer-call.h" #include "qemu/queue.h" #include "qemu/thread.h" +#include "qemu/atomic.h" #include "qemu/coroutine.h" #include "trace.h" #include "block/thread-pool.h" @@ -39,9 +40,13 @@ struct ThreadPoolElementAio { ThreadPoolFunc *func; void *arg; - /* Moving state out of THREAD_QUEUED is protected by lock. After - * that, only the worker thread can write to it. Reads and writes - * of state and ret are ordered with memory barriers. + /* + * Accessed with atomics. Moving state out of THREAD_QUEUED is + * protected by pool->lock and only the worker thread can move + * the state from THREAD_ACTIVE to THREAD_DONE. + * + * When state is THREAD_DONE, ret must have been written already. + * Use acquire/release ordering when reading/writing ret as well. */ enum ThreadState state; int ret; @@ -105,15 +110,14 @@ static void *worker_thread(void *opaque) req = QTAILQ_FIRST(&pool->request_list); QTAILQ_REMOVE(&pool->request_list, req, reqs); - req->state = THREAD_ACTIVE; + qatomic_set(&req->state, THREAD_ACTIVE); qemu_mutex_unlock(&pool->lock); ret = req->func(req->arg); req->ret = ret; - /* Write ret before state. */ - smp_wmb(); - req->state = THREAD_DONE; + /* _release to write ret before state. */ + qatomic_store_release(&req->state, THREAD_DONE); qemu_bh_schedule(pool->completion_bh); qemu_mutex_lock(&pool->lock); @@ -180,7 +184,8 @@ static void thread_pool_completion_bh(void *opaque) restart: QLIST_FOREACH_SAFE(elem, &pool->head, all, next) { - if (elem->state != THREAD_DONE) { + /* _acquire to read state before ret. */ + if (qatomic_load_acquire(&elem->state) != THREAD_DONE) { continue; } @@ -189,9 +194,6 @@ restart: QLIST_REMOVE(elem, all); if (elem->common.cb) { - /* Read state before ret. */ - smp_rmb(); - /* Schedule ourselves in case elem->common.cb() calls aio_poll() to * wait for another request that completed at the same time. */ @@ -223,12 +225,12 @@ static void thread_pool_cancel(BlockAIOCB *acb) trace_thread_pool_cancel_aio(elem, elem->common.opaque); QEMU_LOCK_GUARD(&pool->lock); - if (elem->state == THREAD_QUEUED) { + if (qatomic_read(&elem->state) == THREAD_QUEUED) { QTAILQ_REMOVE(&pool->request_list, elem, reqs); qemu_bh_schedule(pool->completion_bh); - elem->state = THREAD_DONE; - elem->ret = -ECANCELED; + qatomic_set(&elem->ret, -ECANCELED); + qatomic_store_release(&elem->state, THREAD_DONE); } } -- 2.52.0.239.gd5f0c6e74e-goog