Re: sound: deadlock involving snd_hrtimer_callback

[Takashi Iwai <tiwai@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 1962 bytes --]

On Sun, 24 Apr 2016 17:16:32 +0200,
Dmitry Vyukov wrote:
> 
> On Sat, Apr 23, 2016 at 11:02 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > On Sat, 23 Apr 2016 15:40:21 +0200,
> > Dmitry Vyukov wrote:
> >>
> >> Hi Takashi,
> >>
> >> I've incorporated your hrtimer fixes (but also updated to
> >> ddce192106e4f984123884f8e878f66ace94b573) and now I am seeing lots of
> >> the following deadlock messages:
> >>
> >>
> >> [ INFO: possible circular locking dependency detected ]
> >> 4.6.0-rc4+ #351 Not tainted
> >> -------------------------------------------------------
> >> swapper/0/0 is trying to acquire lock:
> >>  (&(&timer->lock)->rlock){-.-...}, at: [<ffffffff8537a749>]
> >> snd_timer_interrupt+0xa9/0xd30 sound/core/timer.c:701
> >>
> >> but task is already holding lock:
> >>  (&(&stime->lock)->rlock){-.....}, at: [<ffffffff85383d3f>]
> >> snd_hrtimer_callback+0x4f/0x2b0 sound/core/hrtimer.c:54
> >>
> >> which lock already depends on the new lock.
> >
> > Oh crap, my second patch is buggy, it leads to ABBA lock, indeed.
> > The first patch is still OK, as it just adds a new behavior mode.
> >
> > Could you replace the second patch with the below one?
> 
> 
> I've replaced the second path with this one. The deadlocks has gone,
> but I've hit these two hangs that look related:
> 
> https://gist.githubusercontent.com/dvyukov/805718ea249c49d17ae759d1b0160684/raw/20891f7e87fe9af3967565559d465d296469244b/gistfile1.txt
> https://gist.githubusercontent.com/dvyukov/7f397ea4aeb9e35596e0c8053cf35a11/raw/3fc22f24f7bab5941e47bab604f96487b5f1944d/gistfile1.txt

Hmm, so it wasn't a good idea to call hrtimer_cancel() in the
spinlock, in anyway.  Scratch the previous one.

OK, below is the yet revised two patches.  One is the simplified
version of the patch, and another is to call hrtimer_cancel() in a new
timer op without spinlock.  Apply these after the first patch
"ALSA: timer: Allow backend disabling start/stop from handler".


thanks,

Takashi


[-- Attachment #2: 0001-ALSA-hrtimer-Use-manual-start-stop-in-callback-v3.patch --]
[-- Type: application/octet-stream, Size: 1510 bytes --]

>From e72f883a9c560fc2edd03774da0c32a64a7b8358 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Wed, 20 Apr 2016 12:10:10 +0200
Subject: [PATCH 1/2] ALSA: hrtimer: Use manual start/stop in callback (v3)

With the new SNDRV_TIMER_HW_RET_CTRL flag, hrtimer can manage the
callback behavior more correctly.  Now it gets a return value from
snd_timer_interrupt() whether to reprogram or stop the timer, and it
can choose the right return value; i.e. we just return
HRTIMER_NORESTART from the handler when the timer is being stopped.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/hrtimer.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/sound/core/hrtimer.c b/sound/core/hrtimer.c
index 656d9a9032dc..79762444bc79 100644
--- a/sound/core/hrtimer.c
+++ b/sound/core/hrtimer.c
@@ -46,12 +46,17 @@ static enum hrtimer_restart snd_hrtimer_callback(struct hrtimer *hrt)
 	struct snd_hrtimer *stime = container_of(hrt, struct snd_hrtimer, hrt);
 	struct snd_timer *t = stime->timer;
 	unsigned long oruns;
+	int ret;
 
 	if (!atomic_read(&stime->running))
 		return HRTIMER_NORESTART;
 
 	oruns = hrtimer_forward_now(hrt, ns_to_ktime(t->sticks * resolution));
-	snd_timer_interrupt(stime->timer, t->sticks * oruns);
+	ret = snd_timer_interrupt(stime->timer, t->sticks * oruns);
+	if (ret == SNDRV_TIMER_RET_STOP) {
+		atomic_set(&stime->running, 0);
+		return HRTIMER_NORESTART;
+	}
 
 	if (!atomic_read(&stime->running))
 		return HRTIMER_NORESTART;
-- 
2.8.1


[-- Attachment #3: 0002-ALSA-timer-Introduce-stop_sync-op.patch --]
[-- Type: application/octet-stream, Size: 4823 bytes --]

>From cd638a915b9b66c1104667658aeaed89df2c6127 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Sun, 24 Apr 2016 18:00:34 +0200
Subject: [PATCH 2/2] ALSA: timer: Introduce stop_sync op

So far, the stop callback is invoked only in the atomic way, and it
couldn't sync with the actual stop of the backend.  This ended up
leaving the possible race opened against the running timer handler.

In this patch, a new op, stop_sync, is introduced to snd_timer
object.  This is called at the end of stop action so that the backend
can synchronize with the proper timer deletion.  As an example, both
systimer and hrtimer backends now call del_timer_sync() and
hrtimer_cancel() there for guaranteeing the termination of the pending
interrupt.

Note that the callback is called outside the timer lock so that it
won't lead to ABBA deadlock.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/timer.h |  1 +
 sound/core/hrtimer.c  |  9 ++++++++-
 sound/core/timer.c    | 19 ++++++++++++++-----
 3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/include/sound/timer.h b/include/sound/timer.h
index 6ca6ed4169da..42dee1244b02 100644
--- a/include/sound/timer.h
+++ b/include/sound/timer.h
@@ -75,6 +75,7 @@ struct snd_timer_hardware {
 	unsigned long (*c_resolution) (struct snd_timer * timer);
 	int (*start) (struct snd_timer * timer);
 	int (*stop) (struct snd_timer * timer);
+	int (*stop_sync) (struct snd_timer * timer);
 	int (*set_period) (struct snd_timer * timer, unsigned long period_num, unsigned long period_den);
 	int (*precise_resolution) (struct snd_timer * timer, unsigned long *num, unsigned long *den);
 };
diff --git a/sound/core/hrtimer.c b/sound/core/hrtimer.c
index 79762444bc79..0e4245c6600a 100644
--- a/sound/core/hrtimer.c
+++ b/sound/core/hrtimer.c
@@ -83,7 +83,6 @@ static int snd_hrtimer_close(struct snd_timer *t)
 	struct snd_hrtimer *stime = t->private_data;
 
 	if (stime) {
-		hrtimer_cancel(&stime->hrt);
 		kfree(stime);
 		t->private_data = NULL;
 	}
@@ -110,12 +109,20 @@ static int snd_hrtimer_stop(struct snd_timer *t)
 	return 0;
 }
 
+static int snd_hrtimer_stop_sync(struct snd_timer *t)
+{
+	struct snd_hrtimer *stime = t->private_data;
+	hrtimer_cancel(&stime->hrt);
+	return 0;
+}
+
 static struct snd_timer_hardware hrtimer_hw = {
 	.flags =	SNDRV_TIMER_HW_AUTO | SNDRV_TIMER_HW_TASKLET,
 	.open =		snd_hrtimer_open,
 	.close =	snd_hrtimer_close,
 	.start =	snd_hrtimer_start,
 	.stop =		snd_hrtimer_stop,
+	.stop_sync =	snd_hrtimer_stop_sync,
 };
 
 /*
diff --git a/sound/core/timer.c b/sound/core/timer.c
index c653c409d74d..5bdb6d3b59af 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -358,8 +358,12 @@ int snd_timer_close(struct snd_timer_instance *timeri)
 	kfree(timeri);
 
 	if (timer) {
-		if (list_empty(&timer->open_list_head) && timer->hw.close)
-			timer->hw.close(timer);
+		if (list_empty(&timer->open_list_head)) {
+			if (timer->hw.stop_sync)
+				timer->hw.stop_sync(timer);
+			if (timer->hw.close)
+				timer->hw.close(timer);
+		}
 		/* release a card refcount for safe disconnection */
 		if (timer->card)
 			put_device(&timer->card->card_dev);
@@ -496,6 +500,7 @@ static int snd_timer_stop1(struct snd_timer_instance *timeri, bool stop)
 {
 	struct snd_timer *timer;
 	int result = 0;
+	bool sync = false;
 	unsigned long flags;
 
 	timer = timeri->timer;
@@ -518,12 +523,14 @@ static int snd_timer_stop1(struct snd_timer_instance *timeri, bool stop)
 	if ((timeri->flags & SNDRV_TIMER_IFLG_RUNNING) &&
 	    !(--timer->running)) {
 		timer->hw.stop(timer);
+		sync = true;
 		if (timer->flags & SNDRV_TIMER_FLG_RESCHED) {
 			timer->flags &= ~SNDRV_TIMER_FLG_RESCHED;
 			snd_timer_reschedule(timer, 0);
 			if (timer->flags & SNDRV_TIMER_FLG_CHANGE) {
 				timer->flags &= ~SNDRV_TIMER_FLG_CHANGE;
 				timer->hw.start(timer);
+				sync = false;
 			}
 		}
 	}
@@ -532,6 +539,8 @@ static int snd_timer_stop1(struct snd_timer_instance *timeri, bool stop)
 			  SNDRV_TIMER_EVENT_CONTINUE);
  unlock:
 	spin_unlock_irqrestore(&timer->lock, flags);
+	if (sync && timer->hw.stop_sync)
+		timer->hw.stop_sync(timer);
 	return result;
 }
 
@@ -1052,7 +1061,7 @@ static int snd_timer_s_stop(struct snd_timer * timer)
 	return 0;
 }
 
-static int snd_timer_s_close(struct snd_timer *timer)
+static int snd_timer_s_stop_sync(struct snd_timer *timer)
 {
 	struct snd_timer_system_private *priv;
 
@@ -1066,9 +1075,9 @@ static struct snd_timer_hardware snd_timer_system =
 	.flags =	SNDRV_TIMER_HW_FIRST | SNDRV_TIMER_HW_TASKLET,
 	.resolution =	1000000000L / HZ,
 	.ticks =	10000000L,
-	.close =	snd_timer_s_close,
 	.start =	snd_timer_s_start,
-	.stop =		snd_timer_s_stop
+	.stop =		snd_timer_s_stop,
+	.stop_sync =	snd_timer_s_stop_sync,
 };
 
 static void snd_timer_free_system(struct snd_timer *timer)
-- 
2.8.1