[Qemu-devel] [PATCH] rcu: do not create thread in pthread_atfork callback

[Qemu-devel] [PATCH] rcu: do not create thread in pthread_atfork callback

[Paolo Bonzini]

If QEMU forks after the CPU threads have been created, qemu_mutex_lock_iothread
will not be able to do qemu_cpu_kick_thread.  There is no solution other than
assuming that forks after the CPU threads have been created will end up in an
exec.  Forks before the CPU threads have been created (such as -daemonize)
have to call rcu_after_fork manually.

Notably, the oxygen theme for GTK+ forks and shows a "No such process" error
without this patch.

This patch can be reverted once the iothread loses the "kick the TCG thread"
magic.

Reported by: Dr. David Alan Gilbert <dgilbert@redhat.com>

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

diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
index 506ab58..7df1e86 100644
--- a/include/qemu/rcu.h
+++ b/include/qemu/rcu.h
@@ -117,6 +117,7 @@ extern void synchronize_rcu(void);
  */
 extern void rcu_register_thread(void);
 extern void rcu_unregister_thread(void);
+extern void rcu_after_fork(void);
 
 struct rcu_head;
 typedef void RCUCBFunc(struct rcu_head *head);
diff --git a/os-posix.c b/os-posix.c
index ba091f1..e4da406 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -39,6 +39,7 @@
 #include "sysemu/sysemu.h"
 #include "net/slirp.h"
 #include "qemu-options.h"
+#include "qemu/rcu.h"
 
 #ifdef CONFIG_LINUX
 #include <sys/prctl.h>
@@ -247,6 +248,7 @@ void os_daemonize(void)
         signal(SIGTSTP, SIG_IGN);
         signal(SIGTTOU, SIG_IGN);
         signal(SIGTTIN, SIG_IGN);
+        rcu_after_fork();
     }
 }
 
diff --git a/util/rcu.c b/util/rcu.c
index 27802a4..7270151 100644
--- a/util/rcu.c
+++ b/util/rcu.c
@@ -311,19 +311,18 @@ static void rcu_init_unlock(void)
 {
     qemu_mutex_unlock(&rcu_gp_lock);
 }
+#endif
 
-static void rcu_init_child(void)
+void rcu_after_fork(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);
+    pthread_atfork(rcu_init_lock, rcu_init_unlock, rcu_init_unlock);
 #endif
     rcu_init_complete();
 }
-- 
2.3.4

Re: [Qemu-devel] [PATCH] rcu: do not create thread in pthread_atfork callback

[Peter Maydell]

On 31 March 2015 at 12:01, Paolo Bonzini <pbonzini@redhat.com> wrote:
> If QEMU forks after the CPU threads have been created, qemu_mutex_lock_iothread
> will not be able to do qemu_cpu_kick_thread.  There is no solution other than
> assuming that forks after the CPU threads have been created will end up in an
> exec.

This assumption is false for linux-user mode...
(though in that case we don't have an iothread).

> diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
> index 506ab58..7df1e86 100644
> --- a/include/qemu/rcu.h
> +++ b/include/qemu/rcu.h
> @@ -117,6 +117,7 @@ extern void synchronize_rcu(void);
>   */
>  extern void rcu_register_thread(void);
>  extern void rcu_unregister_thread(void);
> +extern void rcu_after_fork(void);
>
>  struct rcu_head;
>  typedef void RCUCBFunc(struct rcu_head *head);
> diff --git a/os-posix.c b/os-posix.c
> index ba091f1..e4da406 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -39,6 +39,7 @@
>  #include "sysemu/sysemu.h"
>  #include "net/slirp.h"
>  #include "qemu-options.h"
> +#include "qemu/rcu.h"
>
>  #ifdef CONFIG_LINUX
>  #include <sys/prctl.h>
> @@ -247,6 +248,7 @@ void os_daemonize(void)
>          signal(SIGTSTP, SIG_IGN);
>          signal(SIGTTOU, SIG_IGN);
>          signal(SIGTTIN, SIG_IGN);
> +        rcu_after_fork();
>      }
>  }
>
> diff --git a/util/rcu.c b/util/rcu.c
> index 27802a4..7270151 100644
> --- a/util/rcu.c
> +++ b/util/rcu.c
> @@ -311,19 +311,18 @@ static void rcu_init_unlock(void)
>  {
>      qemu_mutex_unlock(&rcu_gp_lock);
>  }
> +#endif
>
> -static void rcu_init_child(void)
> +void rcu_after_fork(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);
> +    pthread_atfork(rcu_init_lock, rcu_init_unlock, rcu_init_unlock);
>  #endif
>      rcu_init_complete();
>  }

So this is changing the linux-user behaviour so we no
longer do any init after fork; is that a problem?

-- PMM

Re: [Qemu-devel] [PATCH] rcu: do not create thread in pthread_atfork callback

[Paolo Bonzini]

On 31/03/2015 13:41, Peter Maydell wrote:
>> >  static void __attribute__((__constructor__)) rcu_init(void)
>> >  {
>> >  #ifdef CONFIG_POSIX
>> > -    pthread_atfork(rcu_init_lock, rcu_init_unlock, rcu_init_child);
>> > +    pthread_atfork(rcu_init_lock, rcu_init_unlock, rcu_init_unlock);
>> >  #endif
>> >      rcu_init_complete();
>> >  }
> So this is changing the linux-user behaviour so we no
> longer do any init after fork; is that a problem?

Currently linux-user is not using RCU at all, so no.  Should I add an
rcu_after_fork there too?

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 4bd9543..1622ad6 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -4572,6 +4572,7 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
         ret = fork();
         if (ret == 0) {
             /* Child Process.  */
+            rcu_after_fork();
             cpu_clone_regs(env, newsp);
             fork_end(1);
             /* There is a race condition here.  The parent process could

Paolo

Re: [Qemu-devel] [PATCH] rcu: do not create thread in pthread_atfork callback

[Peter Maydell]

On 31 March 2015 at 13:55, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 31/03/2015 13:41, Peter Maydell wrote:
>>> >  static void __attribute__((__constructor__)) rcu_init(void)
>>> >  {
>>> >  #ifdef CONFIG_POSIX
>>> > -    pthread_atfork(rcu_init_lock, rcu_init_unlock, rcu_init_child);
>>> > +    pthread_atfork(rcu_init_lock, rcu_init_unlock, rcu_init_unlock);
>>> >  #endif
>>> >      rcu_init_complete();
>>> >  }
>> So this is changing the linux-user behaviour so we no
>> longer do any init after fork; is that a problem?
>
> Currently linux-user is not using RCU at all, so no.  Should I add an
> rcu_after_fork there too?

Well, I don't really understand the rcu stuff, so I can't
say...

-- PMM