[Qemu-devel] SDL audio and AIO hogging each other's signals

[Qemu-devel] SDL audio and AIO hogging each other's signals

[andrzej zaborowski]

Hi,
  with QEMU_AUDIO_DRV set to "sdl" and booting from CD-ROM with AIO on
a Linux host and with SDL 1.2.11, qemu locks up in sigwait() (the main
thread) and SDL_SemWait() (the audio thread) as soon as music is
playing and CD-ROM is being read at the same time. It appears that
audio/sdlaudio.c:sdl_callback is called by SDL when it shouldn't be
called, and block-raw.c is trying to flush the AIO operations, so it
would seem that the SIGUSR2 which is intended to wake up the sigwait
is instead captured by SDL and SDL tries to be smart and calls
sdl_callback. sdl_callback has a sanity check but this check is
*after* SDL_SemWait() so it is not triggered. The strange thing is
that using a different signal (tried SIGUSR1 and SIGPOLL) for AIO
doesn't help. Does SDL catch all signals?
I could be totally wrong because I don't know SDLAudio at all.

Any ideas about the exact reason why this is happening and how to fix it?

Regards,
Andrew

[Qemu-devel] Re: SDL audio and AIO hogging each other's signals

[malc]

On Tue, 3 Apr 2007, andrzej zaborowski wrote:

> Hi,
> with QEMU_AUDIO_DRV set to "sdl" and booting from CD-ROM with AIO on
> a Linux host and with SDL 1.2.11, qemu locks up in sigwait() (the main
> thread) and SDL_SemWait() (the audio thread) as soon as music is
> playing and CD-ROM is being read at the same time. It appears that
> audio/sdlaudio.c:sdl_callback is called by SDL when it shouldn't be
> called, and block-raw.c is trying to flush the AIO operations, so it
> would seem that the SIGUSR2 which is intended to wake up the sigwait
> is instead captured by SDL and SDL tries to be smart and calls
> sdl_callback. sdl_callback has a sanity check but this check is
> *after* SDL_SemWait() so it is not triggered. The strange thing is
> that using a different signal (tried SIGUSR1 and SIGPOLL) for AIO
> doesn't help. Does SDL catch all signals?
> I could be totally wrong because I don't know SDLAudio at all.

If my reading of SDLs sources are correct then it shouldn't be trying
to catch any signals when explicitly instructed not to do so (which is
done in sdl.c (SDL_INIT_NOPARACHUTE flag)).

>
> Any ideas about the exact reason why this is happening and how to fix it?
>

Given the semantics of signal delivery in POSIX what you describe
might happen, what is a little surprising is that SDL (btw. which
backend SDL uses?) doesn't complain.

Here's my theory: signal will be delivered to the arbitrary thread
that happens to not block it, for whatever reason, the thread SDL
created to do audio processing is picked up, which leads to some
system call being interrupted(eventually) and -1 (errno == EINTR), SDL
happily continues calling stuff.

One solution would be to explicitly block everything upon entering
sdl_callback for the first time. This is ugly and can have any
consequences one cares to imagine, but that's SDL for you (any
particular reason why you are using it?)


P.S. Quoting PTHREAD_SIGNAL(3)

NOTES
        For !sigwait! to work reliably, the signals being waited  for  must  be
        blocked in all threads, not only in the calling thread, since otherwise
        the POSIX semantics for signal delivery do not guarantee that it's  the
        thread  doing the !sigwait! that will receive the signal.  The best way
        to achieve this is block those signals before any threads are  created,
        and  never unblock them in the program other than by calling !sigwait!.

-- 
vale

Re: [Qemu-devel] SDL audio and AIO hogging each other's signals

[Ben Taylor]

---- andrzej zaborowski <balrog@zabor.org> wrote: 
> Hi,
>   with QEMU_AUDIO_DRV set to "sdl" and booting from CD-ROM with AIO on
> a Linux host and with SDL 1.2.11, qemu locks up in sigwait() (the main
> thread) and SDL_SemWait() (the audio thread) as soon as music is
> playing and CD-ROM is being read at the same time. It appears that
> audio/sdlaudio.c:sdl_callback is called by SDL when it shouldn't be
> called, and block-raw.c is trying to flush the AIO operations, so it
> would seem that the SIGUSR2 which is intended to wake up the sigwait
> is instead captured by SDL and SDL tries to be smart and calls
> sdl_callback. sdl_callback has a sanity check but this check is
> *after* SDL_SemWait() so it is not triggered. The strange thing is
> that using a different signal (tried SIGUSR1 and SIGPOLL) for AIO
> doesn't help. Does SDL catch all signals?
> I could be totally wrong because I don't know SDLAudio at all.
> 
> Any ideas about the exact reason why this is happening and how to fix it?

On Solaris 10/11 x86, I was seeing a bunch of semphore errors emitted
from SDL when using SDL audio.

I'll track this down and report back.

Ben

[Qemu-devel] Re: SDL audio and AIO hogging each other's signals

[andrzej zaborowski]

Hi, thanks for quick response!

On 03/04/07, malc <av1474@comtv.ru> wrote:
> On Tue, 3 Apr 2007, andrzej zaborowski wrote:
>
> > Hi,
> > with QEMU_AUDIO_DRV set to "sdl" and booting from CD-ROM with AIO on
> > a Linux host and with SDL 1.2.11, qemu locks up in sigwait() (the main
> > thread) and SDL_SemWait() (the audio thread) as soon as music is
> > playing and CD-ROM is being read at the same time. It appears that
> > audio/sdlaudio.c:sdl_callback is called by SDL when it shouldn't be
> > called, and block-raw.c is trying to flush the AIO operations, so it
> > would seem that the SIGUSR2 which is intended to wake up the sigwait
> > is instead captured by SDL and SDL tries to be smart and calls
> > sdl_callback. sdl_callback has a sanity check but this check is
> > *after* SDL_SemWait() so it is not triggered. The strange thing is
> > that using a different signal (tried SIGUSR1 and SIGPOLL) for AIO
> > doesn't help. Does SDL catch all signals?
> > I could be totally wrong because I don't know SDLAudio at all.
>
> If my reading of SDLs sources are correct then it shouldn't be trying
> to catch any signals when explicitly instructed not to do so (which is
> done in sdl.c (SDL_INIT_NOPARACHUTE flag)).
>
> >
> > Any ideas about the exact reason why this is happening and how to fix it?
> >
>
> Given the semantics of signal delivery in POSIX what you describe
> might happen, what is a little surprising is that SDL (btw. which
> backend SDL uses?) doesn't complain.

It should be using ALSA.

>
> Here's my theory: signal will be delivered to the arbitrary thread
> that happens to not block it, for whatever reason, the thread SDL
> created to do audio processing is picked up, which leads to some
> system call being interrupted(eventually) and -1 (errno == EINTR), SDL
> happily continues calling stuff.

Yes, reading the PTHREAD_SIGNAL(3) note, this sounds very likely.

>
> One solution would be to explicitly block everything upon entering
> sdl_callback for the first time. This is ugly and can have any
> consequences one cares to imagine, but that's SDL for you (any
> particular reason why you are using it?)

Not really - just had only OSS and SDL compiled into qemu at this moment.

Yes, the suggested solution works. Unfortunately it's neither pretty
nor correct, because at the time sdl_callback runs, the signal which
was supposed to wake up sigwait() is already lost and I can't find any
way to prevent that - we can add a kill(0, SIGUSR2) but this is even
uglier and again we don't know when sdl_callback is called as a result
of this signal and when legally. (That's also why I didn't put a
"return" after pthread_sigmask().)

diff --git a/audio/sdlaudio.c b/audio/sdlaudio.c
index f2a6896..efeb3c0 100644
--- a/audio/sdlaudio.c
+++ b/audio/sdlaudio.c
@@ -46,6 +46,7 @@ struct SDLAudioState {
     SDL_mutex *mutex;
     SDL_sem *sem;
     int initialized;
+    int threadstart;
 } glob_sdl;
 typedef struct SDLAudioState SDLAudioState;

@@ -197,13 +198,25 @@ static void sdl_close (SDLAudioState *s)
     }
 }

+#ifdef __sun__
+#define _POSIX_PTHREAD_SEMANTICS 1
+#endif
+#include <signal.h>
+
 static void sdl_callback (void *opaque, Uint8 *buf, int len)
 {
     SDLVoiceOut *sdl = opaque;
     SDLAudioState *s = &glob_sdl;
     HWVoiceOut *hw = &sdl->hw;
     int samples = len >> hw->info.shift;
+    sigset_t set;

+    if (s->threadstart) {
+        /* QEMU uses sigwait() so all threads should block signals.  */
+        s->threadstart = 0;
+        sigfillset(&set);
+        pthread_sigmask(SIG_BLOCK, &set, 0);
+    }
     if (s->exit) {
         return;
     }
@@ -313,6 +326,8 @@ static int sdl_init_out (HWVoiceOut *hw, audsettings_t *as)
     audfmt_e effective_fmt;
     audsettings_t obt_as;

+    s->threadstart = 1;
+
     shift <<= as->nchannels == 2;

     req.freq = as->freq;

What POSIX needs is a way to set the default signal mask for new threads :-/
Regards,
Andrzej

[Qemu-devel] Re: SDL audio and AIO hogging each other's signals

[malc]

On Wed, 4 Apr 2007, andrzej zaborowski wrote:

> Hi, thanks for quick response!
>
> On 03/04/07, malc <av1474@comtv.ru> wrote:
>> On Tue, 3 Apr 2007, andrzej zaborowski wrote:

[..snip..]

> It should be using ALSA.
>
>> 
>> Here's my theory: signal will be delivered to the arbitrary thread
>> that happens to not block it, for whatever reason, the thread SDL
>> created to do audio processing is picked up, which leads to some
>> system call being interrupted(eventually) and -1 (errno == EINTR), SDL
>> happily continues calling stuff.

Actually the signal can be just handled (by whatever signal handler
QEMU installed with sigaction) in a SDL created thread, so things can,
and mostlikely will, be silently wrong, i.e. EINTR while possible will
not necessarily happen.

>
> Yes, reading the PTHREAD_SIGNAL(3) note, this sounds very likely.
>
>> 
>> One solution would be to explicitly block everything upon entering
>> sdl_callback for the first time. This is ugly and can have any
>> consequences one cares to imagine, but that's SDL for you (any
>> particular reason why you are using it?)
>
> Not really - just had only OSS and SDL compiled into qemu at this moment.
>
> Yes, the suggested solution works. Unfortunately it's neither pretty
> nor correct, because at the time sdl_callback runs, the signal which
> was supposed to wake up sigwait() is already lost and I can't find any
> way to prevent that - we can add a kill(0, SIGUSR2) but this is even
> uglier and again we don't know when sdl_callback is called as a result
> of this signal and when legally. (That's also why I didn't put a
> "return" after pthread_sigmask().)

[..snip..]

> What POSIX needs is a way to set the default signal mask for new threads :-/

http://www.opengroup.org/onlinepubs/007908799/xsh/pthread_create.html
in part reads
<quote>
The signal state of the new thread is initialised as follows:

     * The signal mask is inherited from the creating thread.
</quote>

Hence sequence of:

sigfillset (newset)
pthread_sigmask (SIG_BLOCK, newset, oldset)
SDL_OpenAudio (...)
pthread_sigmask (SGI_SETMASK, oldset, NULL)

Will probably achieve the desired effect.

-- 
vale

[Qemu-devel] Re: SDL audio and AIO hogging each other's signals

[andrzej zaborowski]

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

On 04/04/07, malc <av1474@comtv.ru> wrote:
> On Wed, 4 Apr 2007, andrzej zaborowski wrote:
>
> > Hi, thanks for quick response!
> >
> > On 03/04/07, malc <av1474@comtv.ru> wrote:
> >> On Tue, 3 Apr 2007, andrzej zaborowski wrote:
>
> [..snip..]
>
> > It should be using ALSA.
> >
> >>
> >> Here's my theory: signal will be delivered to the arbitrary thread
> >> that happens to not block it, for whatever reason, the thread SDL
> >> created to do audio processing is picked up, which leads to some
> >> system call being interrupted(eventually) and -1 (errno == EINTR), SDL
> >> happily continues calling stuff.
>
> Actually the signal can be just handled (by whatever signal handler
> QEMU installed with sigaction) in a SDL created thread, so things can,
> and mostlikely will, be silently wrong, i.e. EINTR while possible will
> not necessarily happen.
>
> >
> > Yes, reading the PTHREAD_SIGNAL(3) note, this sounds very likely.
> >
> >>
> >> One solution would be to explicitly block everything upon entering
> >> sdl_callback for the first time. This is ugly and can have any
> >> consequences one cares to imagine, but that's SDL for you (any
> >> particular reason why you are using it?)
> >
> > Not really - just had only OSS and SDL compiled into qemu at this moment.
> >
> > Yes, the suggested solution works. Unfortunately it's neither pretty
> > nor correct, because at the time sdl_callback runs, the signal which
> > was supposed to wake up sigwait() is already lost and I can't find any
> > way to prevent that - we can add a kill(0, SIGUSR2) but this is even
> > uglier and again we don't know when sdl_callback is called as a result
> > of this signal and when legally. (That's also why I didn't put a
> > "return" after pthread_sigmask().)
>
> [..snip..]
>
> > What POSIX needs is a way to set the default signal mask for new threads :-/
>
> http://www.opengroup.org/onlinepubs/007908799/xsh/pthread_create.html
> in part reads
> <quote>
> The signal state of the new thread is initialised as follows:
>
>      * The signal mask is inherited from the creating thread.
> </quote>

Oh, right.

>
> Hence sequence of:
>
> sigfillset (newset)
> pthread_sigmask (SIG_BLOCK, newset, oldset)
> SDL_OpenAudio (...)
> pthread_sigmask (SGI_SETMASK, oldset, NULL)
>
> Will probably achieve the desired effect.

Thanks, this works!

Attached is a patch, should be safe to apply. We're still making the
assumption that the thread is created precisely in SDL_OpenAudio and
not later, but this should not be a problem because all signals in the
main thread should be blocked throughout the entire runtime.

Regards,
Andrzej

[-- Attachment #2: 0001-Ensure-signals-are-properly-masked-for-new-SDL-Audio-threads.txt --]
[-- Type: text/plain, Size: 1270 bytes --]

From 6843399e5e751e999f33de09e4476f7c96839974 Mon Sep 17 00:00:00 2001
From: Andrzej Zaborowski <balrog@zabor.org>
Date: Wed, 4 Apr 2007 15:18:20 +0200
Subject: [PATCH] Ensure signals are properly masked for new SDL Audio threads.

---
 audio/sdlaudio.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/audio/sdlaudio.c b/audio/sdlaudio.c
index f2a6896..11edab0 100644
--- a/audio/sdlaudio.c
+++ b/audio/sdlaudio.c
@@ -25,6 +25,13 @@
 #include <SDL_thread.h>
 #include "vl.h"
 
+#ifndef _WIN32
+#ifdef __sun__
+#define _POSIX_PTHREAD_SEMANTICS 1
+#endif
+#include <signal.h>
+#endif
+
 #define AUDIO_CAP "sdl"
 #include "audio_int.h"
 
@@ -177,11 +184,22 @@ static int sdl_to_audfmt (int sdlfmt, audfmt_e *fmt, int *endianess)
 static int sdl_open (SDL_AudioSpec *req, SDL_AudioSpec *obt)
 {
     int status;
+#ifndef _WIN32
+    sigset_t new, old;
+
+    /* Make sure potential threads created by SDL don't hog signals.  */
+    sigfillset (&new);
+    pthread_sigmask (SIG_BLOCK, &new, &old);
+#endif
 
     status = SDL_OpenAudio (req, obt);
     if (status) {
         sdl_logerr ("SDL_OpenAudio failed\n");
     }
+
+#ifndef _WIN32
+    pthread_sigmask (SIG_SETMASK, &old, 0);
+#endif
     return status;
 }
 
-- 
1.4.4.3