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.de>, "Jaroslav Kysela" <perex@perex.cz>, "Ingo Molnar" <mingo@redhat.com>, "Takashi Sakamoto" <o-takashi@sakamocchi.jp> Subject: Re: [PATCH v2 34/37] ALSA/dummy: Replace tasklet with softirq hrtimer Date: Tue, 24 Oct 2017 08:25:04 +0200 [thread overview] Message-ID: <s5hzi8hqczj.wl-tiwai@suse.de> (raw) In-Reply-To: <20171022214053.794128710@linutronix.de> On Sun, 22 Oct 2017 23:40:12 +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 HRTIMER_MODE_SOFT mode invokes the timer > callback in softirq context as well which renders the tasklet useless. > > [o-takashi: avoid stall due to a call of hrtimer_cancel() on a callback > of hrtimer] > 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> Feel free to take my ack: Reviewed-by: Takashi Iwai <tiwai@suse.de> thanks, Takashi > Cc: Takashi Sakamoto <o-takashi@sakamocchi.jp> > Cc: alsa-devel@alsa-project.org > Link: http://lkml.kernel.org/r/20170905161820.jtysvxtfleunbbmf@breakpoint.cc > > --- > On 2017-09-06 01:05:43 [+0900], Takashi Sakamoto wrote: > > As Iwai-san mentioned, in this function, .trigger can be called in two > > cases; XRUN occurs and draining is done. Thus, let you change the comment as > > 'In cases of XRUN and draining, this calls .trigger to stop PCM substream.'. > > I'm sorry to trouble you. > > > > snd_pcm_period_elapsed() > > ->snd_pcm_update_hw_ptr0() > > ->snd_pcm_update_state() > > ->snd_pcm_drain_done() > > ... > > ->.trigger(TRIGGER_STOP) > > ->xrun() > > ->snd_pcm_stop() > > ... > > ->.trigger(TRIGGER_STOP) > > > > I think you asked me just to update the comment. Did I do it right? > > v2...v3: updated the comment as per Takashi Sakamoto's suggestion. > v1...v2: merged Takashi Sakamoto fixup of the original patch into v2. > > sound/drivers/dummy.c | 27 ++++++++++++--------------- > 1 file changed, 12 insertions(+), 15 deletions(-) > > --- a/sound/drivers/dummy.c > +++ b/sound/drivers/dummy.c > @@ -376,17 +376,9 @@ struct dummy_hrtimer_pcm { > ktime_t period_time; > atomic_t running; > struct hrtimer timer; > - struct tasklet_struct tasklet; > struct snd_pcm_substream *substream; > }; > > -static void dummy_hrtimer_pcm_elapsed(unsigned long priv) > -{ > - struct dummy_hrtimer_pcm *dpcm = (struct dummy_hrtimer_pcm *)priv; > - if (atomic_read(&dpcm->running)) > - snd_pcm_period_elapsed(dpcm->substream); > -} > - > static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer) > { > struct dummy_hrtimer_pcm *dpcm; > @@ -394,7 +386,14 @@ static enum hrtimer_restart dummy_hrtime > dpcm = container_of(timer, struct dummy_hrtimer_pcm, timer); > if (!atomic_read(&dpcm->running)) > return HRTIMER_NORESTART; > - tasklet_schedule(&dpcm->tasklet); > + /* > + * In cases of XRUN and draining, this calls .trigger to stop PCM > + * substream. > + */ > + snd_pcm_period_elapsed(dpcm->substream); > + if (!atomic_read(&dpcm->running)) > + return HRTIMER_NORESTART; > + > hrtimer_forward_now(timer, dpcm->period_time); > return HRTIMER_RESTART; > } > @@ -404,7 +403,7 @@ static int dummy_hrtimer_start(struct sn > struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data; > > dpcm->base_time = hrtimer_cb_get_time(&dpcm->timer); > - hrtimer_start(&dpcm->timer, dpcm->period_time, HRTIMER_MODE_REL); > + hrtimer_start(&dpcm->timer, dpcm->period_time, HRTIMER_MODE_REL_SOFT); > atomic_set(&dpcm->running, 1); > return 0; > } > @@ -414,14 +413,14 @@ static int dummy_hrtimer_stop(struct snd > struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data; > > atomic_set(&dpcm->running, 0); > - hrtimer_cancel(&dpcm->timer); > + if (!hrtimer_callback_running(&dpcm->timer)) > + hrtimer_cancel(&dpcm->timer); > return 0; > } > > static inline void dummy_hrtimer_sync(struct dummy_hrtimer_pcm *dpcm) > { > hrtimer_cancel(&dpcm->timer); > - tasklet_kill(&dpcm->tasklet); > } > > static snd_pcm_uframes_t > @@ -466,12 +465,10 @@ static int dummy_hrtimer_create(struct s > if (!dpcm) > return -ENOMEM; > substream->runtime->private_data = dpcm; > - hrtimer_init(&dpcm->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); > + hrtimer_init(&dpcm->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT); > dpcm->timer.function = dummy_hrtimer_callback; > dpcm->substream = substream; > atomic_set(&dpcm->running, 0); > - tasklet_init(&dpcm->tasklet, dummy_hrtimer_pcm_elapsed, > - (unsigned long)dpcm); > 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: alsa-devel@alsa-project.org, keescook@chromium.org, Peter Zijlstra <peterz@infradead.org>, LKML <linux-kernel@vger.kernel.org>, Ingo Molnar <mingo@redhat.com>, John Stultz <john.stultz@linaro.org>, Thomas Gleixner <tglx@linutronix.de>, Takashi Sakamoto <o-takashi@sakamocchi.jp>, Christoph Hellwig <hch@lst.de> Subject: Re: [PATCH v2 34/37] ALSA/dummy: Replace tasklet with softirq hrtimer Date: Tue, 24 Oct 2017 08:25:04 +0200 [thread overview] Message-ID: <s5hzi8hqczj.wl-tiwai@suse.de> (raw) In-Reply-To: <20171022214053.794128710@linutronix.de> On Sun, 22 Oct 2017 23:40:12 +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 HRTIMER_MODE_SOFT mode invokes the timer > callback in softirq context as well which renders the tasklet useless. > > [o-takashi: avoid stall due to a call of hrtimer_cancel() on a callback > of hrtimer] > 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> Feel free to take my ack: Reviewed-by: Takashi Iwai <tiwai@suse.de> thanks, Takashi > Cc: Takashi Sakamoto <o-takashi@sakamocchi.jp> > Cc: alsa-devel@alsa-project.org > Link: http://lkml.kernel.org/r/20170905161820.jtysvxtfleunbbmf@breakpoint.cc > > --- > On 2017-09-06 01:05:43 [+0900], Takashi Sakamoto wrote: > > As Iwai-san mentioned, in this function, .trigger can be called in two > > cases; XRUN occurs and draining is done. Thus, let you change the comment as > > 'In cases of XRUN and draining, this calls .trigger to stop PCM substream.'. > > I'm sorry to trouble you. > > > > snd_pcm_period_elapsed() > > ->snd_pcm_update_hw_ptr0() > > ->snd_pcm_update_state() > > ->snd_pcm_drain_done() > > ... > > ->.trigger(TRIGGER_STOP) > > ->xrun() > > ->snd_pcm_stop() > > ... > > ->.trigger(TRIGGER_STOP) > > > > I think you asked me just to update the comment. Did I do it right? > > v2...v3: updated the comment as per Takashi Sakamoto's suggestion. > v1...v2: merged Takashi Sakamoto fixup of the original patch into v2. > > sound/drivers/dummy.c | 27 ++++++++++++--------------- > 1 file changed, 12 insertions(+), 15 deletions(-) > > --- a/sound/drivers/dummy.c > +++ b/sound/drivers/dummy.c > @@ -376,17 +376,9 @@ struct dummy_hrtimer_pcm { > ktime_t period_time; > atomic_t running; > struct hrtimer timer; > - struct tasklet_struct tasklet; > struct snd_pcm_substream *substream; > }; > > -static void dummy_hrtimer_pcm_elapsed(unsigned long priv) > -{ > - struct dummy_hrtimer_pcm *dpcm = (struct dummy_hrtimer_pcm *)priv; > - if (atomic_read(&dpcm->running)) > - snd_pcm_period_elapsed(dpcm->substream); > -} > - > static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer) > { > struct dummy_hrtimer_pcm *dpcm; > @@ -394,7 +386,14 @@ static enum hrtimer_restart dummy_hrtime > dpcm = container_of(timer, struct dummy_hrtimer_pcm, timer); > if (!atomic_read(&dpcm->running)) > return HRTIMER_NORESTART; > - tasklet_schedule(&dpcm->tasklet); > + /* > + * In cases of XRUN and draining, this calls .trigger to stop PCM > + * substream. > + */ > + snd_pcm_period_elapsed(dpcm->substream); > + if (!atomic_read(&dpcm->running)) > + return HRTIMER_NORESTART; > + > hrtimer_forward_now(timer, dpcm->period_time); > return HRTIMER_RESTART; > } > @@ -404,7 +403,7 @@ static int dummy_hrtimer_start(struct sn > struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data; > > dpcm->base_time = hrtimer_cb_get_time(&dpcm->timer); > - hrtimer_start(&dpcm->timer, dpcm->period_time, HRTIMER_MODE_REL); > + hrtimer_start(&dpcm->timer, dpcm->period_time, HRTIMER_MODE_REL_SOFT); > atomic_set(&dpcm->running, 1); > return 0; > } > @@ -414,14 +413,14 @@ static int dummy_hrtimer_stop(struct snd > struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data; > > atomic_set(&dpcm->running, 0); > - hrtimer_cancel(&dpcm->timer); > + if (!hrtimer_callback_running(&dpcm->timer)) > + hrtimer_cancel(&dpcm->timer); > return 0; > } > > static inline void dummy_hrtimer_sync(struct dummy_hrtimer_pcm *dpcm) > { > hrtimer_cancel(&dpcm->timer); > - tasklet_kill(&dpcm->tasklet); > } > > static snd_pcm_uframes_t > @@ -466,12 +465,10 @@ static int dummy_hrtimer_create(struct s > if (!dpcm) > return -ENOMEM; > substream->runtime->private_data = dpcm; > - hrtimer_init(&dpcm->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); > + hrtimer_init(&dpcm->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT); > dpcm->timer.function = dummy_hrtimer_callback; > dpcm->substream = substream; > atomic_set(&dpcm->running, 0); > - tasklet_init(&dpcm->tasklet, dummy_hrtimer_pcm_elapsed, > - (unsigned long)dpcm); > return 0; > } > > > >
next prev parent reply other threads:[~2017-10-24 6:25 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-10-22 21:39 [PATCH v2 00/37] hrtimer: Provide softirq context hrtimers Anna-Maria Gleixner 2017-10-22 21:39 ` [PATCH v2 01/37] hrtimer: Correct blantanly wrong comment Anna-Maria Gleixner 2017-10-22 21:39 ` [PATCH v2 02/37] hrtimer: Fix kerneldoc for struct hrtimer_cpu_base Anna-Maria Gleixner 2017-10-22 21:39 ` [PATCH v2 03/37] hrtimer: Cleanup clock argument in schedule_hrtimeout_range_clock() Anna-Maria Gleixner 2017-10-22 21:39 ` [PATCH v2 04/37] hrtimer: Fix hrtimer function description Anna-Maria Gleixner 2017-10-22 21:39 ` [PATCH v2 05/37] hrtimer: Ensure POSIX compliance (relative CLOCK_REALTIME hrtimers) Anna-Maria Gleixner 2017-10-22 21:39 ` [PATCH v2 06/37] hrtimer: Cleanup hrtimer_mode enum Anna-Maria Gleixner 2017-10-22 21:39 ` [PATCH v2 07/37] tracing: hrtimer: Take all clock bases and modes into account Anna-Maria Gleixner 2017-10-22 21:39 ` [PATCH v2 08/37] tracing: hrtimer: Print hrtimer mode in hrtimer_start tracepoint Anna-Maria Gleixner 2017-10-22 21:39 ` [PATCH v2 09/37] hrtimer: Switch for loop to _ffs() evaluation Anna-Maria Gleixner 2017-10-22 21:39 ` [PATCH v2 10/37] hrtimer: Store running timer in hrtimer_clock_base Anna-Maria Gleixner 2017-10-22 21:39 ` [PATCH v2 11/37] hrtimer: Change boolean struct members into bitfield Anna-Maria Gleixner 2017-10-22 21:39 ` [PATCH v2 12/37] hrtimer: Make room in struct hrtimer_cpu_base Anna-Maria Gleixner 2017-10-22 21:39 ` [PATCH v2 13/37] hrtimer: Reduce conditional code (hres_active) Anna-Maria Gleixner 2017-10-22 21:39 ` [PATCH v2 14/37] hrtimer: Use accesor functions instead of direct access Anna-Maria Gleixner 2017-10-22 21:39 ` [PATCH v2 15/37] hrtimer: Make the remote enqueue check unconditional Anna-Maria Gleixner 2017-10-22 21:39 ` [PATCH v2 16/37] hrtimer: Make hrtimer_cpu_base.next_timer handling unconditional Anna-Maria Gleixner 2017-10-22 21:39 ` [PATCH v2 17/37] hrtimer: Make hrtimer_reprogramm() unconditional Anna-Maria Gleixner 2017-10-22 21:39 ` [PATCH v2 18/37] hrtimer: Reduce conditional code and make hrtimer_force_reprogramm() unconditional Anna-Maria Gleixner 2017-10-22 21:39 ` [PATCH v2 19/37] hrtimer: Unify handling of hrtimer remove Anna-Maria Gleixner 2017-10-22 21:39 ` [PATCH v2 20/37] hrtimer: Unify handling of remote enqueue Anna-Maria Gleixner 2017-10-22 21:39 ` [PATCH v2 21/37] hrtimer: Make remote enqueue decision less restrictive Anna-Maria Gleixner 2017-10-22 21:40 ` [PATCH v2 22/37] hrtimer: Remove base argument from hrtimer_reprogram() Anna-Maria Gleixner 2017-10-22 21:40 ` [PATCH v2 23/37] hrtimer: Split hrtimer_start_range_ns() Anna-Maria Gleixner 2017-10-22 21:40 ` [PATCH v2 24/37] hrtimer: Split __hrtimer_get_next_event() Anna-Maria Gleixner 2017-10-22 21:40 ` [PATCH v2 25/37] hrtimer: Use irqsave/irqrestore around __run_hrtimer() Anna-Maria Gleixner 2017-10-22 21:40 ` [PATCH v2 26/37] hrtimer: Add clock bases and hrtimer mode for soft irq context Anna-Maria Gleixner 2017-10-22 21:40 ` [PATCH v2 27/37] hrtimer: Prepare handling of hard and softirq based hrtimers Anna-Maria Gleixner 2017-10-22 21:40 ` [PATCH v2 28/37] hrtimer: Implement support for " Anna-Maria Gleixner 2017-11-10 12:42 ` Sebastian Andrzej Siewior 2017-11-13 9:13 ` Anna-Maria Gleixner 2017-10-22 21:40 ` [PATCH v2 29/37] hrtimer: Implement SOFT/HARD clock base selection Anna-Maria Gleixner 2017-10-22 21:40 ` [PATCH v2 30/37] can/bcm: Replace hrtimer_tasklet with softirq based hrtimer Anna-Maria Gleixner 2017-10-27 14:42 ` Oliver Hartkopp 2017-10-22 21:40 ` [PATCH v2 31/37] mac80211_hwsim: Replace hrtimer tasklet with softirq hrtimer Anna-Maria Gleixner 2017-10-22 21:40 ` Anna-Maria Gleixner 2017-10-23 10:14 ` Johannes Berg 2017-10-23 10:23 ` Thomas Gleixner 2017-10-23 10:25 ` Johannes Berg 2017-10-23 10:33 ` Thomas Gleixner 2017-10-23 10:42 ` Johannes Berg 2017-10-22 21:40 ` [PATCH v2 32/37] xfrm: " Anna-Maria Gleixner 2017-10-22 21:40 ` [PATCH v2 33/37] softirq: Remove tasklet_hrtimer Anna-Maria Gleixner 2017-10-22 21:40 ` [PATCH v2 34/37] ALSA/dummy: Replace tasklet with softirq hrtimer Anna-Maria Gleixner 2017-10-22 21:40 ` Anna-Maria Gleixner 2017-10-24 6:25 ` Takashi Iwai [this message] 2017-10-24 6:25 ` Takashi Iwai 2017-10-22 21:40 ` [PATCH v2 36/37] usb/gadget/NCM: " Anna-Maria Gleixner 2017-10-22 21:40 ` [PATCH v2 37/37] net/mvpp2: " Anna-Maria Gleixner 2017-10-23 16:08 ` [PATCH v2 00/37] hrtimer: Provide softirq context hrtimers Peter Zijlstra
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=s5hzi8hqczj.wl-tiwai@suse.de \ --to=tiwai@suse.de \ --cc=alsa-devel@alsa-project.org \ --cc=anna-maria@linutronix.de \ --cc=hch@lst.de \ --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.