[alsa-devel] Races in alsa-lib with threads
Takashi Iwai
tiwai at suse.de
Thu Nov 15 08:39:58 CET 2012
At Wed, 14 Nov 2012 14:49:36 -0500,
Trent Piepho wrote:
>
> On Wed, Nov 14, 2012 at 3:02 AM, David Henningsson
> <david.henningsson at canonical.com> wrote:
> > On 11/13/2012 08:41 PM, Trent Piepho wrote:
> >>
> >> On Tue, Nov 13, 2012 at 8:50 AM, Takashi Iwai <tiwai at suse.de> wrote:
> >>>
> >>> At Tue, 13 Nov 2012 13:39:24 +0000,
> >>> Krakora Robert-B42341 wrote:
> >>>>
> >>>> The way that the GStreamer ALSA Sink Plugin is using ALSA Lib assumes
> >>>> that it is thread safe. The fix crafted by one of Trent's colleagues
> >>>> (attached) seems to be the way to go. However, I don't know how it would
> >>>> impact other functionality within ALSA Lib.
> >>>
> >>>
> >>> No, sorry, we don't want to introduce pthread_lock in alsa-lib PCM
> >>> stuff.
> >>>
> >>
> >> The problem with introduction serialization outside alsa-lib is that
> >> you must now serialize entire ALSA calls. The locks must be held for
> >> far too long.
> >>
> >> Consider snd_pcm_writei(), most of the time is usually spent blocked
> >> waiting for a period to elapse. It is perfectly ok to call
> >> snd_pcm_delay() during this time. But if one isn't allowed to make
> >> any other pcm calls during snd_pcm_writei() then this can't be done.
> >>
> >> It's a pretty big problem. Most code using blocking mode is going to
> >> write another period as soon as the writei call returns from the last
> >> write. It will spend almost all its time inside snd_pcm_writei() and
> >> thus will always be holding the app's pcm stream lock. As soon as it
> >> releases the lock it grabs it again for the next write. Another
> >> thread trying to call snd_pcm_delay() will virtually NEVER get a
> >> chance. Not only is it unnecessary stopped from getting the delay
> >> while another thread is blocked inside writei(), but it won't get a
> >> chance to call it between writei() calls either.
> >>
> >> But there doesn't need to be a conflict, since the actual critical
> >> section that needs locking is very small, far smaller than the time
> >> spent sleeping inside writei() or wait(). How can just the critical
> >> section be protected without placing the locking inside alsa-lib?
> >
> > Hmm, but the great majority of programs are not interested in accessing
> > alsa-lib from multiple threads in the way that's currently unsafe. That
> > includes programs who are very dependent on low latency. I would not like to
> > see all these programs to suffer from the overhead of adding locks here and
> > there, even if those locks are small.
>
> Maybe they just don't know it's unsafe and don't hit a race often
> enough to notice?
Maybe, but many multi-threaded codes are carefully holding locks
already in the caller side.
> > You claim that the Gstreamer ALSA sink plugin accesses alsa-lib from two
> > threads simultaneously. Could you elaborate on how this can happen, maybe it
> > is easy to fix?
>
> gstreamer has no lock around the call to snd_pcm_delay(). So it can
> race with snd_pcm_wait() or snd_pcm_writei(), which are called in
> another thread. There is a lock around the block of code calling
> wait()/writei(), but this lock isn't used for calling delay(). It
> seems that they didn't know there would be a problem.
Actually the atomic lock things found in alsa-lib code is no proper
lock for avoiding this kind of race. It's equivalent with seq_lock()
in kernel, and it's used to make snd_pcm_status() consistent. It
doesn't protect against the data corruption due to concurrent
accesses.
A part of concurrent access problems in the rate plugin can be fixed
by a patch like below. In essence, it avoids updating the internal
hw_ptr value but just calculates the current hw_ptr and returns. Most
of other changes in pcm_local.h are only refactoring. But this is
obviously just a bandaid on the spot, it doesn't solve the whole
problems at all. So I don't buy this.
One modest solution would be to add a bit flag SND_PCM_THREAD_SAFE to
snd_pcm_open(), and activate pthread mutex locks conditionally only
with this bit set. This isn't designed for any performance-wise
codes, and it's just for protecting concurrent accesses, so should be
called like SND_PCM_THREAD_SAFE_FOR_DUMMIES...
Takashi
---
diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h
index 8cf7c3d..af4695b 100644
--- a/src/pcm/pcm_local.h
+++ b/src/pcm/pcm_local.h
@@ -420,10 +420,15 @@ static inline int snd_pcm_check_error(snd_pcm_t *pcm, int err)
return err;
}
-static inline snd_pcm_uframes_t snd_pcm_mmap_playback_avail(snd_pcm_t *pcm)
+static inline snd_pcm_uframes_t
+_snd_pcm_calc_avail(snd_pcm_t *pcm, snd_pcm_uframes_t hw_ptr,
+ snd_pcm_uframes_t appl_ptr)
{
snd_pcm_sframes_t avail;
- avail = *pcm->hw.ptr + pcm->buffer_size - *pcm->appl.ptr;
+
+ avail = hw_ptr - appl_ptr;
+ if (pcm->stream == SND_PCM_STREAM_PLAYBACK)
+ avail += pcm->buffer_size;
if (avail < 0)
avail += pcm->boundary;
else if ((snd_pcm_uframes_t) avail >= pcm->boundary)
@@ -431,44 +436,22 @@ static inline snd_pcm_uframes_t snd_pcm_mmap_playback_avail(snd_pcm_t *pcm)
return avail;
}
-static inline snd_pcm_uframes_t snd_pcm_mmap_capture_avail(snd_pcm_t *pcm)
-{
- snd_pcm_sframes_t avail;
- avail = *pcm->hw.ptr - *pcm->appl.ptr;
- if (avail < 0)
- avail += pcm->boundary;
- return avail;
-}
-
static inline snd_pcm_uframes_t snd_pcm_mmap_avail(snd_pcm_t *pcm)
{
- if (pcm->stream == SND_PCM_STREAM_PLAYBACK)
- return snd_pcm_mmap_playback_avail(pcm);
- else
- return snd_pcm_mmap_capture_avail(pcm);
+ return _snd_pcm_calc_avail(pcm, *pcm->hw.ptr, *pcm->appl.ptr);
}
-static inline snd_pcm_sframes_t snd_pcm_mmap_playback_hw_avail(snd_pcm_t *pcm)
-{
- return pcm->buffer_size - snd_pcm_mmap_playback_avail(pcm);
-}
-
-static inline snd_pcm_sframes_t snd_pcm_mmap_capture_hw_avail(snd_pcm_t *pcm)
-{
- return pcm->buffer_size - snd_pcm_mmap_capture_avail(pcm);
-}
+#define snd_pcm_mmap_playback_avail snd_pcm_mmap_avail
+#define snd_pcm_mmap_capture_avail snd_pcm_mmap_avail
static inline snd_pcm_sframes_t snd_pcm_mmap_hw_avail(snd_pcm_t *pcm)
{
- snd_pcm_sframes_t avail;
- avail = *pcm->hw.ptr - *pcm->appl.ptr;
- if (pcm->stream == SND_PCM_STREAM_PLAYBACK)
- avail += pcm->buffer_size;
- if (avail < 0)
- avail += pcm->boundary;
- return pcm->buffer_size - avail;
+ return pcm->buffer_size - snd_pcm_mmap_avail(pcm);
}
+#define snd_pcm_mmap_playback_hw_avail snd_pcm_mmap_hw_avail
+#define snd_pcm_mmap_capture_hw_avail snd_pcm_mmap_hw_avail
+
static inline const snd_pcm_channel_area_t *snd_pcm_mmap_areas(snd_pcm_t *pcm)
{
if (pcm->stopped_areas &&
diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c
index 54a3e67..340a4d3 100644
--- a/src/pcm/pcm_rate.c
+++ b/src/pcm/pcm_rate.c
@@ -613,21 +613,28 @@ static inline snd_pcm_sframes_t snd_pcm_rate_move_applptr(snd_pcm_t *pcm, snd_pc
return diff;
}
-static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm)
+static inline snd_pcm_uframes_t snd_pcm_rate_get_hwptr(snd_pcm_t *pcm)
{
snd_pcm_rate_t *rate = pcm->private_data;
snd_pcm_uframes_t slave_hw_ptr = *rate->gen.slave->hw.ptr;
if (pcm->stream != SND_PCM_STREAM_PLAYBACK)
- return;
+ return rate->hw_ptr;
/* FIXME: boundary overlap of slave hw_ptr isn't evaluated here!
* e.g. if slave rate is small...
*/
- rate->hw_ptr =
- (slave_hw_ptr / rate->gen.slave->period_size) * pcm->period_size +
+ return (slave_hw_ptr / rate->gen.slave->period_size) * pcm->period_size +
rate->ops.input_frames(rate->obj, slave_hw_ptr % rate->gen.slave->period_size);
}
+static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm)
+{
+ snd_pcm_rate_t *rate = pcm->private_data;
+
+ if (pcm->stream == SND_PCM_STREAM_PLAYBACK)
+ rate->hw_ptr = snd_pcm_rate_get_hwptr(pcm);
+}
+
static int snd_pcm_rate_hwsync(snd_pcm_t *pcm)
{
snd_pcm_rate_t *rate = pcm->private_data;
@@ -642,11 +649,11 @@ static int snd_pcm_rate_hwsync(snd_pcm_t *pcm)
static int snd_pcm_rate_delay(snd_pcm_t *pcm, snd_pcm_sframes_t *delayp)
{
- snd_pcm_rate_hwsync(pcm);
- if (pcm->stream == SND_PCM_STREAM_PLAYBACK)
- *delayp = snd_pcm_mmap_playback_hw_avail(pcm);
- else
- *delayp = snd_pcm_mmap_capture_hw_avail(pcm);
+ snd_pcm_rate_t *rate = pcm->private_data;
+
+ snd_pcm_hwsync(rate->gen.slave);
+ *delayp = _snd_pcm_calc_avail(pcm, snd_pcm_rate_get_hwptr(pcm),
+ *pcm->appl.ptr);
return 0;
}
More information about the Alsa-devel
mailing list