[PATCH] ALSA: aloop - do not reschedule timer if deleted

[PATCH] ALSA: aloop - do not reschedule timer if deleted

[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

Re: [PATCH] ALSA: aloop - do not reschedule timer if deleted

[Takashi Iwai]

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) {

Re: [PATCH] ALSA: aloop - do not reschedule timer if deleted

[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.
>
> 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
>

Re: [PATCH] ALSA: aloop - do not reschedule timer if deleted

[Takashi Iwai]

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