Re: [PATCH 2/2] lockdep: warn on redundant or incorrect irq state changes
On 23/07/2020 20:56, Nicholas Piggin wrote: > With the previous patch, lockdep hardirq state changes should not be > redundant. Softirq state changes already follow that pattern. > > So warn on unexpected enable-when-enabled or disable-when-disabled > conditions, to catch possible errors or sloppy patterns that could > lead to similar bad behavior due to NMIs etc. This one does not apply anymore as Peter's changes went in already but this one is not really a fix so I can live with that. Did 1/2 go anywhere? Thanks, > > Signed-off-by: Nicholas Piggin > --- > kernel/locking/lockdep.c | 80 +- > kernel/locking/lockdep_internals.h | 4 -- > kernel/locking/lockdep_proc.c | 10 +--- > 3 files changed, 35 insertions(+), 59 deletions(-) > > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c > index 29a8de4c50b9..138458fb2234 100644 > --- a/kernel/locking/lockdep.c > +++ b/kernel/locking/lockdep.c > @@ -3649,15 +3649,8 @@ void lockdep_hardirqs_on_prepare(unsigned long ip) > if (unlikely(!debug_locks || current->lockdep_recursion)) > return; > > - if (unlikely(current->hardirqs_enabled)) { > - /* > - * Neither irq nor preemption are disabled here > - * so this is racy by nature but losing one hit > - * in a stat is not a big deal. > - */ > - __debug_atomic_inc(redundant_hardirqs_on); > + if (DEBUG_LOCKS_WARN_ON(current->hardirqs_enabled)) > return; > - } > > /* >* We're enabling irqs and according to our state above irqs weren't > @@ -3695,15 +3688,8 @@ void noinstr lockdep_hardirqs_on(unsigned long ip) > if (unlikely(!debug_locks || curr->lockdep_recursion)) > return; > > - if (curr->hardirqs_enabled) { > - /* > - * Neither irq nor preemption are disabled here > - * so this is racy by nature but losing one hit > - * in a stat is not a big deal. > - */ > - __debug_atomic_inc(redundant_hardirqs_on); > + if (DEBUG_LOCKS_WARN_ON(curr->hardirqs_enabled)) > return; > - } > > /* >* We're enabling irqs and according to our state above irqs weren't > @@ -3738,6 +3724,9 @@ void noinstr lockdep_hardirqs_off(unsigned long ip) > if (unlikely(!debug_locks || curr->lockdep_recursion)) > return; > > + if (DEBUG_LOCKS_WARN_ON(!curr->hardirqs_enabled)) > + return; > + > /* >* So we're supposed to get called after you mask local IRQs, but for >* some reason the hardware doesn't quite think you did a proper job. > @@ -3745,17 +3734,13 @@ void noinstr lockdep_hardirqs_off(unsigned long ip) > if (DEBUG_LOCKS_WARN_ON(!irqs_disabled())) > return; > > - if (curr->hardirqs_enabled) { > - /* > - * We have done an ON -> OFF transition: > - */ > - curr->hardirqs_enabled = 0; > - curr->hardirq_disable_ip = ip; > - curr->hardirq_disable_event = ++curr->irq_events; > - debug_atomic_inc(hardirqs_off_events); > - } else { > - debug_atomic_inc(redundant_hardirqs_off); > - } > + /* > + * We have done an ON -> OFF transition: > + */ > + curr->hardirqs_enabled = 0; > + curr->hardirq_disable_ip = ip; > + curr->hardirq_disable_event = ++curr->irq_events; > + debug_atomic_inc(hardirqs_off_events); > } > EXPORT_SYMBOL_GPL(lockdep_hardirqs_off); > > @@ -3769,6 +3754,9 @@ void lockdep_softirqs_on(unsigned long ip) > if (unlikely(!debug_locks || current->lockdep_recursion)) > return; > > + if (DEBUG_LOCKS_WARN_ON(curr->softirqs_enabled)) > + return; > + > /* >* We fancy IRQs being disabled here, see softirq.c, avoids >* funny state and nesting things. > @@ -3776,11 +3764,6 @@ void lockdep_softirqs_on(unsigned long ip) > if (DEBUG_LOCKS_WARN_ON(!irqs_disabled())) > return; > > - if (curr->softirqs_enabled) { > - debug_atomic_inc(redundant_softirqs_on); > - return; > - } > - > current->lockdep_recursion++; > /* >* We'll do an OFF -> ON transition: > @@ -3809,26 +3792,26 @@ void lockdep_softirqs_off(unsigned long ip) > if (unlikely(!debug_locks || current->lockdep_recursion)) > return; > > + if (DEBUG_LOCKS_WARN_ON(!curr->softirqs_enabled)) > + return; > + > /* >* We fancy IRQs being disabled here, see softirq.c >*/ > if (DEBUG_LOCKS_WARN_ON(!irqs_disabled())) > return; > > - if (curr->softirqs_enabled) { > - /* > - * We have done an ON -> OFF transition: > - */ > - curr->softirqs_enabled = 0; > -
Re: [PATCH 2/2] lockdep: warn on redundant or incorrect irq state changes
Hi Nicholas, I love your patch! Yet something to improve: [auto build test ERROR on linux/master] [also build test ERROR on powerpc/next linus/master v5.8-rc6] [cannot apply to tip/locking/core next-20200723] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Nicholas-Piggin/lockdep-improve-current-hard-soft-irqs_enabled-synchronisation-with-actual-irq-state/20200723-185938 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 9ebcfadb0610322ac537dd7aa5d9cbc2b2894c68 config: x86_64-randconfig-a002-20200723 (attached as .config) compiler: gcc-9 (Debian 9.3.0-14) 9.3.0 reproduce (this is a W=1 build): # save the attached .config to linux build tree make W=1 ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>): kernel/locking/lockdep.c: In function 'lockdep_init': >> kernel/locking/lockdep.c:5673:9: error: 'struct task_struct' has no member >> named 'hardirqs_enabled' 5673 | current->hardirqs_enabled = 1; | ^~ >> kernel/locking/lockdep.c:5674:9: error: 'struct task_struct' has no member >> named 'softirqs_enabled' 5674 | current->softirqs_enabled = 1; | ^~ In file included from kernel/locking/lockdep.c:60: At top level: kernel/locking/lockdep_internals.h:64:28: warning: 'LOCKF_USED_IN_IRQ_READ' defined but not used [-Wunused-const-variable=] 64 | static const unsigned long LOCKF_USED_IN_IRQ_READ = |^~ In file included from kernel/locking/lockdep.c:60: kernel/locking/lockdep_internals.h:58:28: warning: 'LOCKF_ENABLED_IRQ_READ' defined but not used [-Wunused-const-variable=] 58 | static const unsigned long LOCKF_ENABLED_IRQ_READ = |^~ In file included from kernel/locking/lockdep.c:60: kernel/locking/lockdep_internals.h:52:28: warning: 'LOCKF_USED_IN_IRQ' defined but not used [-Wunused-const-variable=] 52 | static const unsigned long LOCKF_USED_IN_IRQ = |^ In file included from kernel/locking/lockdep.c:60: kernel/locking/lockdep_internals.h:46:28: warning: 'LOCKF_ENABLED_IRQ' defined but not used [-Wunused-const-variable=] 46 | static const unsigned long LOCKF_ENABLED_IRQ = |^ vim +5673 kernel/locking/lockdep.c 5667 5668 printk(" per task-struct memory footprint: %zu bytes\n", 5669 sizeof(((struct task_struct *)NULL)->held_locks)); 5670 5671 WARN_ON(irqs_disabled()); 5672 > 5673 current->hardirqs_enabled = 1; > 5674 current->softirqs_enabled = 1; 5675 } 5676 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.gz Description: application/gzip
[PATCH 2/2] lockdep: warn on redundant or incorrect irq state changes
With the previous patch, lockdep hardirq state changes should not be redundant. Softirq state changes already follow that pattern. So warn on unexpected enable-when-enabled or disable-when-disabled conditions, to catch possible errors or sloppy patterns that could lead to similar bad behavior due to NMIs etc. Signed-off-by: Nicholas Piggin --- kernel/locking/lockdep.c | 80 +- kernel/locking/lockdep_internals.h | 4 -- kernel/locking/lockdep_proc.c | 10 +--- 3 files changed, 35 insertions(+), 59 deletions(-) diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index 29a8de4c50b9..138458fb2234 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -3649,15 +3649,8 @@ void lockdep_hardirqs_on_prepare(unsigned long ip) if (unlikely(!debug_locks || current->lockdep_recursion)) return; - if (unlikely(current->hardirqs_enabled)) { - /* -* Neither irq nor preemption are disabled here -* so this is racy by nature but losing one hit -* in a stat is not a big deal. -*/ - __debug_atomic_inc(redundant_hardirqs_on); + if (DEBUG_LOCKS_WARN_ON(current->hardirqs_enabled)) return; - } /* * We're enabling irqs and according to our state above irqs weren't @@ -3695,15 +3688,8 @@ void noinstr lockdep_hardirqs_on(unsigned long ip) if (unlikely(!debug_locks || curr->lockdep_recursion)) return; - if (curr->hardirqs_enabled) { - /* -* Neither irq nor preemption are disabled here -* so this is racy by nature but losing one hit -* in a stat is not a big deal. -*/ - __debug_atomic_inc(redundant_hardirqs_on); + if (DEBUG_LOCKS_WARN_ON(curr->hardirqs_enabled)) return; - } /* * We're enabling irqs and according to our state above irqs weren't @@ -3738,6 +3724,9 @@ void noinstr lockdep_hardirqs_off(unsigned long ip) if (unlikely(!debug_locks || curr->lockdep_recursion)) return; + if (DEBUG_LOCKS_WARN_ON(!curr->hardirqs_enabled)) + return; + /* * So we're supposed to get called after you mask local IRQs, but for * some reason the hardware doesn't quite think you did a proper job. @@ -3745,17 +3734,13 @@ void noinstr lockdep_hardirqs_off(unsigned long ip) if (DEBUG_LOCKS_WARN_ON(!irqs_disabled())) return; - if (curr->hardirqs_enabled) { - /* -* We have done an ON -> OFF transition: -*/ - curr->hardirqs_enabled = 0; - curr->hardirq_disable_ip = ip; - curr->hardirq_disable_event = ++curr->irq_events; - debug_atomic_inc(hardirqs_off_events); - } else { - debug_atomic_inc(redundant_hardirqs_off); - } + /* +* We have done an ON -> OFF transition: +*/ + curr->hardirqs_enabled = 0; + curr->hardirq_disable_ip = ip; + curr->hardirq_disable_event = ++curr->irq_events; + debug_atomic_inc(hardirqs_off_events); } EXPORT_SYMBOL_GPL(lockdep_hardirqs_off); @@ -3769,6 +3754,9 @@ void lockdep_softirqs_on(unsigned long ip) if (unlikely(!debug_locks || current->lockdep_recursion)) return; + if (DEBUG_LOCKS_WARN_ON(curr->softirqs_enabled)) + return; + /* * We fancy IRQs being disabled here, see softirq.c, avoids * funny state and nesting things. @@ -3776,11 +3764,6 @@ void lockdep_softirqs_on(unsigned long ip) if (DEBUG_LOCKS_WARN_ON(!irqs_disabled())) return; - if (curr->softirqs_enabled) { - debug_atomic_inc(redundant_softirqs_on); - return; - } - current->lockdep_recursion++; /* * We'll do an OFF -> ON transition: @@ -3809,26 +3792,26 @@ void lockdep_softirqs_off(unsigned long ip) if (unlikely(!debug_locks || current->lockdep_recursion)) return; + if (DEBUG_LOCKS_WARN_ON(!curr->softirqs_enabled)) + return; + /* * We fancy IRQs being disabled here, see softirq.c */ if (DEBUG_LOCKS_WARN_ON(!irqs_disabled())) return; - if (curr->softirqs_enabled) { - /* -* We have done an ON -> OFF transition: -*/ - curr->softirqs_enabled = 0; - curr->softirq_disable_ip = ip; - curr->softirq_disable_event = ++curr->irq_events; - debug_atomic_inc(softirqs_off_events); - /* -* Whoops, we wanted softirqs off, so why aren't they? -*/ -