From: Takashi Iwai <tiwai@suse.de> To: "Anna-Maria Gleixner" <anna-maria@linutronix.de> Cc: "LKML" <linux-kernel@vger.kernel.org>, <alsa-devel@alsa-project.org>, <keescook@chromium.org>, "Peter Zijlstra" <peterz@infradead.org>, "John Stultz" <john.stultz@linaro.org>, "Thomas Gleixner" <tglx@linutronix.de>, "Christoph Hellwig" <hch@lst.org>, "Jaroslav Kysela" <perex@perex.cz>, "Ingo Molnar" <mingo@redhat.com>, "Takashi Sakamoto" <o-takashi@sakamocchi.jp> Subject: Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer Date: Thu, 31 Aug 2017 17:36:37 +0200 [thread overview] Message-ID: <s5hh8wnvjei.wl-tiwai@suse.de> (raw) In-Reply-To: <20170831105827.401092373@linutronix.de> On Thu, 31 Aug 2017 14:23:45 +0200, Anna-Maria Gleixner wrote: > > From: Thomas Gleixner <tglx@linutronix.de> > > The tasklet is used to defer the execution of snd_pcm_period_elapsed() to > the softirq context. Using the CLOCK_MONOTONIC_SOFT base invokes the timer > callback in softirq context as well which renders the tasklet useless. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de> > Cc: Jaroslav Kysela <perex@perex.cz> > Cc: Takashi Iwai <tiwai@suse.com> > Cc: Takashi Sakamoto <o-takashi@sakamocchi.jp> > Cc: alsa-devel@alsa-project.org I gave it at try, but it caused a kernel hang, unfortunately. The reason is that snd_pcm_period_elapased() may stop the stream (e.g. when reaching at the end). With this patchset, it'll lead to the call of hrtimer_cancel() from the hrtimer callback itself, thus it stalls. Below is the additional fix over your patch for working around it. I believe it should cover most corner cases, and seems working fine through quick tests, so far. thanks, Takashi --- diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c index 273d60c42125..b5dd64e3dab1 100644 --- a/sound/drivers/dummy.c +++ b/sound/drivers/dummy.c @@ -375,6 +375,7 @@ struct dummy_hrtimer_pcm { ktime_t base_time; ktime_t period_time; atomic_t running; + atomic_t callback_running; struct hrtimer timer; struct snd_pcm_substream *substream; }; @@ -387,8 +388,15 @@ static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer) if (!atomic_read(&dpcm->running)) return HRTIMER_NORESTART; + atomic_inc(&dpcm->callback_running); snd_pcm_period_elapsed(dpcm->substream); + atomic_dec(&dpcm->callback_running); + /* may be flipped during snd_pcm_period_elapsed() */ + if (!atomic_read(&dpcm->running)) + return HRTIMER_NORESTART; + hrtimer_forward_now(timer, dpcm->period_time); + atomic_dec(&dpcm->callback_running); return HRTIMER_RESTART; } @@ -407,7 +415,9 @@ static int dummy_hrtimer_stop(struct snd_pcm_substream *substream) struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data; atomic_set(&dpcm->running, 0); - hrtimer_cancel(&dpcm->timer); + /* issue hrtimer_cancel() only when called outside the callback */ + if (!atomic_read(&dpcm->callback_running)) + hrtimer_cancel(&dpcm->timer); return 0; } @@ -462,6 +472,7 @@ static int dummy_hrtimer_create(struct snd_pcm_substream *substream) dpcm->timer.function = dummy_hrtimer_callback; dpcm->substream = substream; atomic_set(&dpcm->running, 0); + atomic_set(&dpcm->callback_running, 0); return 0; }
WARNING: multiple messages have this Message-ID (diff)
From: Takashi Iwai <tiwai@suse.de> To: Anna-Maria Gleixner <anna-maria@linutronix.de> Cc: LKML <linux-kernel@vger.kernel.org>, alsa-devel@alsa-project.org, keescook@chromium.org, Peter Zijlstra <peterz@infradead.org>, John Stultz <john.stultz@linaro.org>, Thomas Gleixner <tglx@linutronix.de>, Christoph Hellwig <hch@lst.org>, Jaroslav Kysela <perex@perex.cz>, Ingo Molnar <mingo@redhat.com>, Takashi Sakamoto <o-takashi@sakamocchi.jp> Subject: Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer Date: Thu, 31 Aug 2017 17:36:37 +0200 [thread overview] Message-ID: <s5hh8wnvjei.wl-tiwai@suse.de> (raw) In-Reply-To: <20170831105827.401092373@linutronix.de> On Thu, 31 Aug 2017 14:23:45 +0200, Anna-Maria Gleixner wrote: > > From: Thomas Gleixner <tglx@linutronix.de> > > The tasklet is used to defer the execution of snd_pcm_period_elapsed() to > the softirq context. Using the CLOCK_MONOTONIC_SOFT base invokes the timer > callback in softirq context as well which renders the tasklet useless. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de> > Cc: Jaroslav Kysela <perex@perex.cz> > Cc: Takashi Iwai <tiwai@suse.com> > Cc: Takashi Sakamoto <o-takashi@sakamocchi.jp> > Cc: alsa-devel@alsa-project.org I gave it at try, but it caused a kernel hang, unfortunately. The reason is that snd_pcm_period_elapased() may stop the stream (e.g. when reaching at the end). With this patchset, it'll lead to the call of hrtimer_cancel() from the hrtimer callback itself, thus it stalls. Below is the additional fix over your patch for working around it. I believe it should cover most corner cases, and seems working fine through quick tests, so far. thanks, Takashi --- diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c index 273d60c42125..b5dd64e3dab1 100644 --- a/sound/drivers/dummy.c +++ b/sound/drivers/dummy.c @@ -375,6 +375,7 @@ struct dummy_hrtimer_pcm { ktime_t base_time; ktime_t period_time; atomic_t running; + atomic_t callback_running; struct hrtimer timer; struct snd_pcm_substream *substream; }; @@ -387,8 +388,15 @@ static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer) if (!atomic_read(&dpcm->running)) return HRTIMER_NORESTART; + atomic_inc(&dpcm->callback_running); snd_pcm_period_elapsed(dpcm->substream); + atomic_dec(&dpcm->callback_running); + /* may be flipped during snd_pcm_period_elapsed() */ + if (!atomic_read(&dpcm->running)) + return HRTIMER_NORESTART; + hrtimer_forward_now(timer, dpcm->period_time); + atomic_dec(&dpcm->callback_running); return HRTIMER_RESTART; } @@ -407,7 +415,9 @@ static int dummy_hrtimer_stop(struct snd_pcm_substream *substream) struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data; atomic_set(&dpcm->running, 0); - hrtimer_cancel(&dpcm->timer); + /* issue hrtimer_cancel() only when called outside the callback */ + if (!atomic_read(&dpcm->callback_running)) + hrtimer_cancel(&dpcm->timer); return 0; } @@ -462,6 +472,7 @@ static int dummy_hrtimer_create(struct snd_pcm_substream *substream) dpcm->timer.function = dummy_hrtimer_callback; dpcm->substream = substream; atomic_set(&dpcm->running, 0); + atomic_set(&dpcm->callback_running, 0); return 0; }
next prev parent reply other threads:[~2017-08-31 15:36 UTC|newest] Thread overview: 86+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-08-31 12:23 [PATCH 00/25] hrtimer: Provide softirq context hrtimers Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 01/25] hrtimer: Use predefined function for updating next_timer Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 02/25] hrtimer: Correct blantanly wrong comment Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 03/25] hrtimer: Fix kerneldoc for struct hrtimer_cpu_base Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 04/25] hrtimer: Cleanup clock argument in schedule_hrtimeout_range_clock() Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 05/25] hrtimer: Switch for loop to _ffs() evaluation Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 07/25] hrtimer: Reduce conditional code (hres_active) Anna-Maria Gleixner 2017-09-25 13:55 ` Peter Zijlstra 2017-09-25 14:28 ` Thomas Gleixner 2017-08-31 12:23 ` [PATCH 06/25] hrtimer: Store running timer in hrtimer_clock_base Anna-Maria Gleixner 2017-09-25 14:44 ` Peter Zijlstra 2017-09-28 12:45 ` Thomas Gleixner 2017-08-31 12:23 ` [PATCH 08/25] hrtimer: Reduce conditional code (expires_next, next_timer) Anna-Maria Gleixner 2017-09-26 12:10 ` Peter Zijlstra 2017-09-28 8:07 ` Thomas Gleixner 2017-08-31 12:23 ` [PATCH 09/25] hrtimer: Reduce conditional code (hrtimer_reprogram()) Anna-Maria Gleixner 2017-09-26 12:07 ` Peter Zijlstra 2017-08-31 12:23 ` [PATCH 10/25] hrtimer: Make handling of hrtimer reprogramming and enqueuing not conditional Anna-Maria Gleixner 2017-09-26 12:14 ` Peter Zijlstra 2017-09-28 8:09 ` Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 11/25] hrtimer: Allow remote hrtimer enqueue with "expires_next" as expiry time Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 13/25] hrtimer: Split out code from hrtimer_start_range_ns() for reuse Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 12/25] hrtimer: Simplify hrtimer_reprogram() call Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 14/25] hrtimer: Split out code from __hrtimer_get_next_event() for reuse Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 15/25] hrtimer: Add clock bases for soft irq context Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 16/25] hrtimer: Allow function reuse for softirq based hrtimer Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 18/25] hrtimer: Enable soft and hard hrtimer Anna-Maria Gleixner 2017-09-26 12:52 ` Peter Zijlstra 2017-09-27 14:18 ` Anna-Maria Gleixner 2017-09-27 15:54 ` Thomas Gleixner 2017-09-27 16:47 ` Peter Zijlstra 2017-08-31 12:23 ` [PATCH 17/25] hrtimer: Implementation of softirq hrtimer handling Anna-Maria Gleixner 2017-09-26 12:40 ` Peter Zijlstra 2017-09-26 15:03 ` Peter Zijlstra 2017-09-27 14:22 ` Anna-Maria Gleixner 2017-09-27 16:46 ` Peter Zijlstra 2017-09-27 16:40 ` Peter Zijlstra 2017-09-28 7:59 ` Thomas Gleixner 2017-09-28 8:15 ` Peter Zijlstra 2017-12-19 8:58 ` Sebastian Andrzej Siewior 2017-12-19 13:21 ` Peter Zijlstra 2017-12-20 8:44 ` Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 20/25] mac80211_hwsim: Replace hrtimer tasklet with softirq hrtimer Anna-Maria Gleixner 2017-08-31 12:23 ` Anna-Maria Gleixner 2017-09-05 7:03 ` Johannes Berg 2017-09-05 8:49 ` Thomas Gleixner 2017-09-05 8:51 ` Johannes Berg 2017-08-31 12:23 ` [PATCH 19/25] can/bcm: Replace hrtimer_tasklet with softirq based hrtimer Anna-Maria Gleixner 2017-09-01 15:49 ` Oliver Hartkopp 2017-09-01 15:56 ` Thomas Gleixner 2017-09-01 17:02 ` Oliver Hartkopp 2017-09-02 17:56 ` Oliver Hartkopp 2017-08-31 12:23 ` [PATCH 21/25] xfrm: Replace hrtimer tasklet with softirq hrtimer Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 23/25] ALSA/dummy: Replace " Anna-Maria Gleixner 2017-08-31 14:21 ` Takashi Sakamoto 2017-08-31 14:21 ` Takashi Sakamoto 2017-08-31 14:26 ` Takashi Iwai 2017-08-31 14:26 ` Takashi Iwai 2017-08-31 15:36 ` Takashi Iwai [this message] 2017-08-31 15:36 ` Takashi Iwai 2017-09-01 10:25 ` Takashi Sakamoto 2017-09-01 10:25 ` Takashi Sakamoto 2017-09-01 11:58 ` Takashi Iwai 2017-09-01 11:58 ` Takashi Iwai 2017-09-02 1:19 ` Takashi Sakamoto 2017-09-02 1:19 ` Takashi Sakamoto 2017-09-04 12:45 ` Takashi Sakamoto 2017-09-05 15:53 ` [PATCH 23/25 v2] " Sebastian Andrzej Siewior 2017-09-05 15:53 ` Sebastian Andrzej Siewior 2017-09-05 16:02 ` Takashi Iwai 2017-09-05 16:02 ` Takashi Iwai 2017-09-05 16:05 ` Takashi Sakamoto 2017-09-05 16:18 ` [PATCH 23/25 v3] " Sebastian Andrzej Siewior 2017-09-05 16:18 ` Sebastian Andrzej Siewior 2017-09-06 4:30 ` Takashi Sakamoto 2017-09-06 4:30 ` Takashi Sakamoto 2017-09-08 8:28 ` [alsa-devel] " Takashi Iwai 2017-09-08 8:28 ` Takashi Iwai 2017-08-31 12:23 ` [PATCH 22/25] softirq: Remove tasklet_hrtimer Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 25/25] usb/gadget/NCM: Replace tasklet with softirq hrtimer Anna-Maria Gleixner 2017-10-24 9:45 ` Felipe Balbi 2017-08-31 12:23 ` [PATCH 24/25] net/cdc_ncm: " Anna-Maria Gleixner 2017-08-31 13:33 ` Greg Kroah-Hartman 2017-08-31 13:57 ` Bjørn Mork 2017-09-05 15:42 ` [PATCH 24/25 v2] " Sebastian Andrzej Siewior 2017-08-31 12:36 ` [PATCH 00/25] hrtimer: Provide softirq context hrtimers Anna-Maria Gleixner
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=s5hh8wnvjei.wl-tiwai@suse.de \ --to=tiwai@suse.de \ --cc=alsa-devel@alsa-project.org \ --cc=anna-maria@linutronix.de \ --cc=hch@lst.org \ --cc=john.stultz@linaro.org \ --cc=keescook@chromium.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mingo@redhat.com \ --cc=o-takashi@sakamocchi.jp \ --cc=perex@perex.cz \ --cc=peterz@infradead.org \ --cc=tglx@linutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.