[Qemu-devel] [PATCH] Fix signal handling of SIG_IPI when io-thread is enabled

[Qemu-devel] [PATCH] Fix signal handling of SIG_IPI when io-thread is enabled

[Alexandre Raymond]

Both the signal thread (via sigwait()) and the cpu thread (via
a normal signal handler) were attempting to catch SIG_IPI.

This resulted in random freezes under Darwin.

This patch separates SIG_IPI from the rest of the signals handled
by the signal thread, because it is independently caught by the cpu
thread.

Signed-off-by: Alexandre Raymond <cerbere@gmail.com>
---
 cpus.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/cpus.c b/cpus.c
index 18a1522..84ffd1c 100644
--- a/cpus.c
+++ b/cpus.c
@@ -394,10 +394,18 @@ static int qemu_signal_init(void)
     sigaddset(&set, SIGUSR2);
     pthread_sigmask(SIG_UNBLOCK, &set, NULL);
 
+    /*
+     * SIG_IPI must be blocked in the main thread and must not be caught
+     * by sigwait() in the signal thread. Otherwise, the cpu thread will
+     * not catch it reliably.
+     */
+    sigemptyset(&set);
+    sigaddset(&set, SIG_IPI);
+    pthread_sigmask(SIG_BLOCK, &set, NULL);
+
     sigemptyset(&set);
     sigaddset(&set, SIGIO);
     sigaddset(&set, SIGALRM);
-    sigaddset(&set, SIG_IPI);
     sigaddset(&set, SIGBUS);
 #else
     sigemptyset(&set);
-- 
1.7.5

Re: [Qemu-devel] [PATCH] Fix signal handling of SIG_IPI when io-thread is enabled

[Jan Kiszka]

On 2011-06-15 07:20, Alexandre Raymond wrote:
> Both the signal thread (via sigwait()) and the cpu thread (via
> a normal signal handler) were attempting to catch SIG_IPI.

Why? Ahh, because of qemu_cpu_kick_self: raise(SIG_IPI)! That should
generate a per-process SIG_IPI. And that may not only affect Darwin.
Looks good.

Acked-by: Jan Kiszka <jan.kiszka@siemens.com>

> 
> This resulted in random freezes under Darwin.
> 
> This patch separates SIG_IPI from the rest of the signals handled
> by the signal thread, because it is independently caught by the cpu
> thread.
> 
> Signed-off-by: Alexandre Raymond <cerbere@gmail.com>
> ---
>  cpus.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 18a1522..84ffd1c 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -394,10 +394,18 @@ static int qemu_signal_init(void)
>      sigaddset(&set, SIGUSR2);
>      pthread_sigmask(SIG_UNBLOCK, &set, NULL);
>  
> +    /*
> +     * SIG_IPI must be blocked in the main thread and must not be caught
> +     * by sigwait() in the signal thread. Otherwise, the cpu thread will
> +     * not catch it reliably.
> +     */
> +    sigemptyset(&set);
> +    sigaddset(&set, SIG_IPI);
> +    pthread_sigmask(SIG_BLOCK, &set, NULL);
> +
>      sigemptyset(&set);
>      sigaddset(&set, SIGIO);
>      sigaddset(&set, SIGALRM);
> -    sigaddset(&set, SIG_IPI);
>      sigaddset(&set, SIGBUS);
>  #else
>      sigemptyset(&set);

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] Fix signal handling of SIG_IPI when io-thread is enabled

[Alexandre Raymond]

Hi Jan,

> Why? Ahh, because of qemu_cpu_kick_self: raise(SIG_IPI)! That should
> generate a per-process SIG_IPI. And that may not only affect Darwin.
> Looks good.

Actually, with io-thread enabled, it goes through qemu_cpu_kick_self()
-> qemu_cpu_kick_thread() -> pthread_kill(..., SIG_IPI).

I think the problem is with sigwait(). It doesn't state so in the
Linux or Darwin man pages, but on Solaris, it says : "All signals
identified by the set argument must be blocked on all threads,
including the calling thread; otherwise, sigwait() might not work
correctly", which might correspond to the issue I've been witnessing
(ie: sigwait() unblocking once in a while on a SIGUSR1 (SIG_IPI) in
the event thread).

In any case, I don't think it should attempt to catch this signal at
all since the cpu thread is already catching it.

Alexandre

Re: [Qemu-devel] [PATCH] Fix signal handling of SIG_IPI when io-thread is enabled

[Alexandre Raymond]

ping?

On Wed, Jun 15, 2011 at 10:11 AM, Alexandre Raymond <cerbere@gmail.com> wrote:
> Hi Jan,
>
>> Why? Ahh, because of qemu_cpu_kick_self: raise(SIG_IPI)! That should
>> generate a per-process SIG_IPI. And that may not only affect Darwin.
>> Looks good.
>
> Actually, with io-thread enabled, it goes through qemu_cpu_kick_self()
> -> qemu_cpu_kick_thread() -> pthread_kill(..., SIG_IPI).
>
> I think the problem is with sigwait(). It doesn't state so in the
> Linux or Darwin man pages, but on Solaris, it says : "All signals
> identified by the set argument must be blocked on all threads,
> including the calling thread; otherwise, sigwait() might not work
> correctly", which might correspond to the issue I've been witnessing
> (ie: sigwait() unblocking once in a while on a SIGUSR1 (SIG_IPI) in
> the event thread).
>
> In any case, I don't think it should attempt to catch this signal at
> all since the cpu thread is already catching it.
>
> Alexandre
>

Re: [Qemu-devel] [PATCH] Fix signal handling of SIG_IPI when io-thread is enabled

[Blue Swirl]

Thanks, applied.

On Sat, Jul 9, 2011 at 5:33 AM, Alexandre Raymond <cerbere@gmail.com> wrote:
> ping?
>
> On Wed, Jun 15, 2011 at 10:11 AM, Alexandre Raymond <cerbere@gmail.com> wrote:
>> Hi Jan,
>>
>>> Why? Ahh, because of qemu_cpu_kick_self: raise(SIG_IPI)! That should
>>> generate a per-process SIG_IPI. And that may not only affect Darwin.
>>> Looks good.
>>
>> Actually, with io-thread enabled, it goes through qemu_cpu_kick_self()
>> -> qemu_cpu_kick_thread() -> pthread_kill(..., SIG_IPI).
>>
>> I think the problem is with sigwait(). It doesn't state so in the
>> Linux or Darwin man pages, but on Solaris, it says : "All signals
>> identified by the set argument must be blocked on all threads,
>> including the calling thread; otherwise, sigwait() might not work
>> correctly", which might correspond to the issue I've been witnessing
>> (ie: sigwait() unblocking once in a while on a SIGUSR1 (SIG_IPI) in
>> the event thread).
>>
>> In any case, I don't think it should attempt to catch this signal at
>> all since the cpu thread is already catching it.
>>
>> Alexandre
>>
>
>