Messages in this thread | | | Date | Fri, 29 May 2020 18:14:01 -0400 | From | Steven Rostedt <> | Subject | Re: [PATCH 13/14] lockdep: Prepare for NMI IRQ state tracking |
| |
On Fri, 29 May 2020 23:27:41 +0200 Peter Zijlstra <peterz@infradead.org> wrote:
> There is no reason not to always, accurately, track IRQ state. > > This change also makes IRQ state tracking ignore lockdep_off(). > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > kernel/locking/lockdep.c | 33 ++++++++++++++++++++++++++++++--- > 1 file changed, 30 insertions(+), 3 deletions(-) > > --- a/kernel/locking/lockdep.c > +++ b/kernel/locking/lockdep.c > @@ -3646,7 +3646,13 @@ static void __trace_hardirqs_on_caller(v > */ > void lockdep_hardirqs_on_prepare(unsigned long ip) > { > - if (unlikely(!debug_locks || current->lockdep_recursion))
Why remove the check for debug_locks? Isn't that there to disable everything at once to prevent more warnings to be printed?
Also, isn't there other ways that we could have recursion besides NMIs? Say we do a printk inside here, or call something that may also enable interrupts? I thought the recursion check was also to prevent lockdep infrastructure calling something that lockdep monitors being a problem?
Or am I missing something?
-- Steve
> + /* > + * NMIs do not (and cannot) track lock dependencies, nothing to do. > + */ > + if (in_nmi()) > + return; > + > + if (DEBUG_LOCKS_WARN_ON(current->lockdep_recursion & LOCKDEP_RECURSION_MASK)) > return; > > if (unlikely(current->hardirqs_enabled)) { > @@ -3692,7 +3698,24 @@ void noinstr lockdep_hardirqs_on(unsigne > { > struct task_struct *curr = current; > > - if (unlikely(!debug_locks || curr->lockdep_recursion)) > + /* > + * NMIs can happen in the middle of local_irq_{en,dis}able() where the > + * tracking state and hardware state are out of sync. > + * > + * NMIs must save lockdep_hardirqs_enabled() to restore IRQ state from, > + * and not rely on hardware state like normal interrupts. > + */ > + if (in_nmi()) { > + /* > + * Skip: > + * - recursion check, because NMI can hit lockdep; > + * - hardware state check, because above; > + * - chain_key check, see lockdep_hardirqs_on_prepare(). > + */ > + goto skip_checks; > + } > + > + if (DEBUG_LOCKS_WARN_ON(curr->lockdep_recursion & LOCKDEP_RECURSION_MASK)) > return; > > if (curr->hardirqs_enabled) { > @@ -3720,6 +3743,7 @@ void noinstr lockdep_hardirqs_on(unsigne > DEBUG_LOCKS_WARN_ON(current->hardirq_chain_key != > current->curr_chain_key); > > +skip_checks: > /* we'll do an OFF -> ON transition: */ > curr->hardirqs_enabled = 1; > curr->hardirq_enable_ip = ip; > @@ -3735,7 +3759,10 @@ void noinstr lockdep_hardirqs_off(unsign > { > struct task_struct *curr = current; > > - if (unlikely(!debug_locks || curr->lockdep_recursion)) > + /* > + * NMIs can happen in lockdep. > + */ > + if (!in_nmi() && DEBUG_LOCKS_WARN_ON(curr->lockdep_recursion & LOCKDEP_RECURSION_MASK)) > return; > > /* >
| |