[alsa-devel] Deadlock over semaphore issue with aplay while using dmix

[mateen]

Hi,



On Friday, April 5, 2013, Takashi Iwai <tiwai at suse.de> wrote:
> At Fri, 5 Apr 2013 19:02:08 +0530,
> mateen wrote:
>>
>> Hi,
>> I checked with Takshi's patch and would like to suggest couple of changes.
>>
>> 1. We keep dmix_down_sem(dmix) and dmix_up_sem(dmix) as macros instead of
>> defining them as functions.
>
> No, no.  That's a very bad practice.  Don't do it.  If any, use static
> inline.
>

Agreed.

> But in this case, there is no merit to do inline.  Let compiler
> optimize.


>> 2. We free up the semaphore in snd_pcm_dmix_close() if it is recognized
>> that the semaphore is held up by the same process, which will be indicated
>> by dmix->semlocked flag.
>
> Why do you need reacquire the very same lock at all...?
> I see no point for it.
>

Agree,
That lock is still with the same process.we need not reacquire it.


> Takashi
>
>>
>> Mateen.
>>
>> diff -Nuar dir2/pcm_direct.h dir1/pcm_direct.h
>> --- dir2/pcm_direct.h    2009-12-16 20:48:51.000000000 +0530
>> +++ dir1/pcm_direct.h    2013-04-05 17:06:48.331497000 +0530
>> @@ -123,6 +123,7 @@
>>      int ipc_gid;            /* IPC socket gid */
>>      int semid;            /* IPC global semaphore identification */
>>      int shmid;            /* IPC global shared memory identification */
>> +       int semlocked;
>>      snd_pcm_direct_share_t *shmptr;    /* pointer to shared memory area */
>>      snd_pcm_t *spcm;         /* slave PCM handle */
>>      snd_pcm_uframes_t appl_ptr;
>> diff -Nuar dir2/pcm_dmix.c dir1/pcm_dmix.c
>> --- dir2/pcm_dmix.c    2009-12-16 20:48:51.000000000 +0530
>> +++ dir1/pcm_dmix.c    2013-04-05 17:04:02.781109000 +0530
>> @@ -285,8 +285,17 @@
>>   */
>>  #ifndef DOC_HIDDEN
>>  #ifdef NO_CONCURRENT_ACCESS
>> -#define dmix_down_sem(dmix) snd_pcm_direct_semaphore_down(dmix,
>> DIRECT_IPC_SEM_CLIENT)
>> -#define dmix_up_sem(dmix) snd_pcm_direct_semaphore_up(dmix,
>> DIRECT_IPC_SEM_CLIENT)
>> +#define dmix_down_sem(dmix)  \
>> +{\
>> +if (!dmix->semlocked++)\
>> +               snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);\
>> +}
>> +#define dmix_up_sem(dmix) \
>> +{\
>> +       if (!--dmix->semlocked)\
>> +               snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT);\
>> +}
>> +
>>  #else
>>  #define dmix_down_sem(dmix)
>>  #define dmix_up_sem(dmix)
>> @@ -764,7 +773,13 @@
>>
>>      if (dmix->timer)
>>          snd_timer_close(dmix->timer);
>> -    snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
>> +      if(!dmix->semlocked)
>> +            snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
>> +      else{
>> +            snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT);
>> +            snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
>> +        }
>> +
>>      snd_pcm_close(dmix->spcm);
>>       if (dmix->server)
>>           snd_pcm_direct_server_discard(dmix);
>>
>>
>>
>>
>> ------------------------------
>> ----------------------------------------
>>
>> Message: 1
>> Date: Thu, 04 Apr 2013 18:30:01 +0200
>> From: Takashi Iwai <tiwai at suse.de>
>> To: Jaroslav Kysela <perex at perex.cz>
>> Cc: alsa-devel at alsa-project.org
>> Subject: Re: [alsa-devel] Deadlock over semaphore issue with aplay
>>         while   using dmix
>> Message-ID: <s5h38v645ti.wl%tiwai at suse.de>
>> Content-Type: text/plain; charset=US-ASCII
>>
>> At Thu, 04 Apr 2013 13:33:08 +0200,
>> Jaroslav Kysela wrote:
>> >
>> > Date 4.4.2013 11:27, mateen wrote:
>> > > Hi,
>> > >
>> > > I seeing sometimes deadlock issue with dmix when I press CTRL+C.
>> > >
>> > > Aplay's signal handler calls snd_pcm_close() if an interrupt occurs.
>> > > snd_pcm_close() will internally call  pcm->ops->close() which will fall
>> to
>> > > snd_pcm_dmix_close() in case you are using dmix.
>> > >
>> > > snd_pcm_dmix_close() will try to acquire the semaphore with
>> > > snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT).
>> > > The same semaphore is acquired in snd_pcm_dmix_sync_area() with
>> > > dmix_down_sem() in case of non-concurrent access.
>> > >
>> > > If semaphore is acquired in sn> [2  <text/html; ISO-8859-1 (quoted-printable)>]
>>
>