Re: [PATCH] Fix data race with the state Field of ThreadPoolElement

[Paolo Bonzini <pbonzini@redhat.com>]

On 2/19/25 17:12, Vitalii Mordan wrote:
> diff --git a/util/thread-pool.c b/util/thread-pool.c
> index 27eb777e85..6c5f4d085b 100644
> --- a/util/thread-pool.c
> +++ b/util/thread-pool.c
> @@ -111,9 +111,8 @@ static void *worker_thread(void *opaque)
>           ret = req->func(req->arg);
>   
>           req->ret = ret;
> -        /* Write ret before state.  */
> -        smp_wmb();
> -        req->state = THREAD_DONE;
> +        /* Atomically update state after setting ret.  */
> +        qatomic_store_release(&req->state, THREAD_DONE);

This is good.

> @@ -180,7 +179,7 @@ static void thread_pool_completion_bh(void *opaque)
>   
>   restart:
>       QLIST_FOREACH_SAFE(elem, &pool->head, all, next) {
> -        if (elem->state != THREAD_DONE) {
> +        if (qatomic_load_acquire(&elem->state) != THREAD_DONE) {

This is good, but it needs a comment and it can replace the smp_rmb() below.

>               continue;
>           }
>   
> @@ -223,12 +222,12 @@ static void thread_pool_cancel(BlockAIOCB *acb)
>       trace_thread_pool_cancel(elem, elem->common.opaque);
>   
>       QEMU_LOCK_GUARD(&pool->lock);
> -    if (elem->state == THREAD_QUEUED) {
> +    if (qatomic_load_acquire(&elem->state) == THREAD_QUEUED) {

This is not ordering anything so it can be qatomic_read/qatomic_set 
(technically the one below doesn't even need that, but it's fine).

>           QTAILQ_REMOVE(&pool->request_list, elem, reqs);
>           qemu_bh_schedule(pool->completion_bh);
>   
> -        elem->state = THREAD_DONE;
>           elem->ret = -ECANCELED;
> +        qatomic_store_release(&elem->state, THREAD_DONE);
>       }
>   
>   }
> @@ -251,8 +250,8 @@ BlockAIOCB *thread_pool_submit_aio(ThreadPoolFunc *func, void *arg,
>       req = qemu_aio_get(&thread_pool_aiocb_info, NULL, cb, opaque);
>       req->func = func;
>       req->arg = arg;
> -    req->state = THREAD_QUEUED;
>       req->pool = pool;
> +    qatomic_store_release(&req->state, THREAD_QUEUED);

This does not need any atomic access, because there is ordering via 
pool->lock (which protects the request_list).  There's no need to do 
store-release and load-acquire except to order with another store or 
load, and in fact adding unnecessary acquire/release is harder to 
understand.

Paolo

>       QLIST_INSERT_HEAD(&pool->head, req, all);
>