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

[Takashi Iwai]

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.

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.


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 snd_pcm_dmix_sync_area() which is in thread
> > > context and interrupt comes, which invokes the signal handler which is
> in
> > > ISR context, which calls snd_pcm_close() which in turn calls
> > > snd_pcm_dmix_close() then we see a deadlock since semaphore is not
> released
> > > from thread context and ISR is waiting indefinitely on the same
> semaphore.
> > >
> > > Please suggest a suitable solution for this.
> >
> > It seems that also other configurations (alsa-lib plugins) have trouble
> > with the closing from the signal handler - I hit mutex issues with the
> > PulseAudio plugin, too.
> >
> > The question is, how we can do a clean path in this case. Looking to the
> > current alsa-lib code, I would suggest to add the snd_pcm_abort()
> > function (may be called from the interrupt handler) to notify the
> > library to not ignore -EINTR return codes from poll() and other i/o ops
> > and pass it to the caller (application) to finish the normal close
> sequence.
> >
> > Opinions?
> 
> Isn't it only about the direct plugins with the case where no coherent
> memory is available?  If so, we may just avoid the deadlock by
> checking some internal flag like the patch below (untested)?
> It's no perfect but just some proof, of course.
> 
> 
> Takashi
> 
> 
> -- 
> Regards,
> Shaikh Mateen S.
> 09423350444
> [2  <text/html; ISO-8859-1 (quoted-printable)>]
>