Re: [PATCH] ALSA: pcm: Fix starvation on down_write_nonblock()

[Takashi Iwai]

On Wed, 28 Nov 2018 00:47:03 +0100,
Chanho Min wrote:
> 
> > On Mon, 26 Nov 2018 06:36:37 +0100,
> > Chanho Min wrote:
> > >
> > > Commit 67ec1072b053 ("ALSA: pcm: Fix rwsem deadlock for non-atomic PCM
> > > stream") fixes deadlock for non-atomic PCM stream. But, This patch
> > causes antother stuck.
> > > If writer is RT thread and reader is a normal thread, the reader
> > > thread will be difficult to get scheduled. It may not give chance to
> > > release readlocks and writer gets stuck for a long time if they are
> > pinned to single cpu.
> > >
> > > The deadlock described in the previous commit is because the linux
> > > rwsem queues like a FIFO. So, we might need non-FIFO writelock, not non-
> > block one.
> > >
> > > My suggestion is that the writer gives reader a chance to be scheduled
> > > by using the minimum msleep() instaed of spinning without blocking by
> > > writer. Also, The *_nonblock may be changed to *_nonfifo appropriately
> > to this concept.
> > > In terms of performance, when trylock is failed, this minimum periodic
> > > msleep will have the same performance as the tick-based
> > schedule()/wake_up_q().
> > >
> > > Suggested-by: Wonmin Jung <wonmin.jung@xxxxxxx>
> > > Signed-off-by: Chanho Min <chanho.min@xxxxxxx>
> > 
> > Hrm, converting unconditionally with msleep() looks too drastic.
> 
> Yes, it looks drastic. But, IMHO, I can't say busy-spin is not non-drastic.
> To fix the root cause, We may need another rwsem that does not work as a
> FIFO.

Right, but applying msleep(1) unconditionally is really bad.

> > I guess you've hit this while not explicitly using the linked PCM streams,
> > i.e. in the call of snd_pcm_unlink() at close, right?
> > 
> > Then this can be worked around by checking the link before calling it.
> > Could you check the patch below?
> 
> More testing is needed, but it seems to be fixed by your patch.
> We may not use the linked PCM.

Then I'm sure that my patch papers over.

> But, If the linked PCM is enabled,  Can snd_pcm_unlink() be called?
> This also seems to be a workaround.

Yes, for the linked streams, something else is needed *in addition*.

The original fix with busy loop also assumed that this code path (via
snd_pcm_link() and snd_pcm_unlink()) is the rare occasion, and it
didn't consider that it were called for regular use cases.  So the fix
to make things just works for regular use cases without any artifact
must be implemented in the first place.  The fix for the linked
streams comes at next.  It might be like your msleep() change as a
workaround, but in anyway it's far less urgency.


Takashi