* [PATCH 1/6] x86/entry: Clarify irq_{enter,exit}_rcu()
2020-05-27 15:45 [PATCH 0/6] x86/entry,lockdep: Improve IRQ state tracking Peter Zijlstra
@ 2020-05-27 15:45 ` Peter Zijlstra
2020-05-27 15:45 ` [PATCH 2/6] lockdep: Change hardirq{s_enabled,_context} to per-cpu variables Peter Zijlstra
` (5 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Peter Zijlstra @ 2020-05-27 15:45 UTC (permalink / raw)
To: mingo, will, tglx; +Cc: x86, linux-kernel, a.darwish, rostedt, bigeasy, peterz
Because:
irq_enter_rcu() includes lockdep_hardirq_enter()
irq_exit_rcu() does *NOT* include lockdep_hardirq_exit()
Which resulted two 'stray' lockdep_hardirq_exit() calls in idtentry.h,
and me spending a long time trying to find the matching enters.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/include/asm/idtentry.h | 2 --
kernel/softirq.c | 19 +++++++++++++------
2 files changed, 13 insertions(+), 8 deletions(-)
--- a/arch/x86/include/asm/idtentry.h
+++ b/arch/x86/include/asm/idtentry.h
@@ -206,7 +206,6 @@ __visible noinstr void func(struct pt_re
kvm_set_cpu_l1tf_flush_l1d(); \
__##func (regs, (u8)error_code); \
irq_exit_rcu(); \
- lockdep_hardirq_exit(); \
instrumentation_end(); \
idtentry_exit_cond_rcu(regs, rcu_exit); \
} \
@@ -249,7 +248,6 @@ __visible noinstr void func(struct pt_re
kvm_set_cpu_l1tf_flush_l1d(); \
run_on_irqstack_cond(__##func, regs, regs); \
irq_exit_rcu(); \
- lockdep_hardirq_exit(); \
instrumentation_end(); \
idtentry_exit_cond_rcu(regs, rcu_exit); \
} \
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -404,12 +404,7 @@ static inline void tick_irq_exit(void)
#endif
}
-/**
- * irq_exit_rcu() - Exit an interrupt context without updating RCU
- *
- * Also processes softirqs if needed and possible.
- */
-void irq_exit_rcu(void)
+static inline void __irq_exit_rcu(void)
{
#ifndef __ARCH_IRQ_EXIT_IRQS_DISABLED
local_irq_disable();
@@ -425,6 +420,18 @@ void irq_exit_rcu(void)
}
/**
+ * irq_exit_rcu() - Exit an interrupt context without updating RCU
+ *
+ * Also processes softirqs if needed and possible.
+ */
+void irq_exit_rcu(void)
+{
+ __irq_exit_rcu();
+ /* must be last! */
+ lockdep_hardirq_exit();
+}
+
+/**
* irq_exit - Exit an interrupt context, update RCU and lockdep
*
* Also processes softirqs if needed and possible.
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 2/6] lockdep: Change hardirq{s_enabled,_context} to per-cpu variables
2020-05-27 15:45 [PATCH 0/6] x86/entry,lockdep: Improve IRQ state tracking Peter Zijlstra
2020-05-27 15:45 ` [PATCH 1/6] x86/entry: Clarify irq_{enter,exit}_rcu() Peter Zijlstra
@ 2020-05-27 15:45 ` Peter Zijlstra
2020-05-27 17:39 ` Peter Zijlstra
2020-05-27 15:45 ` [PATCH 3/6] lockdep: Remove lockdep_hardirq{s_enabled,_context}() argument Peter Zijlstra
` (4 subsequent siblings)
6 siblings, 1 reply; 9+ messages in thread
From: Peter Zijlstra @ 2020-05-27 15:45 UTC (permalink / raw)
To: mingo, will, tglx; +Cc: x86, linux-kernel, a.darwish, rostedt, bigeasy, peterz
Currently all IRQ-tracking state is in task_struct, this means that
task_struct needs to be defined before we use it.
Especially for lockdep_assert_irq*() this can lead to header-hell.
Move the hardirq state into per-cpu variables to avoid the task_struct
dependency.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
include/linux/irqflags.h | 19 ++++++++++++-------
include/linux/lockdep.h | 34 ++++++++++++++++++----------------
include/linux/sched.h | 2 --
kernel/fork.c | 4 +---
kernel/locking/lockdep.c | 30 +++++++++++++++---------------
kernel/softirq.c | 6 ++++++
6 files changed, 52 insertions(+), 43 deletions(-)
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -14,6 +14,7 @@
#include <linux/typecheck.h>
#include <asm/irqflags.h>
+#include <asm/percpu.h>
/* Currently lockdep_softirqs_on/off is used only by lockdep */
#ifdef CONFIG_PROVE_LOCKING
@@ -31,18 +32,22 @@
#endif
#ifdef CONFIG_TRACE_IRQFLAGS
+
+DECLARE_PER_CPU(int, hardirqs_enabled);
+DECLARE_PER_CPU(int, hardirq_context);
+
extern void trace_hardirqs_on_prepare(void);
extern void trace_hardirqs_off_prepare(void);
extern void trace_hardirqs_on(void);
extern void trace_hardirqs_off(void);
-# define lockdep_hardirq_context(p) ((p)->hardirq_context)
+# define lockdep_hardirq_context(p) (this_cpu_read(hardirq_context))
# define lockdep_softirq_context(p) ((p)->softirq_context)
-# define lockdep_hardirqs_enabled(p) ((p)->hardirqs_enabled)
+# define lockdep_hardirqs_enabled(p) (this_cpu_read(hardirqs_enabled))
# define lockdep_softirqs_enabled(p) ((p)->softirqs_enabled)
-# define lockdep_hardirq_enter() \
-do { \
- if (!current->hardirq_context++) \
- current->hardirq_threaded = 0; \
+# define lockdep_hardirq_enter() \
+do { \
+ if (!this_cpu_inc_return(hardirq_context)) \
+ current->hardirq_threaded = 0; \
} while (0)
# define lockdep_hardirq_threaded() \
do { \
@@ -50,7 +55,7 @@ do { \
} while (0)
# define lockdep_hardirq_exit() \
do { \
- current->hardirq_context--; \
+ this_cpu_dec(hardirq_context); \
} while (0)
# define lockdep_softirq_enter() \
do { \
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -20,6 +20,7 @@ extern int lock_stat;
#define MAX_LOCKDEP_SUBCLASSES 8UL
#include <linux/types.h>
+#include <asm/percpu.h>
enum lockdep_wait_type {
LD_WAIT_INV = 0, /* not checked, catch all */
@@ -703,28 +704,29 @@ do { \
lock_release(&(lock)->dep_map, _THIS_IP_); \
} while (0)
-#define lockdep_assert_irqs_enabled() do { \
- WARN_ONCE(debug_locks && !current->lockdep_recursion && \
- !current->hardirqs_enabled, \
- "IRQs not enabled as expected\n"); \
- } while (0)
+DECLARE_PER_CPU(int, hardirqs_enabled);
+DECLARE_PER_CPU(int, hardirq_context);
-#define lockdep_assert_irqs_disabled() do { \
- WARN_ONCE(debug_locks && !current->lockdep_recursion && \
- current->hardirqs_enabled, \
- "IRQs not disabled as expected\n"); \
- } while (0)
+#define lockdep_assert_irqs_enabled() \
+do { \
+ WARN_ON_ONCE(debug_locks && !this_cpu_read(hardirqs_enabled)); \
+} while (0)
-#define lockdep_assert_in_irq() do { \
- WARN_ONCE(debug_locks && !current->lockdep_recursion && \
- !current->hardirq_context, \
- "Not in hardirq as expected\n"); \
- } while (0)
+#define lockdep_assert_irqs_disabled() \
+do { \
+ WARN_ON_ONCE(debug_locks && this_cpu_read(hardirqs_enabled)); \
+} while (0)
+
+#define lockdep_assert_in_irq() \
+do { \
+ WARN_ON_ONCE(debug_locks && !this_cpu_read(hardirq_context)); \
+} while (0)
#else
# define might_lock(lock) do { } while (0)
# define might_lock_read(lock) do { } while (0)
# define might_lock_nested(lock, subclass) do { } while (0)
+
# define lockdep_assert_irqs_enabled() do { } while (0)
# define lockdep_assert_irqs_disabled() do { } while (0)
# define lockdep_assert_in_irq() do { } while (0)
@@ -734,7 +736,7 @@ do { \
# define lockdep_assert_RT_in_threaded_ctx() do { \
WARN_ONCE(debug_locks && !current->lockdep_recursion && \
- current->hardirq_context && \
+ lockdep_hardirq_context(current) && \
!(current->hardirq_threaded || current->irq_config), \
"Not in threaded context on PREEMPT_RT as expected\n"); \
} while (0)
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -991,8 +991,6 @@ struct task_struct {
unsigned long hardirq_disable_ip;
unsigned int hardirq_enable_event;
unsigned int hardirq_disable_event;
- int hardirqs_enabled;
- int hardirq_context;
u64 hardirq_chain_key;
unsigned long softirq_disable_ip;
unsigned long softirq_enable_ip;
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1946,8 +1946,8 @@ static __latent_entropy struct task_stru
rt_mutex_init_task(p);
+ lockdep_assert_irqs_enabled();
#ifdef CONFIG_PROVE_LOCKING
- DEBUG_LOCKS_WARN_ON(!p->hardirqs_enabled);
DEBUG_LOCKS_WARN_ON(!p->softirqs_enabled);
#endif
retval = -EAGAIN;
@@ -2028,7 +2028,6 @@ static __latent_entropy struct task_stru
#endif
#ifdef CONFIG_TRACE_IRQFLAGS
p->irq_events = 0;
- p->hardirqs_enabled = 0;
p->hardirq_enable_ip = 0;
p->hardirq_enable_event = 0;
p->hardirq_disable_ip = _THIS_IP_;
@@ -2038,7 +2037,6 @@ static __latent_entropy struct task_stru
p->softirq_enable_event = 0;
p->softirq_disable_ip = 0;
p->softirq_disable_event = 0;
- p->hardirq_context = 0;
p->softirq_context = 0;
#endif
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -2062,9 +2062,9 @@ print_bad_irq_dependency(struct task_str
pr_warn("-----------------------------------------------------\n");
pr_warn("%s/%d [HC%u[%lu]:SC%u[%lu]:HE%u:SE%u] is trying to acquire:\n",
curr->comm, task_pid_nr(curr),
- curr->hardirq_context, hardirq_count() >> HARDIRQ_SHIFT,
+ lockdep_hardirq_context(curr), hardirq_count() >> HARDIRQ_SHIFT,
curr->softirq_context, softirq_count() >> SOFTIRQ_SHIFT,
- curr->hardirqs_enabled,
+ lockdep_hardirqs_enabled(curr),
curr->softirqs_enabled);
print_lock(next);
@@ -3649,7 +3649,7 @@ void lockdep_hardirqs_on_prepare(unsigne
if (unlikely(!debug_locks || current->lockdep_recursion))
return;
- if (unlikely(current->hardirqs_enabled)) {
+ if (unlikely(lockdep_hardirqs_enabled(current))) {
/*
* Neither irq nor preemption are disabled here
* so this is racy by nature but losing one hit
@@ -3677,7 +3677,7 @@ void lockdep_hardirqs_on_prepare(unsigne
* Can't allow enabling interrupts while in an interrupt handler,
* that's general bad form and such. Recursion, limited stack etc..
*/
- if (DEBUG_LOCKS_WARN_ON(current->hardirq_context))
+ if (DEBUG_LOCKS_WARN_ON(lockdep_hardirq_context(current)))
return;
current->hardirq_chain_key = current->curr_chain_key;
@@ -3695,7 +3695,7 @@ void noinstr lockdep_hardirqs_on(unsigne
if (unlikely(!debug_locks || curr->lockdep_recursion))
return;
- if (curr->hardirqs_enabled) {
+ if (lockdep_hardirqs_enabled(curr)) {
/*
* Neither irq nor preemption are disabled here
* so this is racy by nature but losing one hit
@@ -3721,7 +3721,7 @@ void noinstr lockdep_hardirqs_on(unsigne
current->curr_chain_key);
/* we'll do an OFF -> ON transition: */
- curr->hardirqs_enabled = 1;
+ this_cpu_write(hardirqs_enabled, 1);
curr->hardirq_enable_ip = ip;
curr->hardirq_enable_event = ++curr->irq_events;
debug_atomic_inc(hardirqs_on_events);
@@ -3745,11 +3745,11 @@ void noinstr lockdep_hardirqs_off(unsign
if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
return;
- if (curr->hardirqs_enabled) {
+ if (lockdep_hardirqs_enabled(curr)) {
/*
* We have done an ON -> OFF transition:
*/
- curr->hardirqs_enabled = 0;
+ this_cpu_write(hardirqs_enabled, 0);
curr->hardirq_disable_ip = ip;
curr->hardirq_disable_event = ++curr->irq_events;
debug_atomic_inc(hardirqs_off_events);
@@ -3794,7 +3794,7 @@ void lockdep_softirqs_on(unsigned long i
* usage bit for all held locks, if hardirqs are
* enabled too:
*/
- if (curr->hardirqs_enabled)
+ if (lockdep_hardirqs_enabled(curr))
mark_held_locks(curr, LOCK_ENABLED_SOFTIRQ);
lockdep_recursion_finish();
}
@@ -3843,7 +3843,7 @@ mark_usage(struct task_struct *curr, str
*/
if (!hlock->trylock) {
if (hlock->read) {
- if (curr->hardirq_context)
+ if (lockdep_hardirq_context(curr))
if (!mark_lock(curr, hlock,
LOCK_USED_IN_HARDIRQ_READ))
return 0;
@@ -3852,7 +3852,7 @@ mark_usage(struct task_struct *curr, str
LOCK_USED_IN_SOFTIRQ_READ))
return 0;
} else {
- if (curr->hardirq_context)
+ if (lockdep_hardirq_context(curr))
if (!mark_lock(curr, hlock, LOCK_USED_IN_HARDIRQ))
return 0;
if (curr->softirq_context)
@@ -3890,7 +3890,7 @@ mark_usage(struct task_struct *curr, str
static inline unsigned int task_irq_context(struct task_struct *task)
{
- return LOCK_CHAIN_HARDIRQ_CONTEXT * !!task->hardirq_context +
+ return LOCK_CHAIN_HARDIRQ_CONTEXT * !!lockdep_hardirq_context(task) +
LOCK_CHAIN_SOFTIRQ_CONTEXT * !!task->softirq_context;
}
@@ -3983,7 +3983,7 @@ static inline short task_wait_context(st
* Set appropriate wait type for the context; for IRQs we have to take
* into account force_irqthread as that is implied by PREEMPT_RT.
*/
- if (curr->hardirq_context) {
+ if (lockdep_hardirq_context(curr)) {
/*
* Check if force_irqthreads will run us threaded.
*/
@@ -4826,11 +4826,11 @@ static void check_flags(unsigned long fl
return;
if (irqs_disabled_flags(flags)) {
- if (DEBUG_LOCKS_WARN_ON(current->hardirqs_enabled)) {
+ if (DEBUG_LOCKS_WARN_ON(lockdep_hardirqs_enabled(current))) {
printk("possible reason: unannotated irqs-off.\n");
}
} else {
- if (DEBUG_LOCKS_WARN_ON(!current->hardirqs_enabled)) {
+ if (DEBUG_LOCKS_WARN_ON(!lockdep_hardirqs_enabled(current))) {
printk("possible reason: unannotated irqs-on.\n");
}
}
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -107,6 +107,12 @@ static bool ksoftirqd_running(unsigned l
* where hardirqs are disabled legitimately:
*/
#ifdef CONFIG_TRACE_IRQFLAGS
+
+DEFINE_PER_CPU(int, hardirqs_enabled);
+DEFINE_PER_CPU(int, hardirq_context);
+EXPORT_PER_CPU_SYMBOL_GPL(hardirqs_enabled);
+EXPORT_PER_CPU_SYMBOL_GPL(hardirq_context);
+
void __local_bh_disable_ip(unsigned long ip, unsigned int cnt)
{
unsigned long flags;
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 2/6] lockdep: Change hardirq{s_enabled,_context} to per-cpu variables
2020-05-27 15:45 ` [PATCH 2/6] lockdep: Change hardirq{s_enabled,_context} to per-cpu variables Peter Zijlstra
@ 2020-05-27 17:39 ` Peter Zijlstra
0 siblings, 0 replies; 9+ messages in thread
From: Peter Zijlstra @ 2020-05-27 17:39 UTC (permalink / raw)
To: mingo, will, tglx; +Cc: x86, linux-kernel, a.darwish, rostedt, bigeasy
On Wed, May 27, 2020 at 05:45:29PM +0200, Peter Zijlstra wrote:
> -do { \
> - if (!current->hardirq_context++) \
> - current->hardirq_threaded = 0; \
> +# define lockdep_hardirq_enter() \
> +do { \
> + if (!this_cpu_inc_return(hardirq_context)) \
this_cpu_inc_return(hardirq_context) == 1
or this_cpu_fetch_inc(), which we don't have.
> + current->hardirq_threaded = 0; \
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/6] lockdep: Remove lockdep_hardirq{s_enabled,_context}() argument
2020-05-27 15:45 [PATCH 0/6] x86/entry,lockdep: Improve IRQ state tracking Peter Zijlstra
2020-05-27 15:45 ` [PATCH 1/6] x86/entry: Clarify irq_{enter,exit}_rcu() Peter Zijlstra
2020-05-27 15:45 ` [PATCH 2/6] lockdep: Change hardirq{s_enabled,_context} to per-cpu variables Peter Zijlstra
@ 2020-05-27 15:45 ` Peter Zijlstra
2020-05-27 15:45 ` [PATCH 4/6] x86/entry: Rename trace_hardirqs_off_prepare() Peter Zijlstra
` (3 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Peter Zijlstra @ 2020-05-27 15:45 UTC (permalink / raw)
To: mingo, will, tglx; +Cc: x86, linux-kernel, a.darwish, rostedt, bigeasy, peterz
Now that the functions use per-cpu data, we no longer need the macro
argument. Remove it.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
include/linux/irqflags.h | 8 ++++----
include/linux/lockdep.h | 2 +-
kernel/locking/lockdep.c | 30 +++++++++++++++---------------
kernel/softirq.c | 2 +-
tools/include/linux/irqflags.h | 4 ++--
5 files changed, 23 insertions(+), 23 deletions(-)
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -40,9 +40,9 @@ DECLARE_PER_CPU(int, hardirq_context);
extern void trace_hardirqs_off_prepare(void);
extern void trace_hardirqs_on(void);
extern void trace_hardirqs_off(void);
-# define lockdep_hardirq_context(p) (this_cpu_read(hardirq_context))
+# define lockdep_hardirq_context() (this_cpu_read(hardirq_context))
# define lockdep_softirq_context(p) ((p)->softirq_context)
-# define lockdep_hardirqs_enabled(p) (this_cpu_read(hardirqs_enabled))
+# define lockdep_hardirqs_enabled() (this_cpu_read(hardirqs_enabled))
# define lockdep_softirqs_enabled(p) ((p)->softirqs_enabled)
# define lockdep_hardirq_enter() \
do { \
@@ -109,9 +109,9 @@ do { \
# define trace_hardirqs_off_prepare() do { } while (0)
# define trace_hardirqs_on() do { } while (0)
# define trace_hardirqs_off() do { } while (0)
-# define lockdep_hardirq_context(p) 0
+# define lockdep_hardirq_context() 0
# define lockdep_softirq_context(p) 0
-# define lockdep_hardirqs_enabled(p) 0
+# define lockdep_hardirqs_enabled() 0
# define lockdep_softirqs_enabled(p) 0
# define lockdep_hardirq_enter() do { } while (0)
# define lockdep_hardirq_threaded() do { } while (0)
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -736,7 +736,7 @@ do { \
# define lockdep_assert_RT_in_threaded_ctx() do { \
WARN_ONCE(debug_locks && !current->lockdep_recursion && \
- lockdep_hardirq_context(current) && \
+ lockdep_hardirq_context() && \
!(current->hardirq_threaded || current->irq_config), \
"Not in threaded context on PREEMPT_RT as expected\n"); \
} while (0)
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -2062,9 +2062,9 @@ print_bad_irq_dependency(struct task_str
pr_warn("-----------------------------------------------------\n");
pr_warn("%s/%d [HC%u[%lu]:SC%u[%lu]:HE%u:SE%u] is trying to acquire:\n",
curr->comm, task_pid_nr(curr),
- lockdep_hardirq_context(curr), hardirq_count() >> HARDIRQ_SHIFT,
+ lockdep_hardirq_context(), hardirq_count() >> HARDIRQ_SHIFT,
curr->softirq_context, softirq_count() >> SOFTIRQ_SHIFT,
- lockdep_hardirqs_enabled(curr),
+ lockdep_hardirqs_enabled(),
curr->softirqs_enabled);
print_lock(next);
@@ -3331,9 +3331,9 @@ print_usage_bug(struct task_struct *curr
pr_warn("%s/%d [HC%u[%lu]:SC%u[%lu]:HE%u:SE%u] takes:\n",
curr->comm, task_pid_nr(curr),
- lockdep_hardirq_context(curr), hardirq_count() >> HARDIRQ_SHIFT,
+ lockdep_hardirq_context(), hardirq_count() >> HARDIRQ_SHIFT,
lockdep_softirq_context(curr), softirq_count() >> SOFTIRQ_SHIFT,
- lockdep_hardirqs_enabled(curr),
+ lockdep_hardirqs_enabled(),
lockdep_softirqs_enabled(curr));
print_lock(this);
@@ -3649,7 +3649,7 @@ void lockdep_hardirqs_on_prepare(unsigne
if (unlikely(!debug_locks || current->lockdep_recursion))
return;
- if (unlikely(lockdep_hardirqs_enabled(current))) {
+ if (unlikely(lockdep_hardirqs_enabled())) {
/*
* Neither irq nor preemption are disabled here
* so this is racy by nature but losing one hit
@@ -3677,7 +3677,7 @@ void lockdep_hardirqs_on_prepare(unsigne
* Can't allow enabling interrupts while in an interrupt handler,
* that's general bad form and such. Recursion, limited stack etc..
*/
- if (DEBUG_LOCKS_WARN_ON(lockdep_hardirq_context(current)))
+ if (DEBUG_LOCKS_WARN_ON(lockdep_hardirq_context()))
return;
current->hardirq_chain_key = current->curr_chain_key;
@@ -3695,7 +3695,7 @@ void noinstr lockdep_hardirqs_on(unsigne
if (unlikely(!debug_locks || curr->lockdep_recursion))
return;
- if (lockdep_hardirqs_enabled(curr)) {
+ if (lockdep_hardirqs_enabled()) {
/*
* Neither irq nor preemption are disabled here
* so this is racy by nature but losing one hit
@@ -3745,7 +3745,7 @@ void noinstr lockdep_hardirqs_off(unsign
if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
return;
- if (lockdep_hardirqs_enabled(curr)) {
+ if (lockdep_hardirqs_enabled()) {
/*
* We have done an ON -> OFF transition:
*/
@@ -3794,7 +3794,7 @@ void lockdep_softirqs_on(unsigned long i
* usage bit for all held locks, if hardirqs are
* enabled too:
*/
- if (lockdep_hardirqs_enabled(curr))
+ if (lockdep_hardirqs_enabled())
mark_held_locks(curr, LOCK_ENABLED_SOFTIRQ);
lockdep_recursion_finish();
}
@@ -3843,7 +3843,7 @@ mark_usage(struct task_struct *curr, str
*/
if (!hlock->trylock) {
if (hlock->read) {
- if (lockdep_hardirq_context(curr))
+ if (lockdep_hardirq_context())
if (!mark_lock(curr, hlock,
LOCK_USED_IN_HARDIRQ_READ))
return 0;
@@ -3852,7 +3852,7 @@ mark_usage(struct task_struct *curr, str
LOCK_USED_IN_SOFTIRQ_READ))
return 0;
} else {
- if (lockdep_hardirq_context(curr))
+ if (lockdep_hardirq_context())
if (!mark_lock(curr, hlock, LOCK_USED_IN_HARDIRQ))
return 0;
if (curr->softirq_context)
@@ -3890,7 +3890,7 @@ mark_usage(struct task_struct *curr, str
static inline unsigned int task_irq_context(struct task_struct *task)
{
- return LOCK_CHAIN_HARDIRQ_CONTEXT * !!lockdep_hardirq_context(task) +
+ return LOCK_CHAIN_HARDIRQ_CONTEXT * !!lockdep_hardirq_context() +
LOCK_CHAIN_SOFTIRQ_CONTEXT * !!task->softirq_context;
}
@@ -3983,7 +3983,7 @@ static inline short task_wait_context(st
* Set appropriate wait type for the context; for IRQs we have to take
* into account force_irqthread as that is implied by PREEMPT_RT.
*/
- if (lockdep_hardirq_context(curr)) {
+ if (lockdep_hardirq_context()) {
/*
* Check if force_irqthreads will run us threaded.
*/
@@ -4826,11 +4826,11 @@ static void check_flags(unsigned long fl
return;
if (irqs_disabled_flags(flags)) {
- if (DEBUG_LOCKS_WARN_ON(lockdep_hardirqs_enabled(current))) {
+ if (DEBUG_LOCKS_WARN_ON(lockdep_hardirqs_enabled())) {
printk("possible reason: unannotated irqs-off.\n");
}
} else {
- if (DEBUG_LOCKS_WARN_ON(!lockdep_hardirqs_enabled(current))) {
+ if (DEBUG_LOCKS_WARN_ON(!lockdep_hardirqs_enabled())) {
printk("possible reason: unannotated irqs-on.\n");
}
}
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -230,7 +230,7 @@ static inline bool lockdep_softirq_start
{
bool in_hardirq = false;
- if (lockdep_hardirq_context(current)) {
+ if (lockdep_hardirq_context()) {
in_hardirq = true;
lockdep_hardirq_exit();
}
--- a/tools/include/linux/irqflags.h
+++ b/tools/include/linux/irqflags.h
@@ -2,9 +2,9 @@
#ifndef _LIBLOCKDEP_LINUX_TRACE_IRQFLAGS_H_
#define _LIBLOCKDEP_LINUX_TRACE_IRQFLAGS_H_
-# define lockdep_hardirq_context(p) 0
+# define lockdep_hardirq_context() 0
# define lockdep_softirq_context(p) 0
-# define lockdep_hardirqs_enabled(p) 0
+# define lockdep_hardirqs_enabled() 0
# define lockdep_softirqs_enabled(p) 0
# define lockdep_hardirq_enter() do { } while (0)
# define lockdep_hardirq_exit() do { } while (0)
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 4/6] x86/entry: Rename trace_hardirqs_off_prepare()
2020-05-27 15:45 [PATCH 0/6] x86/entry,lockdep: Improve IRQ state tracking Peter Zijlstra
` (2 preceding siblings ...)
2020-05-27 15:45 ` [PATCH 3/6] lockdep: Remove lockdep_hardirq{s_enabled,_context}() argument Peter Zijlstra
@ 2020-05-27 15:45 ` Peter Zijlstra
2020-05-27 15:45 ` [PATCH 5/6] lockdep: Prepare for NMI IRQ state tracking Peter Zijlstra
` (2 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Peter Zijlstra @ 2020-05-27 15:45 UTC (permalink / raw)
To: mingo, will, tglx; +Cc: x86, linux-kernel, a.darwish, rostedt, bigeasy, peterz
The typical pattern for trace_hardirqs_off_prepare() is:
ENTRY
lockdep_hardirqs_off(); // because hardware
... do entry magic
instrumentation_begin();
trace_hardirqs_off_prepare();
... do actual work
trace_hardirqs_on_prepare();
lockdep_hardirqs_on_prepare();
instrumentation_end();
... do exit magic
lockdep_hardirqs_on();
which is shows that it's named wrong, rename it to
trace_hardirqs_off_finish(), as it concludes the hardirq_off
transition.
Also, given that the above is the only correct order, make the
traditional all-in-one trace_hardirqs_on() follow suit.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/entry/common.c | 6 +++---
arch/x86/kernel/cpu/mce/core.c | 2 +-
arch/x86/kernel/nmi.c | 2 +-
arch/x86/kernel/traps.c | 4 ++--
include/linux/irqflags.h | 4 ++--
kernel/trace/trace_preemptirq.c | 10 +++++-----
6 files changed, 14 insertions(+), 14 deletions(-)
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -65,7 +65,7 @@ static noinstr void enter_from_user_mode
instrumentation_begin();
CT_WARN_ON(state != CONTEXT_USER);
- trace_hardirqs_off_prepare();
+ trace_hardirqs_off_finish();
instrumentation_end();
}
#else
@@ -73,7 +73,7 @@ static __always_inline void enter_from_u
{
lockdep_hardirqs_off(CALLER_ADDR0);
instrumentation_begin();
- trace_hardirqs_off_prepare();
+ trace_hardirqs_off_finish();
instrumentation_end();
}
#endif
@@ -569,7 +569,7 @@ bool noinstr idtentry_enter_cond_rcu(str
lockdep_hardirqs_off(CALLER_ADDR0);
rcu_irq_enter();
instrumentation_begin();
- trace_hardirqs_off_prepare();
+ trace_hardirqs_off_finish();
instrumentation_end();
return true;
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1915,7 +1915,7 @@ static __always_inline void exc_machine_
* that out because it's an indirect call. Annotate it.
*/
instrumentation_begin();
- trace_hardirqs_off_prepare();
+ trace_hardirqs_off_finish();
machine_check_vector(regs);
if (regs->flags & X86_EFLAGS_IF)
trace_hardirqs_on_prepare();
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -330,7 +330,7 @@ static noinstr void default_do_nmi(struc
__this_cpu_write(last_nmi_rip, regs->ip);
instrumentation_begin();
- trace_hardirqs_off_prepare();
+ trace_hardirqs_off_finish();
handled = nmi_handle(NMI_LOCAL, regs);
__this_cpu_add(nmi_stats.normal, handled);
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -634,7 +634,7 @@ DEFINE_IDTENTRY_RAW(exc_int3)
} else {
nmi_enter();
instrumentation_begin();
- trace_hardirqs_off_prepare();
+ trace_hardirqs_off_finish();
if (!do_int3(regs))
die("int3", regs, 0);
if (regs->flags & X86_EFLAGS_IF)
@@ -854,7 +854,7 @@ static __always_inline void exc_debug_ke
{
nmi_enter();
instrumentation_begin();
- trace_hardirqs_off_prepare();
+ trace_hardirqs_off_finish();
instrumentation_end();
/*
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -37,7 +37,7 @@ DECLARE_PER_CPU(int, hardirqs_enabled);
DECLARE_PER_CPU(int, hardirq_context);
extern void trace_hardirqs_on_prepare(void);
- extern void trace_hardirqs_off_prepare(void);
+ extern void trace_hardirqs_off_finish(void);
extern void trace_hardirqs_on(void);
extern void trace_hardirqs_off(void);
# define lockdep_hardirq_context() (this_cpu_read(hardirq_context))
@@ -106,7 +106,7 @@ do { \
#else
# define trace_hardirqs_on_prepare() do { } while (0)
-# define trace_hardirqs_off_prepare() do { } while (0)
+# define trace_hardirqs_off_finish() do { } while (0)
# define trace_hardirqs_on() do { } while (0)
# define trace_hardirqs_off() do { } while (0)
# define lockdep_hardirq_context() 0
--- a/kernel/trace/trace_preemptirq.c
+++ b/kernel/trace/trace_preemptirq.c
@@ -58,7 +58,7 @@ NOKPROBE_SYMBOL(trace_hardirqs_on);
* and lockdep uses a staged approach which splits the lockdep hardirq
* tracking into a RCU on and a RCU off section.
*/
-void trace_hardirqs_off_prepare(void)
+void trace_hardirqs_off_finish(void)
{
if (!this_cpu_read(tracing_irq_cpu)) {
this_cpu_write(tracing_irq_cpu, 1);
@@ -68,19 +68,19 @@ void trace_hardirqs_off_prepare(void)
}
}
-EXPORT_SYMBOL(trace_hardirqs_off_prepare);
-NOKPROBE_SYMBOL(trace_hardirqs_off_prepare);
+EXPORT_SYMBOL(trace_hardirqs_off_finish);
+NOKPROBE_SYMBOL(trace_hardirqs_off_finish);
void trace_hardirqs_off(void)
{
+ lockdep_hardirqs_off(CALLER_ADDR0);
+
if (!this_cpu_read(tracing_irq_cpu)) {
this_cpu_write(tracing_irq_cpu, 1);
tracer_hardirqs_off(CALLER_ADDR0, CALLER_ADDR1);
if (!in_nmi())
trace_irq_disable_rcuidle(CALLER_ADDR0, CALLER_ADDR1);
}
-
- lockdep_hardirqs_off(CALLER_ADDR0);
}
EXPORT_SYMBOL(trace_hardirqs_off);
NOKPROBE_SYMBOL(trace_hardirqs_off);
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 5/6] lockdep: Prepare for NMI IRQ state tracking
2020-05-27 15:45 [PATCH 0/6] x86/entry,lockdep: Improve IRQ state tracking Peter Zijlstra
` (3 preceding siblings ...)
2020-05-27 15:45 ` [PATCH 4/6] x86/entry: Rename trace_hardirqs_off_prepare() Peter Zijlstra
@ 2020-05-27 15:45 ` Peter Zijlstra
2020-05-27 15:45 ` [PATCH 6/6] x86/entry: Fix NMI vs " Peter Zijlstra
2020-05-27 16:43 ` [PATCH 0/6] x86/entry,lockdep: Improve " Peter Zijlstra
6 siblings, 0 replies; 9+ messages in thread
From: Peter Zijlstra @ 2020-05-27 15:45 UTC (permalink / raw)
To: mingo, will, tglx; +Cc: x86, linux-kernel, a.darwish, rostedt, bigeasy, peterz
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 | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -3646,7 +3646,12 @@ static void __trace_hardirqs_on_caller(v
*/
void lockdep_hardirqs_on_prepare(unsigned long ip)
{
- if (unlikely(!debug_locks || current->lockdep_recursion))
+ /*
+ * Even though NMIs can happen while in lockdep; we only call this
+ * then the NMI returns to an IRQs enabled context, which guarantees
+ * we're not in lockdep.
+ */
+ if (DEBUG_LOCKS_WARN_ON(current->lockdep_recursion & LOCKDEP_RECURSION_MASK))
return;
if (unlikely(lockdep_hardirqs_enabled())) {
@@ -3682,9 +3687,11 @@ void lockdep_hardirqs_on_prepare(unsigne
current->hardirq_chain_key = current->curr_chain_key;
- current->lockdep_recursion++;
- __trace_hardirqs_on_caller();
- lockdep_recursion_finish();
+ if (!in_nmi()) {
+ current->lockdep_recursion++;
+ __trace_hardirqs_on_caller();
+ lockdep_recursion_finish();
+ }
}
EXPORT_SYMBOL_GPL(lockdep_hardirqs_on_prepare);
@@ -3692,7 +3699,7 @@ void noinstr lockdep_hardirqs_on(unsigne
{
struct task_struct *curr = current;
- if (unlikely(!debug_locks || curr->lockdep_recursion))
+ if (DEBUG_LOCKS_WARN_ON(curr->lockdep_recursion & LOCKDEP_RECURSION_MASK))
return;
if (lockdep_hardirqs_enabled()) {
@@ -3735,8 +3742,10 @@ void noinstr lockdep_hardirqs_off(unsign
{
struct task_struct *curr = current;
- if (unlikely(!debug_locks || curr->lockdep_recursion))
- return;
+ /*
+ * Can't test recursion; NMIs can happen while in lockdep.
+ * Harmless though; all we do is clear hardirqs_enabled.
+ */
/*
* So we're supposed to get called after you mask local IRQs, but for
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 6/6] x86/entry: Fix NMI vs IRQ state tracking
2020-05-27 15:45 [PATCH 0/6] x86/entry,lockdep: Improve IRQ state tracking Peter Zijlstra
` (4 preceding siblings ...)
2020-05-27 15:45 ` [PATCH 5/6] lockdep: Prepare for NMI IRQ state tracking Peter Zijlstra
@ 2020-05-27 15:45 ` Peter Zijlstra
2020-05-27 16:43 ` [PATCH 0/6] x86/entry,lockdep: Improve " Peter Zijlstra
6 siblings, 0 replies; 9+ messages in thread
From: Peter Zijlstra @ 2020-05-27 15:45 UTC (permalink / raw)
To: mingo, will, tglx; +Cc: x86, linux-kernel, a.darwish, rostedt, bigeasy, peterz
While the nmi_enter() users did
trace_hardirqs_{off_prepare,on_finish}() there was no matching
lockdep_hardirqs_*() calls to complete the picture.
Introduce idtentry_{enter,exit}_nmi() to enable proper IRQ state
tracking across the NMIs.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/entry/common.c | 32 ++++++++++++++++++++++++++++----
arch/x86/include/asm/idtentry.h | 3 +++
arch/x86/kernel/nmi.c | 7 ++-----
arch/x86/kernel/traps.c | 21 ++++++---------------
include/linux/hardirq.h | 22 ++++++++++++++++------
5 files changed, 55 insertions(+), 30 deletions(-)
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -550,7 +550,7 @@ SYSCALL_DEFINE0(ni_syscall)
* The return value must be fed into the rcu_exit argument of
* idtentry_exit_cond_rcu().
*/
-bool noinstr idtentry_enter_cond_rcu(struct pt_regs *regs)
+noinstr bool idtentry_enter_cond_rcu(struct pt_regs *regs)
{
if (user_mode(regs)) {
enter_from_user_mode();
@@ -619,7 +619,7 @@ static void idtentry_exit_cond_resched(s
* Counterpart to idtentry_enter_cond_rcu(). The return value of the entry
* function must be fed into the @rcu_exit argument.
*/
-void noinstr idtentry_exit_cond_rcu(struct pt_regs *regs, bool rcu_exit)
+noinstr void idtentry_exit_cond_rcu(struct pt_regs *regs, bool rcu_exit)
{
lockdep_assert_irqs_disabled();
@@ -663,7 +663,7 @@ void noinstr idtentry_exit_cond_rcu(stru
* Invokes enter_from_user_mode() to establish the proper context for
* NOHZ_FULL. Otherwise scheduling on exit would not be possible.
*/
-void noinstr idtentry_enter_user(struct pt_regs *regs)
+noinstr void idtentry_enter_user(struct pt_regs *regs)
{
enter_from_user_mode();
}
@@ -680,13 +680,37 @@ void noinstr idtentry_enter_user(struct
*
* Counterpart to idtentry_enter_user().
*/
-void noinstr idtentry_exit_user(struct pt_regs *regs)
+noinstr void idtentry_exit_user(struct pt_regs *regs)
{
lockdep_assert_irqs_disabled();
prepare_exit_to_usermode(regs);
}
+noinstr void idtentry_enter_nmi(struct pt_regs *regs)
+{
+ lockdep_hardirqs_off(CALLER_ADDR0);
+ __nmi_enter();
+ instrumentation_begin();
+ trace_hardirqs_off_finish();
+ instrumentation_end();
+ lockdep_hardirq_enter();
+}
+
+noinstr void idtentry_exit_nmi(struct pt_regs *regs)
+{
+ lockdep_hardirq_exit();
+ if (regs->flags & X86_EFLAGS_IF) {
+ instrumentation_begin();
+ trace_hardirqs_on_prepare();
+ lockdep_hardirqs_on_prepare(CALLER_ADDR0);
+ instrumentation_end();
+ }
+ __nmi_exit();
+ if (regs->flags & X86_EFLAGS_IF)
+ lockdep_hardirqs_on(CALLER_ADDR0);
+}
+
#ifdef CONFIG_XEN_PV
#ifndef CONFIG_PREEMPTION
/*
--- a/arch/x86/include/asm/idtentry.h
+++ b/arch/x86/include/asm/idtentry.h
@@ -16,6 +16,9 @@ void idtentry_exit_user(struct pt_regs *
bool idtentry_enter_cond_rcu(struct pt_regs *regs);
void idtentry_exit_cond_rcu(struct pt_regs *regs, bool rcu_exit);
+void idtentry_enter_nmi(struct pt_regs *regs);
+void idtentry_exit_nmi(struct pt_regs *regs);
+
/**
* DECLARE_IDTENTRY - Declare functions for simple IDT entry points
* No error code pushed by hardware
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -330,7 +330,6 @@ static noinstr void default_do_nmi(struc
__this_cpu_write(last_nmi_rip, regs->ip);
instrumentation_begin();
- trace_hardirqs_off_finish();
handled = nmi_handle(NMI_LOCAL, regs);
__this_cpu_add(nmi_stats.normal, handled);
@@ -417,8 +416,6 @@ static noinstr void default_do_nmi(struc
unknown_nmi_error(reason, regs);
out:
- if (regs->flags & X86_EFLAGS_IF)
- trace_hardirqs_on_prepare();
instrumentation_end();
}
@@ -535,14 +532,14 @@ DEFINE_IDTENTRY_NMI(exc_nmi)
}
#endif
- nmi_enter();
+ idtentry_enter_nmi(regs);
inc_irq_stat(__nmi_count);
if (!ignore_nmis)
default_do_nmi(regs);
- nmi_exit();
+ idtentry_exit_nmi(regs);
#ifdef CONFIG_X86_64
if (unlikely(this_cpu_read(update_debug_stack))) {
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -387,7 +387,8 @@ DEFINE_IDTENTRY_DF(exc_double_fault)
}
#endif
- nmi_enter();
+
+ idtentry_enter_nmi(regs);
instrumentation_begin();
notify_die(DIE_TRAP, str, regs, error_code, X86_TRAP_DF, SIGSEGV);
@@ -632,15 +633,12 @@ DEFINE_IDTENTRY_RAW(exc_int3)
instrumentation_end();
idtentry_exit_user(regs);
} else {
- nmi_enter();
+ idtentry_enter_nmi(regs);
instrumentation_begin();
- trace_hardirqs_off_finish();
if (!do_int3(regs))
die("int3", regs, 0);
- if (regs->flags & X86_EFLAGS_IF)
- trace_hardirqs_on_prepare();
instrumentation_end();
- nmi_exit();
+ idtentry_exit_nmi(regs);
}
}
@@ -852,10 +850,7 @@ static void noinstr handle_debug(struct
static __always_inline void exc_debug_kernel(struct pt_regs *regs,
unsigned long dr6)
{
- nmi_enter();
- instrumentation_begin();
- trace_hardirqs_off_finish();
- instrumentation_end();
+ idtentry_enter_nmi(regs);
/*
* The SDM says "The processor clears the BTF flag when it
@@ -878,11 +873,7 @@ static __always_inline void exc_debug_ke
if (dr6)
handle_debug(regs, dr6, false);
- instrumentation_begin();
- if (regs->flags & X86_EFLAGS_IF)
- trace_hardirqs_on_prepare();
- instrumentation_end();
- nmi_exit();
+ idtentry_exit_nmi(regs);
}
static __always_inline void exc_debug_user(struct pt_regs *regs,
--- a/include/linux/hardirq.h
+++ b/include/linux/hardirq.h
@@ -111,32 +111,42 @@ extern void rcu_nmi_exit(void);
/*
* nmi_enter() can nest up to 15 times; see NMI_BITS.
*/
-#define nmi_enter() \
+#define __nmi_enter() \
do { \
+ lockdep_off(); \
arch_nmi_enter(); \
printk_nmi_enter(); \
- lockdep_off(); \
BUG_ON(in_nmi() == NMI_MASK); \
__preempt_count_add(NMI_OFFSET + HARDIRQ_OFFSET); \
rcu_nmi_enter(); \
- lockdep_hardirq_enter(); \
instrumentation_begin(); \
ftrace_nmi_enter(); \
instrumentation_end(); \
} while (0)
-#define nmi_exit() \
+#define nmi_enter() \
+ do { \
+ lockdep_hardirq_enter(); \
+ __nmi_enter(); \
+ } while (0)
+
+#define __nmi_exit() \
do { \
instrumentation_begin(); \
ftrace_nmi_exit(); \
instrumentation_end(); \
- lockdep_hardirq_exit(); \
rcu_nmi_exit(); \
BUG_ON(!in_nmi()); \
__preempt_count_sub(NMI_OFFSET + HARDIRQ_OFFSET); \
- lockdep_on(); \
printk_nmi_exit(); \
arch_nmi_exit(); \
+ lockdep_on(); \
+ } while (0)
+
+#define nmi_exit() \
+ do { \
+ __nmi_exit(); \
+ lockdep_hardirq_exit(); \
} while (0)
#endif /* LINUX_HARDIRQ_H */
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 0/6] x86/entry,lockdep: Improve IRQ state tracking
2020-05-27 15:45 [PATCH 0/6] x86/entry,lockdep: Improve IRQ state tracking Peter Zijlstra
` (5 preceding siblings ...)
2020-05-27 15:45 ` [PATCH 6/6] x86/entry: Fix NMI vs " Peter Zijlstra
@ 2020-05-27 16:43 ` Peter Zijlstra
6 siblings, 0 replies; 9+ messages in thread
From: Peter Zijlstra @ 2020-05-27 16:43 UTC (permalink / raw)
To: mingo, will, tglx; +Cc: x86, linux-kernel, a.darwish, rostedt, bigeasy
On Wed, May 27, 2020 at 05:45:27PM +0200, Peter Zijlstra wrote:
> Ahmed and Sebastian wanted additional lockdep_assert*() macros and ran
> into header hell.
>
> Move the IRQ state into per-cpu variables, which removes the dependency on
> task_struct, which is what generated the header-hell.
>
> And fix IRQ state tracking to not be affected by lockdep_off() (it really
> should not have been anyway) and extends IRQ state tracking across (x86)
> NMIs.
After having it run kernel builds for a while, I've got a realy weird
(false-positive) lockdep_assert_irqs_enabled() trigger.
So something isn't quite right. I'll prod at it some moar.
^ permalink raw reply [flat|nested] 9+ messages in thread