Re: [alsa-devel] Deadlock over semaphore issue with aplay while using dmix
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
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.
- 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.
- 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;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)>]
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.
- 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.
- 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)>]
participants (2)
-
mateen -
Takashi Iwai