Re: [Qemu-devel] [PATCH for-2.11] rcu: init globals only once

[Paolo Bonzini]

On 08/08/2017 09:00, Peter Xu wrote:
> We were calling rcu_init_complete() twice in the child processes when
> fork happened. However the pthread library does not really suggest to do
> it that way:
> 
> http://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_mutex_init.html
> 
> "Attempting to initialise an already initialised mutex results in
>  undefined behaviour."
> 
> Actually, IMHO we can do it in a more natural way: Firstly, we only init
> the RCU globals once in rcu_init(). Then, in rcu_init_child(), we unlock
> all the locks held in rcu_init_lock() just like what we do in the parent
> process, then do the rest of RCU re-init (e.g., create the RCU thread).

This doesn't work for error-checking mutexes: rcu_init_child has a
different PID than the parent, so the mutexes aren't unlocked.  It's
also true that right now we don't use error-checking mutexes (commit
24fa90499f, "qemu-thread: do not use PTHREAD_MUTEX_ERRORCHECK",
2015-03-10); however, that's also a bit sad.

The reason for the undefined behavior is probably that some operating
systems allocate memory in pthread_mutex_init, and initializing twice
causes a memory leak.  One such operating system is OpenBSD. :(

Eric, you chimed in on the patch that became commit 24fa90499f, what do
you suggest?

Paolo

> CC: Paolo Bonzini <address@hidden>
> Signed-off-by: Peter Xu <address@hidden>
> ---
> this is based on Paolo's series:
> "[PATCH for-2.10 0/2] RCU: forking fix and cleanups"
> ---
>  util/rcu.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/util/rcu.c b/util/rcu.c
> index ca5a63e..6fbbe4c 100644
> --- a/util/rcu.c
> +++ b/util/rcu.c
> @@ -299,15 +299,17 @@ void rcu_unregister_thread(void)
>      qemu_mutex_unlock(&rcu_registry_lock);
>  }
>  
> -static void rcu_init_complete(void)
> +static void rcu_init_globals(void)
>  {
> -    QemuThread thread;
> -
>      qemu_mutex_init(&rcu_registry_lock);
>      qemu_mutex_init(&rcu_sync_lock);
>      qemu_event_init(&rcu_gp_event, true);
> -
>      qemu_event_init(&rcu_call_ready_event, false);
> +}
> +
> +static void rcu_init_complete(void)
> +{
> +    QemuThread thread;
>  
>      /* The caller is assumed to have iothread lock, so the call_rcu thread
>       * must have been quiescent even after forking, just recreate it.
> @@ -357,6 +359,13 @@ static void rcu_init_child(void)
>          return;
>      }
>  
> +    rcu_init_unlock();
> +
> +    /*
> +     * For the newly forked child, we need something extra: since
> +     * after fork the threads are all gone, we need to re-init the RCU
> +     * thread, along with the globals.
> +     */
>      memset(&registry, 0, sizeof(registry));
>      rcu_init_complete();
>  }
> @@ -367,5 +376,6 @@ static void __attribute__((__constructor__)) 
> rcu_init(void)
>  #ifdef CONFIG_POSIX
>      pthread_atfork(rcu_init_lock, rcu_init_unlock, rcu_init_child);
>  #endif
> +    rcu_init_globals();
>      rcu_init_complete();
>  }
>