Series comparison

-[PATCH] Fix data race with the state Field of ThreadPoolElement
+[PATCH v2] Fix data race with the state Field of ThreadPoolElement
 ...
 Fixes: d354c7eccf ("aio: add generic thread-pool facility")
 Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2822
 Signed-off-by: Vitalii Mordan <mordan@ispras.ru>
 ---
- util/thread-pool.c | 13 ++++++-------
+v2: Addressed the Paolo Bonzini comments.
-file changed, 6 insertions(+), 7 deletions(-)
+ util/thread-pool.c | 18 +++++++++---------
 file changed, 9 insertions(+), 9 deletions(-)
 diff --git a/util/thread-pool.c b/util/thread-pool.c
 index XXXXXXX..XXXXXXX 100644
 --- a/util/thread-pool.c
 +++ b/util/thread-pool.c
 ...
 @@ -XXX,XX +XXX,XX @@ static void thread_pool_completion_bh(void *opaque)
  restart:
      QLIST_FOREACH_SAFE(elem, &pool->head, all, next) {
 -        if (elem->state != THREAD_DONE) {
++        /* Atomically access the state field to synchronize with the
++         * worker_thread and thread_pool_cancel.
++         */
 +        if (qatomic_load_acquire(&elem->state) != THREAD_DONE) {
              continue;
          }
+@@ -XXX,XX +XXX,XX @@ 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.
+              */
 @@ -XXX,XX +XXX,XX @@ 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) {
++    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;
++        /* Atomically update state after setting ret. */
 +        qatomic_store_release(&elem->state, THREAD_DONE);
      }
  }
-@@ -XXX,XX +XXX,XX @@ 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);
-     QLIST_INSERT_HEAD(&pool->head, req, all);
 --
 .34.1

TSAN reports a potential data race on the state field of
ThreadPoolElement. This is fixed by using atomic access to the field.

Fixes: d354c7eccf ("aio: add generic thread-pool facility")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2822
Signed-off-by: Vitalii Mordan <mordan@ispras.ru>
---
 util/thread-pool.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/util/thread-pool.c b/util/thread-pool.c
index XXXXXXX..XXXXXXX 100644
--- a/util/thread-pool.c
+++ b/util/thread-pool.c
@@ -XXX,XX +XXX,XX @@ 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);
 
         qemu_bh_schedule(pool->completion_bh);
         qemu_mutex_lock(&pool->lock);
@@ -XXX,XX +XXX,XX @@ 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) {
             continue;
         }
 
@@ -XXX,XX +XXX,XX @@ 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) {
         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);
     }
 
 }
@@ -XXX,XX +XXX,XX @@ 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);
 
     QLIST_INSERT_HEAD(&pool->head, req, all);
 
-- 
2.34.1

TSAN reports a potential data race on the state field of
ThreadPoolElement. This is fixed by using atomic access to the field.

Fixes: d354c7eccf ("aio: add generic thread-pool facility")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2822
Signed-off-by: Vitalii Mordan <mordan@ispras.ru>
---
v2: Addressed the Paolo Bonzini comments.
 util/thread-pool.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/util/thread-pool.c b/util/thread-pool.c
index XXXXXXX..XXXXXXX 100644
--- a/util/thread-pool.c
+++ b/util/thread-pool.c
@@ -XXX,XX +XXX,XX @@ 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);
 
         qemu_bh_schedule(pool->completion_bh);
         qemu_mutex_lock(&pool->lock);
@@ -XXX,XX +XXX,XX @@ static void thread_pool_completion_bh(void *opaque)
 
 restart:
     QLIST_FOREACH_SAFE(elem, &pool->head, all, next) {
-        if (elem->state != THREAD_DONE) {
+        /* Atomically access the state field to synchronize with the
+         * worker_thread and thread_pool_cancel.
+         */
+        if (qatomic_load_acquire(&elem->state) != THREAD_DONE) {
             continue;
         }
 
@@ -XXX,XX +XXX,XX @@ 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.
              */
@@ -XXX,XX +XXX,XX @@ 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_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;
+        /* Atomically update state after setting ret. */
+        qatomic_store_release(&elem->state, THREAD_DONE);
     }
 
 }
-- 
2.34.1