[alsa-devel] 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);