[alsa-devel] Races in alsa-lib with threads

[Trent Piepho]

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?

> 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.

And if you're not using the alsa rate plugin, then I don't think
snd_pcm_delay() has a race with snd_pcm_writei().  delay for a hw
device will call an ioctl to just get the delay.  I think that's ok,
assuming the kernel part has no races.  It's only when using the rate
plugin that snd_pcm_delay() will try to update the pointer and race
with writei.

So unless you use the rate plugin, you'll never notice this race.