[alsa-devel] locking looks odd

[David Henningsson]

(Sorry if this email reaches you twice, had some issues)


On 2016-08-17 19:46, Jaroslav Kysela wrote:
> Dne 16.8.2016 v 23:03 Samuel Thibault napsal(a):
>> Hello,
>>
>> We are having odd issues with libasound 1.1.2 which we didn't have with
>> libasound 1.1.1, more precisely
>>
>> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=833950
>>
>> so I'm having a look at the locking API introduced in 1.1.2, and there
>> are some oddities:
>>
>> - snd_pcm_new seems to initialize pcm->thread_safe to 0 by default, this
>>    does not seem safe. The attached patch initializes it to 1, which
>>    fixes the bug in our tests.
>>
>> - snd_pcm_hw_open_fd forces it to 1, thus ignoring what snd_pcm_new set.
> The thread_safe has this meaning:
>
> 0  - the pcm plugin is not thread safe
> 1  - the pcm plugin is thread safe (actually only the hw plugin)
> -1 - disable thread safety
>
> Your patch does not look correct. It's necessary to determine where the
> mutex is locked the second time (use gdb and backtrace for all threads
> for that). Note that plugins may be chained.

Still Samuel's point about -1 being ignored is valid, so I just sent a 
patch about that.

But I'm quite sceptic about having that environment variable in the 
first place - it seems to me that new apps will start to rely on 
alsa-lib doing the locking for them, second a blog post comes along that 
tells people to set that environment variable to 0 to maximize 
performance, third those apps will crash and the user doesn't understand 
why.

>
>> - one can find both __snd_pcm_lock and snd_pcm_lock functions, what is
>>    the expected difference between them?
> __snd_pcm_lock/unlock is for forced lock
>
> snd_pcm_lock/unlock skips locking for safe plugins (only hw plugin)



These are quite confusing names, one would expect them to be the same 
(snd_pcm_lock being a compatibility wrapper around some internal 
__snd_pcm_lock).

I'm not sure about better names, but maybe something like 
snd_pcm_lock_all and snd_pcm_lock_ts0 would at least indicate that they 
are different functions.