From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752888AbcDXQQQ (ORCPT ); Sun, 24 Apr 2016 12:16:16 -0400 Received: from mx2.suse.de ([195.135.220.15]:54386 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752691AbcDXQQP (ORCPT ); Sun, 24 Apr 2016 12:16:15 -0400 Date: Sun, 24 Apr 2016 18:16:12 +0200 Message-ID: From: Takashi Iwai To: Dmitry Vyukov Cc: Jaroslav Kysela , alsa-devel@alsa-project.org, LKML , Alexander Potapenko , Kostya Serebryany , Sasha Levin , syzkaller Subject: Re: sound: deadlock involving snd_hrtimer_callback In-Reply-To: References: User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.5 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: multipart/mixed; boundary="Multipart_Sun_Apr_24_18:16:12_2016-1" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --Multipart_Sun_Apr_24_18:16:12_2016-1 Content-Type: text/plain; charset=US-ASCII On Sun, 24 Apr 2016 17:16:32 +0200, Dmitry Vyukov wrote: > > On Sat, Apr 23, 2016 at 11:02 PM, Takashi Iwai 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: [] > >> snd_timer_interrupt+0xa9/0xd30 sound/core/timer.c:701 > >> > >> but task is already holding lock: > >> (&(&stime->lock)->rlock){-.....}, at: [] > >> 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 --Multipart_Sun_Apr_24_18:16:12_2016-1 Content-Type: application/octet-stream; type=patch Content-Disposition: attachment; filename="0001-ALSA-hrtimer-Use-manual-start-stop-in-callback-v3.patch" Content-Transfer-Encoding: 7bit >>From e72f883a9c560fc2edd03774da0c32a64a7b8358 Mon Sep 17 00:00:00 2001 From: Takashi Iwai 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 --- 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 --Multipart_Sun_Apr_24_18:16:12_2016-1 Content-Type: application/octet-stream; type=patch Content-Disposition: attachment; filename="0002-ALSA-timer-Introduce-stop_sync-op.patch" Content-Transfer-Encoding: 7bit >>From cd638a915b9b66c1104667658aeaed89df2c6127 Mon Sep 17 00:00:00 2001 From: Takashi Iwai 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 --- 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 --Multipart_Sun_Apr_24_18:16:12_2016-1--