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?
-*/
-
