| Message ID | 1543210597-6717-1-git-send-email-chanho.min@lge.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: pcm: Fix starvation on down_write_nonblock() | expand |
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@lge.com> > Signed-off-by: Chanho Min <chanho.min@lge.com> Hrm, converting unconditionally with msleep() looks too drastic. 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? thanks, Takashi --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2369,7 +2369,8 @@ int snd_pcm_hw_constraints_complete(struct snd_pcm_substream *substream) static void pcm_release_private(struct snd_pcm_substream *substream) { - snd_pcm_unlink(substream); + if (snd_pcm_stream_linked(substream)) + snd_pcm_unlink(substream); } void snd_pcm_release_substream(struct snd_pcm_substream *substream)
> 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@lge.com> > > Signed-off-by: Chanho Min <chanho.min@lge.com> > > 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. > > 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. But, If the linked PCM is enabled, Can snd_pcm_unlink() be called? This also seems to be a workaround. > > > thanks, > > Takashi > > --- a/sound/core/pcm_native.c > +++ b/sound/core/pcm_native.c > @@ -2369,7 +2369,8 @@ int snd_pcm_hw_constraints_complete(struct > snd_pcm_substream *substream) > > static void pcm_release_private(struct snd_pcm_substream *substream) { > - snd_pcm_unlink(substream); > + if (snd_pcm_stream_linked(substream)) > + snd_pcm_unlink(substream); > } > > void snd_pcm_release_substream(struct snd_pcm_substream *substream)
> > > > Hrm, converting unconditionally with msleep() looks too drastic. > > Yes, it looks drastic. But, IMHO, I can't say busy-spin is not non-drastic. Fix typo in this comment: I can't say busy-spin is not drastic. Thanks Chanho
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@lge.com> > > > Signed-off-by: Chanho Min <chanho.min@lge.com> > > > > 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
> > > 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@lge.com> > > > > Signed-off-by: Chanho Min <chanho.min@lge.com> > > > > > > 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. Thanks, Plz let me know when the patch is merged. > > > 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. msleep is worst, but If it is harmless, can I apply my patch to the private tree temporarily until your next fix comes? We may use the linked streams in the near future. It makes our product unstable. It's urgency for us. How is your opinion? Thanks Chanho
On Thu, 29 Nov 2018 00:48:20 +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@lge.com> > > > > > Signed-off-by: Chanho Min <chanho.min@lge.com> > > > > > > > > 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. > Thanks, Plz let me know when the patch is merged. I'm going to merge the patch below now. It slips from the pull request to Linus in today, but will be the next one for 4.20-rc6. > > > 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. > > msleep is worst, but If it is harmless, can I apply my patch to the private > tree > temporarily until your next fix comes? > We may use the linked streams in the near future. It makes our product > unstable. > It's urgency for us. How is your opinion? I'll add your fix on top of mine for now. The msleep() is applied only for linked streams, so it's not that bad any longer. thanks, Takashi -- 8< -- From: Takashi Iwai <tiwai@suse.de> Subject: [PATCH] ALSA: pcm: Call snd_pcm_unlink() conditionally at closing Currently the PCM core calls snd_pcm_unlink() always unconditionally at closing a stream. However, since snd_pcm_unlink() invokes the global rwsem down, the lock can be easily contended. More badly, when a thread runs in a high priority RT-FIFO, it may stall at spinning. Basically the call of snd_pcm_unlink() is required only for the linked streams that are already rare occasion. For normal use cases, this code path is fairly superfluous. As an optimization (and also as a workaround for the RT problem above in normal situations without linked streams), this patch adds a check before calling snd_pcm_unlink() and calls it only when needed. Reported-by: Chanho Min <chanho.min@lge.com> Cc: <stable@vger.kernel.org> Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/core/pcm_native.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 66c90f486af9..6afcc393113a 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2369,7 +2369,8 @@ int snd_pcm_hw_constraints_complete(struct snd_pcm_substream *substream) static void pcm_release_private(struct snd_pcm_substream *substream) { - snd_pcm_unlink(substream); + if (snd_pcm_stream_linked(substream)) + snd_pcm_unlink(substream); } void snd_pcm_release_substream(struct snd_pcm_substream *substream)
> 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@lge.com> > > > > > > Signed-off-by: Chanho Min <chanho.min@lge.com> > > > > > > > > > > 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. > > Thanks, Plz let me know when the patch is merged. > > I'm going to merge the patch below now. > It slips from the pull request to Linus in today, but will be the next > one for 4.20-rc6. > > > > > 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. > > > > msleep is worst, but If it is harmless, can I apply my patch to the > private > > tree > > temporarily until your next fix comes? > > We may use the linked streams in the near future. It makes our product > > unstable. > > It's urgency for us. How is your opinion? > > I'll add your fix on top of mine for now. The msleep() is applied > only for linked streams, so it's not that bad any longer. > > > thanks, > > Takashi > > -- 8< -- > From: Takashi Iwai <tiwai@suse.de> > Subject: [PATCH] ALSA: pcm: Call snd_pcm_unlink() conditionally at closing > > Currently the PCM core calls snd_pcm_unlink() always unconditionally > at closing a stream. However, since snd_pcm_unlink() invokes the > global rwsem down, the lock can be easily contended. More badly, when > a thread runs in a high priority RT-FIFO, it may stall at spinning. > > Basically the call of snd_pcm_unlink() is required only for the linked > streams that are already rare occasion. For normal use cases, this > code path is fairly superfluous. > > As an optimization (and also as a workaround for the RT problem > above in normal situations without linked streams), this patch adds a > check before calling snd_pcm_unlink() and calls it only when needed. > > Reported-by: Chanho Min <chanho.min@lge.com> > Cc: <stable@vger.kernel.org> > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > sound/core/pcm_native.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > index 66c90f486af9..6afcc393113a 100644 > --- a/sound/core/pcm_native.c > +++ b/sound/core/pcm_native.c > @@ -2369,7 +2369,8 @@ int snd_pcm_hw_constraints_complete(struct > snd_pcm_substream *substream) > > static void pcm_release_private(struct snd_pcm_substream *substream) > { > - snd_pcm_unlink(substream); > + if (snd_pcm_stream_linked(substream)) > + snd_pcm_unlink(substream); > } > > void snd_pcm_release_substream(struct snd_pcm_substream *substream) > -- > 2.19.1 Great, Many thanks for the fast response. Chanho
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 66c90f4..bdca0e1 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -36,6 +36,7 @@ #include <sound/timer.h> #include <sound/minors.h> #include <linux/uio.h> +#include <linux/delay.h> #include "pcm_local.h" @@ -91,12 +92,12 @@ static DECLARE_RWSEM(snd_pcm_link_rwsem); * and this may lead to a deadlock when the code path takes read sem * twice (e.g. one in snd_pcm_action_nonatomic() and another in * snd_pcm_stream_lock()). As a (suboptimal) workaround, let writer to - * spin until it gets the lock. + * sleep until all the readers are completed without blocking by writer. */ -static inline void down_write_nonblock(struct rw_semaphore *lock) +static inline void down_write_nonfifo(struct rw_semaphore *lock) { while (!down_write_trylock(lock)) - cond_resched(); + msleep(1); } #define PCM_LOCK_DEFAULT 0 @@ -1967,7 +1968,7 @@ static int snd_pcm_link(struct snd_pcm_substream *substream, int fd) res = -ENOMEM; goto _nolock; } - down_write_nonblock(&snd_pcm_link_rwsem); + down_write_nonfifo(&snd_pcm_link_rwsem); write_lock_irq(&snd_pcm_link_rwlock); if (substream->runtime->status->state == SNDRV_PCM_STATE_OPEN || substream->runtime->status->state != substream1->runtime->status->state || @@ -2014,7 +2015,7 @@ static int snd_pcm_unlink(struct snd_pcm_substream *substream) struct snd_pcm_substream *s; int res = 0; - down_write_nonblock(&snd_pcm_link_rwsem); + down_write_nonfifo(&snd_pcm_link_rwsem); write_lock_irq(&snd_pcm_link_rwlock); if (!snd_pcm_stream_linked(substream)) { res = -EALREADY;
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@lge.com> Signed-off-by: Chanho Min <chanho.min@lge.com> --- sound/core/pcm_native.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)