Re: locking looks odd

[Takashi Iwai <tiwai@suse.de>]

On Thu, 01 Sep 2016 08:27:07 +0200,
David Henningsson wrote:
> 
> (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.

Setting the env variable to 0 doesn't mean to give you the maximum 
performance.  The hw plugin is always without locking, and it's only
with other plugins.  If you need the "maximum" performance, you need
to use only hw.  Using other plugins already contradicts your purpose
from the very beginning.

That said, the current implementation shouldn't matter for JACK or
such systems where they expect more or less the direct access to hw
plugin.

Still, for buggy pthread implementation in applications, the locking
inside alsa-lib may cause an issue.  The env variable is a help to
identify that.  It's not provided as a solution.


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

Well, both snd_pcm_lock_all() and snd_pcm_lock_ts0() would be
confusing, too, since these don't match with the behavior as well.

__snd_pcm_lock() -> lock a plugin when it's not disabled by env
                    variable (no matter whether it declares itself as
                    safe or unsafe).
snd_pcm_lock()   -> lock a plugin when it's unsafe and not disabled by
                    env variable

I'm open for any better names.  Patches are welcome.


thanks,

Takashi