[Qemu-devel] [PATCH v2 00/11] convert CPU list to RCU

[Qemu-devel] [PATCH v2 00/11] convert CPU list to RCU

[Emilio G. Cota]

v1: https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg02179.html

Changes since v1:

- Rebase on master
- Add David's Acked-by tag to the spapr patch
- Add 2 patches on QLIST_{EMPTY,REMOVE}_RCU
- Add some fixes for test-rcu-list. I wanted to be able to get no
  races with ThreadSanitizer, but it still warns about two races.
  I'm appending the report just in case, but I think tsan is getting
  confused.
- Add RCU QSIMPLEQ and RCU QTAILQ, piggy-backing their testing
  on test-rcu-list.
- Use an RCU QTAILQ instead of an RCU QLIST for the CPU list.
  - Drop the patch that added the CPUState.in_list field,
    since with the QTAILQ it's not necessary.
- Convert a caller in target/s390x that I missed in v1.

You can fetch this series from:
  https://github.com/cota/qemu/tree/rcu-cpulist-v2

Thanks,

		Emilio
---
The aforementioned TSan report:

$ make -j 12 tests/test-rcu-simpleq tests/test-rcu-list && tests/test-rcu-list 1 1
  CC      tests/test-rcu-simpleq.o
  CC      tests/test-rcu-list.o
  LINK    tests/test-rcu-list
  LINK    tests/test-rcu-simpleq
==================
WARNING: ThreadSanitizer: data race (pid=15248)
  Atomic read of size 8 at 0x7b0800005600 by thread T2:
    #0 __tsan_atomic64_load ../../../../gcc-8.1.0/libsanitizer/tsan/tsan_interface_atomic.cc:538 (libtsan.so.0+0x6080e)
    #1 rcu_q_reader /data/src/qemu/tests/test-rcu-list.c:166 (test-rcu-list+0x9294)
    #2 qemu_thread_start /data/src/qemu/util/qemu-thread-posix.c:504 (test-rcu-list+0x9af8)

  Previous write of size 8 at 0x7b0800005600 by thread T3:
    #0 malloc ../../../../gcc-8.1.0/libsanitizer/tsan/tsan_interceptors.cc:606 (libtsan.so.0+0x2a2f3)
    #1 g_malloc <null> (libglib-2.0.so.0+0x51858)
    #2 qemu_thread_start /data/src/qemu/util/qemu-thread-posix.c:504 (test-rcu-list+0x9af8)

  As if synchronized via sleep:
    #0 nanosleep ../../../../gcc-8.1.0/libsanitizer/tsan/tsan_interceptors.cc:366 (libtsan.so.0+0x48d20)
    #1 g_usleep <null> (libglib-2.0.so.0+0x754de)
    #2 qemu_thread_start /data/src/qemu/util/qemu-thread-posix.c:504 (test-rcu-list+0x9af8)

  Location is heap block of size 32 at 0x7b0800005600 allocated by thread T3:
    #0 malloc ../../../../gcc-8.1.0/libsanitizer/tsan/tsan_interceptors.cc:606 (libtsan.so.0+0x2a2f3)
    #1 g_malloc <null> (libglib-2.0.so.0+0x51858)
    #2 qemu_thread_start /data/src/qemu/util/qemu-thread-posix.c:504 (test-rcu-list+0x9af8)

  Thread T2 (tid=15251, running) created by main thread at:
    #0 pthread_create ../../../../gcc-8.1.0/libsanitizer/tsan/tsan_interceptors.cc:915 (libtsan.so.0+0x2af6b)
    #1 qemu_thread_create /data/src/qemu/util/qemu-thread-posix.c:534 (test-rcu-list+0xadc8)
    #2 create_thread /data/src/qemu/tests/test-rcu-list.c:70 (test-rcu-list+0x944f)
    #3 rcu_qtest /data/src/qemu/tests/test-rcu-list.c:278 (test-rcu-list+0x95ea)
    #4 main /data/src/qemu/tests/test-rcu-list.c:357 (test-rcu-list+0x893f)

  Thread T3 (tid=15252, running) created by main thread at:
    #0 pthread_create ../../../../gcc-8.1.0/libsanitizer/tsan/tsan_interceptors.cc:915 (libtsan.so.0+0x2af6b)
    #1 qemu_thread_create /data/src/qemu/util/qemu-thread-posix.c:534 (test-rcu-list+0xadc8)
    #2 create_thread /data/src/qemu/tests/test-rcu-list.c:70 (test-rcu-list+0x944f)
    #3 rcu_qtest /data/src/qemu/tests/test-rcu-list.c:280 (test-rcu-list+0x9606)
    #4 main /data/src/qemu/tests/test-rcu-list.c:357 (test-rcu-list+0x893f)

SUMMARY: ThreadSanitizer: data race /data/src/qemu/tests/test-rcu-list.c:166 in rcu_q_reader
==================
==================
WARNING: ThreadSanitizer: data race (pid=15248)
  Write of size 8 at 0x7b080000e880 by thread T1:
    #0 free ../../../../gcc-8.1.0/libsanitizer/tsan/tsan_interceptors.cc:649 (libtsan.so.0+0x2a5ba)
    #1 reclaim_list_el /data/src/qemu/tests/test-rcu-list.c:105 (test-rcu-list+0x8e66)
    #2 call_rcu_thread /data/src/qemu/util/rcu.c:284 (test-rcu-list+0xbb57)
    #3 qemu_thread_start /data/src/qemu/util/qemu-thread-posix.c:504 (test-rcu-list+0x9af8)

  Previous atomic read of size 8 at 0x7b080000e880 by thread T2:
    #0 __tsan_atomic64_load ../../../../gcc-8.1.0/libsanitizer/tsan/tsan_interface_atomic.cc:538 (libtsan.so.0+0x6080e)
    #1 rcu_q_reader /data/src/qemu/tests/test-rcu-list.c:166 (test-rcu-list+0x9294)
    #2 qemu_thread_start /data/src/qemu/util/qemu-thread-posix.c:504 (test-rcu-list+0x9af8)

  Thread T1 (tid=15250, running) created by main thread at:
    #0 pthread_create ../../../../gcc-8.1.0/libsanitizer/tsan/tsan_interceptors.cc:915 (libtsan.so.0+0x2af6b)
    #1 qemu_thread_create /data/src/qemu/util/qemu-thread-posix.c:534 (test-rcu-list+0xadc8)
    #2 rcu_init_complete /data/src/qemu/util/rcu.c:327 (test-rcu-list+0xb9f2)
    #3 rcu_init /data/src/qemu/util/rcu.c:383 (test-rcu-list+0x89fc)
    #4 __libc_csu_init <null> (test-rcu-list+0x35f9c)

  Thread T2 (tid=15251, running) created by main thread at:
    #0 pthread_create ../../../../gcc-8.1.0/libsanitizer/tsan/tsan_interceptors.cc:915 (libtsan.so.0+0x2af6b)
    #1 qemu_thread_create /data/src/qemu/util/qemu-thread-posix.c:534 (test-rcu-list+0xadc8)
    #2 create_thread /data/src/qemu/tests/test-rcu-list.c:70 (test-rcu-list+0x944f)
    #3 rcu_qtest /data/src/qemu/tests/test-rcu-list.c:278 (test-rcu-list+0x95ea)
    #4 main /data/src/qemu/tests/test-rcu-list.c:357 (test-rcu-list+0x893f)

SUMMARY: ThreadSanitizer: data race /data/src/qemu/tests/test-rcu-list.c:105 in reclaim_list_el
==================
tests/test-rcu-list: 1 readers; 1 updater; nodes read: 375386, nodes removed: 78728; nodes reclaimed: 78728
ThreadSanitizer: reported 2 warnings

[Qemu-devel] [PATCH v2 01/11] rcu_queue: use atomic_set in QLIST_REMOVE_RCU

[Emilio G. Cota]

To avoid undefined behaviour.

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 include/qemu/rcu_queue.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/qemu/rcu_queue.h b/include/qemu/rcu_queue.h
index 01be77407b..dd7b3be043 100644
--- a/include/qemu/rcu_queue.h
+++ b/include/qemu/rcu_queue.h
@@ -112,7 +112,7 @@ extern "C" {
        (elm)->field.le_next->field.le_prev =        \
         (elm)->field.le_prev;                       \
     }                                               \
-    *(elm)->field.le_prev =  (elm)->field.le_next;  \
+    atomic_set((elm)->field.le_prev, (elm)->field.le_next); \
 } while (/*CONSTCOND*/0)
 
 /* List traversal must occur within an RCU critical section.  */
-- 
2.17.1

[Qemu-devel] [PATCH v2 02/11] rcu_queue: remove barrier from QLIST_EMPTY_RCU

[Emilio G. Cota]

It's unnecessary because the pointer isn't dereferenced.

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 include/qemu/rcu_queue.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/qemu/rcu_queue.h b/include/qemu/rcu_queue.h
index dd7b3be043..6881ea5274 100644
--- a/include/qemu/rcu_queue.h
+++ b/include/qemu/rcu_queue.h
@@ -36,7 +36,7 @@ extern "C" {
 /*
  * List access methods.
  */
-#define QLIST_EMPTY_RCU(head) (atomic_rcu_read(&(head)->lh_first) == NULL)
+#define QLIST_EMPTY_RCU(head) (atomic_read(&(head)->lh_first) == NULL)
 #define QLIST_FIRST_RCU(head) (atomic_rcu_read(&(head)->lh_first))
 #define QLIST_NEXT_RCU(elm, field) (atomic_rcu_read(&(elm)->field.le_next))
 
-- 
2.17.1

[Qemu-devel] [PATCH v2 03/11] rcu_queue: add RCU QSIMPLEQ

[Emilio G. Cota]

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 include/qemu/rcu_queue.h | 65 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/include/qemu/rcu_queue.h b/include/qemu/rcu_queue.h
index 6881ea5274..e0395c989a 100644
--- a/include/qemu/rcu_queue.h
+++ b/include/qemu/rcu_queue.h
@@ -128,6 +128,71 @@ extern "C" {
           ((next_var) = atomic_rcu_read(&(var)->field.le_next), 1);  \
            (var) = (next_var))
 
+/*
+ * RCU simple queue
+ */
+
+/* Simple queue access methods */
+#define QSIMPLEQ_EMPTY_RCU(head)      (atomic_read(&(head)->sqh_first) == NULL)
+#define QSIMPLEQ_FIRST_RCU(head)       atomic_rcu_read(&(head)->sqh_first)
+#define QSIMPLEQ_NEXT_RCU(elm, field)  atomic_rcu_read(&(elm)->field.sqe_next)
+
+/* Simple queue functions */
+#define QSIMPLEQ_INSERT_HEAD_RCU(head, elm, field) do {         \
+    (elm)->field.sqe_next = (head)->sqh_first;                  \
+    if ((elm)->field.sqe_next == NULL) {                        \
+        (head)->sqh_last = &(elm)->field.sqe_next;              \
+    }                                                           \
+    atomic_rcu_set(&(head)->sqh_first, (elm));                  \
+} while (/*CONSTCOND*/0)
+
+#define QSIMPLEQ_INSERT_TAIL_RCU(head, elm, field) do {    \
+    (elm)->field.sqe_next = NULL;                          \
+    atomic_rcu_set((head)->sqh_last, (elm));               \
+    (head)->sqh_last = &(elm)->field.sqe_next;             \
+} while (/*CONSTCOND*/0)
+
+#define QSIMPLEQ_INSERT_AFTER_RCU(head, listelm, elm, field) do {       \
+    (elm)->field.sqe_next = (listelm)->field.sqe_next;                  \
+    if ((elm)->field.sqe_next == NULL) {                                \
+        (head)->sqh_last = &(elm)->field.sqe_next;                      \
+    }                                                                   \
+    atomic_rcu_set(&(listelm)->field.sqe_next, (elm));                  \
+} while (/*CONSTCOND*/0)
+
+#define QSIMPLEQ_REMOVE_HEAD_RCU(head, field) do {                     \
+    atomic_set(&(head)->sqh_first, (head)->sqh_first->field.sqe_next); \
+    if ((head)->sqh_first == NULL) {                                   \
+        (head)->sqh_last = &(head)->sqh_first;                         \
+    }                                                                  \
+} while (/*CONSTCOND*/0)
+
+#define QSIMPLEQ_REMOVE_RCU(head, elm, type, field) do {            \
+    if ((head)->sqh_first == (elm)) {                               \
+        QSIMPLEQ_REMOVE_HEAD_RCU((head), field);                    \
+    } else {                                                        \
+        struct type *curr = (head)->sqh_first;                      \
+        while (curr->field.sqe_next != (elm)) {                     \
+            curr = curr->field.sqe_next;                            \
+        }                                                           \
+        atomic_set(&curr->field.sqe_next,                           \
+                   curr->field.sqe_next->field.sqe_next);           \
+        if (curr->field.sqe_next == NULL) {                         \
+            (head)->sqh_last = &(curr)->field.sqe_next;             \
+        }                                                           \
+    }                                                               \
+} while (/*CONSTCOND*/0)
+
+#define QSIMPLEQ_FOREACH_RCU(var, head, field)                          \
+    for ((var) = atomic_rcu_read(&(head)->sqh_first);                   \
+         (var);                                                         \
+         (var) = atomic_rcu_read(&(var)->field.sqe_next))
+
+#define QSIMPLEQ_FOREACH_SAFE_RCU(var, head, field, next)                \
+    for ((var) = atomic_rcu_read(&(head)->sqh_first);                    \
+         (var) && ((next) = atomic_rcu_read(&(var)->field.sqe_next), 1); \
+         (var) = (next))
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.17.1

[Qemu-devel] [PATCH v2 04/11] rcu_queue: add RCU QTAILQ

[Emilio G. Cota]

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 include/qemu/rcu_queue.h | 66 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/include/qemu/rcu_queue.h b/include/qemu/rcu_queue.h
index e0395c989a..904b3372dc 100644
--- a/include/qemu/rcu_queue.h
+++ b/include/qemu/rcu_queue.h
@@ -193,6 +193,72 @@ extern "C" {
          (var) && ((next) = atomic_rcu_read(&(var)->field.sqe_next), 1); \
          (var) = (next))
 
+/*
+ * RCU tail queue
+ */
+
+/* Tail queue access methods */
+#define QTAILQ_EMPTY_RCU(head)      (atomic_read(&(head)->tqh_first) == NULL)
+#define QTAILQ_FIRST_RCU(head)       atomic_rcu_read(&(head)->tqh_first)
+#define QTAILQ_NEXT_RCU(elm, field)  atomic_rcu_read(&(elm)->field.tqe_next)
+
+/* Tail queue functions */
+#define QTAILQ_INSERT_HEAD_RCU(head, elm, field) do {                   \
+    (elm)->field.tqe_next = (head)->tqh_first;                          \
+    if ((elm)->field.tqe_next != NULL) {                                \
+        (head)->tqh_first->field.tqe_prev = &(elm)->field.tqe_next;     \
+    } else {                                                            \
+        (head)->tqh_last = &(elm)->field.tqe_next;                      \
+    }                                                                   \
+    atomic_rcu_set(&(head)->tqh_first, (elm));                          \
+    (elm)->field.tqe_prev = &(head)->tqh_first;                         \
+} while (/*CONSTCOND*/0)
+
+#define QTAILQ_INSERT_TAIL_RCU(head, elm, field) do {               \
+    (elm)->field.tqe_next = NULL;                                   \
+    (elm)->field.tqe_prev = (head)->tqh_last;                       \
+    atomic_rcu_set((head)->tqh_last, (elm));                        \
+    (head)->tqh_last = &(elm)->field.tqe_next;                      \
+} while (/*CONSTCOND*/0)
+
+#define QTAILQ_INSERT_AFTER_RCU(head, listelm, elm, field) do {         \
+    (elm)->field.tqe_next = (listelm)->field.tqe_next;                  \
+    if ((elm)->field.tqe_next != NULL) {                                \
+        (elm)->field.tqe_next->field.tqe_prev = &(elm)->field.tqe_next; \
+    } else {                                                            \
+        (head)->tqh_last = &(elm)->field.tqe_next;                      \
+    }                                                                   \
+    atomic_rcu_set(&(listelm)->field.tqe_next, (elm));                  \
+    (elm)->field.tqe_prev = &(listelm)->field.tqe_next;                 \
+} while (/*CONSTCOND*/0)
+
+#define QTAILQ_INSERT_BEFORE_RCU(listelm, elm, field) do {          \
+    (elm)->field.tqe_prev = (listelm)->field.tqe_prev;              \
+    (elm)->field.tqe_next = (listelm);                              \
+    atomic_rcu_set((listelm)->field.tqe_prev, (elm));               \
+    (listelm)->field.tqe_prev = &(elm)->field.tqe_next;             \
+    } while (/*CONSTCOND*/0)
+
+#define QTAILQ_REMOVE_RCU(head, elm, field) do {                        \
+    if (((elm)->field.tqe_next) != NULL) {                              \
+        (elm)->field.tqe_next->field.tqe_prev = (elm)->field.tqe_prev;  \
+    } else {                                                            \
+        (head)->tqh_last = (elm)->field.tqe_prev;                       \
+    }                                                                   \
+    atomic_set((elm)->field.tqe_prev, (elm)->field.tqe_next);           \
+    (elm)->field.tqe_prev = NULL;                                       \
+} while (/*CONSTCOND*/0)
+
+#define QTAILQ_FOREACH_RCU(var, head, field)                            \
+    for ((var) = atomic_rcu_read(&(head)->tqh_first);                   \
+         (var);                                                         \
+         (var) = atomic_rcu_read(&(var)->field.tqe_next))
+
+#define QTAILQ_FOREACH_SAFE_RCU(var, head, field, next)                  \
+    for ((var) = atomic_rcu_read(&(head)->tqh_first);                    \
+         (var) && ((next) = atomic_rcu_read(&(var)->field.tqe_next), 1); \
+         (var) = (next))
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.17.1

[Qemu-devel] [PATCH v2 05/11] test-rcu-list: access goflag with atomics

[Emilio G. Cota]

Instead of declaring it volatile.

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 tests/test-rcu-list.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/tests/test-rcu-list.c b/tests/test-rcu-list.c
index 1514d7ec97..b4ed130081 100644
--- a/tests/test-rcu-list.c
+++ b/tests/test-rcu-list.c
@@ -44,7 +44,7 @@ static int nthreadsrunning;
 #define GOFLAG_RUN  1
 #define GOFLAG_STOP 2
 
-static volatile int goflag = GOFLAG_INIT;
+static int goflag = GOFLAG_INIT;
 
 #define RCU_READ_RUN 1000
 #define RCU_UPDATE_RUN 10
@@ -107,15 +107,15 @@ static void *rcu_q_reader(void *arg)
 
     *(struct rcu_reader_data **)arg = &rcu_reader;
     atomic_inc(&nthreadsrunning);
-    while (goflag == GOFLAG_INIT) {
+    while (atomic_read(&goflag) == GOFLAG_INIT) {
         g_usleep(1000);
     }
 
-    while (goflag == GOFLAG_RUN) {
+    while (atomic_read(&goflag) == GOFLAG_RUN) {
         rcu_read_lock();
         QLIST_FOREACH_RCU(el, &Q_list_head, entry) {
             n_reads_local++;
-            if (goflag == GOFLAG_STOP) {
+            if (atomic_read(&goflag) == GOFLAG_STOP) {
                 break;
             }
         }
@@ -142,11 +142,11 @@ static void *rcu_q_updater(void *arg)
 
     *(struct rcu_reader_data **)arg = &rcu_reader;
     atomic_inc(&nthreadsrunning);
-    while (goflag == GOFLAG_INIT) {
+    while (atomic_read(&goflag) == GOFLAG_INIT) {
         g_usleep(1000);
     }
 
-    while (goflag == GOFLAG_RUN) {
+    while (atomic_read(&goflag) == GOFLAG_RUN) {
         target_el = select_random_el(RCU_Q_LEN);
         j = 0;
         /* FOREACH_RCU could work here but let's use both macros */
@@ -160,7 +160,7 @@ static void *rcu_q_updater(void *arg)
                 break;
             }
         }
-        if (goflag == GOFLAG_STOP) {
+        if (atomic_read(&goflag) == GOFLAG_STOP) {
             break;
         }
         target_el = select_random_el(RCU_Q_LEN);
@@ -209,9 +209,9 @@ static void rcu_qtest_run(int duration, int nreaders)
         g_usleep(1000);
     }
 
-    goflag = GOFLAG_RUN;
+    atomic_set(&goflag, GOFLAG_RUN);
     sleep(duration);
-    goflag = GOFLAG_STOP;
+    atomic_set(&goflag, GOFLAG_STOP);
     wait_all_threads();
 }
 
-- 
2.17.1

[Qemu-devel] [PATCH v2 06/11] test-rcu-list: access counters with atomics

[Emilio G. Cota]

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 tests/test-rcu-list.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/tests/test-rcu-list.c b/tests/test-rcu-list.c
index b4ed130081..dc58091996 100644
--- a/tests/test-rcu-list.c
+++ b/tests/test-rcu-list.c
@@ -93,7 +93,7 @@ static void reclaim_list_el(struct rcu_head *prcu)
     struct list_element *el = container_of(prcu, struct list_element, rcu);
     g_free(el);
     /* Accessed only from call_rcu thread.  */
-    n_reclaims++;
+    atomic_set(&n_reclaims, n_reclaims + 1);
 }
 
 static QLIST_HEAD(q_list_head, list_element) Q_list_head;
@@ -182,7 +182,7 @@ static void *rcu_q_updater(void *arg)
     qemu_mutex_lock(&counts_mutex);
     n_nodes += n_nodes_local;
     n_updates += n_updates_local;
-    n_nodes_removed += n_removed_local;
+    atomic_set(&n_nodes_removed, n_nodes_removed + n_removed_local);
     qemu_mutex_unlock(&counts_mutex);
     return NULL;
 }
@@ -239,16 +239,18 @@ static void rcu_qtest(const char *test, int duration, int nreaders)
     n_nodes_removed += n_removed_local;
     qemu_mutex_unlock(&counts_mutex);
     synchronize_rcu();
-    while (n_nodes_removed > n_reclaims) {
+    while (atomic_read(&n_nodes_removed) > atomic_read(&n_reclaims)) {
         g_usleep(100);
         synchronize_rcu();
     }
     if (g_test_in_charge) {
-        g_assert_cmpint(n_nodes_removed, ==, n_reclaims);
+        g_assert_cmpint(atomic_read(&n_nodes_removed), ==,
+                        atomic_read(&n_reclaims));
     } else {
         printf("%s: %d readers; 1 updater; nodes read: "  \
                "%lld, nodes removed: %lld; nodes reclaimed: %lld\n",
-               test, nthreadsrunning - 1, n_reads, n_nodes_removed, n_reclaims);
+               test, nthreadsrunning - 1, n_reads,
+               atomic_read(&n_nodes_removed), atomic_read(&n_reclaims));
         exit(0);
     }
 }
-- 
2.17.1

[Qemu-devel] [PATCH v2 07/11] test-rcu-list: abstract the list implementation

[Emilio G. Cota]

So that we can test other implementations.

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 tests/test-rcu-list.c | 42 ++++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 12 deletions(-)

diff --git a/tests/test-rcu-list.c b/tests/test-rcu-list.c
index dc58091996..9bd11367a0 100644
--- a/tests/test-rcu-list.c
+++ b/tests/test-rcu-list.c
@@ -82,9 +82,16 @@ static void wait_all_threads(void)
     n_threads = 0;
 }
 
+#ifndef TEST_LIST_TYPE
+#define TEST_LIST_TYPE 1
+#endif
 
 struct list_element {
+#if TEST_LIST_TYPE == 1
     QLIST_ENTRY(list_element) entry;
+#else
+#error Invalid TEST_LIST_TYPE
+#endif
     struct rcu_head rcu;
 };
 
@@ -96,8 +103,19 @@ static void reclaim_list_el(struct rcu_head *prcu)
     atomic_set(&n_reclaims, n_reclaims + 1);
 }
 
+#if TEST_LIST_TYPE == 1
 static QLIST_HEAD(q_list_head, list_element) Q_list_head;
 
+#define TEST_NAME "qlist"
+#define TEST_LIST_REMOVE_RCU        QLIST_REMOVE_RCU
+#define TEST_LIST_INSERT_AFTER_RCU  QLIST_INSERT_AFTER_RCU
+#define TEST_LIST_INSERT_HEAD_RCU   QLIST_INSERT_HEAD_RCU
+#define TEST_LIST_FOREACH_RCU       QLIST_FOREACH_RCU
+#define TEST_LIST_FOREACH_SAFE_RCU  QLIST_FOREACH_SAFE_RCU
+#else
+#error Invalid TEST_LIST_TYPE
+#endif
+
 static void *rcu_q_reader(void *arg)
 {
     long long n_reads_local = 0;
@@ -113,7 +131,7 @@ static void *rcu_q_reader(void *arg)
 
     while (atomic_read(&goflag) == GOFLAG_RUN) {
         rcu_read_lock();
-        QLIST_FOREACH_RCU(el, &Q_list_head, entry) {
+        TEST_LIST_FOREACH_RCU(el, &Q_list_head, entry) {
             n_reads_local++;
             if (atomic_read(&goflag) == GOFLAG_STOP) {
                 break;
@@ -150,10 +168,10 @@ static void *rcu_q_updater(void *arg)
         target_el = select_random_el(RCU_Q_LEN);
         j = 0;
         /* FOREACH_RCU could work here but let's use both macros */
-        QLIST_FOREACH_SAFE_RCU(prev_el, &Q_list_head, entry, el) {
+        TEST_LIST_FOREACH_SAFE_RCU(prev_el, &Q_list_head, entry, el) {
             j++;
             if (target_el == j) {
-                QLIST_REMOVE_RCU(prev_el, entry);
+                TEST_LIST_REMOVE_RCU(prev_el, entry);
                 /* may be more than one updater in the future */
                 call_rcu1(&prev_el->rcu, reclaim_list_el);
                 n_removed_local++;
@@ -165,12 +183,12 @@ static void *rcu_q_updater(void *arg)
         }
         target_el = select_random_el(RCU_Q_LEN);
         j = 0;
-        QLIST_FOREACH_RCU(el, &Q_list_head, entry) {
+        TEST_LIST_FOREACH_RCU(el, &Q_list_head, entry) {
             j++;
             if (target_el == j) {
-                prev_el = g_new(struct list_element, 1);
+                struct list_element *new_el = g_new(struct list_element, 1);
                 n_nodes += n_nodes_local;
-                QLIST_INSERT_BEFORE_RCU(el, prev_el, entry);
+                TEST_LIST_INSERT_AFTER_RCU(el, new_el, entry);
                 break;
             }
         }
@@ -195,7 +213,7 @@ static void rcu_qtest_init(void)
     srand(time(0));
     for (i = 0; i < RCU_Q_LEN; i++) {
         new_el = g_new(struct list_element, 1);
-        QLIST_INSERT_HEAD_RCU(&Q_list_head, new_el, entry);
+        TEST_LIST_INSERT_HEAD_RCU(&Q_list_head, new_el, entry);
     }
     qemu_mutex_lock(&counts_mutex);
     n_nodes += RCU_Q_LEN;
@@ -230,8 +248,8 @@ static void rcu_qtest(const char *test, int duration, int nreaders)
     create_thread(rcu_q_updater);
     rcu_qtest_run(duration, nreaders);
 
-    QLIST_FOREACH_SAFE_RCU(prev_el, &Q_list_head, entry, el) {
-        QLIST_REMOVE_RCU(prev_el, entry);
+    TEST_LIST_FOREACH_SAFE_RCU(prev_el, &Q_list_head, entry, el) {
+        TEST_LIST_REMOVE_RCU(prev_el, entry);
         call_rcu1(&prev_el->rcu, reclaim_list_el);
         n_removed_local++;
     }
@@ -292,9 +310,9 @@ int main(int argc, char *argv[])
             } else {
                 gtest_seconds = 20;
             }
-            g_test_add_func("/rcu/qlist/single-threaded", gtest_rcuq_one);
-            g_test_add_func("/rcu/qlist/short-few", gtest_rcuq_few);
-            g_test_add_func("/rcu/qlist/long-many", gtest_rcuq_many);
+            g_test_add_func("/rcu/"TEST_NAME"/single-threaded", gtest_rcuq_one);
+            g_test_add_func("/rcu/"TEST_NAME"/short-few", gtest_rcuq_few);
+            g_test_add_func("/rcu/"TEST_NAME"/long-many", gtest_rcuq_many);
             g_test_in_charge = 1;
             return g_test_run();
         }
-- 
2.17.1

[Qemu-devel] [PATCH v2 08/11] tests: add test-list-simpleq

[Emilio G. Cota]

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 tests/test-rcu-list.c    | 17 +++++++++++++++++
 tests/test-rcu-simpleq.c |  2 ++
 tests/Makefile.include   |  4 ++++
 3 files changed, 23 insertions(+)
 create mode 100644 tests/test-rcu-simpleq.c

diff --git a/tests/test-rcu-list.c b/tests/test-rcu-list.c
index 9bd11367a0..c8bdf49743 100644
--- a/tests/test-rcu-list.c
+++ b/tests/test-rcu-list.c
@@ -89,6 +89,8 @@ static void wait_all_threads(void)
 struct list_element {
 #if TEST_LIST_TYPE == 1
     QLIST_ENTRY(list_element) entry;
+#elif TEST_LIST_TYPE == 2
+    QSIMPLEQ_ENTRY(list_element) entry;
 #else
 #error Invalid TEST_LIST_TYPE
 #endif
@@ -112,6 +114,21 @@ static QLIST_HEAD(q_list_head, list_element) Q_list_head;
 #define TEST_LIST_INSERT_HEAD_RCU   QLIST_INSERT_HEAD_RCU
 #define TEST_LIST_FOREACH_RCU       QLIST_FOREACH_RCU
 #define TEST_LIST_FOREACH_SAFE_RCU  QLIST_FOREACH_SAFE_RCU
+
+#elif TEST_LIST_TYPE == 2
+static QSIMPLEQ_HEAD(, list_element) Q_list_head =
+    QSIMPLEQ_HEAD_INITIALIZER(Q_list_head);
+
+#define TEST_NAME "qsimpleq"
+#define TEST_LIST_REMOVE_RCU(el, f)                             \
+         QSIMPLEQ_REMOVE_RCU(&Q_list_head, el, list_element, f)
+
+#define TEST_LIST_INSERT_AFTER_RCU(list_el, el, f)               \
+         QSIMPLEQ_INSERT_AFTER_RCU(&Q_list_head, list_el, el, f)
+
+#define TEST_LIST_INSERT_HEAD_RCU   QSIMPLEQ_INSERT_HEAD_RCU
+#define TEST_LIST_FOREACH_RCU       QSIMPLEQ_FOREACH_RCU
+#define TEST_LIST_FOREACH_SAFE_RCU  QSIMPLEQ_FOREACH_SAFE_RCU
 #else
 #error Invalid TEST_LIST_TYPE
 #endif
diff --git a/tests/test-rcu-simpleq.c b/tests/test-rcu-simpleq.c
new file mode 100644
index 0000000000..057f7d33f7
--- /dev/null
+++ b/tests/test-rcu-simpleq.c
@@ -0,0 +1,2 @@
+#define TEST_LIST_TYPE 2
+#include "test-rcu-list.c"
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 760a0f18b6..997c27421a 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -116,6 +116,8 @@ check-unit-y += tests/rcutorture$(EXESUF)
 gcov-files-rcutorture-y = util/rcu.c
 check-unit-y += tests/test-rcu-list$(EXESUF)
 gcov-files-test-rcu-list-y = util/rcu.c
+check-unit-y += tests/test-rcu-simpleq$(EXESUF)
+gcov-files-test-rcu-simpleq-y = util/rcu.c
 check-unit-y += tests/test-qdist$(EXESUF)
 gcov-files-test-qdist-y = util/qdist.c
 check-unit-y += tests/test-qht$(EXESUF)
@@ -600,6 +602,7 @@ test-obj-y = tests/check-qnum.o tests/check-qstring.o tests/check-qdict.o \
 	tests/test-x86-cpuid.o tests/test-mul64.o tests/test-int128.o \
 	tests/test-opts-visitor.o tests/test-qmp-event.o \
 	tests/rcutorture.o tests/test-rcu-list.o \
+	tests/test-rcu-simpleq.o \
 	tests/test-qdist.o tests/test-shift128.o \
 	tests/test-qht.o tests/qht-bench.o tests/test-qht-par.o \
 	tests/atomic_add-bench.o
@@ -649,6 +652,7 @@ tests/test-cutils$(EXESUF): tests/test-cutils.o util/cutils.o $(test-util-obj-y)
 tests/test-int128$(EXESUF): tests/test-int128.o
 tests/rcutorture$(EXESUF): tests/rcutorture.o $(test-util-obj-y)
 tests/test-rcu-list$(EXESUF): tests/test-rcu-list.o $(test-util-obj-y)
+tests/test-rcu-simpleq$(EXESUF): tests/test-rcu-simpleq.o $(test-util-obj-y)
 tests/test-qdist$(EXESUF): tests/test-qdist.o $(test-util-obj-y)
 tests/test-qht$(EXESUF): tests/test-qht.o $(test-util-obj-y)
 tests/test-qht-par$(EXESUF): tests/test-qht-par.o tests/qht-bench$(EXESUF) $(test-util-obj-y)
-- 
2.17.1

[Qemu-devel] [PATCH v2 09/11] tests: add test-rcu-tailq

[Emilio G. Cota]

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 tests/test-rcu-list.c  | 15 +++++++++++++++
 tests/test-rcu-tailq.c |  2 ++
 tests/Makefile.include |  4 ++++
 3 files changed, 21 insertions(+)
 create mode 100644 tests/test-rcu-tailq.c

diff --git a/tests/test-rcu-list.c b/tests/test-rcu-list.c
index c8bdf49743..8434700746 100644
--- a/tests/test-rcu-list.c
+++ b/tests/test-rcu-list.c
@@ -91,6 +91,8 @@ struct list_element {
     QLIST_ENTRY(list_element) entry;
 #elif TEST_LIST_TYPE == 2
     QSIMPLEQ_ENTRY(list_element) entry;
+#elif TEST_LIST_TYPE == 3
+    QTAILQ_ENTRY(list_element) entry;
 #else
 #error Invalid TEST_LIST_TYPE
 #endif
@@ -129,6 +131,19 @@ static QSIMPLEQ_HEAD(, list_element) Q_list_head =
 #define TEST_LIST_INSERT_HEAD_RCU   QSIMPLEQ_INSERT_HEAD_RCU
 #define TEST_LIST_FOREACH_RCU       QSIMPLEQ_FOREACH_RCU
 #define TEST_LIST_FOREACH_SAFE_RCU  QSIMPLEQ_FOREACH_SAFE_RCU
+
+#elif TEST_LIST_TYPE == 3
+static QTAILQ_HEAD(, list_element) Q_list_head;
+
+#define TEST_NAME "qtailq"
+#define TEST_LIST_REMOVE_RCU(el, f) QTAILQ_REMOVE_RCU(&Q_list_head, el, f)
+
+#define TEST_LIST_INSERT_AFTER_RCU(list_el, el, f)               \
+           QTAILQ_INSERT_AFTER_RCU(&Q_list_head, list_el, el, f)
+
+#define TEST_LIST_INSERT_HEAD_RCU   QTAILQ_INSERT_HEAD_RCU
+#define TEST_LIST_FOREACH_RCU       QTAILQ_FOREACH_RCU
+#define TEST_LIST_FOREACH_SAFE_RCU  QTAILQ_FOREACH_SAFE_RCU
 #else
 #error Invalid TEST_LIST_TYPE
 #endif
diff --git a/tests/test-rcu-tailq.c b/tests/test-rcu-tailq.c
new file mode 100644
index 0000000000..8d487e0ee0
--- /dev/null
+++ b/tests/test-rcu-tailq.c
@@ -0,0 +1,2 @@
+#define TEST_LIST_TYPE 3
+#include "test-rcu-list.c"
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 997c27421a..5fe32fcfd0 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -118,6 +118,8 @@ check-unit-y += tests/test-rcu-list$(EXESUF)
 gcov-files-test-rcu-list-y = util/rcu.c
 check-unit-y += tests/test-rcu-simpleq$(EXESUF)
 gcov-files-test-rcu-simpleq-y = util/rcu.c
+check-unit-y += tests/test-rcu-tailq$(EXESUF)
+gcov-files-test-rcu-tailq-y = util/rcu.c
 check-unit-y += tests/test-qdist$(EXESUF)
 gcov-files-test-qdist-y = util/qdist.c
 check-unit-y += tests/test-qht$(EXESUF)
@@ -603,6 +605,7 @@ test-obj-y = tests/check-qnum.o tests/check-qstring.o tests/check-qdict.o \
 	tests/test-opts-visitor.o tests/test-qmp-event.o \
 	tests/rcutorture.o tests/test-rcu-list.o \
 	tests/test-rcu-simpleq.o \
+	tests/test-rcu-tailq.o \
 	tests/test-qdist.o tests/test-shift128.o \
 	tests/test-qht.o tests/qht-bench.o tests/test-qht-par.o \
 	tests/atomic_add-bench.o
@@ -653,6 +656,7 @@ tests/test-int128$(EXESUF): tests/test-int128.o
 tests/rcutorture$(EXESUF): tests/rcutorture.o $(test-util-obj-y)
 tests/test-rcu-list$(EXESUF): tests/test-rcu-list.o $(test-util-obj-y)
 tests/test-rcu-simpleq$(EXESUF): tests/test-rcu-simpleq.o $(test-util-obj-y)
+tests/test-rcu-tailq$(EXESUF): tests/test-rcu-tailq.o $(test-util-obj-y)
 tests/test-qdist$(EXESUF): tests/test-qdist.o $(test-util-obj-y)
 tests/test-qht$(EXESUF): tests/test-qht.o $(test-util-obj-y)
 tests/test-qht-par$(EXESUF): tests/test-qht-par.o tests/qht-bench$(EXESUF) $(test-util-obj-y)
-- 
2.17.1

[Qemu-devel] [PATCH v2 10/11] spapr: do not use CPU_FOREACH_REVERSE

[Emilio G. Cota]

This paves the way for implementing the CPU list with an RCU list,
which cannot be traversed in reverse order.

Note that this is the only caller of CPU_FOREACH_REVERSE.

Acked-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 hw/ppc/spapr.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 421b2dd09b..2ef5be2790 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -622,9 +622,12 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
 
 static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr)
 {
+    CPUState **rev;
     CPUState *cs;
+    int n_cpus;
     int cpus_offset;
     char *nodename;
+    int i;
 
     cpus_offset = fdt_add_subnode(fdt, 0, "cpus");
     _FDT(cpus_offset);
@@ -635,8 +638,19 @@ static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr)
      * We walk the CPUs in reverse order to ensure that CPU DT nodes
      * created by fdt_add_subnode() end up in the right order in FDT
      * for the guest kernel the enumerate the CPUs correctly.
+     *
+     * The CPU list cannot be traversed in reverse order, so we need
+     * to do extra work.
      */
-    CPU_FOREACH_REVERSE(cs) {
+    n_cpus = 0;
+    rev = NULL;
+    CPU_FOREACH(cs) {
+        rev = g_renew(CPUState *, rev, n_cpus + 1);
+        rev[n_cpus++] = cs;
+    }
+
+    for (i = n_cpus - 1; i >= 0; i--) {
+        CPUState *cs = rev[i];
         PowerPCCPU *cpu = POWERPC_CPU(cs);
         int index = spapr_get_vcpu_id(cpu);
         DeviceClass *dc = DEVICE_GET_CLASS(cs);
-- 
2.17.1

[Qemu-devel] [PATCH v2 11/11] qom: convert the CPU list to RCU

[Emilio G. Cota]

Iterating over the list without using atomics is undefined behaviour,
since the list can be modified concurrently by other threads (e.g.
every time a new thread is created in user-mode).

Fix it by implementing the CPU list as an RCU QTAILQ. This requires
a little bit of extra work to traverse list in reverse order (see
previous patch), but other than that the conversion is trivial.

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 include/qom/cpu.h         | 11 +++++------
 cpus-common.c             |  4 ++--
 cpus.c                    |  2 +-
 linux-user/main.c         |  2 +-
 linux-user/syscall.c      |  2 +-
 target/s390x/cpu_models.c |  2 +-
 6 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index ecf6ed556a..dc130cd307 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -26,6 +26,7 @@
 #include "exec/memattrs.h"
 #include "qapi/qapi-types-run-state.h"
 #include "qemu/bitmap.h"
+#include "qemu/rcu_queue.h"
 #include "qemu/queue.h"
 #include "qemu/thread.h"
 
@@ -442,13 +443,11 @@ struct CPUState {
 
 QTAILQ_HEAD(CPUTailQ, CPUState);
 extern struct CPUTailQ cpus;
-#define CPU_NEXT(cpu) QTAILQ_NEXT(cpu, node)
-#define CPU_FOREACH(cpu) QTAILQ_FOREACH(cpu, &cpus, node)
+#define first_cpu        QTAILQ_FIRST_RCU(&cpus)
+#define CPU_NEXT(cpu)    QTAILQ_NEXT_RCU(cpu, node)
+#define CPU_FOREACH(cpu) QTAILQ_FOREACH_RCU(cpu, &cpus, node)
 #define CPU_FOREACH_SAFE(cpu, next_cpu) \
-    QTAILQ_FOREACH_SAFE(cpu, &cpus, node, next_cpu)
-#define CPU_FOREACH_REVERSE(cpu) \
-    QTAILQ_FOREACH_REVERSE(cpu, &cpus, CPUTailQ, node)
-#define first_cpu QTAILQ_FIRST(&cpus)
+    QTAILQ_FOREACH_SAFE_RCU(cpu, &cpus, node, next_cpu)
 
 extern __thread CPUState *current_cpu;
 
diff --git a/cpus-common.c b/cpus-common.c
index 59f751ecf9..98dd8c6ff1 100644
--- a/cpus-common.c
+++ b/cpus-common.c
@@ -84,7 +84,7 @@ void cpu_list_add(CPUState *cpu)
     } else {
         assert(!cpu_index_auto_assigned);
     }
-    QTAILQ_INSERT_TAIL(&cpus, cpu, node);
+    QTAILQ_INSERT_TAIL_RCU(&cpus, cpu, node);
     qemu_mutex_unlock(&qemu_cpu_list_lock);
 
     finish_safe_work(cpu);
@@ -101,7 +101,7 @@ void cpu_list_remove(CPUState *cpu)
 
     assert(!(cpu_index_auto_assigned && cpu != QTAILQ_LAST(&cpus, CPUTailQ)));
 
-    QTAILQ_REMOVE(&cpus, cpu, node);
+    QTAILQ_REMOVE_RCU(&cpus, cpu, node);
     cpu->cpu_index = UNASSIGNED_CPU_INDEX;
     qemu_mutex_unlock(&qemu_cpu_list_lock);
 }
diff --git a/cpus.c b/cpus.c
index b5844b7103..d60f8603fd 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1491,7 +1491,7 @@ static void *qemu_tcg_rr_cpu_thread_fn(void *arg)
             atomic_mb_set(&cpu->exit_request, 0);
         }
 
-        qemu_tcg_rr_wait_io_event(cpu ? cpu : QTAILQ_FIRST(&cpus));
+        qemu_tcg_rr_wait_io_event(cpu ? cpu : first_cpu);
         deal_with_unplugged_cpus();
     }
 
diff --git a/linux-user/main.c b/linux-user/main.c
index ea00dd9057..923cbb753a 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -126,7 +126,7 @@ void fork_end(int child)
            Discard information about the parent threads.  */
         CPU_FOREACH_SAFE(cpu, next_cpu) {
             if (cpu != thread_cpu) {
-                QTAILQ_REMOVE(&cpus, cpu, node);
+                QTAILQ_REMOVE_RCU(&cpus, cpu, node);
             }
         }
         qemu_init_cpu_list();
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index bb42a225eb..95ac0102bf 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8051,7 +8051,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             TaskState *ts;
 
             /* Remove the CPU from the list.  */
-            QTAILQ_REMOVE(&cpus, cpu, node);
+            QTAILQ_REMOVE_RCU(&cpus, cpu, node);
 
             cpu_list_unlock();
 
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 604898a882..d1a45bd8c0 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -1110,7 +1110,7 @@ void s390_set_qemu_cpu_model(uint16_t type, uint8_t gen, uint8_t ec_ga,
     const S390CPUDef *def = s390_find_cpu_def(type, gen, ec_ga, NULL);
 
     g_assert(def);
-    g_assert(QTAILQ_EMPTY(&cpus));
+    g_assert(QTAILQ_EMPTY_RCU(&cpus));
 
     /* TCG emulates some features that can usually not be enabled with
      * the emulated machine generation. Make sure they can be enabled
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 00/11] convert CPU list to RCU

[Paolo Bonzini]

On 19/08/2018 11:13, Emilio G. Cota wrote:
> - Add some fixes for test-rcu-list. I wanted to be able to get no
>   races with ThreadSanitizer, but it still warns about two races.
>   I'm appending the report just in case, but I think tsan is getting
>   confused.

I cannot understand the first.  The second might be fixed by this untested
patch (the second hunk; the first is an optimization and clarification):

diff --git a/util/rcu.c b/util/rcu.c
index 5676c22bd1..314b7db1a6 100644
--- a/util/rcu.c
+++ b/util/rcu.c
@@ -192,7 +192,9 @@ static void enqueue(struct rcu_head *node)
 
     node->next = NULL;
     old_tail = atomic_xchg(&tail, &node->next);
-    atomic_mb_set(old_tail, node);
+
+    /* Pairs with smb_mb_acquire() in try_dequeue.  */
+    atomic_store_release(old_tail, node);
 }
 
 static struct rcu_head *try_dequeue(void)
@@ -213,7 +215,10 @@ retry:
      * wrong and we need to wait until its enqueuer finishes the update.
      */
     node = head;
-    next = atomic_mb_read(&head->next);
+    smp_mb_acquire();
+
+    /* atomic_read is enough because the pointer is never dereferenced.  */
+    next = atomic_read(&head->next);
     if (!next) {
         return NULL;
     }


The idea being that enqueue() writes so to speak node->prev->next in
the atomic_store_release when it enqueues node; try_dequeue() instead reads
node->next, so there is no synchronizes-with edge.  However, I'm not that
convinced that it's necessary...

Paolo

Re: [Qemu-devel] [PATCH v2 00/11] convert CPU list to RCU

[Emilio G. Cota]

On Mon, Aug 20, 2018 at 11:30:07 +0200, Paolo Bonzini wrote:
> On 19/08/2018 11:13, Emilio G. Cota wrote:
> > - Add some fixes for test-rcu-list. I wanted to be able to get no
> >   races with ThreadSanitizer, but it still warns about two races.
> >   I'm appending the report just in case, but I think tsan is getting
> >   confused.
> 
> I cannot understand the first.  The second might be fixed by this untested
> patch (the second hunk; the first is an optimization and clarification):
> 
> diff --git a/util/rcu.c b/util/rcu.c
> index 5676c22bd1..314b7db1a6 100644
> --- a/util/rcu.c
> +++ b/util/rcu.c
> @@ -192,7 +192,9 @@ static void enqueue(struct rcu_head *node)
>  
>      node->next = NULL;
>      old_tail = atomic_xchg(&tail, &node->next);
> -    atomic_mb_set(old_tail, node);
> +
> +    /* Pairs with smb_mb_acquire() in try_dequeue.  */
> +    atomic_store_release(old_tail, node);
>  }
>  
>  static struct rcu_head *try_dequeue(void)
> @@ -213,7 +215,10 @@ retry:
>       * wrong and we need to wait until its enqueuer finishes the update.
>       */
>      node = head;
> -    next = atomic_mb_read(&head->next);
> +    smp_mb_acquire();
> +
> +    /* atomic_read is enough because the pointer is never dereferenced.  */
> +    next = atomic_read(&head->next);
>      if (!next) {
>          return NULL;
>      }
> 
> 
> The idea being that enqueue() writes so to speak node->prev->next in
> the atomic_store_release when it enqueues node; try_dequeue() instead reads
> node->next, so there is no synchronizes-with edge.  However, I'm not that
> convinced that it's necessary...

This doesn't seem to help :-( I'd try to avoid standalone barriers
as much as possible, otherwise TSan gets confused (BTW this is why
seqlocks cannot be "understood" by TSan).

The cause of the warnings seem to be the calls to g_usleep (both in rcu.c
and in the test file), which lead TSan to believe that we might be
coordinating threads via sleep calls.

Substituting the sleep calls with cpu_relax gets rid of the warnings,
so I would say these warnings can be safely ignored.

Thanks,

		Emilio