* [PATCH] ALSA: aloop - do not reschedule timer if deleted @ 2012-10-17 13:35 Omair Mohammed Abdullah 2012-10-17 14:06 ` Takashi Iwai 0 siblings, 1 reply; 4+ messages in thread From: Omair Mohammed Abdullah @ 2012-10-17 13:35 UTC (permalink / raw) To: alsa-devel; +Cc: tiwai, Vinod Koul, Omair Mohammed Abdullah If the timer is deleted while the timer handler is running, it may get rescheduled and an unnecessary period elapsed will be sent. Add a flag to reschedule the timer only if it has not been stopped. Signed-off-by: Omair Mohammed Abdullah <omair.m.abdullah@linux.intel.com> Signed-off-by: Vinod Koul <vinod.koul@linux.intel.com> --- sound/drivers/aloop.c | 21 ++++++++++++++++++--- 1 files changed, 18 insertions(+), 3 deletions(-) diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c index 0fe6d64..fec4e17 100644 --- a/sound/drivers/aloop.c +++ b/sound/drivers/aloop.c @@ -121,6 +121,7 @@ struct loopback_pcm { unsigned long last_jiffies; struct timer_list timer; spinlock_t timer_lock; + bool timer_stopped; }; static struct platform_device *devices[SNDRV_CARDS]; @@ -166,12 +167,11 @@ static inline unsigned int get_rate_shift(struct loopback_pcm *dpcm) return get_setup(dpcm)->rate_shift; } -static void loopback_timer_start(struct loopback_pcm *dpcm) +static void do_timer_start(struct loopback_pcm *dpcm) { unsigned long tick; unsigned int rate_shift = get_rate_shift(dpcm); - spin_lock(&dpcm->timer_lock); if (rate_shift != dpcm->pcm_rate_shift) { dpcm->pcm_rate_shift = rate_shift; dpcm->period_size_frac = frac_pos(dpcm, dpcm->pcm_period_size); @@ -184,6 +184,13 @@ static void loopback_timer_start(struct loopback_pcm *dpcm) tick = (tick + dpcm->pcm_bps - 1) / dpcm->pcm_bps; dpcm->timer.expires = jiffies + tick; add_timer(&dpcm->timer); +} + +static inline void loopback_timer_start(struct loopback_pcm *dpcm) +{ + spin_lock(&dpcm->timer_lock); + dpcm->timer_stopped = false; + do_timer_start(dpcm); spin_unlock(&dpcm->timer_lock); } @@ -192,6 +199,7 @@ static inline void loopback_timer_stop(struct loopback_pcm *dpcm) spin_lock(&dpcm->timer_lock); del_timer(&dpcm->timer); dpcm->timer.expires = 0; + dpcm->timer_stopped = true; spin_unlock(&dpcm->timer_lock); } @@ -540,7 +548,13 @@ static void loopback_timer_function(unsigned long data) running = loopback_pos_update(dpcm->cable); if (running & (1 << dpcm->substream->stream)) { - loopback_timer_start(dpcm); + spin_lock(&dpcm->timer_lock); + if (dpcm->timer_stopped) { + spin_unlock(&dpcm->timer_lock); + return; + } + do_timer_start(dpcm); + spin_unlock(&dpcm->timer_lock); if (dpcm->period_update_pending) { dpcm->period_update_pending = 0; snd_pcm_period_elapsed(dpcm->substream); @@ -670,6 +684,7 @@ static int loopback_open(struct snd_pcm_substream *substream) } dpcm->loopback = loopback; dpcm->substream = substream; + dpcm->timer_stopped = true; setup_timer(&dpcm->timer, loopback_timer_function, (unsigned long)dpcm); spin_lock_init(&dpcm->timer_lock); -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ALSA: aloop - do not reschedule timer if deleted 2012-10-17 13:35 [PATCH] ALSA: aloop - do not reschedule timer if deleted Omair Mohammed Abdullah @ 2012-10-17 14:06 ` Takashi Iwai 2012-10-20 12:42 ` Omair Mohammed Abdullah 0 siblings, 1 reply; 4+ messages in thread From: Takashi Iwai @ 2012-10-17 14:06 UTC (permalink / raw) To: Omair Mohammed Abdullah; +Cc: alsa-devel, Vinod Koul At Wed, 17 Oct 2012 19:05:46 +0530, Omair Mohammed Abdullah wrote: > > If the timer is deleted while the timer handler is running, it may get > rescheduled and an unnecessary period elapsed will be sent. > > Add a flag to reschedule the timer only if it has not been stopped. In which situation is it rescheduled exactly? Actually there is a small race in the check of running bit and the timer lock. Do you mean this? If so, wouldn't it be cleaner to get rid of timer_lock and protect the whole start/stop with cable->lock, too, like below? thanks, Takashi --- diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c index 0fe6d64..1904046 100644 --- a/sound/drivers/aloop.c +++ b/sound/drivers/aloop.c @@ -120,7 +120,6 @@ struct loopback_pcm { unsigned int last_drift; unsigned long last_jiffies; struct timer_list timer; - spinlock_t timer_lock; }; static struct platform_device *devices[SNDRV_CARDS]; @@ -166,12 +165,12 @@ static inline unsigned int get_rate_shift(struct loopback_pcm *dpcm) return get_setup(dpcm)->rate_shift; } +/* call in cable->lock */ static void loopback_timer_start(struct loopback_pcm *dpcm) { unsigned long tick; unsigned int rate_shift = get_rate_shift(dpcm); - spin_lock(&dpcm->timer_lock); if (rate_shift != dpcm->pcm_rate_shift) { dpcm->pcm_rate_shift = rate_shift; dpcm->period_size_frac = frac_pos(dpcm, dpcm->pcm_period_size); @@ -184,15 +183,13 @@ static void loopback_timer_start(struct loopback_pcm *dpcm) tick = (tick + dpcm->pcm_bps - 1) / dpcm->pcm_bps; dpcm->timer.expires = jiffies + tick; add_timer(&dpcm->timer); - spin_unlock(&dpcm->timer_lock); } +/* call in cable->lock */ static inline void loopback_timer_stop(struct loopback_pcm *dpcm) { - spin_lock(&dpcm->timer_lock); del_timer(&dpcm->timer); dpcm->timer.expires = 0; - spin_unlock(&dpcm->timer_lock); } #define CABLE_VALID_PLAYBACK (1 << SNDRV_PCM_STREAM_PLAYBACK) @@ -274,8 +271,8 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd) spin_lock(&cable->lock); cable->running |= stream; cable->pause &= ~stream; - spin_unlock(&cable->lock); loopback_timer_start(dpcm); + spin_unlock(&cable->lock); if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) loopback_active_notify(dpcm); break; @@ -283,23 +280,23 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd) spin_lock(&cable->lock); cable->running &= ~stream; cable->pause &= ~stream; - spin_unlock(&cable->lock); loopback_timer_stop(dpcm); + spin_unlock(&cable->lock); if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) loopback_active_notify(dpcm); break; case SNDRV_PCM_TRIGGER_PAUSE_PUSH: spin_lock(&cable->lock); cable->pause |= stream; - spin_unlock(&cable->lock); loopback_timer_stop(dpcm); + spin_unlock(&cable->lock); break; case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: spin_lock(&cable->lock); dpcm->last_jiffies = jiffies; cable->pause &= ~stream; - spin_unlock(&cable->lock); loopback_timer_start(dpcm); + spin_unlock(&cable->lock); break; default: return -EINVAL; @@ -477,6 +474,7 @@ static inline void bytepos_finish(struct loopback_pcm *dpcm, dpcm->buf_pos %= dpcm->pcm_buffer_size; } +/* call in cable->lock */ static unsigned int loopback_pos_update(struct loopback_cable *cable) { struct loopback_pcm *dpcm_play = @@ -485,9 +483,7 @@ static unsigned int loopback_pos_update(struct loopback_cable *cable) cable->streams[SNDRV_PCM_STREAM_CAPTURE]; unsigned long delta_play = 0, delta_capt = 0; unsigned int running, count1, count2; - unsigned long flags; - spin_lock_irqsave(&cable->lock, flags); running = cable->running ^ cable->pause; if (running & (1 << SNDRV_PCM_STREAM_PLAYBACK)) { delta_play = jiffies - dpcm_play->last_jiffies; @@ -529,32 +525,39 @@ static unsigned int loopback_pos_update(struct loopback_cable *cable) bytepos_finish(dpcm_play, count1); bytepos_finish(dpcm_capt, count1); unlock: - spin_unlock_irqrestore(&cable->lock, flags); return running; } static void loopback_timer_function(unsigned long data) { struct loopback_pcm *dpcm = (struct loopback_pcm *)data; - unsigned int running; + unsigned long flags; - running = loopback_pos_update(dpcm->cable); - if (running & (1 << dpcm->substream->stream)) { + spin_lock_irqsave(&dpcm->cable->lock, flags); + if (loopback_pos_update(dpcm->cable) & (1 << dpcm->substream->stream)) { loopback_timer_start(dpcm); if (dpcm->period_update_pending) { dpcm->period_update_pending = 0; + spin_unlock_irqrestore(&dpcm->cable->lock, flags); + /* need to unlock before calling below */ snd_pcm_period_elapsed(dpcm->substream); + return; } } + spin_unlock_irqrestore(&dpcm->cable->lock, flags); } static snd_pcm_uframes_t loopback_pointer(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime; struct loopback_pcm *dpcm = runtime->private_data; + snd_pcm_uframes_t pos; + spin_lock(&dpcm->cable->lock); loopback_pos_update(dpcm->cable); - return bytes_to_frames(runtime, dpcm->buf_pos); + pos = dpcm->buf_pos; + spin_unlock(&dpcm->cable->lock); + return bytes_to_frames(runtime, pos); } static struct snd_pcm_hardware loopback_pcm_hardware = @@ -672,7 +675,6 @@ static int loopback_open(struct snd_pcm_substream *substream) dpcm->substream = substream; setup_timer(&dpcm->timer, loopback_timer_function, (unsigned long)dpcm); - spin_lock_init(&dpcm->timer_lock); cable = loopback->cables[substream->number][dev]; if (!cable) { ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ALSA: aloop - do not reschedule timer if deleted 2012-10-17 14:06 ` Takashi Iwai @ 2012-10-20 12:42 ` Omair Mohammed Abdullah 2012-10-21 8:55 ` Takashi Iwai 0 siblings, 1 reply; 4+ messages in thread From: Omair Mohammed Abdullah @ 2012-10-20 12:42 UTC (permalink / raw) To: alsa-devel; +Cc: Takashi Iwai >> >> If the timer is deleted while the timer handler is running, it may get >> rescheduled and an unnecessary period elapsed will be sent. >> >> Add a flag to reschedule the timer only if it has not been stopped. > > In which situation is it rescheduled exactly? When the loopback_timer_stop() runs on one CPU while loopback_timer_function() runs on the other CPU. This causes the loopback_timer_function() to reschedule the timer beacuse del_timer() will not wait for the timer function to finish. > > Actually there is a small race in the check of running bit and the > timer lock. Do you mean this? > > If so, wouldn't it be cleaner to get rid of timer_lock and protect the > whole start/stop with cable->lock, too, like below? > Yes, I think this fixes the problem quite well. > > thanks, > > Takashi > > --- > diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c > index 0fe6d64..1904046 100644 > --- a/sound/drivers/aloop.c > +++ b/sound/drivers/aloop.c > @@ -120,7 +120,6 @@ struct loopback_pcm { > unsigned int last_drift; > unsigned long last_jiffies; > struct timer_list timer; > - spinlock_t timer_lock; > }; > > static struct platform_device *devices[SNDRV_CARDS]; > @@ -166,12 +165,12 @@ static inline unsigned int get_rate_shift(struct loopback_pcm *dpcm) > return get_setup(dpcm)->rate_shift; > } > > +/* call in cable->lock */ > static void loopback_timer_start(struct loopback_pcm *dpcm) > { > unsigned long tick; > unsigned int rate_shift = get_rate_shift(dpcm); > > - spin_lock(&dpcm->timer_lock); > if (rate_shift != dpcm->pcm_rate_shift) { > dpcm->pcm_rate_shift = rate_shift; > dpcm->period_size_frac = frac_pos(dpcm, dpcm->pcm_period_size); > @@ -184,15 +183,13 @@ static void loopback_timer_start(struct loopback_pcm *dpcm) > tick = (tick + dpcm->pcm_bps - 1) / dpcm->pcm_bps; > dpcm->timer.expires = jiffies + tick; > add_timer(&dpcm->timer); > - spin_unlock(&dpcm->timer_lock); > } > > +/* call in cable->lock */ > static inline void loopback_timer_stop(struct loopback_pcm *dpcm) > { > - spin_lock(&dpcm->timer_lock); > del_timer(&dpcm->timer); > dpcm->timer.expires = 0; > - spin_unlock(&dpcm->timer_lock); > } > > #define CABLE_VALID_PLAYBACK (1 << SNDRV_PCM_STREAM_PLAYBACK) > @@ -274,8 +271,8 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd) > spin_lock(&cable->lock); > cable->running |= stream; > cable->pause &= ~stream; > - spin_unlock(&cable->lock); > loopback_timer_start(dpcm); > + spin_unlock(&cable->lock); > if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) > loopback_active_notify(dpcm); > break; > @@ -283,23 +280,23 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd) > spin_lock(&cable->lock); > cable->running &= ~stream; > cable->pause &= ~stream; > - spin_unlock(&cable->lock); > loopback_timer_stop(dpcm); > + spin_unlock(&cable->lock); > if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) > loopback_active_notify(dpcm); > break; > case SNDRV_PCM_TRIGGER_PAUSE_PUSH: > spin_lock(&cable->lock); > cable->pause |= stream; > - spin_unlock(&cable->lock); > loopback_timer_stop(dpcm); > + spin_unlock(&cable->lock); > break; > case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: > spin_lock(&cable->lock); > dpcm->last_jiffies = jiffies; > cable->pause &= ~stream; > - spin_unlock(&cable->lock); > loopback_timer_start(dpcm); > + spin_unlock(&cable->lock); > break; > default: > return -EINVAL; > @@ -477,6 +474,7 @@ static inline void bytepos_finish(struct loopback_pcm *dpcm, > dpcm->buf_pos %= dpcm->pcm_buffer_size; > } > > +/* call in cable->lock */ > static unsigned int loopback_pos_update(struct loopback_cable *cable) > { > struct loopback_pcm *dpcm_play = > @@ -485,9 +483,7 @@ static unsigned int loopback_pos_update(struct loopback_cable *cable) > cable->streams[SNDRV_PCM_STREAM_CAPTURE]; > unsigned long delta_play = 0, delta_capt = 0; > unsigned int running, count1, count2; > - unsigned long flags; > > - spin_lock_irqsave(&cable->lock, flags); > running = cable->running ^ cable->pause; > if (running & (1 << SNDRV_PCM_STREAM_PLAYBACK)) { > delta_play = jiffies - dpcm_play->last_jiffies; > @@ -529,32 +525,39 @@ static unsigned int loopback_pos_update(struct loopback_cable *cable) > bytepos_finish(dpcm_play, count1); > bytepos_finish(dpcm_capt, count1); > unlock: > - spin_unlock_irqrestore(&cable->lock, flags); > return running; > } > > static void loopback_timer_function(unsigned long data) > { > struct loopback_pcm *dpcm = (struct loopback_pcm *)data; > - unsigned int running; > + unsigned long flags; > > - running = loopback_pos_update(dpcm->cable); > - if (running & (1 << dpcm->substream->stream)) { > + spin_lock_irqsave(&dpcm->cable->lock, flags); > + if (loopback_pos_update(dpcm->cable) & (1 << dpcm->substream->stream)) { > loopback_timer_start(dpcm); > if (dpcm->period_update_pending) { > dpcm->period_update_pending = 0; > + spin_unlock_irqrestore(&dpcm->cable->lock, flags); > + /* need to unlock before calling below */ > snd_pcm_period_elapsed(dpcm->substream); > + return; > } > } > + spin_unlock_irqrestore(&dpcm->cable->lock, flags); > } > > static snd_pcm_uframes_t loopback_pointer(struct snd_pcm_substream *substream) > { > struct snd_pcm_runtime *runtime = substream->runtime; > struct loopback_pcm *dpcm = runtime->private_data; > + snd_pcm_uframes_t pos; > > + spin_lock(&dpcm->cable->lock); > loopback_pos_update(dpcm->cable); > - return bytes_to_frames(runtime, dpcm->buf_pos); > + pos = dpcm->buf_pos; > + spin_unlock(&dpcm->cable->lock); > + return bytes_to_frames(runtime, pos); > } > > static struct snd_pcm_hardware loopback_pcm_hardware = > @@ -672,7 +675,6 @@ static int loopback_open(struct snd_pcm_substream *substream) > dpcm->substream = substream; > setup_timer(&dpcm->timer, loopback_timer_function, > (unsigned long)dpcm); > - spin_lock_init(&dpcm->timer_lock); > > cable = loopback->cables[substream->number][dev]; > if (!cable) { > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ALSA: aloop - do not reschedule timer if deleted 2012-10-20 12:42 ` Omair Mohammed Abdullah @ 2012-10-21 8:55 ` Takashi Iwai 0 siblings, 0 replies; 4+ messages in thread From: Takashi Iwai @ 2012-10-21 8:55 UTC (permalink / raw) To: Omair Mohammed Abdullah; +Cc: alsa-devel At Sat, 20 Oct 2012 18:12:29 +0530, Omair Mohammed Abdullah wrote: > > > >> > >> If the timer is deleted while the timer handler is running, it may get > >> rescheduled and an unnecessary period elapsed will be sent. > >> > >> Add a flag to reschedule the timer only if it has not been stopped. > > > > In which situation is it rescheduled exactly? > When the loopback_timer_stop() runs on one CPU while > loopback_timer_function() runs on the other CPU. This causes the > loopback_timer_function() to reschedule the timer beacuse del_timer() > will not wait for the timer function to finish. > > > > > Actually there is a small race in the check of running bit and the > > timer lock. Do you mean this? > > > > If so, wouldn't it be cleaner to get rid of timer_lock and protect the > > whole start/stop with cable->lock, too, like below? > > > Yes, I think this fixes the problem quite well. OK, now applied my patch. Thanks for checking. Takashi ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-10-21 8:55 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-10-17 13:35 [PATCH] ALSA: aloop - do not reschedule timer if deleted Omair Mohammed Abdullah 2012-10-17 14:06 ` Takashi Iwai 2012-10-20 12:42 ` Omair Mohammed Abdullah 2012-10-21 8:55 ` Takashi Iwai
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.