FW: Further goals of thread-safe PCM API

FW: Further goals of thread-safe PCM API

[Wischer, Timo (ADITG/SW1)]

Hi everyone,

I am wondering about the implementation of the new thread-safety feature [1].
There are so many issues with deadlocks (e.g. [3]) which were already solved but also which are not yet solved.

Why do you not using a recursive mutex to avoid most of this deadlocks?
Using PTHREAD_MUTEX_RECURSIVE as the pthread attribute [2].

What is the preferred solution for external ALSA plugins which are using the external IO plugin SDK to prevent such deadlocks?
For example I have one plugin which calls snd_pcm_avail() in the ext IO plug start callback.
This ends up in a deadlock.

#0  0x0000003f44e0feec in __lll_lock_wait () from /lib64/libpthread.so.0
#1  0x0000003f44e09b15 in pthread_mutex_lock () from /lib64/libpthread.so.0
#2  0x00007ffff7d3c8b4 in snd_pcm_lock (pcm=0x623150) at ../../../alsa-lib-1.1.2/src/pcm/pcm_local.h:1101
#3  snd_pcm_avail (pcm=0x623150) at ../../../alsa-lib-1.1.2/src/pcm/pcm.c:2762
#4  0x00007ffff7ae1e03 in ?? () from /usr/lib64/alsa-lib/libasound_module_pcm_loop.so
#5  0x00007ffff7d7df85 in snd_pcm_ioplug_start (pcm=0x623150) at ../../../alsa-lib-1.1.2/src/pcm/pcm_ioplug.c:458
#6  0x00007ffff7d41065 in __snd_pcm_start (pcm=0x623150, pcm=0x623150) at ../../../alsa-lib-1.1.2/src/pcm/pcm_local.h:427
#7  snd1_pcm_write_areas (pcm=pcm@entry=0x623150, areas=areas@entry=0x7fffffffe750, offset=offset@entry=0, size=size@entry=64, func=func@entry=0x7ffff7d7de00 <ioplug_priv_transfer_areas>)
    at ../../../alsa-lib-1.1.2/src/pcm/pcm.c:7248
#8  0x00007ffff7d7eb65 in snd_pcm_ioplug_writei (pcm=0x623150, buffer=<optimized out>, size=64) at ../../../alsa-lib-1.1.2/src/pcm/pcm_ioplug.c:579
#9  0x000000000040601a in pcm_write (
    data=0x623350 "\006\003\271\375N\004\063\376j\002n\374)\005\331\376I\005Z\376w\001\n\374\025\003\373\376{\003i\377T\001~\375}\375\244\372\267\002c\376%\004\352\377\312\b[\005\261\001\216\376\b\001,\377\352",
    count=count@entry=64) at ../../alsa-utils-1.1.2/aplay/aplay.c:2001
#10 0x00000000004062cb in playback_go (fd=6, loaded=<optimized out>, loaded@entry=0, count=147256, rtype=rtype@entry=2, name=name@entry=0x7fffffffed9a "/opt/platform/unit_tests/audio8k16S.wav")
    at ../../alsa-utils-1.1.2/aplay/aplay.c:2750
#11 0x0000000000407417 in playback (name=0x7fffffffed9a "/opt/platform/unit_tests/audio8k16S.wav") at ../../alsa-utils-1.1.2/aplay/aplay.c:2811
#12 0x0000000000409778 in main (argc=8, argv=0x7fffffffeb18) at ../../alsa-utils-1.1.2/aplay/aplay.c:859


Therefore do I have to replace all snd_pcm_* function calls in the external plugin with the non-thread-safe variants __snd_pcm_*?
Or is there a better solution available?

>From my point of view the usage of a recursive mutex would be the best and most stable solution.


[1] http://git.alsa-project.org/?p=alsa-lib.git;a=commit;h=54931e5a5455ac681a32a673d4b360d43f34b6b5
[2] https://linux.die.net/man/3/pthread_mutexattr_settype
[3] http://git.alsa-project.org/?p=alsa-lib.git;a=commitdiff;h=24e63b75275e9c923c336b8dba3919b980e8f234
[4] http://www.alsa-project.org/alsa-doc/alsa-lib/group___p_c_m___i_o_plug.html#ga7fb5213a5e776246e2b4dc53ec8d7604


Best regards

Timo Wischer

Advanced Driver Information Technology GmbH Software Group I (ADITG/SW1) Robert-Bosch-Str. 200
31139 Hildesheim
Germany

twischer@de.adit-jv.com

ADIT is a joint venture company of Robert Bosch GmbH/Robert Bosch Car Multimedia GmbH and DENSO Corporation
Sitz: Hildesheim, Registergericht: Amtsgericht Hildesheim HRB 3438
Geschaeftsfuehrung: Wilhelm Grabow, Ken Yaguchi

Re: FW: Further goals of thread-safe PCM API

[Takashi Iwai]

On Thu, 20 Apr 2017 16:01:47 +0200,
Wischer, Timo (ADITG/SW1) wrote:
> 
> Hi everyone,
> 
> I am wondering about the implementation of the new thread-safety feature [1].
> There are so many issues with deadlocks (e.g. [3]) which were already solved but also which are not yet solved.
> 
> Why do you not using a recursive mutex to avoid most of this deadlocks?
> Using PTHREAD_MUTEX_RECURSIVE as the pthread attribute [2].

It sounds like a good idea.
Although the plugin should be written not to cause deadlock, it's
better to avoid such a pain by allowing the recursive lock.

Care to test and submit the proper patch?


thanks,

Takashi

Re: FW: Further goals of thread-safe PCM API

[Takashi Iwai]

On Thu, 20 Apr 2017 17:42:06 +0200,
Takashi Iwai wrote:
> 
> On Thu, 20 Apr 2017 16:01:47 +0200,
> Wischer, Timo (ADITG/SW1) wrote:
> > 
> > Hi everyone,
> > 
> > I am wondering about the implementation of the new thread-safety feature [1].
> > There are so many issues with deadlocks (e.g. [3]) which were already solved but also which are not yet solved.
> > 
> > Why do you not using a recursive mutex to avoid most of this deadlocks?
> > Using PTHREAD_MUTEX_RECURSIVE as the pthread attribute [2].
> 
> It sounds like a good idea.
> Although the plugin should be written not to cause deadlock, it's
> better to avoid such a pain by allowing the recursive lock.
> 
> Care to test and submit the proper patch?

Never mind, I committed a quick fix to git repo now.
Thanks for the suggestion!


Takashi

Re: FW: Further goals of thread-safe PCM API

[Wischer, Timo (ADITG/SW1)]

Hi Takashi,

All our test cases are working fine again with your commit [1].
Thanks a lot.

I think all commits which introducing unlocked versions of the API functions can be reverted e.g [2].
Or what do you think? 


[1] http://git.alsa-project.org/?p=alsa-lib.git;a=commit;h=1cb217ead9aff029f194208bf484be1ba956b194
[2] http://git.alsa-project.org/?p=alsa-lib.git;a=commitdiff;h=24e63b75275e9c923c336b8dba3919b980e8f234

Best regards

Timo Wischer
Software Group I (ADITG/SW1)

Tel. +49 5121 49 6938

-----Original Message-----
From: Takashi Iwai [mailto:tiwai@suse.de] 
Sent: Freitag, 21. April 2017 20:44
To: Wischer, Timo (ADITG/SW1)
Cc: alsa-devel@alsa-project.org
Subject: Re: [alsa-devel] FW: Further goals of thread-safe PCM API

On Thu, 20 Apr 2017 17:42:06 +0200,
Takashi Iwai wrote:
> 
> On Thu, 20 Apr 2017 16:01:47 +0200,
> Wischer, Timo (ADITG/SW1) wrote:
> > 
> > Hi everyone,
> > 
> > I am wondering about the implementation of the new thread-safety feature [1].
> > There are so many issues with deadlocks (e.g. [3]) which were already solved but also which are not yet solved.
> > 
> > Why do you not using a recursive mutex to avoid most of this deadlocks?
> > Using PTHREAD_MUTEX_RECURSIVE as the pthread attribute [2].
> 
> It sounds like a good idea.
> Although the plugin should be written not to cause deadlock, it's 
> better to avoid such a pain by allowing the recursive lock.
> 
> Care to test and submit the proper patch?

Never mind, I committed a quick fix to git repo now.
Thanks for the suggestion!


Takashi

Re: FW: Further goals of thread-safe PCM API

[Takashi Iwai]

On Thu, 27 Apr 2017 10:45:55 +0200,
Wischer, Timo (ADITG/SW1) wrote:
> 
> Hi Takashi,
> 
> All our test cases are working fine again with your commit [1].
> Thanks a lot.

Thanks for checking.

> I think all commits which introducing unlocked versions of the API functions can be reverted e.g [2].
> Or what do you think? 

Using the unlocked version is correct per se, so I don't think we need
to remove it.


thanks,

Takashi

> 
> [1] http://git.alsa-project.org/?p=alsa-lib.git;a=commit;h=1cb217ead9aff029f194208bf484be1ba956b194
> [2] http://git.alsa-project.org/?p=alsa-lib.git;a=commitdiff;h=24e63b75275e9c923c336b8dba3919b980e8f234
> 
> Best regards
> 
> Timo Wischer
> Software Group I (ADITG/SW1)
> 
> Tel. +49 5121 49 6938
> 
> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de] 
> Sent: Freitag, 21. April 2017 20:44
> To: Wischer, Timo (ADITG/SW1)
> Cc: alsa-devel@alsa-project.org
> Subject: Re: [alsa-devel] FW: Further goals of thread-safe PCM API
> 
> On Thu, 20 Apr 2017 17:42:06 +0200,
> Takashi Iwai wrote:
> > 
> > On Thu, 20 Apr 2017 16:01:47 +0200,
> > Wischer, Timo (ADITG/SW1) wrote:
> > > 
> > > Hi everyone,
> > > 
> > > I am wondering about the implementation of the new thread-safety feature [1].
> > > There are so many issues with deadlocks (e.g. [3]) which were already solved but also which are not yet solved.
> > > 
> > > Why do you not using a recursive mutex to avoid most of this deadlocks?
> > > Using PTHREAD_MUTEX_RECURSIVE as the pthread attribute [2].
> > 
> > It sounds like a good idea.
> > Although the plugin should be written not to cause deadlock, it's 
> > better to avoid such a pain by allowing the recursive lock.
> > 
> > Care to test and submit the proper patch?
> 
> Never mind, I committed a quick fix to git repo now.
> Thanks for the suggestion!
> 
> 
> Takashi
>