On Sun, Apr 24, 2016 at 11:31 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> On Sun, 24 Apr 2016 19:09:48 +0200,
> Dmitry Vyukov wrote:
>>
>> On Sun, Apr 24, 2016 at 6:16 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
>> > On Sun, 24 Apr 2016 17:16:32 +0200,
>> > Dmitry Vyukov wrote:
>> >>
>> >> On Sat, Apr 23, 2016 at 11:02 PM, Takashi Iwai <tiwai@xxxxxxx> 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".
>>
>> Done. I will let you know if I see any failures.
>
> After reconsideration, I rewrote the whole patchset again.
> Could you scratch all the previous three patches and replace with the
> single patch below? Sorry for inconvenience!
Reapplied.
I will let you know if I see any crashes in sound.
> -- 8< --
> From: Takashi Iwai <tiwai@xxxxxxx>
> Subject: [PATCH] ALSA: hrtimer: Handle start/stop more properly
>
> This patch tries to address the still remaining issues in ALSA hrtimer
> driver:
> - Spurious use-after-free was detected in hrtimer callback
> - Incorrect rescheduling due to delayed start
> - WARN_ON() is triggered in hrtimer_forward() invoked in hrtimer
> callback
>
> The first issue happens only when the new timer is scheduled even
> while hrtimer is being closed. It's related with the second and third
> items; since ALSA timer core invokes hw.start callback during hrtimer
> interrupt, this may result in the explicit call of hrtimer_start().
>
> Also, the similar problem is seen for the stop; ALSA timer core
> invokes hw.stop callback even in the hrtimer handler, too. Since we
> must not call the synced hrtimer_cancel() in such a context, it's just
> a hrtimer_try_to_cancel() call that doesn't properly work.
>
> Another culprit of the second and third items is the call of
> hrtimer_forward_now() before snd_timer_interrupt(). The timer->stick
> value may change during snd_timer_interrupt() call, but this
> possibility is ignored completely.
>
> For covering these subtle and messy issues, the following changes have
> been done in this patch:
> - A new flag, in_callback, is introduced in the private data to
> indicate that the hrtimer handler is being processed.
> - Both start and stop callbacks skip when called from (during)
> in_callback flag.
> - The hrtimer handler returns properly HRTIMER_RESTART and NORESTART
> depending on the running state now.
> - The hrtimer handler reprograms the expiry properly after
> snd_timer_interrupt() call, instead of before.
> - The close callback clears running flag and sets in_callback flag
> to block any further start/stop calls.
>
> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> ---
> sound/core/hrtimer.c | 56 ++++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 39 insertions(+), 17 deletions(-)
>
> diff --git a/sound/core/hrtimer.c b/sound/core/hrtimer.c
> index 656d9a9032dc..e2f27022b363 100644
> --- a/sound/core/hrtimer.c
> +++ b/sound/core/hrtimer.c
> @@ -38,37 +38,53 @@ static unsigned int resolution;
> struct snd_hrtimer {
> struct snd_timer *timer;
> struct hrtimer hrt;
> - atomic_t running;
> + bool in_callback;
> };
>
> 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;
> -
> - 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);
> + ktime_t delta;
> + unsigned long ticks;
> + enum hrtimer_restart ret = HRTIMER_NORESTART;
> +
> + spin_lock(&t->lock);
> + if (!t->running)
> + goto out; /* fast path */
> + stime->in_callback = true;
> + ticks = t->sticks;
> + spin_unlock(&t->lock);
> +
> + /* calculate the drift */
> + delta = ktime_sub(hrt->base->get_time(), hrtimer_get_expires(hrt));
> + if (delta.tv64 > 0)
> + ticks += ktime_divns(delta, ticks * resolution);
> +
> + snd_timer_interrupt(stime->timer, ticks);
> +
> + spin_lock(&t->lock);
> + if (t->running) {
> + hrtimer_add_expires_ns(hrt, t->sticks * resolution);
> + ret = HRTIMER_RESTART;
> + }
>
> - if (!atomic_read(&stime->running))
> - return HRTIMER_NORESTART;
> - return HRTIMER_RESTART;
> + stime->in_callback = false;
> + out:
> + spin_unlock(&t->lock);
> + return ret;
> }
>
> static int snd_hrtimer_open(struct snd_timer *t)
> {
> struct snd_hrtimer *stime;
>
> - stime = kmalloc(sizeof(*stime), GFP_KERNEL);
> + stime = kzalloc(sizeof(*stime), GFP_KERNEL);
> if (!stime)
> return -ENOMEM;
> hrtimer_init(&stime->hrt, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> stime->timer = t;
> stime->hrt.function = snd_hrtimer_callback;
> - atomic_set(&stime->running, 0);
> t->private_data = stime;
> return 0;
> }
> @@ -78,6 +94,11 @@ static int snd_hrtimer_close(struct snd_timer *t)
> struct snd_hrtimer *stime = t->private_data;
>
> if (stime) {
> + spin_lock_irq(&t->lock);
> + t->running = 0; /* just to be sure */
> + stime->in_callback = 1; /* skip start/stop */
> + spin_unlock_irq(&t->lock);
> +
> hrtimer_cancel(&stime->hrt);
> kfree(stime);
> t->private_data = NULL;
> @@ -89,18 +110,19 @@ static int snd_hrtimer_start(struct snd_timer *t)
> {
> struct snd_hrtimer *stime = t->private_data;
>
> - atomic_set(&stime->running, 0);
> - hrtimer_try_to_cancel(&stime->hrt);
> + if (stime->in_callback)
> + return 0;
> hrtimer_start(&stime->hrt, ns_to_ktime(t->sticks * resolution),
> HRTIMER_MODE_REL);
> - atomic_set(&stime->running, 1);
> return 0;
> }
>
> static int snd_hrtimer_stop(struct snd_timer *t)
> {
> struct snd_hrtimer *stime = t->private_data;
> - atomic_set(&stime->running, 0);
> +
> + if (stime->in_callback)
> + return 0;
> hrtimer_try_to_cancel(&stime->hrt);
> return 0;
> }
> --
> 2.8.1
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
