[Qemu-devel] [PATCH 0/2] rcu: support fork

[Qemu-devel] [PATCH 0/2] rcu: support fork

[Paolo Bonzini]

Supporting fork in multithreaded programs is somewhat complicated,
however in QEMU we fork in two places (daemonize and smbd) and none of
them are complicated:

- daemonize happens before threads and mutexes proliferate unpredictably;
only the RCU state has to be reset and the call_rcu thread recreated

- smbd mostly does an exec in the child.

Unfortunately, glibc also makes forking terminally incompatible with
PTHREAD_MUTEX_ERRORCHECK, so you also need to disable that.

Paolo

Paolo Bonzini (2):
  qemu-thread: do not use PTHREAD_MUTEX_ERRORCHECK
  rcu: handle forks safely

 util/qemu-thread-posix.c |  6 +-----
 util/rcu.c               | 34 +++++++++++++++++++++++++++++++++-
 2 files changed, 34 insertions(+), 6 deletions(-)

-- 
2.3.0

[Qemu-devel] [PATCH 1/2] qemu-thread: do not use PTHREAD_MUTEX_ERRORCHECK

[Paolo Bonzini]

PTHREAD_MUTEX_ERRORCHECK is completely broken with respect to fork.
The way to safely do fork is to bring all threads to a quiescent
state by acquiring locks (either in callers---as we do for the
iothread mutex---or using pthread_atfork's prepare callbacks)
and then release them in the child.

The problem is that releasing error-checking locks in the child
fails under glibc with EPERM, because the mutex stores a different
owner tid than the duplicated thread in the child process.  We
could make it work for locks acquired via pthread_atfork, by
recreating the mutex in the child instead of unlocking it
(we know that there are no other threads that could have taken
the mutex; but when the lock is acquired in fork's caller
that would not be possible.

The simplest solution is just to forgo error checking.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 util/qemu-thread-posix.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index 50a29d8..ba67cec 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -51,12 +51,8 @@ static void error_exit(int err, const char *msg)
 void qemu_mutex_init(QemuMutex *mutex)
 {
     int err;
-    pthread_mutexattr_t mutexattr;
 
-    pthread_mutexattr_init(&mutexattr);
-    pthread_mutexattr_settype(&mutexattr, PTHREAD_MUTEX_ERRORCHECK);
-    err = pthread_mutex_init(&mutex->lock, &mutexattr);
-    pthread_mutexattr_destroy(&mutexattr);
+    err = pthread_mutex_init(&mutex->lock, NULL);
     if (err)
         error_exit(err, __func__);
 }
-- 
2.3.0

Re: [Qemu-devel] [PATCH 1/2] qemu-thread: do not use PTHREAD_MUTEX_ERRORCHECK

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2239 bytes --]

On 03/06/2015 02:22 AM, Paolo Bonzini wrote:
> PTHREAD_MUTEX_ERRORCHECK is completely broken with respect to fork.
> The way to safely do fork is to bring all threads to a quiescent
> state by acquiring locks (either in callers---as we do for the
> iothread mutex---or using pthread_atfork's prepare callbacks)
> and then release them in the child.

That, and POSIX itself says that pthread_atfork is a dangerous API, and
should not be used if at all possible, because it is broken by design.

> 
> The problem is that releasing error-checking locks in the child
> fails under glibc with EPERM, because the mutex stores a different
> owner tid than the duplicated thread in the child process.

Is that a bug in glibc?

>  We
> could make it work for locks acquired via pthread_atfork, by
> recreating the mutex in the child instead of unlocking it
> (we know that there are no other threads that could have taken
> the mutex; but when the lock is acquired in fork's caller
> that would not be possible.
> 
> The simplest solution is just to forgo error checking.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  util/qemu-thread-posix.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)

I'm not sure that weakening things is always the wisest idea, but you've
provided some good arguments for why we want it here, so:

Reviewed-by: Eric Blake <eblake@redhat.com>


> 
> diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
> index 50a29d8..ba67cec 100644
> --- a/util/qemu-thread-posix.c
> +++ b/util/qemu-thread-posix.c
> @@ -51,12 +51,8 @@ static void error_exit(int err, const char *msg)
>  void qemu_mutex_init(QemuMutex *mutex)
>  {
>      int err;
> -    pthread_mutexattr_t mutexattr;
>  
> -    pthread_mutexattr_init(&mutexattr);
> -    pthread_mutexattr_settype(&mutexattr, PTHREAD_MUTEX_ERRORCHECK);
> -    err = pthread_mutex_init(&mutex->lock, &mutexattr);
> -    pthread_mutexattr_destroy(&mutexattr);
> +    err = pthread_mutex_init(&mutex->lock, NULL);
>      if (err)
>          error_exit(err, __func__);
>  }
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/2] qemu-thread: do not use PTHREAD_MUTEX_ERRORCHECK

[Paolo Bonzini]

On 06/03/2015 16:54, Eric Blake wrote:
> > The problem is that releasing error-checking locks in the child
> > fails under glibc with EPERM, because the mutex stores a different
> > owner tid than the duplicated thread in the child process.
> 
> Is that a bug in glibc?

Possibly, but I wouldn't be surprised if other libcs had the same bug.
And if you ran it through the Austin Group, I wouldn't be surprised if
it were declared undefined.

Paolo

[Qemu-devel] [PATCH 2/2] rcu: handle forks safely

[Paolo Bonzini]

After forking, only the calling thread is duplicated in the child process.
The call_rcu thread has to be recreated in the child.  Exploit the fact
that only one thread exists (same as when constructors run), and just redo
the entire initialization to ensure the threads are in the proper state.

The only additional things to do are emptying the list of threads
registered with RCU, and unlocking the lock that was taken in the prepare
callback (implementations are allowed to fail pthread_mutex_init()
if the mutex is still locked).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 util/rcu.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/util/rcu.c b/util/rcu.c
index bd73b8e..4585ce1 100644
--- a/util/rcu.c
+++ b/util/rcu.c
@@ -283,7 +283,7 @@ void rcu_unregister_thread(void)
     qemu_mutex_unlock(&rcu_gp_lock);
 }
 
-static void __attribute__((__constructor__)) rcu_init(void)
+static void rcu_init_complete(void)
 {
     QemuThread thread;
 
@@ -291,8 +291,39 @@ static void __attribute__((__constructor__)) rcu_init(void)
     qemu_event_init(&rcu_gp_event, true);
 
     qemu_event_init(&rcu_call_ready_event, false);
+
+    /* The caller is assumed to have iothread lock, so the call_rcu thread
+     * must have been quiescent even after forking; just recreate it.
+     */
     qemu_thread_create(&thread, "call_rcu", call_rcu_thread,
                        NULL, QEMU_THREAD_DETACHED);
 
     rcu_register_thread();
 }
+
+#ifdef CONFIG_POSIX
+static void rcu_init_lock(void)
+{
+    qemu_mutex_lock(&rcu_gp_lock);
+}
+
+static void rcu_init_unlock(void)
+{
+    qemu_mutex_unlock(&rcu_gp_lock);
+}
+
+static void rcu_init_child(void)
+{
+    qemu_mutex_unlock(&rcu_gp_lock);
+    memset(&registry, 0, sizeof(registry));
+    rcu_init_complete();
+}
+#endif
+
+static void __attribute__((__constructor__)) rcu_init(void)
+{
+#ifdef CONFIG_POSIX
+    pthread_atfork(rcu_init_lock, rcu_init_unlock, rcu_init_child);
+#endif
+    rcu_init_complete();
+}
-- 
2.3.0