Patchwork [v2,upstream,19/22] move blocking of signals to qemu_signalfd_init

login
register
mail settings
Submitter Paolo Bonzini
Date Feb. 26, 2011, 3:40 p.m.
Message ID <1298734819-1960-20-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/84653/
State New
Headers show

Comments

Paolo Bonzini - Feb. 26, 2011, 3:40 p.m.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpus.c |   87 ++++++++++++++++++++++++++-------------------------------------
 1 files changed, 36 insertions(+), 51 deletions(-)
Jan Kiszka - Feb. 27, 2011, 9:41 a.m.
On 2011-02-26 16:40, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  cpus.c |   87 ++++++++++++++++++++++++++-------------------------------------
>  1 files changed, 36 insertions(+), 51 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 32e9352..8c440f1 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -346,11 +346,37 @@ static void sigfd_handler(void *opaque)
>      }
>  }
>  
> -static int qemu_signalfd_init(sigset_t mask)
> +static int qemu_signalfd_init(void)
>  {
>      int sigfd;
> +    sigset_t set;
>  
> -    sigfd = qemu_signalfd(&mask);
> +#ifdef CONFIG_IOTHREAD
> +    /* SIGUSR2 used by posix-aio-compat.c */
> +    sigemptyset(&set);
> +    sigaddset(&set, SIGUSR2);
> +    pthread_sigmask(SIG_UNBLOCK, &set, NULL);

Didn't you want to rename the function for the sake of non-signalfd
blocks like above?

> +
> +    sigemptyset(&set);
> +    sigaddset(&set, SIGIO);
> +    sigaddset(&set, SIGALRM);
> +    sigaddset(&set, SIG_IPI);
> +    sigaddset(&set, SIGBUS);
> +    pthread_sigmask(SIG_BLOCK, &set, NULL);
> +#else
> +    sigemptyset(&set);

This line is shared and can be moved out of the #ifdef.

> +    sigaddset(&set, SIGBUS);
> +    if (kvm_enabled()) {
> +        /*
> +         * We need to process timer signals synchronously to avoid a race
> +         * between exit_request check and KVM vcpu entry.
> +         */
> +        sigaddset(&set, SIGIO);
> +        sigaddset(&set, SIGALRM);
> +    }
> +#endif
> +
> +    sigfd = qemu_signalfd(&set);
>      if (sigfd == -1) {
>          fprintf(stderr, "failed to create signalfd\n");
>          return -errno;
> @@ -438,6 +464,12 @@ static void qemu_event_increment(void)
>  static void qemu_kvm_eat_signals(CPUState *env)
>  {
>  }
> +
> +static int qemu_signalfd_init(void)
> +{
> +    return 0;
> +}
> +
>  #endif /* _WIN32 */
>  
>  #ifndef CONFIG_IOTHREAD
> @@ -471,39 +503,14 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
>  #endif
>  }
>  
> -#ifndef _WIN32
> -static sigset_t block_synchronous_signals(void)
> -{
> -    sigset_t set;
> -
> -    sigemptyset(&set);
> -    sigaddset(&set, SIGBUS);
> -    if (kvm_enabled()) {
> -        /*
> -         * We need to process timer signals synchronously to avoid a race
> -         * between exit_request check and KVM vcpu entry.
> -         */
> -        sigaddset(&set, SIGIO);
> -        sigaddset(&set, SIGALRM);
> -    }
> -
> -    return set;
> -}
> -#endif
> -
>  int qemu_init_main_loop(void)
>  {
> -#ifndef _WIN32
> -    sigset_t blocked_signals;
>      int ret;
>  
> -    blocked_signals = block_synchronous_signals();
> -
> -    ret = qemu_signalfd_init(blocked_signals);
> +    ret = qemu_signalfd_init();
>      if (ret) {
>          return ret;
>      }
> -#endif
>  
>      qemu_init_sigbus();
>  
> @@ -651,35 +658,13 @@ static void qemu_tcg_init_cpu_signals(void)
>      pthread_sigmask(SIG_UNBLOCK, &set, NULL);
>  }
>  
> -static sigset_t block_io_signals(void)
> -{
> -    sigset_t set;
> -
> -    /* SIGUSR2 used by posix-aio-compat.c */
> -    sigemptyset(&set);
> -    sigaddset(&set, SIGUSR2);
> -    pthread_sigmask(SIG_UNBLOCK, &set, NULL);
> -
> -    sigemptyset(&set);
> -    sigaddset(&set, SIGIO);
> -    sigaddset(&set, SIGALRM);
> -    sigaddset(&set, SIG_IPI);
> -    sigaddset(&set, SIGBUS);
> -    pthread_sigmask(SIG_BLOCK, &set, NULL);
> -
> -    return set;
> -}
> -
>  int qemu_init_main_loop(void)
>  {
>      int ret;
> -    sigset_t blocked_signals;
>  
>      qemu_init_sigbus();
>  
> -    blocked_signals = block_io_signals();
> -
> -    ret = qemu_signalfd_init(blocked_signals);
> +    ret = qemu_signalfd_init();
>      if (ret) {
>          return ret;
>      }

Beside the minor nits, this looks good.

Jan
Paolo Bonzini - Feb. 27, 2011, 3:07 p.m.
On 02/27/2011 10:41 AM, Jan Kiszka wrote:
>> >  +#ifdef CONFIG_IOTHREAD
>> >  +    /* SIGUSR2 used by posix-aio-compat.c */
>> >  +    sigemptyset(&set);
>> >  +    sigaddset(&set, SIGUSR2);
>> >  +    pthread_sigmask(SIG_UNBLOCK,&set, NULL);
>
> Didn't you want to rename the function for the sake of non-signalfd
> blocks like above?

Right.

>> >  +
>> >  +    sigemptyset(&set);
>> >  +    sigaddset(&set, SIGIO);
>> >  +    sigaddset(&set, SIGALRM);
>> >  +    sigaddset(&set, SIG_IPI);
>> >  +    sigaddset(&set, SIGBUS);
>> >  +    pthread_sigmask(SIG_BLOCK,&set, NULL);
>> >  +#else
>> >  +    sigemptyset(&set);
>
> This line is shared and can be moved out of the #ifdef.

It's shared but for different purposes (UNBLOCK for iothread, 
BLOCK/signalfd for !iothread), so I decided not to hoist it out.

Paolo

Patch

diff --git a/cpus.c b/cpus.c
index 32e9352..8c440f1 100644
--- a/cpus.c
+++ b/cpus.c
@@ -346,11 +346,37 @@  static void sigfd_handler(void *opaque)
     }
 }
 
-static int qemu_signalfd_init(sigset_t mask)
+static int qemu_signalfd_init(void)
 {
     int sigfd;
+    sigset_t set;
 
-    sigfd = qemu_signalfd(&mask);
+#ifdef CONFIG_IOTHREAD
+    /* SIGUSR2 used by posix-aio-compat.c */
+    sigemptyset(&set);
+    sigaddset(&set, SIGUSR2);
+    pthread_sigmask(SIG_UNBLOCK, &set, NULL);
+
+    sigemptyset(&set);
+    sigaddset(&set, SIGIO);
+    sigaddset(&set, SIGALRM);
+    sigaddset(&set, SIG_IPI);
+    sigaddset(&set, SIGBUS);
+    pthread_sigmask(SIG_BLOCK, &set, NULL);
+#else
+    sigemptyset(&set);
+    sigaddset(&set, SIGBUS);
+    if (kvm_enabled()) {
+        /*
+         * We need to process timer signals synchronously to avoid a race
+         * between exit_request check and KVM vcpu entry.
+         */
+        sigaddset(&set, SIGIO);
+        sigaddset(&set, SIGALRM);
+    }
+#endif
+
+    sigfd = qemu_signalfd(&set);
     if (sigfd == -1) {
         fprintf(stderr, "failed to create signalfd\n");
         return -errno;
@@ -438,6 +464,12 @@  static void qemu_event_increment(void)
 static void qemu_kvm_eat_signals(CPUState *env)
 {
 }
+
+static int qemu_signalfd_init(void)
+{
+    return 0;
+}
+
 #endif /* _WIN32 */
 
 #ifndef CONFIG_IOTHREAD
@@ -471,39 +503,14 @@  static void qemu_kvm_init_cpu_signals(CPUState *env)
 #endif
 }
 
-#ifndef _WIN32
-static sigset_t block_synchronous_signals(void)
-{
-    sigset_t set;
-
-    sigemptyset(&set);
-    sigaddset(&set, SIGBUS);
-    if (kvm_enabled()) {
-        /*
-         * We need to process timer signals synchronously to avoid a race
-         * between exit_request check and KVM vcpu entry.
-         */
-        sigaddset(&set, SIGIO);
-        sigaddset(&set, SIGALRM);
-    }
-
-    return set;
-}
-#endif
-
 int qemu_init_main_loop(void)
 {
-#ifndef _WIN32
-    sigset_t blocked_signals;
     int ret;
 
-    blocked_signals = block_synchronous_signals();
-
-    ret = qemu_signalfd_init(blocked_signals);
+    ret = qemu_signalfd_init();
     if (ret) {
         return ret;
     }
-#endif
 
     qemu_init_sigbus();
 
@@ -651,35 +658,13 @@  static void qemu_tcg_init_cpu_signals(void)
     pthread_sigmask(SIG_UNBLOCK, &set, NULL);
 }
 
-static sigset_t block_io_signals(void)
-{
-    sigset_t set;
-
-    /* SIGUSR2 used by posix-aio-compat.c */
-    sigemptyset(&set);
-    sigaddset(&set, SIGUSR2);
-    pthread_sigmask(SIG_UNBLOCK, &set, NULL);
-
-    sigemptyset(&set);
-    sigaddset(&set, SIGIO);
-    sigaddset(&set, SIGALRM);
-    sigaddset(&set, SIG_IPI);
-    sigaddset(&set, SIGBUS);
-    pthread_sigmask(SIG_BLOCK, &set, NULL);
-
-    return set;
-}
-
 int qemu_init_main_loop(void)
 {
     int ret;
-    sigset_t blocked_signals;
 
     qemu_init_sigbus();
 
-    blocked_signals = block_io_signals();
-
-    ret = qemu_signalfd_init(blocked_signals);
+    ret = qemu_signalfd_init();
     if (ret) {
         return ret;
     }