Re: [Qemu-devel] [PULL 08/11] rcu: detect missing rcu_register_thread()

[Wen Congyang]

On 07/17/2015 12:55 AM, Paolo Bonzini wrote:
> Use an "impossible" value for the .depth field in order to quickly
> detect threads that have not registered themselves with the RCU
> subsystem.
> 
> Avoid a false positive around forking by unregistering and registering
> the forking thread explicitly.  Previously, it was enough to re-register
> the thread.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  include/qemu/rcu.h |  4 +++-
>  util/rcu.c         | 35 ++++++++++++++++++++++++++++++-----
>  2 files changed, 33 insertions(+), 6 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 e21bb46..2490273 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;
> @@ -279,7 +282,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);
> @@ -290,6 +298,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);
> @@ -301,7 +315,6 @@ static void rcu_init_complete(void)
>  {
>      QemuThread thread;
>  
> -    qemu_mutex_init(&rcu_gp_lock);
>      qemu_event_init(&rcu_gp_event, true);
>  
>      qemu_event_init(&rcu_call_ready_event, false);
> @@ -311,8 +324,6 @@ static void rcu_init_complete(void)
>       */
>      qemu_thread_create(&thread, "call_rcu", call_rcu_thread,
>                         NULL, QEMU_THREAD_DETACHED);
> -
> -    rcu_register_thread();
>  }
>  
>  #ifdef CONFIG_POSIX
> @@ -329,14 +340,28 @@ static void rcu_init_unlock(void)
>  
>  void rcu_after_fork(void)
>  {
> +    int save_depth = rcu_reader.depth;
> +    if (save_depth != -1) {

If we allow call fork() in RCU read-side critical section,
rcu_reader.depth should be set to 0 before calling
rcu_unregister_thread().

Thanks
Wen Congyang

> +        rcu_unregister_thread();
> +    }
> +
>      memset(&registry, 0, sizeof(registry));
> +
>      rcu_init_complete();
> +
> +    if (save_depth != -1) {
> +        rcu_register_thread();
> +        rcu_reader.depth = save_depth;
> +    }
>  }
>  
>  static void __attribute__((__constructor__)) rcu_init(void)
>  {
> +    qemu_mutex_init(&rcu_gp_lock);
>  #ifdef CONFIG_POSIX
>      pthread_atfork(rcu_init_lock, rcu_init_unlock, rcu_init_unlock);
>  #endif
>      rcu_init_complete();
> +
> +    rcu_register_thread();
>  }
>