[alsa-devel] [PATCH] Add async support to ioplug

[Lennart Poettering]

On Tue, 20.05.08 13:10, Takashi Iwai (tiwai at suse.de) wrote:

> Hi,
> 
> the patch below adds a framework to support async on ioplug.  Now
> an external ioplug plugin can implement appropriate async methods if
> possible.
> 
> The async callback is just a copy of snd_pcm_async().  When sig >= 0,
> activate the async, and with a negative value, deactivate async.
> 
> The test patch to alsa-plugins will follow.

Humm, I am not really happy about this. 

Firstly I thing that async is a really bad thing anyway. Signal handler
segmantics are very different from normal process semantics, i.e. a
lot of API functions are not reentrant and not signal handler
safe. Also, a lot of people implementing signal handlers don't
save/restore errno the way the should (libasound being one example
btw, as I just checked, this needs fixing). It's a fact: implementing
signal handlers is difficult, and people almost never get it right. I
mean, because it is so difficult there's even a valgrind module that
helps identifying problems with signal handlers!  

Also, signals are a shared resource where no reservation scheme
exists. If one library in a process takes posession of a signal then
it might overwrite the previous signal handler, there is no sane way
to detect that. Flash 9 for example registers an alsa async handler,
which causes SIGIO to be overwritten. It's sheer luck that noone else
in the Firefox process wants to use SIGIO, because that would clash
with Flash -- and this would not even be detected! And SIGIO is used
for a lot of stuff, among them the whole aio subsystem, dnotify alsa
and a few others. To emphasize this: if you use alsa async, you cannot
use aio or dnotify in the same process!

Signal delivery for usage in event loops on Linux is slow. Much slower
than poll() based event loop approaches (somewhere on the web is a detailed
comparison of poll() loops vs. RT sig based event loops in regards to
throughput, I can dig that up if someone wants to see that.)

In summary: async doesn't give any benefits. However, it creates quite
a few problems:

In the case of flash it just signals a semaphore from the async
handler which is slept on by the sound loop thread. This is really not
a clean design, anyway. They should directly sleep on the audio
fds. With Flash 10 they responded to criticism of mine. It doesn't use
async anymore and is those compatible with PA and other ioplug
modules.

What I want to say is: using signals is bad style. Don't use
signals. I do believe the best would be to remove async support from
ALSA entirely. Grepping through google code search I couldn't find a
single program using alsa async where using it was actually a good
idea. Adding even further async support to ALSA does not strike me as
a good idea. I believe the plan must be to remove async, not extend
its support. And such a plan I think would be perfectly in line with
what happens on the kernel front anyway. We now have stuff like
signalfd(), explicitly because signal handlers are so ugly. For aio
people are looking for alternatives without sig handlers, too. And
yes, one of the reasons dnotify got replaced by inotify is that it
used SIGIO.

What however strikes me particularly questionnable about this patch is
that this way ioplug async handlers are not called from signal handler
context, instead they are called directly from process context. This
is really problematic. Firstly, if people start to use async on ioplug
a lot they will very likely produce code that only runs fine from
normal process context and will break horribly from signal handler
process (e.g. use synchronization based on mutexes). More importantly
however, if a program uses signal masks to make sure that the async
handler is run from a very specific thread or temporarily disabled for
criticial sections this will be breaking for ioplug drivers. 

If you really think that extending async to iolpug is a good idea then
this should probably done by using sigqueue() to manually enqueue
SIGIO and thus make sure the async events pass the usual signal
delivery logic.

When I analyzed the Flash+PA incompatibility I though about extending
ioplug for async myself. But in the end I thought that enqueing SIGIOs
from userspace was so ugly it would never have a chance to go upstream.

Again, please let's drop async altogether. It's error-prone, easy
to misuse, and especially Flash 9 (which I assume is the reason you
came up with the patch) is a prominent misuser of it, so with Flash as
only reason to do this I think the whole idea is void.

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net         ICQ# 11060553
http://0pointer.net/lennart/           GnuPG 0x1A015CC4