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

[mordan@ispras.ru]

Hello! Please take a look at the new version of the patch here: https://lore.kernel.org/all/20250224161719.3831357-1-mordan@ispras.ru
Thank you!

February 20, 2025 6:10 AM, "Paolo Bonzini" <pbonzini@redhat.com> wrote:

> 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);

-- Vitalii Mordan
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: mordan@ispras.ru