[Qemu-devel] [PATCH 0/3] rcu: missing thread registration

[Qemu-devel] [PATCH 0/3] rcu: missing thread registration

[Paolo Bonzini]

Patch 2 fixes the problem, whereby threads were not being registered
but still happily used RCU.  Patch 1 simplifies the registration by
making rcu_unregister_thread automatic.  Patch 3 avoids having the
same bug in the future.

Paolo

Paolo Bonzini (3):
  rcu: automatically unregister threads when they exit
  rcu: actually register threads that have RCU read-side critical
    sections
  rcu: detect missing rcu_register_thread()

 cpus.c                |  6 ++++++
 include/qemu/rcu.h    |  4 +++-
 iothread.c            |  3 +++
 migration/migration.c |  3 +++
 tests/rcutorture.c    | 10 ----------
 tests/test-rcu-list.c |  2 ++
 util/rcu.c            | 30 ++++++++++++++++++++++++++++--
 7 files changed, 45 insertions(+), 13 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/3] rcu: automatically unregister threads when they exit

[Paolo Bonzini]

This simplifies management within the threads themselves.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/rcutorture.c | 10 ----------
 util/rcu.c         | 12 ++++++++++++
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/tests/rcutorture.c b/tests/rcutorture.c
index d6b304d..000b216 100644
--- a/tests/rcutorture.c
+++ b/tests/rcutorture.c
@@ -134,8 +134,6 @@ static void *rcu_read_perf_test(void *arg)
     qemu_mutex_lock(&counts_mutex);
     n_reads += n_reads_local;
     qemu_mutex_unlock(&counts_mutex);
-
-    rcu_unregister_thread();
     return NULL;
 }
 
@@ -157,8 +155,6 @@ static void *rcu_update_perf_test(void *arg)
     qemu_mutex_lock(&counts_mutex);
     n_updates += n_updates_local;
     qemu_mutex_unlock(&counts_mutex);
-
-    rcu_unregister_thread();
     return NULL;
 }
 
@@ -283,8 +279,6 @@ static void *rcu_read_stress_test(void *arg)
         rcu_stress_count[i] += rcu_stress_local[i];
     }
     qemu_mutex_unlock(&counts_mutex);
-
-    rcu_unregister_thread();
     return NULL;
 }
 
@@ -319,8 +313,6 @@ static void *rcu_update_stress_test(void *arg)
         synchronize_rcu();
         n_updates++;
     }
-
-    rcu_unregister_thread();
     return NULL;
 }
 
@@ -336,8 +328,6 @@ static void *rcu_fake_update_stress_test(void *arg)
         synchronize_rcu();
         g_usleep(1000);
     }
-
-    rcu_unregister_thread();
     return NULL;
 }
 
diff --git a/util/rcu.c b/util/rcu.c
index 7270151..8830295 100644
--- a/util/rcu.c
+++ b/util/rcu.c
@@ -268,12 +268,22 @@ void call_rcu1(struct rcu_head *node, void (*func)(struct rcu_head *node))
     qemu_event_set(&rcu_call_ready_event);
 }
 
+static __thread Notifier unregister_thread_notifier;
+
+static void rcu_unregister_thread_notify(Notifier *n, void *data)
+{
+    rcu_unregister_thread();
+}
+
 void rcu_register_thread(void)
 {
     assert(rcu_reader.ctr == 0);
     qemu_mutex_lock(&rcu_gp_lock);
     QLIST_INSERT_HEAD(&registry, &rcu_reader, node);
     qemu_mutex_unlock(&rcu_gp_lock);
+
+    unregister_thread_notifier.notify = rcu_unregister_thread_notify;
+    qemu_thread_atexit_add(&unregister_thread_notifier);
 }
 
 void rcu_unregister_thread(void)
@@ -281,6 +291,8 @@ void rcu_unregister_thread(void)
     qemu_mutex_lock(&rcu_gp_lock);
     QLIST_REMOVE(&rcu_reader, node);
     qemu_mutex_unlock(&rcu_gp_lock);
+
+    qemu_thread_atexit_remove(&unregister_thread_notifier);
 }
 
 static void rcu_init_complete(void)
-- 
2.4.3

[Qemu-devel] [PATCH 2/3] rcu: actually register threads that have RCU read-side critical sections

[Paolo Bonzini]

Otherwise, grace periods are detected too early!

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpus.c                | 6 ++++++
 iothread.c            | 3 +++
 migration/migration.c | 3 +++
 tests/test-rcu-list.c | 2 ++
 4 files changed, 14 insertions(+)

diff --git a/cpus.c b/cpus.c
index f547aeb..36e93fa 100644
--- a/cpus.c
+++ b/cpus.c
@@ -954,6 +954,8 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
     CPUState *cpu = arg;
     int r;
 
+    rcu_register_thread();
+
     qemu_mutex_lock_iothread();
     qemu_thread_get_self(cpu->thread);
     cpu->thread_id = qemu_get_thread_id();
@@ -995,6 +997,8 @@ static void *qemu_dummy_cpu_thread_fn(void *arg)
     sigset_t waitset;
     int r;
 
+    rcu_register_thread();
+
     qemu_mutex_lock_iothread();
     qemu_thread_get_self(cpu->thread);
     cpu->thread_id = qemu_get_thread_id();
@@ -1034,6 +1038,8 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
 {
     CPUState *cpu = arg;
 
+    rcu_register_thread();
+
     qemu_mutex_lock_iothread();
     qemu_tcg_init_cpu_signals();
     qemu_thread_get_self(cpu->thread);
diff --git a/iothread.c b/iothread.c
index 6d2a33f..443d176 100644
--- a/iothread.c
+++ b/iothread.c
@@ -18,6 +18,7 @@
 #include "sysemu/iothread.h"
 #include "qmp-commands.h"
 #include "qemu/error-report.h"
+#include "qemu/rcu.h"
 
 typedef ObjectClass IOThreadClass;
 
@@ -31,6 +32,8 @@ static void *iothread_run(void *opaque)
     IOThread *iothread = opaque;
     bool blocking;
 
+    rcu_register_thread();
+
     qemu_mutex_lock(&iothread->init_done_lock);
     iothread->thread_id = qemu_get_thread_id();
     qemu_cond_signal(&iothread->init_done_cond);
diff --git a/migration/migration.c b/migration/migration.c
index 45719a0..7f1e05a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -22,6 +22,7 @@
 #include "block/block.h"
 #include "qapi/qmp/qerror.h"
 #include "qemu/sockets.h"
+#include "qemu/rcu.h"
 #include "migration/block.h"
 #include "qemu/thread.h"
 #include "qmp-commands.h"
@@ -911,6 +912,8 @@ static void *migration_thread(void *opaque)
     int64_t start_time = initial_time;
     bool old_vm_running = false;
 
+    rcu_register_thread();
+
     qemu_savevm_state_header(s->file);
     qemu_savevm_state_begin(s->file, &s->params);
 
diff --git a/tests/test-rcu-list.c b/tests/test-rcu-list.c
index 4c5f62e..af98bdb 100644
--- a/tests/test-rcu-list.c
+++ b/tests/test-rcu-list.c
@@ -108,6 +108,8 @@ static void *rcu_q_reader(void *arg)
     long long n_reads_local = 0;
     struct list_element *el;
 
+    rcu_register_thread();
+
     *(struct rcu_reader_data **)arg = &rcu_reader;
     atomic_inc(&nthreadsrunning);
     while (goflag == GOFLAG_INIT) {
-- 
2.4.3

[Qemu-devel] [PATCH 3/3] rcu: detect missing rcu_register_thread()

[Paolo Bonzini]

Use an "impossible" value for the .depth field in order to quickly
detect threads that have not registered themselves with the RCU
subsystem.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/rcu.h |  4 +++-
 util/rcu.c         | 18 ++++++++++++++++--
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
index 7df1e86..4facb35 100644
--- a/include/qemu/rcu.h
+++ b/include/qemu/rcu.h
@@ -82,7 +82,9 @@ static inline void rcu_read_lock(void)
     struct rcu_reader_data *p_rcu_reader = &rcu_reader;
     unsigned ctr;
 
-    if (p_rcu_reader->depth++ > 0) {
+    p_rcu_reader->depth++;
+    assert(p_rcu_reader->depth >= 1);
+    if (p_rcu_reader->depth > 1) {
         return;
     }
 
diff --git a/util/rcu.c b/util/rcu.c
index 8830295..435cdbe 100644
--- a/util/rcu.c
+++ b/util/rcu.c
@@ -63,8 +63,11 @@ static inline int rcu_gp_ongoing(unsigned long *ctr)
 
 /* Written to only by each individual reader. Read by both the reader and the
  * writers.
+ *
+ * Initializing the depth to -1 causes an assertion failure on the first
+ * call to rcu_read_lock() if the thread does not call rcu_register_thread().
  */
-__thread struct rcu_reader_data rcu_reader;
+__thread struct rcu_reader_data rcu_reader = { .depth = -1 };
 
 /* Protected by rcu_gp_lock.  */
 typedef QLIST_HEAD(, rcu_reader_data) ThreadList;
@@ -277,7 +280,12 @@ static void rcu_unregister_thread_notify(Notifier *n, void *data)
 
 void rcu_register_thread(void)
 {
-    assert(rcu_reader.ctr == 0);
+    /* rcu_reader.depth is also used to detect whether the thread is
+     * registered.
+     */
+    assert(rcu_reader.depth == -1);
+    rcu_reader.depth = 0;
+
     qemu_mutex_lock(&rcu_gp_lock);
     QLIST_INSERT_HEAD(&registry, &rcu_reader, node);
     qemu_mutex_unlock(&rcu_gp_lock);
@@ -288,6 +296,12 @@ void rcu_register_thread(void)
 
 void rcu_unregister_thread(void)
 {
+    /* Resetting the depth to -1 causes an assertion failure on the next
+     * call to rcu_read_lock().
+     */
+    assert(rcu_reader.depth == 0);
+    rcu_reader.depth = -1;
+
     qemu_mutex_lock(&rcu_gp_lock);
     QLIST_REMOVE(&rcu_reader, node);
     qemu_mutex_unlock(&rcu_gp_lock);
-- 
2.4.3

Re: [Qemu-devel] [PATCH 0/3] rcu: missing thread registration

[Fam Zheng]

On Mon, 07/13 13:31, Paolo Bonzini wrote:
> Patch 2 fixes the problem, whereby threads were not being registered
> but still happily used RCU.  Patch 1 simplifies the registration by
> making rcu_unregister_thread automatic.  Patch 3 avoids having the
> same bug in the future.

Reviewed-by: Fam Zheng <famz@redhat.com>

> 
> Paolo
> 
> Paolo Bonzini (3):
>   rcu: automatically unregister threads when they exit
>   rcu: actually register threads that have RCU read-side critical
>     sections
>   rcu: detect missing rcu_register_thread()
> 
>  cpus.c                |  6 ++++++
>  include/qemu/rcu.h    |  4 +++-
>  iothread.c            |  3 +++
>  migration/migration.c |  3 +++
>  tests/rcutorture.c    | 10 ----------
>  tests/test-rcu-list.c |  2 ++
>  util/rcu.c            | 30 ++++++++++++++++++++++++++++--
>  7 files changed, 45 insertions(+), 13 deletions(-)
> 
> -- 
> 2.4.3
> 
>