[alsa-devel] [PATCH] Add async support to ioplug
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.
Takashi
--- diff -r 8b22022a861d include/local.h --- a/include/local.h Fri May 09 16:02:02 2008 +0200 +++ b/include/local.h Tue May 20 09:02:35 2008 +0200 @@ -149,7 +149,13 @@ void *private_data; struct list_head glist; struct list_head hlist; + int no_signal; }; + +int _snd_async_add_handler(snd_async_handler_t **handler, int fd, + snd_async_callback_t callback, + void *private_data, int no_signal); +void _snd_async_call_handlers(int fd);
typedef enum _snd_set_mode { SND_CHANGE, diff -r 8b22022a861d include/pcm_ioplug.h --- a/include/pcm_ioplug.h Fri May 09 16:02:02 2008 +0200 +++ b/include/pcm_ioplug.h Tue May 20 09:02:35 2008 +0200 @@ -66,7 +66,7 @@ */ #define SND_PCM_IOPLUG_VERSION_MAJOR 1 /**< Protocol major version */ #define SND_PCM_IOPLUG_VERSION_MINOR 0 /**< Protocol minor version */ -#define SND_PCM_IOPLUG_VERSION_TINY 1 /**< Protocol tiny version */ +#define SND_PCM_IOPLUG_VERSION_TINY 2 /**< Protocol tiny version */ /** * IO-plugin protocol version */ @@ -114,6 +114,10 @@ unsigned int rate; /**< rate; filled after hw_params is called */ snd_pcm_uframes_t period_size; /**< period size; filled after hw_params is called */ snd_pcm_uframes_t buffer_size; /**< buffer size; filled after hw_params is called */ + /** + * fields added by version 1.0.2 + */ + unsigned int no_signal_async; /**< async support without signal */ };
/** Callback table of ioplug */ @@ -189,6 +193,10 @@ * get the delay for the running PCM; optional */ int (*delay)(snd_pcm_ioplug_t *io, snd_pcm_sframes_t *delayp); + /** + * set up async handler; optional; added in version 1.0.2 + */ + int (*async)(snd_pcm_ioplug_t *io, int sig, pid_t pid); };
@@ -212,6 +220,9 @@ /* change PCM status */ int snd_pcm_ioplug_set_state(snd_pcm_ioplug_t *ioplug, snd_pcm_state_t state);
+/* call async handlers */ +void snd_pcm_ioplug_call_async_handlers(snd_pcm_ioplug_t *ioplug); + /** } */
#endif /* __ALSA_PCM_IOPLUG_H */ diff -r 8b22022a861d src/async.c --- a/src/async.c Fri May 09 16:02:02 2008 +0200 +++ b/src/async.c Tue May 20 09:02:35 2008 +0200 @@ -48,19 +48,48 @@ #endif
static LIST_HEAD(snd_async_handlers); +static int sig_count;
-static void snd_async_handler(int signo ATTRIBUTE_UNUSED, siginfo_t *siginfo, void *context ATTRIBUTE_UNUSED) +#ifndef DOC_HIDDEN +void _snd_async_call_handlers(int fd) { - int fd; struct list_head *i; - //assert(siginfo->si_code == SI_SIGIO); - fd = siginfo->si_fd; list_for_each(i, &snd_async_handlers) { snd_async_handler_t *h = list_entry(i, snd_async_handler_t, glist); if (h->fd == fd && h->callback) h->callback(h); } } +#endif + +static void snd_async_handler(int signo ATTRIBUTE_UNUSED, siginfo_t *siginfo, void *context ATTRIBUTE_UNUSED) +{ + int fd; + //assert(siginfo->si_code == SI_SIGIO); + fd = siginfo->si_fd; + _snd_async_call_handlers(fd); +} + +#ifndef DOC_HIDDEN +int _snd_async_add_handler(snd_async_handler_t **handler, int fd, + snd_async_callback_t callback, + void *private_data, int no_signal) +{ + snd_async_handler_t *h; + assert(handler); + h = malloc(sizeof(*h)); + if (!h) + return -ENOMEM; + h->fd = fd; + h->callback = callback; + h->private_data = private_data; + list_add_tail(&h->glist, &snd_async_handlers); + INIT_LIST_HEAD(&h->hlist); + h->no_signal = no_signal; + *handler = h; + return 0; +} +#endif
/** * \brief Registers an async handler. @@ -94,20 +123,13 @@ int snd_async_add_handler(snd_async_handler_t **handler, int fd, snd_async_callback_t callback, void *private_data) { - snd_async_handler_t *h; - int was_empty; - assert(handler); - h = malloc(sizeof(*h)); - if (!h) - return -ENOMEM; - h->fd = fd; - h->callback = callback; - h->private_data = private_data; - was_empty = list_empty(&snd_async_handlers); - list_add_tail(&h->glist, &snd_async_handlers); - INIT_LIST_HEAD(&h->hlist); - *handler = h; - if (was_empty) { + int err; + + err = _snd_async_add_handler(handler, fd, callback, private_data, 0); + if (err < 0) + return err; + + if (!sig_count) { int err; struct sigaction act; memset(&act, 0, sizeof(act)); @@ -116,10 +138,15 @@ sigemptyset(&act.sa_mask); err = sigaction(snd_async_signo, &act, NULL); if (err < 0) { + err = -errno; SYSERR("sigaction"); - return -errno; + list_del(&(*handler)->glist); + free(*handler); + *handler = NULL; + return err; } } + sig_count++; return 0; }
@@ -133,15 +160,18 @@ int err = 0; assert(handler); list_del(&handler->glist); - if (list_empty(&snd_async_handlers)) { - struct sigaction act; - memset(&act, 0, sizeof(act)); - act.sa_flags = 0; - act.sa_handler = SIG_DFL; - err = sigaction(snd_async_signo, &act, NULL); - if (err < 0) { - SYSERR("sigaction"); - return -errno; + if (!handler->no_signal) { + sig_count--; + if (!sig_count) { + struct sigaction act; + memset(&act, 0, sizeof(act)); + act.sa_flags = 0; + act.sa_handler = SIG_DFL; + err = sigaction(snd_async_signo, &act, NULL); + if (err < 0) { + SYSERR("sigaction"); + /* return -errno; */ + } } } if (handler->type == SND_ASYNC_HANDLER_GENERIC) diff -r 8b22022a861d src/pcm/pcm.c --- a/src/pcm/pcm.c Fri May 09 16:02:02 2008 +0200 +++ b/src/pcm/pcm.c Tue May 20 09:02:36 2008 +0200 @@ -1984,8 +1984,10 @@ int err; int was_empty; snd_async_handler_t *h; - err = snd_async_add_handler(&h, _snd_pcm_async_descriptor(pcm), - callback, private_data); + + err = _snd_async_add_handler(&h, _snd_pcm_async_descriptor(pcm), + callback, private_data, + pcm->no_signal_async); if (err < 0) return err; h->type = SND_ASYNC_HANDLER_PCM; diff -r 8b22022a861d src/pcm/pcm_ioplug.c --- a/src/pcm/pcm_ioplug.c Fri May 09 16:02:02 2008 +0200 +++ b/src/pcm/pcm_ioplug.c Tue May 20 09:02:36 2008 +0200 @@ -698,11 +698,15 @@ return 0; }
-static int snd_pcm_ioplug_async(snd_pcm_t *pcm ATTRIBUTE_UNUSED, - int sig ATTRIBUTE_UNUSED, - pid_t pid ATTRIBUTE_UNUSED) +static int snd_pcm_ioplug_async(snd_pcm_t *pcm, int sig, pid_t pid) { - return -ENOSYS; + ioplug_priv_t *io = pcm->private_data; + + if (io->data->version >= 0x010002 && + io->data->callback->async) + return io->data->callback->async(io->data, sig, pid); + else + return -ENOSYS; }
static int snd_pcm_ioplug_munmap(snd_pcm_t *pcm ATTRIBUTE_UNUSED) @@ -1037,6 +1041,9 @@ ioplug->pcm->poll_events = ioplug->poll_events; ioplug->pcm->monotonic = (ioplug->flags & SND_PCM_IOPLUG_FLAG_MONOTONIC) != 0; ioplug->pcm->mmap_rw = ioplug->mmap_rw; + if (ioplug->version >= 0x010002) + ioplug->pcm->no_signal_async = ioplug->no_signal_async; + return 0; }
@@ -1070,3 +1077,16 @@ ioplug->state = state; return 0; } + +/** + * \brief Call the associated async handlers + * \param ioplug the ioplug handle + * + * Call the async handlers associated with the given ioplug. + * This function is usually called from the plugin's async handling + * routine appropriately. + */ +void snd_pcm_ioplug_call_async_handlers(snd_pcm_ioplug_t *ioplug) +{ + _snd_async_call_handlers(ioplug->poll_fd); +} diff -r 8b22022a861d src/pcm/pcm_local.h --- a/src/pcm/pcm_local.h Fri May 09 16:02:02 2008 +0200 +++ b/src/pcm/pcm_local.h Tue May 20 09:02:36 2008 +0200 @@ -220,6 +220,7 @@ * use the mmaped buffer of the slave */ unsigned int donot_close: 1; /* don't close this PCM */ + unsigned int no_signal_async :1; /* async without signal */ snd_pcm_channel_info_t *mmap_channels; snd_pcm_channel_area_t *running_areas; snd_pcm_channel_area_t *stopped_areas;
This patch adds async support to pulse and jack plugins. I tested only jack plugin, so far. The pulse might be buggy due to possible races.
Anyway, as you can see, the implementation is pretty simple for a plugin with a parallel thread like jack or pulse. For other cases like oss plugin, it might be more difficult.
Takashi
--- diff -r 0833aa56dbe2 jack/pcm_jack.c --- a/jack/pcm_jack.c Tue May 13 13:16:06 2008 +0200 +++ b/jack/pcm_jack.c Tue May 20 13:07:57 2008 +0200 @@ -48,6 +48,8 @@
jack_port_t **ports; jack_client_t *client; + + int call_async; } snd_pcm_jack_t;
static void snd_pcm_jack_free(snd_pcm_jack_t *jack) @@ -90,6 +92,18 @@ *revents = pfds[0].revents; return 0; } + +#if SND_PCM_IOPLUG_VERSION >= 0x010002 +static int snd_pcm_jack_async(snd_pcm_ioplug_t *io, int sig, pid_t pid) +{ + snd_pcm_jack_t *jack = io->private_data; + if (sig < 0) + jack->call_async = 0; + else + jack->call_async = 1; + return 0; +} +#endif
static snd_pcm_sframes_t snd_pcm_jack_pointer(snd_pcm_ioplug_t *io) { @@ -145,6 +159,11 @@
write(jack->fd, buf, 1); /* for polling */
+#if SND_PCM_IOPLUG_VERSION >= 0x010002 + /* call async handlers if needed */ + if (jack->call_async) + snd_pcm_ioplug_call_async_handlers(io); +#endif return 0; }
@@ -238,6 +257,9 @@ .pointer = snd_pcm_jack_pointer, .prepare = snd_pcm_jack_prepare, .poll_revents = snd_pcm_jack_poll_revents, +#if SND_PCM_IOPLUG_VERSION >= 0x010002 + .async = snd_pcm_jack_async, +#endif };
#define ARRAY_SIZE(ary) (sizeof(ary)/sizeof(ary[0])) @@ -372,6 +394,9 @@ jack->io.poll_fd = fd[1]; jack->io.poll_events = POLLIN; jack->io.mmap_rw = 1; +#if SND_PCM_IOPLUG_VERSION >= 0x010002 + jack->io.no_signal_async = 1; +#endif
err = snd_pcm_ioplug_create(&jack->io, name, stream, mode); if (err < 0) { diff -r 0833aa56dbe2 pulse/pcm_pulse.c --- a/pulse/pcm_pulse.c Tue May 13 13:16:06 2008 +0200 +++ b/pulse/pcm_pulse.c Tue May 20 13:07:57 2008 +0200 @@ -45,6 +45,8 @@ pa_sample_spec ss; unsigned int frame_size; pa_buffer_attr buffer_attr; + + int call_async; } snd_pcm_pulse_t;
static void update_ptr(snd_pcm_pulse_t *pcm) @@ -365,6 +367,11 @@ assert(pcm->p);
pulse_poll_activate(pcm->p); + +#if SND_PCM_IOPLUG_VERSION >= 0x010002 + if (pcm->call_async) + snd_pcm_ioplug_call_async_handlers(&pcm->io); +#endif }
static void stream_underrun_cb(pa_stream *p, void *userdata) { @@ -446,6 +453,18 @@
return err; } + +#if SND_PCM_IOPLUG_VERSION >= 0x010002 +static int pulse_pcm_async(snd_pcm_ioplug_t *io, int sig, pid_t pid) +{ + snd_pcm_pulse_t *pcm = io->private_data; + if (sig < 0) + pcm->call_async = 0; + else + pcm->call_async = 1; + return 0; +} +#endif
static int pulse_prepare(snd_pcm_ioplug_t *io) { @@ -620,6 +639,9 @@ .prepare = pulse_prepare, .hw_params = pulse_hw_params, .close = pulse_close, +#if SND_PCM_IOPLUG_VERSION >= 0x010002 + .async = pulse_pcm_async, +#endif };
@@ -635,6 +657,9 @@ .prepare = pulse_prepare, .hw_params = pulse_hw_params, .close = pulse_close, +#if SND_PCM_IOPLUG_VERSION >= 0x010002 + .async = pulse_pcm_async, +#endif };
@@ -746,6 +771,9 @@ pcm->io.poll_fd = -1; pcm->io.poll_events = 0; pcm->io.mmap_rw = 0; +#if SND_PCM_IOPLUG_VERSION >= 0x010002 + pcm->io.no_signal_async = 1; +#endif pcm->io.callback = stream == SND_PCM_STREAM_PLAYBACK ? &pulse_playback_callback : &pulse_capture_callback; pcm->io.private_data = pcm;
On Tue, 20.05.08 13:10, Takashi Iwai (tiwai@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, you don't need lengthy explanation :) I was already convinced that async is evil; that's why I didn't implement it in ioplug...
At Thu, 22 May 2008 00:16:43 +0200, Lennart Poettering wrote:
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.
IIRC, the wine code used async, too. I guess it was already fixed, though.
It was just a 10 minutes test code, and I'm willing to drop it if flash9 doesn't work with pulseaudio anyway.
thanks,
Takashi
participants (2)
-
Lennart Poettering -
Takashi Iwai