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 <pbonz...@redhat.com>
> Signed-off-by: Peter Xu <pet...@redhat.com>
> ---
> 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(®istry, 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();
> }
>
