[Qemu-devel] [PATCH for-2.10 0/2] RCU: forking fix and cleanups

[Qemu-devel] [PATCH for-2.10 0/2] RCU: forking fix and cleanups

[Paolo Bonzini]

The first patch fixes a migration deadlock.  The second reverts
a now-unnecessary hack (introduced to support the pre-MTTCG "kick
the CPU thread" special behavior of qemu_mutex_lock_iothread).

Paolo

Paolo Bonzini (2):
  rcu: completely disable pthread_atfork callbacks as soon as possible
  Revert "rcu: do not create thread in pthread_atfork callback"

 include/qemu/rcu.h   |  7 ++++++-
 linux-user/syscall.c |  1 -
 os-posix.c           |  2 --
 util/rcu.c           | 30 +++++++++++++++++++++++++++---
 vl.c                 |  1 +
 5 files changed, 34 insertions(+), 7 deletions(-)

-- 
2.13.3

[Qemu-devel] [PATCH 1/2] rcu: completely disable pthread_atfork callbacks as soon as possible

[Paolo Bonzini]

Because of -daemonize, system mode QEMU sometimes needs to fork() and
keep RCU enabled in the child.  However, there is a possible deadlock
with synchronize_rcu:

- the CPU thread is inside a RCU critical section and wants to take
  the BQL in order to do MMIO

- the monitor thread, which is owning the BQL, calls rcu_init_lock
  which tries to take the rcu_sync_lock

- the call_rcu thread has taken rcu_sync_lock in synchronize_rcu, but
  synchronize_rcu needs the CPU thread to end the critical section
  before returning.

This cannot happen for user-mode emulation, because it does not have
a BQL.

To fix it, assume that system mode QEMU only forks in preparation for
exec (except when daemonizing) and disable pthread_atfork as soon as
the double fork has happened.

Reported-by: David Gilbert <dgilbert@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/rcu.h |  6 ++++++
 util/rcu.c         | 20 ++++++++++++++++++++
 vl.c               |  1 +
 3 files changed, 27 insertions(+)

diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
index 83ae2808be..c0da9907e8 100644
--- a/include/qemu/rcu.h
+++ b/include/qemu/rcu.h
@@ -105,6 +105,12 @@ extern void synchronize_rcu(void);
  */
 extern void rcu_register_thread(void);
 extern void rcu_unregister_thread(void);
+
+/*
+ * Support for fork().  fork() support is enabled at startup.
+ */
+extern void rcu_enable_atfork(void);
+extern void rcu_disable_atfork(void);
 extern void rcu_after_fork(void);
 
 struct rcu_head;
diff --git a/util/rcu.c b/util/rcu.c
index 9adc5e4a36..2142ddd93b 100644
--- a/util/rcu.c
+++ b/util/rcu.c
@@ -318,15 +318,35 @@ static void rcu_init_complete(void)
     rcu_register_thread();
 }
 
+static int atfork_depth = 1;
+
+void rcu_enable_atfork(void)
+{
+    atfork_depth++;
+}
+
+void rcu_disable_atfork(void)
+{
+    atfork_depth--;
+}
+
 #ifdef CONFIG_POSIX
 static void rcu_init_lock(void)
 {
+    if (atfork_depth < 1) {
+        return;
+    }
+
     qemu_mutex_lock(&rcu_sync_lock);
     qemu_mutex_lock(&rcu_registry_lock);
 }
 
 static void rcu_init_unlock(void)
 {
+    if (atfork_depth < 1) {
+        return;
+    }
+
     qemu_mutex_unlock(&rcu_registry_lock);
     qemu_mutex_unlock(&rcu_sync_lock);
 }
diff --git a/vl.c b/vl.c
index 99fcfa0442..8967115514 100644
--- a/vl.c
+++ b/vl.c
@@ -4121,6 +4121,7 @@ int main(int argc, char **argv, char **envp)
     set_memory_options(&ram_slots, &maxram_size, machine_class);
 
     os_daemonize();
+    rcu_disable_atfork();
 
     if (pid_file && qemu_create_pidfile(pid_file) != 0) {
         error_report("could not acquire pid file: %s", strerror(errno));
-- 
2.13.3

Re: [Qemu-devel] [PATCH 1/2] rcu: completely disable pthread_atfork callbacks as soon as possible

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> Because of -daemonize, system mode QEMU sometimes needs to fork() and
> keep RCU enabled in the child.  However, there is a possible deadlock
> with synchronize_rcu:
> 
> - the CPU thread is inside a RCU critical section and wants to take
>   the BQL in order to do MMIO
> 
> - the monitor thread, which is owning the BQL, calls rcu_init_lock
>   which tries to take the rcu_sync_lock
> 
> - the call_rcu thread has taken rcu_sync_lock in synchronize_rcu, but
>   synchronize_rcu needs the CPU thread to end the critical section
>   before returning.
> 
> This cannot happen for user-mode emulation, because it does not have
> a BQL.
> 
> To fix it, assume that system mode QEMU only forks in preparation for
> exec (except when daemonizing) and disable pthread_atfork as soon as
> the double fork has happened.
> 
> Reported-by: David Gilbert <dgilbert@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

That seems to fix that issue (not tested it any more than that though),
so for fixing that issue:

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

> ---
>  include/qemu/rcu.h |  6 ++++++
>  util/rcu.c         | 20 ++++++++++++++++++++
>  vl.c               |  1 +
>  3 files changed, 27 insertions(+)
> 
> diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
> index 83ae2808be..c0da9907e8 100644
> --- a/include/qemu/rcu.h
> +++ b/include/qemu/rcu.h
> @@ -105,6 +105,12 @@ extern void synchronize_rcu(void);
>   */
>  extern void rcu_register_thread(void);
>  extern void rcu_unregister_thread(void);
> +
> +/*
> + * Support for fork().  fork() support is enabled at startup.
> + */
> +extern void rcu_enable_atfork(void);
> +extern void rcu_disable_atfork(void);
>  extern void rcu_after_fork(void);
>  
>  struct rcu_head;
> diff --git a/util/rcu.c b/util/rcu.c
> index 9adc5e4a36..2142ddd93b 100644
> --- a/util/rcu.c
> +++ b/util/rcu.c
> @@ -318,15 +318,35 @@ static void rcu_init_complete(void)
>      rcu_register_thread();
>  }
>  
> +static int atfork_depth = 1;
> +
> +void rcu_enable_atfork(void)
> +{
> +    atfork_depth++;
> +}
> +
> +void rcu_disable_atfork(void)
> +{
> +    atfork_depth--;
> +}
> +
>  #ifdef CONFIG_POSIX
>  static void rcu_init_lock(void)
>  {
> +    if (atfork_depth < 1) {
> +        return;
> +    }
> +
>      qemu_mutex_lock(&rcu_sync_lock);
>      qemu_mutex_lock(&rcu_registry_lock);
>  }
>  
>  static void rcu_init_unlock(void)
>  {
> +    if (atfork_depth < 1) {
> +        return;
> +    }
> +
>      qemu_mutex_unlock(&rcu_registry_lock);
>      qemu_mutex_unlock(&rcu_sync_lock);
>  }
> diff --git a/vl.c b/vl.c
> index 99fcfa0442..8967115514 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4121,6 +4121,7 @@ int main(int argc, char **argv, char **envp)
>      set_memory_options(&ram_slots, &maxram_size, machine_class);
>  
>      os_daemonize();
> +    rcu_disable_atfork();
>  
>      if (pid_file && qemu_create_pidfile(pid_file) != 0) {
>          error_report("could not acquire pid file: %s", strerror(errno));
> -- 
> 2.13.3
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 2/2] Revert "rcu: do not create thread in pthread_atfork callback"

[Paolo Bonzini]

This reverts commit a59629fcc6f603e19b516dc08f75334e5c480bd0.
This is not needed anymore because the IOThread mutex is not
"magic" anymore (need not kick the CPU thread) and also because
fork callbacks are only enabled at the very beginning of
QEMU's execution.

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

diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
index c0da9907e8..f19413d649 100644
--- a/include/qemu/rcu.h
+++ b/include/qemu/rcu.h
@@ -111,7 +111,6 @@ extern void rcu_unregister_thread(void);
  */
 extern void rcu_enable_atfork(void);
 extern void rcu_disable_atfork(void);
-extern void rcu_after_fork(void);
 
 struct rcu_head;
 typedef void RCUCBFunc(struct rcu_head *head);
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 54343c06be..9b6364a266 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -6354,7 +6354,6 @@ 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
diff --git a/os-posix.c b/os-posix.c
index c6ddb7d830..92e9d85215 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -34,7 +34,6 @@
 #include "sysemu/sysemu.h"
 #include "net/slirp.h"
 #include "qemu-options.h"
-#include "qemu/rcu.h"
 #include "qemu/error-report.h"
 #include "qemu/log.h"
 #include "qemu/cutils.h"
@@ -249,7 +248,6 @@ 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 2142ddd93b..ca5a63e36a 100644
--- a/util/rcu.c
+++ b/util/rcu.c
@@ -350,18 +350,22 @@ static void rcu_init_unlock(void)
     qemu_mutex_unlock(&rcu_registry_lock);
     qemu_mutex_unlock(&rcu_sync_lock);
 }
-#endif
 
-void rcu_after_fork(void)
+static void rcu_init_child(void)
 {
+    if (atfork_depth < 1) {
+        return;
+    }
+
     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_unlock);
+    pthread_atfork(rcu_init_lock, rcu_init_unlock, rcu_init_child);
 #endif
     rcu_init_complete();
 }
-- 
2.13.3