Deadlock over semaphore issue with aplay while using dmix

Deadlock over semaphore issue with aplay while using dmix

[mateen]

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.

-- 
Regards,
Shaikh Mateen S.
09423350444

Re: Deadlock over semaphore issue with aplay while using dmix

[Jaroslav Kysela]

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?

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.

Re: Deadlock over semaphore issue with aplay while using dmix

[Takashi Iwai]

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

---
diff --git a/src/pcm/pcm_direct.h b/src/pcm/pcm_direct.h
index 1c35dcb..0ad1475 100644
--- a/src/pcm/pcm_direct.h
+++ b/src/pcm/pcm_direct.h
@@ -123,6 +123,7 @@ struct snd_pcm_direct {
 	int ipc_gid;			/* IPC socket gid */
 	int semid;			/* IPC global semaphore identification */
 	int shmid;			/* IPC global shared memory identification */
+	int locked;
 	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 --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c
index 16dba14..1b9aa6a 100644
--- a/src/pcm/pcm_dmix.c
+++ b/src/pcm/pcm_dmix.c
@@ -293,8 +293,17 @@ static void remix_areas(snd_pcm_direct_t *dmix,
  */
 #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)
+static void dmix_down_sem(snd_pcm_direct_t *dmix)
+{
+	if (!dmix->locked++)
+		snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
+}
+
+static void dmix_up_sem(snd_pcm_direct_t *dmix)
+{
+	if (!--dmix->locked)
+		snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT);
+}
 #else
 #define dmix_down_sem(dmix)
 #define dmix_up_sem(dmix)
@@ -772,7 +781,8 @@ static int snd_pcm_dmix_close(snd_pcm_t *pcm)
 
 	if (dmix->timer)
 		snd_timer_close(dmix->timer);
-	snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
+	if (!dmix->locked)
+		snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
 	snd_pcm_close(dmix->spcm);
  	if (dmix->server)
  		snd_pcm_direct_server_discard(dmix);

Re: Deadlock over semaphore issue with aplay while using dmix

[mateen]

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

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@suse.de>
To: Jaroslav Kysela <perex@perex.cz>
Cc: alsa-devel@alsa-project.org
Subject: Re: [alsa-devel] Deadlock over semaphore issue with aplay
        while   using dmix
Message-ID: <s5h38v645ti.wl%tiwai@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

Re: 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@suse.de>
> To: Jaroslav Kysela <perex@perex.cz>
> Cc: alsa-devel@alsa-project.org
> Subject: Re: [alsa-devel] Deadlock over semaphore issue with aplay
>         while   using dmix
> Message-ID: <s5h38v645ti.wl%tiwai@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)>]
> 

Re: Deadlock over semaphore issue with aplay while using dmix

[Jaroslav Kysela]

Date 4.4.2013 18:30, Takashi Iwai wrote:
> 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.

It does not seem like a clean solution. It's just a workaround. I see
similar locking problems inside the PulseAudio plugin (client library)
when it's closed from the interrupt handler. Also, looking to apps, the
best way is to handle the graceful shutdown outside the interrupt
handler and remove whole "extra" code from the interrupt handler like
the file header fixups (arecord) from it.

The "atomic" notification, that alsa-lib should abort all i/o wait
operations, is a straigh way to do it. Again, syscalls returns EINTR,
so we can detect this case and return to the app immediatelly.

					Jaroslav

> 
> 
> Takashi
> 
> ---
> diff --git a/src/pcm/pcm_direct.h b/src/pcm/pcm_direct.h
> index 1c35dcb..0ad1475 100644
> --- a/src/pcm/pcm_direct.h
> +++ b/src/pcm/pcm_direct.h
> @@ -123,6 +123,7 @@ struct snd_pcm_direct {
>  	int ipc_gid;			/* IPC socket gid */
>  	int semid;			/* IPC global semaphore identification */
>  	int shmid;			/* IPC global shared memory identification */
> +	int locked;
>  	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 --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c
> index 16dba14..1b9aa6a 100644
> --- a/src/pcm/pcm_dmix.c
> +++ b/src/pcm/pcm_dmix.c
> @@ -293,8 +293,17 @@ static void remix_areas(snd_pcm_direct_t *dmix,
>   */
>  #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)
> +static void dmix_down_sem(snd_pcm_direct_t *dmix)
> +{
> +	if (!dmix->locked++)
> +		snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
> +}
> +
> +static void dmix_up_sem(snd_pcm_direct_t *dmix)
> +{
> +	if (!--dmix->locked)
> +		snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT);
> +}
>  #else
>  #define dmix_down_sem(dmix)
>  #define dmix_up_sem(dmix)
> @@ -772,7 +781,8 @@ static int snd_pcm_dmix_close(snd_pcm_t *pcm)
>  
>  	if (dmix->timer)
>  		snd_timer_close(dmix->timer);
> -	snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
> +	if (!dmix->locked)
> +		snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
>  	snd_pcm_close(dmix->spcm);
>   	if (dmix->server)
>   		snd_pcm_direct_server_discard(dmix);
> 


-- 
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.

Re: Deadlock over semaphore issue with aplay while using dmix

[Takashi Iwai]

At Fri, 05 Apr 2013 17:19:52 +0200,
Jaroslav Kysela wrote:
> 
> Date 4.4.2013 18:30, Takashi Iwai wrote:
> > 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.
> 
> It does not seem like a clean solution. It's just a workaround. I see
> similar locking problems inside the PulseAudio plugin (client library)
> when it's closed from the interrupt handler.

OK, that needs a fix.  But, the fix is anyway done to the plugin code
itself, so the necessary change would be pretty similar, I guess.

> Also, looking to apps, the
> best way is to handle the graceful shutdown outside the interrupt
> handler and remove whole "extra" code from the interrupt handler like
> the file header fixups (arecord) from it.

Yeah, agreed.  I would suggest to rewrite the code at first.

But this won't "fix" the existing code doing that.  It's a bad code,
but resulting in a silent deadlock just due to snd_pcm_close() doesn't
sound good, either.

> The "atomic" notification, that alsa-lib should abort all i/o wait
> operations, is a straigh way to do it. Again, syscalls returns EINTR,
> so we can detect this case and return to the app immediatelly.

The abort state handling itself looks useful.


thanks,

Takashi

> 
> 					Jaroslav
> 
> > 
> > 
> > Takashi
> > 
> > ---
> > diff --git a/src/pcm/pcm_direct.h b/src/pcm/pcm_direct.h
> > index 1c35dcb..0ad1475 100644
> > --- a/src/pcm/pcm_direct.h
> > +++ b/src/pcm/pcm_direct.h
> > @@ -123,6 +123,7 @@ struct snd_pcm_direct {
> >  	int ipc_gid;			/* IPC socket gid */
> >  	int semid;			/* IPC global semaphore identification */
> >  	int shmid;			/* IPC global shared memory identification */
> > +	int locked;
> >  	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 --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c
> > index 16dba14..1b9aa6a 100644
> > --- a/src/pcm/pcm_dmix.c
> > +++ b/src/pcm/pcm_dmix.c
> > @@ -293,8 +293,17 @@ static void remix_areas(snd_pcm_direct_t *dmix,
> >   */
> >  #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)
> > +static void dmix_down_sem(snd_pcm_direct_t *dmix)
> > +{
> > +	if (!dmix->locked++)
> > +		snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
> > +}
> > +
> > +static void dmix_up_sem(snd_pcm_direct_t *dmix)
> > +{
> > +	if (!--dmix->locked)
> > +		snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT);
> > +}
> >  #else
> >  #define dmix_down_sem(dmix)
> >  #define dmix_up_sem(dmix)
> > @@ -772,7 +781,8 @@ static int snd_pcm_dmix_close(snd_pcm_t *pcm)
> >  
> >  	if (dmix->timer)
> >  		snd_timer_close(dmix->timer);
> > -	snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
> > +	if (!dmix->locked)
> > +		snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
> >  	snd_pcm_close(dmix->spcm);
> >   	if (dmix->server)
> >   		snd_pcm_direct_server_discard(dmix);
> > 
> 
> 
> -- 
> Jaroslav Kysela <perex@perex.cz>
> Linux Kernel Sound Maintainer
> ALSA Project; Red Hat, Inc.
> 

Re: Deadlock over semaphore issue with aplay while using dmix

[Jaroslav Kysela]

Date 5.4.2013 17:27, Takashi Iwai wrote:
> At Fri, 05 Apr 2013 17:19:52 +0200,
> Jaroslav Kysela wrote:
>>
>> Date 4.4.2013 18:30, Takashi Iwai wrote:
>>> 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.
>>
>> It does not seem like a clean solution. It's just a workaround. I see
>> similar locking problems inside the PulseAudio plugin (client library)
>> when it's closed from the interrupt handler.
> 
> OK, that needs a fix.  But, the fix is anyway done to the plugin code
> itself, so the necessary change would be pretty similar, I guess.
> 
>> Also, looking to apps, the
>> best way is to handle the graceful shutdown outside the interrupt
>> handler and remove whole "extra" code from the interrupt handler like
>> the file header fixups (arecord) from it.
> 
> Yeah, agreed.  I would suggest to rewrite the code at first.
> 
> But this won't "fix" the existing code doing that.  It's a bad code,
> but resulting in a silent deadlock just due to snd_pcm_close() doesn't
> sound good, either.

I would just call abort() in this dead-lock case like the PA library
code does. In this way, the problem will be reported. It's better than a
silent acceptance.

>> The "atomic" notification, that alsa-lib should abort all i/o wait
>> operations, is a straigh way to do it. Again, syscalls returns EINTR,
>> so we can detect this case and return to the app immediatelly.
> 
> The abort state handling itself looks useful.

Good.

				Jaroslav

> 
> 
> thanks,
> 
> Takashi
> 
>>
>> 					Jaroslav
>>
>>>
>>>
>>> Takashi
>>>
>>> ---
>>> diff --git a/src/pcm/pcm_direct.h b/src/pcm/pcm_direct.h
>>> index 1c35dcb..0ad1475 100644
>>> --- a/src/pcm/pcm_direct.h
>>> +++ b/src/pcm/pcm_direct.h
>>> @@ -123,6 +123,7 @@ struct snd_pcm_direct {
>>>  	int ipc_gid;			/* IPC socket gid */
>>>  	int semid;			/* IPC global semaphore identification */
>>>  	int shmid;			/* IPC global shared memory identification */
>>> +	int locked;
>>>  	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 --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c
>>> index 16dba14..1b9aa6a 100644
>>> --- a/src/pcm/pcm_dmix.c
>>> +++ b/src/pcm/pcm_dmix.c
>>> @@ -293,8 +293,17 @@ static void remix_areas(snd_pcm_direct_t *dmix,
>>>   */
>>>  #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)
>>> +static void dmix_down_sem(snd_pcm_direct_t *dmix)
>>> +{
>>> +	if (!dmix->locked++)
>>> +		snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
>>> +}
>>> +
>>> +static void dmix_up_sem(snd_pcm_direct_t *dmix)
>>> +{
>>> +	if (!--dmix->locked)
>>> +		snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT);
>>> +}
>>>  #else
>>>  #define dmix_down_sem(dmix)
>>>  #define dmix_up_sem(dmix)
>>> @@ -772,7 +781,8 @@ static int snd_pcm_dmix_close(snd_pcm_t *pcm)
>>>  
>>>  	if (dmix->timer)
>>>  		snd_timer_close(dmix->timer);
>>> -	snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
>>> +	if (!dmix->locked)
>>> +		snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
>>>  	snd_pcm_close(dmix->spcm);
>>>   	if (dmix->server)
>>>   		snd_pcm_dir

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.

Re: Deadlock over semaphore issue with aplay while using dmix

[Takashi Iwai]

At Fri, 05 Apr 2013 17:43:41 +0200,
Jaroslav Kysela wrote:
> 
> Date 5.4.2013 17:27, Takashi Iwai wrote:
> > At Fri, 05 Apr 2013 17:19:52 +0200,
> > Jaroslav Kysela wrote:
> >>
> >> Date 4.4.2013 18:30, Takashi Iwai wrote:
> >>> 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.
> >>
> >> It does not seem like a clean solution. It's just a workaround. I see
> >> similar locking problems inside the PulseAudio plugin (client library)
> >> when it's closed from the interrupt handler.
> > 
> > OK, that needs a fix.  But, the fix is anyway done to the plugin code
> > itself, so the necessary change would be pretty similar, I guess.
> > 
> >> Also, looking to apps, the
> >> best way is to handle the graceful shutdown outside the interrupt
> >> handler and remove whole "extra" code from the interrupt handler like
> >> the file header fixups (arecord) from it.
> > 
> > Yeah, agreed.  I would suggest to rewrite the code at first.
> > 
> > But this won't "fix" the existing code doing that.  It's a bad code,
> > but resulting in a silent deadlock just due to snd_pcm_close() doesn't
> > sound good, either.
> 
> I would just call abort() in this dead-lock case like the PA library
> code does. In this way, the problem will be reported. It's better than a
> silent acceptance.

Hm, maybe it's more educational, too.

> >> The "atomic" notification, that alsa-lib should abort all i/o wait
> >> operations, is a straigh way to do it. Again, syscalls returns EINTR,
> >> so we can detect this case and return to the app immediatelly.
> > 
> > The abort state handling itself looks useful.
> 
> Good.


Takashi

Re: Deadlock over semaphore issue with aplay while using dmix

[mateen]

Hi,



On Friday, April 5, 2013, Takashi Iwai <tiwai@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@suse.de>
>> To: Jaroslav Kysela <perex@perex.cz>
>> Cc: alsa-devel@alsa-project.org
>> Subject: Re: [alsa-devel] Deadlock over semaphore issue with aplay
>>         while   using dmix
>> Message-ID: <s5h38v645ti.wl%tiwai@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)>]
>>
>