| Message ID | CA+icZUUV5cOpUqsx7PAvfi3hs9pa11=DXH=BEbjGnOJ+kDi_5Q@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
On Wed, 30 Sep 2015 08:41:57 +0200 Sedat Dilek <sedat.dilek@gmail.com> wrote: > > What shall I do... play with lockdep (print_irqtrace_events) in > > del_timer_sync()? > > I recalled that when Jiri was thinking towards a "compiler bug" that > the part in del_timer_sync() emebedded in the "ifdef CONFIG_LOCKDEP" > is somehow "mis-compiled". > Furthermore, I see that try_to_del_timer_sync() is invoked within > del_timer_sync() which does a spin_unlock_irqrestore(). Which is fine. > > [ kernel/time/timer.c ] > > int del_timer_sync(struct timer_list *timer) > { > #ifdef CONFIG_LOCKDEP > unsigned long flags; > > /* > * If lockdep gives a backtrace here, please reference > * the synchronization rules above. > */ > local_irq_save(flags); > lock_map_acquire(&timer->lockdep_map); > lock_map_release(&timer->lockdep_map); > local_irq_restore(flags); > #endif > /* > * don't use it in hardirq context, because it > * could lead to deadlock. > */ > WARN_ON(in_irq() && !(timer->flags & TIMER_IRQSAFE)); > for (;;) { > int ret = try_to_del_timer_sync(timer); > if (ret >= 0) > return ret; > cpu_relax(); > } > } > EXPORT_SYMBOL(del_timer_sync); > #endif > ... > int try_to_del_timer_sync(struct timer_list *timer) > { > struct tvec_base *base; > unsigned long flags; > int ret = -1; > > debug_assert_init(timer); > > base = lock_timer_base(timer, &flags); lock_timer_base() will grab the base->lock and save the current state of interrupts in flags, and then disable interrupts. > > if (base->running_timer != timer) { > timer_stats_timer_clear_start_info(timer); > ret = detach_if_pending(timer, base, true); > } > spin_unlock_irqrestore(&base->lock, flags); this will release the base->lock and put the interrupt state back to what it was before it took the lock. I still don't see anything wrong with the code. -- Steve > > return ret; > } > EXPORT_SYMBOL(try_to_del_timer_sync); > ... > > Is the attached patch suitable for a test? > > I remember I tried to turn lockdep kernel-config for amd64 but that > was somehow tricky. > > Shall I ask timer folks? > > Thoughts? > > Thanks! > > - Sedat - -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From edb4cb72c631c5e588af40794830c5bb5d6a927a Mon Sep 17 00:00:00 2001 From: Sedat Dilek <sedat.dilek@gmail.com> Date: Wed, 30 Sep 2015 08:20:15 +0200 Subject: [PATCH] timer: lockdep: Add WARN_ON(irqs_disabled()) to del_timer_sync() --- kernel/locking/lockdep.c | 1 + kernel/time/timer.c | 3 +++ 2 files changed, 4 insertions(+) diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index 8acfbf773e06..8b29b3dd518f 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -2410,6 +2410,7 @@ void print_irqtrace_events(struct task_struct *curr) printk("softirqs last disabled at (%u): ", curr->softirq_disable_event); print_ip_sym(curr->softirq_disable_ip); } +EXPORT_SYMBOL(print_irqtrace_events); static int HARDIRQ_verbose(struct lock_class *class) { diff --git a/kernel/time/timer.c b/kernel/time/timer.c index 84190f02b521..f434b2dce642 100644 --- a/kernel/time/timer.c +++ b/kernel/time/timer.c @@ -1082,6 +1082,9 @@ int del_timer_sync(struct timer_list *timer) #ifdef CONFIG_LOCKDEP unsigned long flags; + if(WARN_ON(irqs_disabled())) + print_irqtrace_events(current); + /* * If lockdep gives a backtrace here, please reference * the synchronization rules above. -- 2.5.3