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@xxxxxxx>
> To: Jaroslav Kysela <perex@xxxxxxxx>
> Cc: alsa-devel@xxxxxxxxxxxxxxxx
> Subject: Re: Deadlock over semaphore issue with aplay
> while using dmix
> Message-ID: <s5h38v645ti.wl%tiwai@xxxxxxx>
> 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)>]
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
