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@xxxxxxxx>
> Linux Kernel Sound Maintainer
> ALSA Project; Red Hat, Inc.
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
