* [PATCH v2 00/11] TRACE_IRQFLAGS wreckage
@ 2020-08-21 8:47 Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 01/11] lockdep: Use raw_cpu_*() for per-cpu variables Peter Zijlstra
` (12 more replies)
0 siblings, 13 replies; 53+ messages in thread
From: Peter Zijlstra @ 2020-08-21 8:47 UTC (permalink / raw)
To: linux-kernel, mingo, will
Cc: npiggin, elver, jgross, paulmck, rostedt, rjw, joel, svens, tglx, peterz
TRACE_IRQFLAGS
local_irq_*() keeps a software state that mirrors the hardware state,
used for lockdep, includes tracepoints.
raw_local_irq_*() does not update the software state, no tracing.
---
Problem 1:
raw_local_irq_save(); // software state on
local_irq_save(); // software state off
...
local_irq_restore(); // software state still off, because we don't enable IRQs
raw_local_irq_restore(); // software state still off, *whoopsie*
existing instances:
- lock_acquire()
raw_local_irq_save()
__lock_acquire()
arch_spin_lock(&graph_lock)
pv_wait() := kvm_wait() (same or worse for Xen/HyperV)
local_irq_save()
- trace_clock_global()
raw_local_irq_save()
arch_spin_lock()
pv_wait() := kvm_wait()
local_irq_save()
- apic_retrigger_irq()
raw_local_irq_save()
apic->send_IPI() := default_send_IPI_single_phys()
local_irq_save()
Possible solutions:
A) make it work by enabling the tracing inside raw_*()
B) make it work by keeping tracing disabled inside raw_*()
C) call it broken and clean it up now
Now, given that the only reason to use the raw_* variant is because you don't
want tracing. Therefore A) seems like a weird option (although it can be done).
C) is tempting, but OTOH it ends up converting a _lot_ of code to raw just
because there is one raw user, this strips the validation/tracing off for all
the other users.
So we pick B) and declare any code that ends up doing:
raw_local_irq_save()
local_irq_save()
lockdep_assert_irqs_disabled();
broken. AFAICT this problem has existed forever, the only reason it came
up is because I changed IRQ tracing vs lockdep recursion and the first
instance is fairly common, the other cases hardly ever happen.
---
Problem 2:
raw_local_irq_save(); // software state on
trace_*()
...
perf_tp_event()
...
perf_callchain()
<#PF>
trace_hardirqs_off(); // software state off
...
if (regs_irqs_disabled(regs)) // false
trace_hardirqs_on();
</#PF>
raw_local_irq_restore(); // software state stays off, *whoopsie*
existing instances:
- lock_acquire() / lock_release()
raw_local_irq_save()
trace_lock_acquire() / trace_lock_release()
- function tracing
Possible solutions:
A) fix every architecture's entry code
B) only fix kernel/entry/common.c
C) fix lockdep tracepoints and pray
This series does C, AFAICT this problem has existed forever.
---
Problem 3:
raw_local_irq_save(); // software state on
<#NMI>
trace_hardirqs_off(); // software state off
...
if (regs_irqs_disabled(regs)) // false
trace_hardirqs_on();
</#NMI>
raw_local_irq_restore(); // software state stays off, *whoopsie*
Possible solutions:
This *should* not be a problem if an architecture has it's entry ordering
right. In particular we rely on the architecture doing nmi_enter() before
trace_hardirqs_off().
In that case, in_nmi() will be true, and lockdep_hardirqs_*() should NO-OP,
except if CONFIG_TRACE_IRQFLAGS_NMI (x86).
There might be a problem with using lockdep_assert_irqs_disabled() from NMI
context, if so, those needs a little TLC.
---
The patches in this series do (in reverse order):
- 2C
- 1B
- fix fallout in idle due to the trace_lock_*() tracepoints suddenly
being visible to rcu-lockdep.
---
Change since -v1:
- typo (rostedt)
- split WARN (rostedt)
- reorder start_critical_section / rcu_idle_enter (rostedt)
- added arm64 patch (kernel test robot)
---
arch/arm/mach-omap2/pm34xx.c | 4 --
arch/arm64/include/asm/irqflags.h | 5 ++
arch/arm64/kernel/process.c | 2 -
arch/nds32/include/asm/irqflags.h | 5 ++
arch/powerpc/include/asm/hw_irq.h | 11 ++---
arch/s390/kernel/idle.c | 3 -
arch/x86/entry/thunk_32.S | 5 --
arch/x86/include/asm/mmu.h | 1
arch/x86/kernel/process.c | 4 --
arch/x86/mm/tlb.c | 13 +-----
drivers/cpuidle/cpuidle.c | 19 +++++++--
drivers/idle/intel_idle.c | 16 --------
include/linux/cpuidle.h | 13 +++---
include/linux/irqflags.h | 73 ++++++++++++++++++++------------------
include/linux/lockdep.h | 18 ++++++---
include/linux/mmu_context.h | 5 ++
kernel/locking/lockdep.c | 18 +++++----
kernel/sched/idle.c | 25 +++++--------
18 files changed, 118 insertions(+), 122 deletions(-)
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v2 01/11] lockdep: Use raw_cpu_*() for per-cpu variables
2020-08-21 8:47 [PATCH v2 00/11] TRACE_IRQFLAGS wreckage Peter Zijlstra
@ 2020-08-21 8:47 ` Peter Zijlstra
2020-08-27 1:05 ` Joel Fernandes
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 02/11] cpuidle: Fixup IRQ state Peter Zijlstra
` (11 subsequent siblings)
12 siblings, 2 replies; 53+ messages in thread
From: Peter Zijlstra @ 2020-08-21 8:47 UTC (permalink / raw)
To: linux-kernel, mingo, will
Cc: npiggin, elver, jgross, paulmck, rostedt, rjw, joel, svens, tglx, peterz
Sven reported that commit a21ee6055c30 ("lockdep: Change
hardirq{s_enabled,_context} to per-cpu variables") caused trouble on
s390 because their this_cpu_*() primitives disable preemption which
then lands back tracing.
On the one hand, per-cpu ops should use preempt_*able_notrace() and
raw_local_irq_*(), on the other hand, we can trivialy use raw_cpu_*()
ops for this.
Fixes: a21ee6055c30 ("lockdep: Change hardirq{s_enabled,_context} to per-cpu variables")
Reported-by: Sven Schnelle <svens@linux.ibm.com>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Marco Elver <elver@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
include/linux/irqflags.h | 6 +++---
include/linux/lockdep.h | 18 +++++++++++++-----
kernel/locking/lockdep.c | 4 ++--
3 files changed, 18 insertions(+), 10 deletions(-)
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -53,13 +53,13 @@ DECLARE_PER_CPU(int, hardirq_context);
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))
+# define lockdep_hardirq_context() (raw_cpu_read(hardirq_context))
# define lockdep_softirq_context(p) ((p)->softirq_context)
# define lockdep_hardirqs_enabled() (this_cpu_read(hardirqs_enabled))
# define lockdep_softirqs_enabled(p) ((p)->softirqs_enabled)
# define lockdep_hardirq_enter() \
do { \
- if (this_cpu_inc_return(hardirq_context) == 1) \
+ if (__this_cpu_inc_return(hardirq_context) == 1)\
current->hardirq_threaded = 0; \
} while (0)
# define lockdep_hardirq_threaded() \
@@ -68,7 +68,7 @@ do { \
} while (0)
# define lockdep_hardirq_exit() \
do { \
- this_cpu_dec(hardirq_context); \
+ __this_cpu_dec(hardirq_context); \
} while (0)
# define lockdep_softirq_enter() \
do { \
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -535,19 +535,27 @@ do { \
DECLARE_PER_CPU(int, hardirqs_enabled);
DECLARE_PER_CPU(int, hardirq_context);
+/*
+ * The below lockdep_assert_*() macros use raw_cpu_read() to access the above
+ * per-cpu variables. This is required because this_cpu_read() will potentially
+ * call into preempt/irq-disable and that obviously isn't right. This is also
+ * correct because when IRQs are enabled, it doesn't matter if we accidentally
+ * read the value from our previous CPU.
+ */
+
#define lockdep_assert_irqs_enabled() \
do { \
- WARN_ON_ONCE(debug_locks && !this_cpu_read(hardirqs_enabled)); \
+ WARN_ON_ONCE(debug_locks && !raw_cpu_read(hardirqs_enabled)); \
} while (0)
#define lockdep_assert_irqs_disabled() \
do { \
- WARN_ON_ONCE(debug_locks && this_cpu_read(hardirqs_enabled)); \
+ WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled)); \
} while (0)
#define lockdep_assert_in_irq() \
do { \
- WARN_ON_ONCE(debug_locks && !this_cpu_read(hardirq_context)); \
+ WARN_ON_ONCE(debug_locks && !raw_cpu_read(hardirq_context)); \
} while (0)
#define lockdep_assert_preemption_enabled() \
@@ -555,7 +563,7 @@ do { \
WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_COUNT) && \
debug_locks && \
(preempt_count() != 0 || \
- !this_cpu_read(hardirqs_enabled))); \
+ !raw_cpu_read(hardirqs_enabled))); \
} while (0)
#define lockdep_assert_preemption_disabled() \
@@ -563,7 +571,7 @@ do { \
WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_COUNT) && \
debug_locks && \
(preempt_count() == 0 && \
- this_cpu_read(hardirqs_enabled))); \
+ raw_cpu_read(hardirqs_enabled))); \
} while (0)
#else
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -3756,7 +3756,7 @@ void noinstr lockdep_hardirqs_on(unsigne
skip_checks:
/* we'll do an OFF -> ON transition: */
- this_cpu_write(hardirqs_enabled, 1);
+ __this_cpu_write(hardirqs_enabled, 1);
trace->hardirq_enable_ip = ip;
trace->hardirq_enable_event = ++trace->irq_events;
debug_atomic_inc(hardirqs_on_events);
@@ -3795,7 +3795,7 @@ void noinstr lockdep_hardirqs_off(unsign
/*
* We have done an ON -> OFF transition:
*/
- this_cpu_write(hardirqs_enabled, 0);
+ __this_cpu_write(hardirqs_enabled, 0);
trace->hardirq_disable_ip = ip;
trace->hardirq_disable_event = ++trace->irq_events;
debug_atomic_inc(hardirqs_off_events);
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v2 02/11] cpuidle: Fixup IRQ state
2020-08-21 8:47 [PATCH v2 00/11] TRACE_IRQFLAGS wreckage Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 01/11] lockdep: Use raw_cpu_*() for per-cpu variables Peter Zijlstra
@ 2020-08-21 8:47 ` Peter Zijlstra
2020-08-21 17:36 ` Rafael J. Wysocki
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 03/11] sched,idle,rcu: Push rcu_idle deeper into the idle path Peter Zijlstra
` (10 subsequent siblings)
12 siblings, 2 replies; 53+ messages in thread
From: Peter Zijlstra @ 2020-08-21 8:47 UTC (permalink / raw)
To: linux-kernel, mingo, will
Cc: npiggin, elver, jgross, paulmck, rostedt, rjw, joel, svens, tglx, peterz
Match the pattern elsewhere in this file.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Marco Elver <elver@google.com>
---
drivers/cpuidle/cpuidle.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -153,7 +153,8 @@ static void enter_s2idle_proper(struct c
*/
stop_critical_timings();
drv->states[index].enter_s2idle(dev, drv, index);
- WARN_ON(!irqs_disabled());
+ if (WARN_ON_ONCE(!irqs_disabled()))
+ local_irq_disable();
/*
* timekeeping_resume() that will be called by tick_unfreeze() for the
* first CPU executing it calls functions containing RCU read-side
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v2 03/11] sched,idle,rcu: Push rcu_idle deeper into the idle path
2020-08-21 8:47 [PATCH v2 00/11] TRACE_IRQFLAGS wreckage Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 01/11] lockdep: Use raw_cpu_*() for per-cpu variables Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 02/11] cpuidle: Fixup IRQ state Peter Zijlstra
@ 2020-08-21 8:47 ` Peter Zijlstra
2020-08-27 1:18 ` Joel Fernandes
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 04/11] cpuidle: Make CPUIDLE_FLAG_TLB_FLUSHED generic Peter Zijlstra
` (9 subsequent siblings)
12 siblings, 2 replies; 53+ messages in thread
From: Peter Zijlstra @ 2020-08-21 8:47 UTC (permalink / raw)
To: linux-kernel, mingo, will
Cc: npiggin, elver, jgross, paulmck, rostedt, rjw, joel, svens, tglx, peterz
Lots of things take locks, due to a wee bug, rcu_lockdep didn't notice
that the locking tracepoints were using RCU.
Push rcu_idle_{enter,exit}() as deep as possible into the idle paths,
this also resolves a lot of _rcuidle()/RCU_NONIDLE() usage.
Specifically, sched_clock_idle_wakeup_event() will use ktime which
will use seqlocks which will tickle lockdep, and
stop_critical_timings() uses lock.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Marco Elver <elver@google.com>
---
drivers/cpuidle/cpuidle.c | 12 ++++++++----
kernel/sched/idle.c | 22 ++++++++--------------
2 files changed, 16 insertions(+), 18 deletions(-)
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -145,13 +145,14 @@ static void enter_s2idle_proper(struct c
* executing it contains RCU usage regarded as invalid in the idle
* context, so tell RCU about that.
*/
- RCU_NONIDLE(tick_freeze());
+ tick_freeze();
/*
* The state used here cannot be a "coupled" one, because the "coupled"
* cpuidle mechanism enables interrupts and doing that with timekeeping
* suspended is generally unsafe.
*/
stop_critical_timings();
+ rcu_idle_enter();
drv->states[index].enter_s2idle(dev, drv, index);
if (WARN_ON_ONCE(!irqs_disabled()))
local_irq_disable();
@@ -160,7 +161,8 @@ static void enter_s2idle_proper(struct c
* first CPU executing it calls functions containing RCU read-side
* critical sections, so tell RCU about that.
*/
- RCU_NONIDLE(tick_unfreeze());
+ rcu_idle_exit();
+ tick_unfreeze();
start_critical_timings();
time_end = ns_to_ktime(local_clock());
@@ -229,16 +231,18 @@ int cpuidle_enter_state(struct cpuidle_d
/* Take note of the planned idle state. */
sched_idle_set_state(target_state);
- trace_cpu_idle_rcuidle(index, dev->cpu);
+ trace_cpu_idle(index, dev->cpu);
time_start = ns_to_ktime(local_clock());
stop_critical_timings();
+ rcu_idle_enter();
entered_state = target_state->enter(dev, drv, index);
+ rcu_idle_exit();
start_critical_timings();
sched_clock_idle_wakeup_event();
time_end = ns_to_ktime(local_clock());
- trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
+ trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
/* The cpu is no longer idle or about to enter idle. */
sched_idle_set_state(NULL);
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -54,17 +54,18 @@ __setup("hlt", cpu_idle_nopoll_setup);
static noinline int __cpuidle cpu_idle_poll(void)
{
+ trace_cpu_idle(0, smp_processor_id());
+ stop_critical_timings();
rcu_idle_enter();
- trace_cpu_idle_rcuidle(0, smp_processor_id());
local_irq_enable();
- stop_critical_timings();
while (!tif_need_resched() &&
- (cpu_idle_force_poll || tick_check_broadcast_expired()))
+ (cpu_idle_force_poll || tick_check_broadcast_expired()))
cpu_relax();
- start_critical_timings();
- trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
+
rcu_idle_exit();
+ start_critical_timings();
+ trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
return 1;
}
@@ -91,7 +92,9 @@ void __cpuidle default_idle_call(void)
local_irq_enable();
} else {
stop_critical_timings();
+ rcu_idle_enter();
arch_cpu_idle();
+ rcu_idle_exit();
start_critical_timings();
}
}
@@ -158,7 +161,6 @@ static void cpuidle_idle_call(void)
if (cpuidle_not_available(drv, dev)) {
tick_nohz_idle_stop_tick();
- rcu_idle_enter();
default_idle_call();
goto exit_idle;
@@ -178,21 +180,17 @@ static void cpuidle_idle_call(void)
u64 max_latency_ns;
if (idle_should_enter_s2idle()) {
- rcu_idle_enter();
entered_state = call_cpuidle_s2idle(drv, dev);
if (entered_state > 0)
goto exit_idle;
- rcu_idle_exit();
-
max_latency_ns = U64_MAX;
} else {
max_latency_ns = dev->forced_idle_latency_limit_ns;
}
tick_nohz_idle_stop_tick();
- rcu_idle_enter();
next_state = cpuidle_find_deepest_state(drv, dev, max_latency_ns);
call_cpuidle(drv, dev, next_state);
@@ -209,8 +207,6 @@ static void cpuidle_idle_call(void)
else
tick_nohz_idle_retain_tick();
- rcu_idle_enter();
-
entered_state = call_cpuidle(drv, dev, next_state);
/*
* Give the governor an opportunity to reflect on the outcome
@@ -226,8 +222,6 @@ static void cpuidle_idle_call(void)
*/
if (WARN_ON_ONCE(irqs_disabled()))
local_irq_enable();
-
- rcu_idle_exit();
}
/*
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v2 04/11] cpuidle: Make CPUIDLE_FLAG_TLB_FLUSHED generic
2020-08-21 8:47 [PATCH v2 00/11] TRACE_IRQFLAGS wreckage Peter Zijlstra
` (2 preceding siblings ...)
2020-08-21 8:47 ` [PATCH v2 03/11] sched,idle,rcu: Push rcu_idle deeper into the idle path Peter Zijlstra
@ 2020-08-21 8:47 ` Peter Zijlstra
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 05/11] cpuidle: Move trace_cpu_idle() into generic code Peter Zijlstra
` (8 subsequent siblings)
12 siblings, 1 reply; 53+ messages in thread
From: Peter Zijlstra @ 2020-08-21 8:47 UTC (permalink / raw)
To: linux-kernel, mingo, will
Cc: npiggin, elver, jgross, paulmck, rostedt, rjw, joel, svens, tglx, peterz
This allows moving the leave_mm() call into generic code before
rcu_idle_enter(). Gets rid of more trace_*_rcuidle() users.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Marco Elver <elver@google.com>
---
arch/x86/include/asm/mmu.h | 1 +
arch/x86/mm/tlb.c | 13 ++-----------
drivers/cpuidle/cpuidle.c | 4 ++++
drivers/idle/intel_idle.c | 16 ----------------
include/linux/cpuidle.h | 13 +++++++------
include/linux/mmu_context.h | 5 +++++
6 files changed, 19 insertions(+), 33 deletions(-)
--- a/arch/x86/include/asm/mmu.h
+++ b/arch/x86/include/asm/mmu.h
@@ -59,5 +59,6 @@ typedef struct {
}
void leave_mm(int cpu);
+#define leave_mm leave_mm
#endif /* _ASM_X86_MMU_H */
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -555,21 +555,12 @@ void switch_mm_irqs_off(struct mm_struct
this_cpu_write(cpu_tlbstate.ctxs[new_asid].tlb_gen, next_tlb_gen);
load_new_mm_cr3(next->pgd, new_asid, true);
- /*
- * NB: This gets called via leave_mm() in the idle path
- * where RCU functions differently. Tracing normally
- * uses RCU, so we need to use the _rcuidle variant.
- *
- * (There is no good reason for this. The idle code should
- * be rearranged to call this before rcu_idle_enter().)
- */
- trace_tlb_flush_rcuidle(TLB_FLUSH_ON_TASK_SWITCH, TLB_FLUSH_ALL);
+ trace_tlb_flush(TLB_FLUSH_ON_TASK_SWITCH, TLB_FLUSH_ALL);
} else {
/* The new ASID is already up to date. */
load_new_mm_cr3(next->pgd, new_asid, false);
- /* See above wrt _rcuidle. */
- trace_tlb_flush_rcuidle(TLB_FLUSH_ON_TASK_SWITCH, 0);
+ trace_tlb_flush(TLB_FLUSH_ON_TASK_SWITCH, 0);
}
/* Make sure we write CR3 before loaded_mm. */
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -22,6 +22,7 @@
#include <linux/module.h>
#include <linux/suspend.h>
#include <linux/tick.h>
+#include <linux/mmu_context.h>
#include <trace/events/power.h>
#include "cpuidle.h"
@@ -230,6 +231,9 @@ int cpuidle_enter_state(struct cpuidle_d
broadcast = false;
}
+ if (target_state->flags & CPUIDLE_FLAG_TLB_FLUSHED)
+ leave_mm(dev->cpu);
+
/* Take note of the planned idle state. */
sched_idle_set_state(target_state);
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -90,14 +90,6 @@ static unsigned int mwait_substates __in
#define CPUIDLE_FLAG_ALWAYS_ENABLE BIT(15)
/*
- * Set this flag for states where the HW flushes the TLB for us
- * and so we don't need cross-calls to keep it consistent.
- * If this flag is set, SW flushes the TLB, so even if the
- * HW doesn't do the flushing, this flag is safe to use.
- */
-#define CPUIDLE_FLAG_TLB_FLUSHED BIT(16)
-
-/*
* MWAIT takes an 8-bit "hint" in EAX "suggesting"
* the C-state (top nibble) and sub-state (bottom nibble)
* 0x00 means "MWAIT(C1)", 0x10 means "MWAIT(C2)" etc.
@@ -131,14 +123,6 @@ static __cpuidle int intel_idle(struct c
unsigned long eax = flg2MWAIT(state->flags);
unsigned long ecx = 1; /* break on interrupt flag */
bool tick;
- int cpu = smp_processor_id();
-
- /*
- * leave_mm() to avoid costly and often unnecessary wakeups
- * for flushing the user TLB's associated with the active mm.
- */
- if (state->flags & CPUIDLE_FLAG_TLB_FLUSHED)
- leave_mm(cpu);
if (!static_cpu_has(X86_FEATURE_ARAT)) {
/*
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -75,12 +75,13 @@ struct cpuidle_state {
};
/* Idle State Flags */
-#define CPUIDLE_FLAG_NONE (0x00)
-#define CPUIDLE_FLAG_POLLING BIT(0) /* polling state */
-#define CPUIDLE_FLAG_COUPLED BIT(1) /* state applies to multiple cpus */
-#define CPUIDLE_FLAG_TIMER_STOP BIT(2) /* timer is stopped on this state */
-#define CPUIDLE_FLAG_UNUSABLE BIT(3) /* avoid using this state */
-#define CPUIDLE_FLAG_OFF BIT(4) /* disable this state by default */
+#define CPUIDLE_FLAG_NONE (0x00)
+#define CPUIDLE_FLAG_POLLING BIT(0) /* polling state */
+#define CPUIDLE_FLAG_COUPLED BIT(1) /* state applies to multiple cpus */
+#define CPUIDLE_FLAG_TIMER_STOP BIT(2) /* timer is stopped on this state */
+#define CPUIDLE_FLAG_UNUSABLE BIT(3) /* avoid using this state */
+#define CPUIDLE_FLAG_OFF BIT(4) /* disable this state by default */
+#define CPUIDLE_FLAG_TLB_FLUSHED BIT(5) /* idle-state flushes TLBs */
struct cpuidle_device_kobj;
struct cpuidle_state_kobj;
--- a/include/linux/mmu_context.h
+++ b/include/linux/mmu_context.h
@@ -3,10 +3,15 @@
#define _LINUX_MMU_CONTEXT_H
#include <asm/mmu_context.h>
+#include <asm/mmu.h>
/* Architectures that care about IRQ state in switch_mm can override this. */
#ifndef switch_mm_irqs_off
# define switch_mm_irqs_off switch_mm
#endif
+#ifndef leave_mm
+static inline void leave_mm(int cpu) { }
+#endif
+
#endif
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v2 05/11] cpuidle: Move trace_cpu_idle() into generic code
2020-08-21 8:47 [PATCH v2 00/11] TRACE_IRQFLAGS wreckage Peter Zijlstra
` (3 preceding siblings ...)
2020-08-21 8:47 ` [PATCH v2 04/11] cpuidle: Make CPUIDLE_FLAG_TLB_FLUSHED generic Peter Zijlstra
@ 2020-08-21 8:47 ` Peter Zijlstra
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 06/11] x86/entry: Remove unused THUNKs Peter Zijlstra
` (7 subsequent siblings)
12 siblings, 1 reply; 53+ messages in thread
From: Peter Zijlstra @ 2020-08-21 8:47 UTC (permalink / raw)
To: linux-kernel, mingo, will
Cc: npiggin, elver, jgross, paulmck, rostedt, rjw, joel, svens, tglx, peterz
Remove trace_cpu_idle() from the arch_cpu_idle() implementations and
put it in the generic code, right before disabling RCU. Gets rid of
more trace_*_rcuidle() users.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Marco Elver <elver@google.com>
---
arch/arm/mach-omap2/pm34xx.c | 4 ----
arch/arm64/kernel/process.c | 2 --
arch/s390/kernel/idle.c | 3 +--
arch/x86/kernel/process.c | 4 ----
kernel/sched/idle.c | 3 +++
5 files changed, 4 insertions(+), 12 deletions(-)
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -298,11 +298,7 @@ static void omap3_pm_idle(void)
if (omap_irq_pending())
return;
- trace_cpu_idle_rcuidle(1, smp_processor_id());
-
omap_sram_idle();
-
- trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
}
#ifdef CONFIG_SUSPEND
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -123,10 +123,8 @@ void arch_cpu_idle(void)
* This should do all the clock switching and wait for interrupt
* tricks
*/
- trace_cpu_idle_rcuidle(1, smp_processor_id());
cpu_do_idle();
local_irq_enable();
- trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
}
#ifdef CONFIG_HOTPLUG_CPU
--- a/arch/s390/kernel/idle.c
+++ b/arch/s390/kernel/idle.c
@@ -33,14 +33,13 @@ void enabled_wait(void)
PSW_MASK_IO | PSW_MASK_EXT | PSW_MASK_MCHECK;
clear_cpu_flag(CIF_NOHZ_DELAY);
- trace_cpu_idle_rcuidle(1, smp_processor_id());
local_irq_save(flags);
/* Call the assembler magic in entry.S */
psw_idle(idle, psw_mask);
local_irq_restore(flags);
- trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
/* Account time spent with enabled wait psw loaded as idle time. */
+ /* XXX seqcount has tracepoints that require RCU */
write_seqcount_begin(&idle->seqcount);
idle_time = idle->clock_idle_exit - idle->clock_idle_enter;
idle->clock_idle_enter = idle->clock_idle_exit = 0ULL;
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -684,9 +684,7 @@ void arch_cpu_idle(void)
*/
void __cpuidle default_idle(void)
{
- trace_cpu_idle_rcuidle(1, smp_processor_id());
safe_halt();
- trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
}
#if defined(CONFIG_APM_MODULE) || defined(CONFIG_HALTPOLL_CPUIDLE_MODULE)
EXPORT_SYMBOL(default_idle);
@@ -792,7 +790,6 @@ static int prefer_mwait_c1_over_halt(con
static __cpuidle void mwait_idle(void)
{
if (!current_set_polling_and_test()) {
- trace_cpu_idle_rcuidle(1, smp_processor_id());
if (this_cpu_has(X86_BUG_CLFLUSH_MONITOR)) {
mb(); /* quirk */
clflush((void *)¤t_thread_info()->flags);
@@ -804,7 +801,6 @@ static __cpuidle void mwait_idle(void)
__sti_mwait(0, 0);
else
local_irq_enable();
- trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
} else {
local_irq_enable();
}
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -91,11 +91,14 @@ void __cpuidle default_idle_call(void)
if (current_clr_polling_and_test()) {
local_irq_enable();
} else {
+
+ trace_cpu_idle(1, smp_processor_id());
stop_critical_timings();
rcu_idle_enter();
arch_cpu_idle();
rcu_idle_exit();
start_critical_timings();
+ trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
}
}
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v2 06/11] x86/entry: Remove unused THUNKs
2020-08-21 8:47 [PATCH v2 00/11] TRACE_IRQFLAGS wreckage Peter Zijlstra
` (4 preceding siblings ...)
2020-08-21 8:47 ` [PATCH v2 05/11] cpuidle: Move trace_cpu_idle() into generic code Peter Zijlstra
@ 2020-08-21 8:47 ` Peter Zijlstra
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 07/11] locking/lockdep: Cleanup Peter Zijlstra
` (6 subsequent siblings)
12 siblings, 1 reply; 53+ messages in thread
From: Peter Zijlstra @ 2020-08-21 8:47 UTC (permalink / raw)
To: linux-kernel, mingo, will
Cc: npiggin, elver, jgross, paulmck, rostedt, rjw, joel, svens, tglx, peterz
Unused remnants
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Marco Elver <elver@google.com>
---
--- a/arch/x86/entry/thunk_32.S
+++ b/arch/x86/entry/thunk_32.S
@@ -29,11 +29,6 @@ SYM_CODE_START_NOALIGN(\name)
SYM_CODE_END(\name)
.endm
-#ifdef CONFIG_TRACE_IRQFLAGS
- THUNK trace_hardirqs_on_thunk,trace_hardirqs_on_caller,1
- THUNK trace_hardirqs_off_thunk,trace_hardirqs_off_caller,1
-#endif
-
#ifdef CONFIG_PREEMPTION
THUNK preempt_schedule_thunk, preempt_schedule
THUNK preempt_schedule_notrace_thunk, preempt_schedule_notrace
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v2 07/11] locking/lockdep: Cleanup
2020-08-21 8:47 [PATCH v2 00/11] TRACE_IRQFLAGS wreckage Peter Zijlstra
` (5 preceding siblings ...)
2020-08-21 8:47 ` [PATCH v2 06/11] x86/entry: Remove unused THUNKs Peter Zijlstra
@ 2020-08-21 8:47 ` Peter Zijlstra
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 08/11] nds32: Implement arch_irqs_disabled() Peter Zijlstra
` (5 subsequent siblings)
12 siblings, 1 reply; 53+ messages in thread
From: Peter Zijlstra @ 2020-08-21 8:47 UTC (permalink / raw)
To: linux-kernel, mingo, will
Cc: npiggin, elver, jgross, paulmck, rostedt, rjw, joel, svens, tglx, peterz
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Marco Elver <elver@google.com>
---
include/linux/irqflags.h | 54 ++++++++++++++++++++++++++---------------------
1 file changed, 30 insertions(+), 24 deletions(-)
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -49,10 +49,11 @@ struct irqtrace_events {
DECLARE_PER_CPU(int, hardirqs_enabled);
DECLARE_PER_CPU(int, hardirq_context);
- extern void trace_hardirqs_on_prepare(void);
- extern void trace_hardirqs_off_finish(void);
- extern void trace_hardirqs_on(void);
- extern void trace_hardirqs_off(void);
+extern void trace_hardirqs_on_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() (raw_cpu_read(hardirq_context))
# define lockdep_softirq_context(p) ((p)->softirq_context)
# define lockdep_hardirqs_enabled() (this_cpu_read(hardirqs_enabled))
@@ -120,17 +121,17 @@ do { \
#else
# define trace_hardirqs_on_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
-# define lockdep_softirq_context(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)
-# define lockdep_hardirq_exit() do { } while (0)
-# define lockdep_softirq_enter() do { } while (0)
-# define lockdep_softirq_exit() do { } while (0)
+# define trace_hardirqs_on() do { } while (0)
+# define trace_hardirqs_off() do { } while (0)
+# define lockdep_hardirq_context() 0
+# define lockdep_softirq_context(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)
+# define lockdep_hardirq_exit() do { } while (0)
+# define lockdep_softirq_enter() do { } while (0)
+# define lockdep_softirq_exit() do { } while (0)
# define lockdep_hrtimer_enter(__hrtimer) false
# define lockdep_hrtimer_exit(__context) do { } while (0)
# define lockdep_posixtimer_enter() do { } while (0)
@@ -181,17 +182,25 @@ do { \
* if !TRACE_IRQFLAGS.
*/
#ifdef CONFIG_TRACE_IRQFLAGS
-#define local_irq_enable() \
- do { trace_hardirqs_on(); raw_local_irq_enable(); } while (0)
-#define local_irq_disable() \
- do { raw_local_irq_disable(); trace_hardirqs_off(); } while (0)
+
+#define local_irq_enable() \
+ do { \
+ trace_hardirqs_on(); \
+ raw_local_irq_enable(); \
+ } while (0)
+
+#define local_irq_disable() \
+ do { \
+ raw_local_irq_disable(); \
+ trace_hardirqs_off(); \
+ } while (0)
+
#define local_irq_save(flags) \
do { \
raw_local_irq_save(flags); \
trace_hardirqs_off(); \
} while (0)
-
#define local_irq_restore(flags) \
do { \
if (raw_irqs_disabled_flags(flags)) { \
@@ -214,10 +223,7 @@ do { \
#define local_irq_enable() do { raw_local_irq_enable(); } while (0)
#define local_irq_disable() do { raw_local_irq_disable(); } while (0)
-#define local_irq_save(flags) \
- do { \
- raw_local_irq_save(flags); \
- } while (0)
+#define local_irq_save(flags) do { raw_local_irq_save(flags); } while (0)
#define local_irq_restore(flags) do { raw_local_irq_restore(flags); } while (0)
#define safe_halt() do { raw_safe_halt(); } while (0)
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v2 08/11] nds32: Implement arch_irqs_disabled()
2020-08-21 8:47 [PATCH v2 00/11] TRACE_IRQFLAGS wreckage Peter Zijlstra
` (6 preceding siblings ...)
2020-08-21 8:47 ` [PATCH v2 07/11] locking/lockdep: Cleanup Peter Zijlstra
@ 2020-08-21 8:47 ` Peter Zijlstra
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 09/11] arm64: " Peter Zijlstra
` (4 subsequent siblings)
12 siblings, 1 reply; 53+ messages in thread
From: Peter Zijlstra @ 2020-08-21 8:47 UTC (permalink / raw)
To: linux-kernel, mingo, will
Cc: npiggin, elver, jgross, paulmck, rostedt, rjw, joel, svens, tglx,
peterz, Nick Hu, Greentime Hu, Vincent Chen
Cc: Nick Hu <nickhu@andestech.com>
Cc: Greentime Hu <green.hu@gmail.com>
Cc: Vincent Chen <deanbo422@gmail.com>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Marco Elver <elver@google.com>
---
arch/nds32/include/asm/irqflags.h | 5 +++++
1 file changed, 5 insertions(+)
--- a/arch/nds32/include/asm/irqflags.h
+++ b/arch/nds32/include/asm/irqflags.h
@@ -34,3 +34,8 @@ static inline int arch_irqs_disabled_fla
{
return !flags;
}
+
+static inline int arch_irqs_disabled(void)
+{
+ return arch_irqs_disabled_flags(arch_local_save_flags());
+}
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v2 09/11] arm64: Implement arch_irqs_disabled()
2020-08-21 8:47 [PATCH v2 00/11] TRACE_IRQFLAGS wreckage Peter Zijlstra
` (7 preceding siblings ...)
2020-08-21 8:47 ` [PATCH v2 08/11] nds32: Implement arch_irqs_disabled() Peter Zijlstra
@ 2020-08-21 8:47 ` Peter Zijlstra
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 10/11] lockdep: Only trace IRQ edges Peter Zijlstra
` (3 subsequent siblings)
12 siblings, 1 reply; 53+ messages in thread
From: Peter Zijlstra @ 2020-08-21 8:47 UTC (permalink / raw)
To: linux-kernel, mingo, will
Cc: npiggin, elver, jgross, paulmck, rostedt, rjw, joel, svens, tglx,
peterz, Catalin Marinas
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/arm64/include/asm/irqflags.h | 5 +++++
1 file changed, 5 insertions(+)
--- a/arch/arm64/include/asm/irqflags.h
+++ b/arch/arm64/include/asm/irqflags.h
@@ -95,6 +95,11 @@ static inline int arch_irqs_disabled_fla
return res;
}
+static inline int arch_irqs_disabled(void)
+{
+ return arch_irqs_disabled_flags(arch_local_save_flags());
+}
+
static inline unsigned long arch_local_irq_save(void)
{
unsigned long flags;
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v2 10/11] lockdep: Only trace IRQ edges
2020-08-21 8:47 [PATCH v2 00/11] TRACE_IRQFLAGS wreckage Peter Zijlstra
` (8 preceding siblings ...)
2020-08-21 8:47 ` [PATCH v2 09/11] arm64: " Peter Zijlstra
@ 2020-08-21 8:47 ` Peter Zijlstra
2020-08-26 0:47 ` Thomas Gleixner
2020-09-02 4:21 ` Guenter Roeck
2020-08-21 8:47 ` [PATCH v2 11/11] lockdep,trace: Expose tracepoints Peter Zijlstra
` (2 subsequent siblings)
12 siblings, 2 replies; 53+ messages in thread
From: Peter Zijlstra @ 2020-08-21 8:47 UTC (permalink / raw)
To: linux-kernel, mingo, will
Cc: npiggin, elver, jgross, paulmck, rostedt, rjw, joel, svens, tglx, peterz
From: Nicholas Piggin <npiggin@gmail.com>
Problem:
raw_local_irq_save();
local_irq_save();
...
local_irq_restore();
raw_local_irq_restore();
existing instances:
- lock_acquire()
raw_local_irq_save()
__lock_acquire()
arch_spin_lock(&graph_lock)
pv_wait() := kvm_wait() (same or worse for Xen/HyperV)
local_irq_save()
- trace_clock_global()
raw_local_irq_save()
arch_spin_lock()
pv_wait() := kvm_wait()
local_irq_save()
- apic_retrigger_irq()
raw_local_irq_save()
apic->send_IPI() := default_send_IPI_single_phys()
local_irq_save()
Possible solutions:
A) make it work by enabling the tracing inside raw_*()
B) make it work by keeping tracing disabled inside raw_*()
Now, given that the only reason to use the raw_* variant is because you don't
want tracing, A) seems like a weird option (although it can be done), so we
pick B) and declare any code that ends up doing:
raw_local_irq_save()
local_irq_save()
lockdep_assert_irqs_disabled();
broken. AFAICT this problem has existed forever, the only reason it came
up is because I changed IRQ tracing vs lockdep recursion and the first
instance is fairly common, the other cases hardly ever happen.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
[rewrote changelog]
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Marco Elver <elver@google.com>
Link: https://lkml.kernel.org/r/20200723105615.1268126-1-npiggin@gmail.com
---
arch/powerpc/include/asm/hw_irq.h | 11 ++++-------
include/linux/irqflags.h | 15 +++++++--------
2 files changed, 11 insertions(+), 15 deletions(-)
--- a/arch/powerpc/include/asm/hw_irq.h
+++ b/arch/powerpc/include/asm/hw_irq.h
@@ -200,17 +200,14 @@ static inline bool arch_irqs_disabled(vo
#define powerpc_local_irq_pmu_save(flags) \
do { \
raw_local_irq_pmu_save(flags); \
- trace_hardirqs_off(); \
+ if (!raw_irqs_disabled_flags(flags)) \
+ trace_hardirqs_off(); \
} while(0)
#define powerpc_local_irq_pmu_restore(flags) \
do { \
- if (raw_irqs_disabled_flags(flags)) { \
- raw_local_irq_pmu_restore(flags); \
- trace_hardirqs_off(); \
- } else { \
+ if (!raw_irqs_disabled_flags(flags)) \
trace_hardirqs_on(); \
- raw_local_irq_pmu_restore(flags); \
- } \
+ raw_local_irq_pmu_restore(flags); \
} while(0)
#else
#define powerpc_local_irq_pmu_save(flags) \
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -191,25 +191,24 @@ do { \
#define local_irq_disable() \
do { \
+ bool was_disabled = raw_irqs_disabled();\
raw_local_irq_disable(); \
- trace_hardirqs_off(); \
+ if (!was_disabled) \
+ trace_hardirqs_off(); \
} while (0)
#define local_irq_save(flags) \
do { \
raw_local_irq_save(flags); \
- trace_hardirqs_off(); \
+ if (!raw_irqs_disabled_flags(flags)) \
+ trace_hardirqs_off(); \
} while (0)
#define local_irq_restore(flags) \
do { \
- if (raw_irqs_disabled_flags(flags)) { \
- raw_local_irq_restore(flags); \
- trace_hardirqs_off(); \
- } else { \
+ if (!raw_irqs_disabled_flags(flags)) \
trace_hardirqs_on(); \
- raw_local_irq_restore(flags); \
- } \
+ raw_local_irq_restore(flags); \
} while (0)
#define safe_halt() \
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v2 11/11] lockdep,trace: Expose tracepoints
2020-08-21 8:47 [PATCH v2 00/11] TRACE_IRQFLAGS wreckage Peter Zijlstra
` (9 preceding siblings ...)
2020-08-21 8:47 ` [PATCH v2 10/11] lockdep: Only trace IRQ edges Peter Zijlstra
@ 2020-08-21 8:47 ` Peter Zijlstra
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
2020-09-02 3:51 ` [PATCH v2 11/11] " Guenter Roeck
2020-08-21 12:58 ` [PATCH v2 00/11] TRACE_IRQFLAGS wreckage peterz
2020-08-26 10:16 ` [PATCH v2 12/11] mips: Implement arch_irqs_disabled() peterz
12 siblings, 2 replies; 53+ messages in thread
From: Peter Zijlstra @ 2020-08-21 8:47 UTC (permalink / raw)
To: linux-kernel, mingo, will
Cc: npiggin, elver, jgross, paulmck, rostedt, rjw, joel, svens, tglx, peterz
The lockdep tracepoints are under the lockdep recursion counter, this
has a bunch of nasty side effects:
- TRACE_IRQFLAGS doesn't work across the entire tracepoint
- RCU-lockdep doesn't see the tracepoints either, hiding numerous
"suspicious RCU usage" warnings.
Pull the trace_lock_*() tracepoints completely out from under the
lockdep recursion handling and completely rely on the trace level
recusion handling -- also, tracing *SHOULD* not be taking locks in any
case.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Tested-by: Marco Elver <elver@google.com>
---
kernel/locking/lockdep.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -5044,6 +5044,8 @@ void lock_acquire(struct lockdep_map *lo
{
unsigned long flags;
+ trace_lock_acquire(lock, subclass, trylock, read, check, nest_lock, ip);
+
if (unlikely(current->lockdep_recursion)) {
/* XXX allow trylock from NMI ?!? */
if (lockdep_nmi() && !trylock) {
@@ -5068,7 +5070,6 @@ void lock_acquire(struct lockdep_map *lo
check_flags(flags);
current->lockdep_recursion++;
- trace_lock_acquire(lock, subclass, trylock, read, check, nest_lock, ip);
__lock_acquire(lock, subclass, trylock, read, check,
irqs_disabled_flags(flags), nest_lock, ip, 0, 0);
lockdep_recursion_finish();
@@ -5080,13 +5081,15 @@ void lock_release(struct lockdep_map *lo
{
unsigned long flags;
+ trace_lock_release(lock, ip);
+
if (unlikely(current->lockdep_recursion))
return;
raw_local_irq_save(flags);
check_flags(flags);
+
current->lockdep_recursion++;
- trace_lock_release(lock, ip);
if (__lock_release(lock, ip))
check_chain_key(current);
lockdep_recursion_finish();
@@ -5272,8 +5275,6 @@ __lock_acquired(struct lockdep_map *lock
hlock->holdtime_stamp = now;
}
- trace_lock_acquired(lock, ip);
-
stats = get_lock_stats(hlock_class(hlock));
if (waittime) {
if (hlock->read)
@@ -5292,6 +5293,8 @@ void lock_contended(struct lockdep_map *
{
unsigned long flags;
+ trace_lock_contended(lock, ip);
+
if (unlikely(!lock_stat || !debug_locks))
return;
@@ -5301,7 +5304,6 @@ void lock_contended(struct lockdep_map *
raw_local_irq_save(flags);
check_flags(flags);
current->lockdep_recursion++;
- trace_lock_contended(lock, ip);
__lock_contended(lock, ip);
lockdep_recursion_finish();
raw_local_irq_restore(flags);
@@ -5312,6 +5314,8 @@ void lock_acquired(struct lockdep_map *l
{
unsigned long flags;
+ trace_lock_acquired(lock, ip);
+
if (unlikely(!lock_stat || !debug_locks))
return;
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 00/11] TRACE_IRQFLAGS wreckage
2020-08-21 8:47 [PATCH v2 00/11] TRACE_IRQFLAGS wreckage Peter Zijlstra
` (10 preceding siblings ...)
2020-08-21 8:47 ` [PATCH v2 11/11] lockdep,trace: Expose tracepoints Peter Zijlstra
@ 2020-08-21 12:58 ` peterz
2020-08-26 10:16 ` [PATCH v2 12/11] mips: Implement arch_irqs_disabled() peterz
12 siblings, 0 replies; 53+ messages in thread
From: peterz @ 2020-08-21 12:58 UTC (permalink / raw)
To: linux-kernel, mingo, will
Cc: npiggin, elver, jgross, paulmck, rostedt, rjw, joel, svens, tglx
On Fri, Aug 21, 2020 at 10:47:38AM +0200, Peter Zijlstra wrote:
> TRACE_IRQFLAGS
> ---
>
> arch/arm/mach-omap2/pm34xx.c | 4 --
> arch/arm64/include/asm/irqflags.h | 5 ++
> arch/arm64/kernel/process.c | 2 -
> arch/nds32/include/asm/irqflags.h | 5 ++
> arch/powerpc/include/asm/hw_irq.h | 11 ++---
> arch/s390/kernel/idle.c | 3 -
> arch/x86/entry/thunk_32.S | 5 --
> arch/x86/include/asm/mmu.h | 1
> arch/x86/kernel/process.c | 4 --
> arch/x86/mm/tlb.c | 13 +-----
> drivers/cpuidle/cpuidle.c | 19 +++++++--
> drivers/idle/intel_idle.c | 16 --------
> include/linux/cpuidle.h | 13 +++---
> include/linux/irqflags.h | 73 ++++++++++++++++++++------------------
> include/linux/lockdep.h | 18 ++++++---
> include/linux/mmu_context.h | 5 ++
> kernel/locking/lockdep.c | 18 +++++----
> kernel/sched/idle.c | 25 +++++--------
> 18 files changed, 118 insertions(+), 122 deletions(-)
Whole set also available at:
git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git locking/wip
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 02/11] cpuidle: Fixup IRQ state
2020-08-21 8:47 ` [PATCH v2 02/11] cpuidle: Fixup IRQ state Peter Zijlstra
@ 2020-08-21 17:36 ` Rafael J. Wysocki
2020-08-27 1:06 ` Joel Fernandes
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
1 sibling, 1 reply; 53+ messages in thread
From: Rafael J. Wysocki @ 2020-08-21 17:36 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, joel, svens, tglx
On Friday, August 21, 2020 10:47:40 AM CEST Peter Zijlstra wrote:
> Match the pattern elsewhere in this file.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Tested-by: Marco Elver <elver@google.com>
For all patches in the series:
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/cpuidle/cpuidle.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -153,7 +153,8 @@ static void enter_s2idle_proper(struct c
> */
> stop_critical_timings();
> drv->states[index].enter_s2idle(dev, drv, index);
> - WARN_ON(!irqs_disabled());
> + if (WARN_ON_ONCE(!irqs_disabled()))
> + local_irq_disable();
> /*
> * timekeeping_resume() that will be called by tick_unfreeze() for the
> * first CPU executing it calls functions containing RCU read-side
>
>
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 10/11] lockdep: Only trace IRQ edges
2020-08-21 8:47 ` [PATCH v2 10/11] lockdep: Only trace IRQ edges Peter Zijlstra
@ 2020-08-26 0:47 ` Thomas Gleixner
2020-09-02 4:21 ` Guenter Roeck
1 sibling, 0 replies; 53+ messages in thread
From: Thomas Gleixner @ 2020-08-26 0:47 UTC (permalink / raw)
To: Peter Zijlstra, linux-kernel, mingo, will
Cc: npiggin, elver, jgross, paulmck, rostedt, rjw, joel, svens, peterz
On Fri, Aug 21 2020 at 10:47, Peter Zijlstra wrote:
> Now, given that the only reason to use the raw_* variant is because you don't
> want tracing, A) seems like a weird option (although it can be done), so we
> pick B) and declare any code that ends up doing:
>
> raw_local_irq_save()
> local_irq_save()
> lockdep_assert_irqs_disabled();
>
> broken. AFAICT this problem has existed forever, the only reason it came
> up is because I changed IRQ tracing vs lockdep recursion and the first
Who is 'I'? I know you made that change and you also rewrote the
changelog, but I only figured that out after scrolling further down. As
this patch is authored by Nick, the above is simply inconsistent. Can we
please have just a reference to the commit which changed that code
instead of a puzzle?
Other than that:
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v2 12/11] mips: Implement arch_irqs_disabled()
2020-08-21 8:47 [PATCH v2 00/11] TRACE_IRQFLAGS wreckage Peter Zijlstra
` (11 preceding siblings ...)
2020-08-21 12:58 ` [PATCH v2 00/11] TRACE_IRQFLAGS wreckage peterz
@ 2020-08-26 10:16 ` peterz
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
12 siblings, 1 reply; 53+ messages in thread
From: peterz @ 2020-08-26 10:16 UTC (permalink / raw)
To: linux-kernel, mingo, will
Cc: npiggin, elver, jgross, paulmck, rostedt, rjw, joel, svens, tglx,
Thomas Bogendoerfer, Paul Burton
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/mips/include/asm/irqflags.h | 5 +++++
1 file changed, 5 insertions(+)
--- a/arch/mips/include/asm/irqflags.h
+++ b/arch/mips/include/asm/irqflags.h
@@ -137,6 +137,11 @@ static inline int arch_irqs_disabled_fla
return !(flags & 1);
}
+static inline int arch_irqs_disabled(void)
+{
+ return arch_irqs_disabled_flags(arch_local_save_flags());
+}
+
#endif /* #ifndef __ASSEMBLY__ */
/*
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 01/11] lockdep: Use raw_cpu_*() for per-cpu variables
2020-08-21 8:47 ` [PATCH v2 01/11] lockdep: Use raw_cpu_*() for per-cpu variables Peter Zijlstra
@ 2020-08-27 1:05 ` Joel Fernandes
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
1 sibling, 0 replies; 53+ messages in thread
From: Joel Fernandes @ 2020-08-27 1:05 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, svens, tglx
On Fri, Aug 21, 2020 at 10:47:39AM +0200, Peter Zijlstra wrote:
> Sven reported that commit a21ee6055c30 ("lockdep: Change
> hardirq{s_enabled,_context} to per-cpu variables") caused trouble on
> s390 because their this_cpu_*() primitives disable preemption which
> then lands back tracing.
>
> On the one hand, per-cpu ops should use preempt_*able_notrace() and
> raw_local_irq_*(), on the other hand, we can trivialy use raw_cpu_*()
> ops for this.
>
> Fixes: a21ee6055c30 ("lockdep: Change hardirq{s_enabled,_context} to per-cpu variables")
> Reported-by: Sven Schnelle <svens@linux.ibm.com>
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Tested-by: Marco Elver <elver@google.com>
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
thanks,
- Joel
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> include/linux/irqflags.h | 6 +++---
> include/linux/lockdep.h | 18 +++++++++++++-----
> kernel/locking/lockdep.c | 4 ++--
> 3 files changed, 18 insertions(+), 10 deletions(-)
>
> --- a/include/linux/irqflags.h
> +++ b/include/linux/irqflags.h
> @@ -53,13 +53,13 @@ DECLARE_PER_CPU(int, hardirq_context);
> 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))
> +# define lockdep_hardirq_context() (raw_cpu_read(hardirq_context))
> # define lockdep_softirq_context(p) ((p)->softirq_context)
> # define lockdep_hardirqs_enabled() (this_cpu_read(hardirqs_enabled))
> # define lockdep_softirqs_enabled(p) ((p)->softirqs_enabled)
> # define lockdep_hardirq_enter() \
> do { \
> - if (this_cpu_inc_return(hardirq_context) == 1) \
> + if (__this_cpu_inc_return(hardirq_context) == 1)\
> current->hardirq_threaded = 0; \
> } while (0)
> # define lockdep_hardirq_threaded() \
> @@ -68,7 +68,7 @@ do { \
> } while (0)
> # define lockdep_hardirq_exit() \
> do { \
> - this_cpu_dec(hardirq_context); \
> + __this_cpu_dec(hardirq_context); \
> } while (0)
> # define lockdep_softirq_enter() \
> do { \
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -535,19 +535,27 @@ do { \
> DECLARE_PER_CPU(int, hardirqs_enabled);
> DECLARE_PER_CPU(int, hardirq_context);
>
> +/*
> + * The below lockdep_assert_*() macros use raw_cpu_read() to access the above
> + * per-cpu variables. This is required because this_cpu_read() will potentially
> + * call into preempt/irq-disable and that obviously isn't right. This is also
> + * correct because when IRQs are enabled, it doesn't matter if we accidentally
> + * read the value from our previous CPU.
> + */
> +
> #define lockdep_assert_irqs_enabled() \
> do { \
> - WARN_ON_ONCE(debug_locks && !this_cpu_read(hardirqs_enabled)); \
> + WARN_ON_ONCE(debug_locks && !raw_cpu_read(hardirqs_enabled)); \
> } while (0)
>
> #define lockdep_assert_irqs_disabled() \
> do { \
> - WARN_ON_ONCE(debug_locks && this_cpu_read(hardirqs_enabled)); \
> + WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled)); \
> } while (0)
>
> #define lockdep_assert_in_irq() \
> do { \
> - WARN_ON_ONCE(debug_locks && !this_cpu_read(hardirq_context)); \
> + WARN_ON_ONCE(debug_locks && !raw_cpu_read(hardirq_context)); \
> } while (0)
>
> #define lockdep_assert_preemption_enabled() \
> @@ -555,7 +563,7 @@ do { \
> WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_COUNT) && \
> debug_locks && \
> (preempt_count() != 0 || \
> - !this_cpu_read(hardirqs_enabled))); \
> + !raw_cpu_read(hardirqs_enabled))); \
> } while (0)
>
> #define lockdep_assert_preemption_disabled() \
> @@ -563,7 +571,7 @@ do { \
> WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_COUNT) && \
> debug_locks && \
> (preempt_count() == 0 && \
> - this_cpu_read(hardirqs_enabled))); \
> + raw_cpu_read(hardirqs_enabled))); \
> } while (0)
>
> #else
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -3756,7 +3756,7 @@ void noinstr lockdep_hardirqs_on(unsigne
>
> skip_checks:
> /* we'll do an OFF -> ON transition: */
> - this_cpu_write(hardirqs_enabled, 1);
> + __this_cpu_write(hardirqs_enabled, 1);
> trace->hardirq_enable_ip = ip;
> trace->hardirq_enable_event = ++trace->irq_events;
> debug_atomic_inc(hardirqs_on_events);
> @@ -3795,7 +3795,7 @@ void noinstr lockdep_hardirqs_off(unsign
> /*
> * We have done an ON -> OFF transition:
> */
> - this_cpu_write(hardirqs_enabled, 0);
> + __this_cpu_write(hardirqs_enabled, 0);
> trace->hardirq_disable_ip = ip;
> trace->hardirq_disable_event = ++trace->irq_events;
> debug_atomic_inc(hardirqs_off_events);
>
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 02/11] cpuidle: Fixup IRQ state
2020-08-21 17:36 ` Rafael J. Wysocki
@ 2020-08-27 1:06 ` Joel Fernandes
0 siblings, 0 replies; 53+ messages in thread
From: Joel Fernandes @ 2020-08-27 1:06 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Peter Zijlstra, linux-kernel, mingo, will, npiggin, elver,
jgross, paulmck, rostedt, svens, tglx
On Fri, Aug 21, 2020 at 07:36:43PM +0200, Rafael J. Wysocki wrote:
> On Friday, August 21, 2020 10:47:40 AM CEST Peter Zijlstra wrote:
> > Match the pattern elsewhere in this file.
> >
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > Tested-by: Marco Elver <elver@google.com>
>
> For all patches in the series:
>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
thanks,
- Joel
>
> > ---
> > drivers/cpuidle/cpuidle.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > --- a/drivers/cpuidle/cpuidle.c
> > +++ b/drivers/cpuidle/cpuidle.c
> > @@ -153,7 +153,8 @@ static void enter_s2idle_proper(struct c
> > */
> > stop_critical_timings();
> > drv->states[index].enter_s2idle(dev, drv, index);
> > - WARN_ON(!irqs_disabled());
> > + if (WARN_ON_ONCE(!irqs_disabled()))
> > + local_irq_disable();
> > /*
> > * timekeeping_resume() that will be called by tick_unfreeze() for the
> > * first CPU executing it calls functions containing RCU read-side
> >
> >
> >
>
>
>
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 03/11] sched,idle,rcu: Push rcu_idle deeper into the idle path
2020-08-21 8:47 ` [PATCH v2 03/11] sched,idle,rcu: Push rcu_idle deeper into the idle path Peter Zijlstra
@ 2020-08-27 1:18 ` Joel Fernandes
2020-08-27 1:24 ` Joel Fernandes
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
1 sibling, 1 reply; 53+ messages in thread
From: Joel Fernandes @ 2020-08-27 1:18 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, svens, tglx
On Fri, Aug 21, 2020 at 10:47:41AM +0200, Peter Zijlstra wrote:
> Lots of things take locks, due to a wee bug, rcu_lockdep didn't notice
> that the locking tracepoints were using RCU.
>
> Push rcu_idle_{enter,exit}() as deep as possible into the idle paths,
> this also resolves a lot of _rcuidle()/RCU_NONIDLE() usage.
>
> Specifically, sched_clock_idle_wakeup_event() will use ktime which
> will use seqlocks which will tickle lockdep, and
> stop_critical_timings() uses lock.
I was wondering if those tracepoints should just use _rcuidle variant of the
trace call. But that's a terrible idea considering that would add unwanted
overhead I think.
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
thanks,
- Joel
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Tested-by: Marco Elver <elver@google.com>
> ---
> drivers/cpuidle/cpuidle.c | 12 ++++++++----
> kernel/sched/idle.c | 22 ++++++++--------------
> 2 files changed, 16 insertions(+), 18 deletions(-)
>
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -145,13 +145,14 @@ static void enter_s2idle_proper(struct c
> * executing it contains RCU usage regarded as invalid in the idle
> * context, so tell RCU about that.
> */
> - RCU_NONIDLE(tick_freeze());
> + tick_freeze();
> /*
> * The state used here cannot be a "coupled" one, because the "coupled"
> * cpuidle mechanism enables interrupts and doing that with timekeeping
> * suspended is generally unsafe.
> */
> stop_critical_timings();
> + rcu_idle_enter();
> drv->states[index].enter_s2idle(dev, drv, index);
> if (WARN_ON_ONCE(!irqs_disabled()))
> local_irq_disable();
> @@ -160,7 +161,8 @@ static void enter_s2idle_proper(struct c
> * first CPU executing it calls functions containing RCU read-side
> * critical sections, so tell RCU about that.
> */
> - RCU_NONIDLE(tick_unfreeze());
> + rcu_idle_exit();
> + tick_unfreeze();
> start_critical_timings();
>
> time_end = ns_to_ktime(local_clock());
> @@ -229,16 +231,18 @@ int cpuidle_enter_state(struct cpuidle_d
> /* Take note of the planned idle state. */
> sched_idle_set_state(target_state);
>
> - trace_cpu_idle_rcuidle(index, dev->cpu);
> + trace_cpu_idle(index, dev->cpu);
> time_start = ns_to_ktime(local_clock());
>
> stop_critical_timings();
> + rcu_idle_enter();
> entered_state = target_state->enter(dev, drv, index);
> + rcu_idle_exit();
> start_critical_timings();
>
> sched_clock_idle_wakeup_event();
> time_end = ns_to_ktime(local_clock());
> - trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
> + trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
>
> /* The cpu is no longer idle or about to enter idle. */
> sched_idle_set_state(NULL);
> --- a/kernel/sched/idle.c
> +++ b/kernel/sched/idle.c
> @@ -54,17 +54,18 @@ __setup("hlt", cpu_idle_nopoll_setup);
>
> static noinline int __cpuidle cpu_idle_poll(void)
> {
> + trace_cpu_idle(0, smp_processor_id());
> + stop_critical_timings();
> rcu_idle_enter();
> - trace_cpu_idle_rcuidle(0, smp_processor_id());
> local_irq_enable();
> - stop_critical_timings();
>
> while (!tif_need_resched() &&
> - (cpu_idle_force_poll || tick_check_broadcast_expired()))
> + (cpu_idle_force_poll || tick_check_broadcast_expired()))
> cpu_relax();
> - start_critical_timings();
> - trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
> +
> rcu_idle_exit();
> + start_critical_timings();
> + trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
>
> return 1;
> }
> @@ -91,7 +92,9 @@ void __cpuidle default_idle_call(void)
> local_irq_enable();
> } else {
> stop_critical_timings();
> + rcu_idle_enter();
> arch_cpu_idle();
> + rcu_idle_exit();
> start_critical_timings();
> }
> }
> @@ -158,7 +161,6 @@ static void cpuidle_idle_call(void)
>
> if (cpuidle_not_available(drv, dev)) {
> tick_nohz_idle_stop_tick();
> - rcu_idle_enter();
>
> default_idle_call();
> goto exit_idle;
> @@ -178,21 +180,17 @@ static void cpuidle_idle_call(void)
> u64 max_latency_ns;
>
> if (idle_should_enter_s2idle()) {
> - rcu_idle_enter();
>
> entered_state = call_cpuidle_s2idle(drv, dev);
> if (entered_state > 0)
> goto exit_idle;
>
> - rcu_idle_exit();
> -
> max_latency_ns = U64_MAX;
> } else {
> max_latency_ns = dev->forced_idle_latency_limit_ns;
> }
>
> tick_nohz_idle_stop_tick();
> - rcu_idle_enter();
>
> next_state = cpuidle_find_deepest_state(drv, dev, max_latency_ns);
> call_cpuidle(drv, dev, next_state);
> @@ -209,8 +207,6 @@ static void cpuidle_idle_call(void)
> else
> tick_nohz_idle_retain_tick();
>
> - rcu_idle_enter();
> -
> entered_state = call_cpuidle(drv, dev, next_state);
> /*
> * Give the governor an opportunity to reflect on the outcome
> @@ -226,8 +222,6 @@ static void cpuidle_idle_call(void)
> */
> if (WARN_ON_ONCE(irqs_disabled()))
> local_irq_enable();
> -
> - rcu_idle_exit();
> }
>
> /*
>
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 03/11] sched,idle,rcu: Push rcu_idle deeper into the idle path
2020-08-27 1:18 ` Joel Fernandes
@ 2020-08-27 1:24 ` Joel Fernandes
2020-08-27 7:47 ` peterz
0 siblings, 1 reply; 53+ messages in thread
From: Joel Fernandes @ 2020-08-27 1:24 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, svens, tglx
On Wed, Aug 26, 2020 at 09:18:26PM -0400, Joel Fernandes wrote:
> On Fri, Aug 21, 2020 at 10:47:41AM +0200, Peter Zijlstra wrote:
> > Lots of things take locks, due to a wee bug, rcu_lockdep didn't notice
> > that the locking tracepoints were using RCU.
> >
> > Push rcu_idle_{enter,exit}() as deep as possible into the idle paths,
> > this also resolves a lot of _rcuidle()/RCU_NONIDLE() usage.
> >
> > Specifically, sched_clock_idle_wakeup_event() will use ktime which
> > will use seqlocks which will tickle lockdep, and
> > stop_critical_timings() uses lock.
>
> I was wondering if those tracepoints should just use _rcuidle variant of the
> trace call. But that's a terrible idea considering that would add unwanted
> overhead I think.
>
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
BTW, if tracepoint is converted to use RCU-trace flavor, then these kinds of
issues go away, no? That RCU flavor is always watching.
thanks,
- Joel
>
> thanks,
>
> - Joel
>
> >
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > Tested-by: Marco Elver <elver@google.com>
> > ---
> > drivers/cpuidle/cpuidle.c | 12 ++++++++----
> > kernel/sched/idle.c | 22 ++++++++--------------
> > 2 files changed, 16 insertions(+), 18 deletions(-)
> >
> > --- a/drivers/cpuidle/cpuidle.c
> > +++ b/drivers/cpuidle/cpuidle.c
> > @@ -145,13 +145,14 @@ static void enter_s2idle_proper(struct c
> > * executing it contains RCU usage regarded as invalid in the idle
> > * context, so tell RCU about that.
> > */
> > - RCU_NONIDLE(tick_freeze());
> > + tick_freeze();
> > /*
> > * The state used here cannot be a "coupled" one, because the "coupled"
> > * cpuidle mechanism enables interrupts and doing that with timekeeping
> > * suspended is generally unsafe.
> > */
> > stop_critical_timings();
> > + rcu_idle_enter();
> > drv->states[index].enter_s2idle(dev, drv, index);
> > if (WARN_ON_ONCE(!irqs_disabled()))
> > local_irq_disable();
> > @@ -160,7 +161,8 @@ static void enter_s2idle_proper(struct c
> > * first CPU executing it calls functions containing RCU read-side
> > * critical sections, so tell RCU about that.
> > */
> > - RCU_NONIDLE(tick_unfreeze());
> > + rcu_idle_exit();
> > + tick_unfreeze();
> > start_critical_timings();
> >
> > time_end = ns_to_ktime(local_clock());
> > @@ -229,16 +231,18 @@ int cpuidle_enter_state(struct cpuidle_d
> > /* Take note of the planned idle state. */
> > sched_idle_set_state(target_state);
> >
> > - trace_cpu_idle_rcuidle(index, dev->cpu);
> > + trace_cpu_idle(index, dev->cpu);
> > time_start = ns_to_ktime(local_clock());
> >
> > stop_critical_timings();
> > + rcu_idle_enter();
> > entered_state = target_state->enter(dev, drv, index);
> > + rcu_idle_exit();
> > start_critical_timings();
> >
> > sched_clock_idle_wakeup_event();
> > time_end = ns_to_ktime(local_clock());
> > - trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
> > + trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
> >
> > /* The cpu is no longer idle or about to enter idle. */
> > sched_idle_set_state(NULL);
> > --- a/kernel/sched/idle.c
> > +++ b/kernel/sched/idle.c
> > @@ -54,17 +54,18 @@ __setup("hlt", cpu_idle_nopoll_setup);
> >
> > static noinline int __cpuidle cpu_idle_poll(void)
> > {
> > + trace_cpu_idle(0, smp_processor_id());
> > + stop_critical_timings();
> > rcu_idle_enter();
> > - trace_cpu_idle_rcuidle(0, smp_processor_id());
> > local_irq_enable();
> > - stop_critical_timings();
> >
> > while (!tif_need_resched() &&
> > - (cpu_idle_force_poll || tick_check_broadcast_expired()))
> > + (cpu_idle_force_poll || tick_check_broadcast_expired()))
> > cpu_relax();
> > - start_critical_timings();
> > - trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
> > +
> > rcu_idle_exit();
> > + start_critical_timings();
> > + trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
> >
> > return 1;
> > }
> > @@ -91,7 +92,9 @@ void __cpuidle default_idle_call(void)
> > local_irq_enable();
> > } else {
> > stop_critical_timings();
> > + rcu_idle_enter();
> > arch_cpu_idle();
> > + rcu_idle_exit();
> > start_critical_timings();
> > }
> > }
> > @@ -158,7 +161,6 @@ static void cpuidle_idle_call(void)
> >
> > if (cpuidle_not_available(drv, dev)) {
> > tick_nohz_idle_stop_tick();
> > - rcu_idle_enter();
> >
> > default_idle_call();
> > goto exit_idle;
> > @@ -178,21 +180,17 @@ static void cpuidle_idle_call(void)
> > u64 max_latency_ns;
> >
> > if (idle_should_enter_s2idle()) {
> > - rcu_idle_enter();
> >
> > entered_state = call_cpuidle_s2idle(drv, dev);
> > if (entered_state > 0)
> > goto exit_idle;
> >
> > - rcu_idle_exit();
> > -
> > max_latency_ns = U64_MAX;
> > } else {
> > max_latency_ns = dev->forced_idle_latency_limit_ns;
> > }
> >
> > tick_nohz_idle_stop_tick();
> > - rcu_idle_enter();
> >
> > next_state = cpuidle_find_deepest_state(drv, dev, max_latency_ns);
> > call_cpuidle(drv, dev, next_state);
> > @@ -209,8 +207,6 @@ static void cpuidle_idle_call(void)
> > else
> > tick_nohz_idle_retain_tick();
> >
> > - rcu_idle_enter();
> > -
> > entered_state = call_cpuidle(drv, dev, next_state);
> > /*
> > * Give the governor an opportunity to reflect on the outcome
> > @@ -226,8 +222,6 @@ static void cpuidle_idle_call(void)
> > */
> > if (WARN_ON_ONCE(irqs_disabled()))
> > local_irq_enable();
> > -
> > - rcu_idle_exit();
> > }
> >
> > /*
> >
> >
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 03/11] sched,idle,rcu: Push rcu_idle deeper into the idle path
2020-08-27 1:24 ` Joel Fernandes
@ 2020-08-27 7:47 ` peterz
2020-08-27 7:53 ` Thomas Gleixner
2020-08-27 22:30 ` Joel Fernandes
0 siblings, 2 replies; 53+ messages in thread
From: peterz @ 2020-08-27 7:47 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, svens, tglx
On Wed, Aug 26, 2020 at 09:24:19PM -0400, Joel Fernandes wrote:
> On Wed, Aug 26, 2020 at 09:18:26PM -0400, Joel Fernandes wrote:
> > On Fri, Aug 21, 2020 at 10:47:41AM +0200, Peter Zijlstra wrote:
> > > Lots of things take locks, due to a wee bug, rcu_lockdep didn't notice
> > > that the locking tracepoints were using RCU.
> > >
> > > Push rcu_idle_{enter,exit}() as deep as possible into the idle paths,
> > > this also resolves a lot of _rcuidle()/RCU_NONIDLE() usage.
> > >
> > > Specifically, sched_clock_idle_wakeup_event() will use ktime which
> > > will use seqlocks which will tickle lockdep, and
> > > stop_critical_timings() uses lock.
> >
> > I was wondering if those tracepoints should just use _rcuidle variant of the
> > trace call. But that's a terrible idea considering that would add unwanted
> > overhead I think.
> >
> > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>
> BTW, if tracepoint is converted to use RCU-trace flavor, then these kinds of
> issues go away, no? That RCU flavor is always watching.
All trace_*_rcuidle() and RCU_NONIDLE() usage is a bug IMO.
Ideally RCU-trace goes away too.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 03/11] sched,idle,rcu: Push rcu_idle deeper into the idle path
2020-08-27 7:47 ` peterz
@ 2020-08-27 7:53 ` Thomas Gleixner
2020-08-27 22:30 ` Joel Fernandes
1 sibling, 0 replies; 53+ messages in thread
From: Thomas Gleixner @ 2020-08-27 7:53 UTC (permalink / raw)
To: peterz, Joel Fernandes
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, svens
On Thu, Aug 27 2020 at 09:47, peterz wrote:
> On Wed, Aug 26, 2020 at 09:24:19PM -0400, Joel Fernandes wrote:
>> On Wed, Aug 26, 2020 at 09:18:26PM -0400, Joel Fernandes wrote:
>> > On Fri, Aug 21, 2020 at 10:47:41AM +0200, Peter Zijlstra wrote:
>> > > Lots of things take locks, due to a wee bug, rcu_lockdep didn't notice
>> > > that the locking tracepoints were using RCU.
>> > >
>> > > Push rcu_idle_{enter,exit}() as deep as possible into the idle paths,
>> > > this also resolves a lot of _rcuidle()/RCU_NONIDLE() usage.
>> > >
>> > > Specifically, sched_clock_idle_wakeup_event() will use ktime which
>> > > will use seqlocks which will tickle lockdep, and
>> > > stop_critical_timings() uses lock.
>> >
>> > I was wondering if those tracepoints should just use _rcuidle variant of the
>> > trace call. But that's a terrible idea considering that would add unwanted
>> > overhead I think.
>> >
>> > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>>
>> BTW, if tracepoint is converted to use RCU-trace flavor, then these kinds of
>> issues go away, no? That RCU flavor is always watching.
>
> All trace_*_rcuidle() and RCU_NONIDLE() usage is a bug IMO.
It's the same problem as low level entry/exit. And that stuff is a hack
which papers over the problem instead of fixing it from ground up. But
we are talking about tracing, right?
Thanks,
tglx
^ permalink raw reply [flat|nested] 53+ messages in thread
* [tip: locking/core] lockdep,trace: Expose tracepoints
2020-08-21 8:47 ` [PATCH v2 11/11] lockdep,trace: Expose tracepoints Peter Zijlstra
@ 2020-08-27 7:54 ` tip-bot2 for Peter Zijlstra
2020-09-02 3:51 ` [PATCH v2 11/11] " Guenter Roeck
1 sibling, 0 replies; 53+ messages in thread
From: tip-bot2 for Peter Zijlstra @ 2020-08-27 7:54 UTC (permalink / raw)
To: linux-tip-commits
Cc: Peter Zijlstra (Intel), Steven Rostedt (VMware),
Thomas Gleixner, Rafael J. Wysocki, Marco Elver, x86, LKML
The following commit has been merged into the locking/core branch of tip:
Commit-ID: eb1f00237aca2e368b93db79303f8433d1976d10
Gitweb: https://git.kernel.org/tip/eb1f00237aca2e368b93db79303f8433d1976d10
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Fri, 07 Aug 2020 20:53:16 +02:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:56 +02:00
lockdep,trace: Expose tracepoints
The lockdep tracepoints are under the lockdep recursion counter, this
has a bunch of nasty side effects:
- TRACE_IRQFLAGS doesn't work across the entire tracepoint
- RCU-lockdep doesn't see the tracepoints either, hiding numerous
"suspicious RCU usage" warnings.
Pull the trace_lock_*() tracepoints completely out from under the
lockdep recursion handling and completely rely on the trace level
recusion handling -- also, tracing *SHOULD* not be taking locks in any
case.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Marco Elver <elver@google.com>
Link: https://lkml.kernel.org/r/20200821085348.782688941@infradead.org
---
kernel/locking/lockdep.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index c872e95..54b74fa 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -4977,6 +4977,8 @@ void lock_acquire(struct lockdep_map *lock, unsigned int subclass,
{
unsigned long flags;
+ trace_lock_acquire(lock, subclass, trylock, read, check, nest_lock, ip);
+
if (unlikely(current->lockdep_recursion)) {
/* XXX allow trylock from NMI ?!? */
if (lockdep_nmi() && !trylock) {
@@ -5001,7 +5003,6 @@ void lock_acquire(struct lockdep_map *lock, unsigned int subclass,
check_flags(flags);
current->lockdep_recursion++;
- trace_lock_acquire(lock, subclass, trylock, read, check, nest_lock, ip);
__lock_acquire(lock, subclass, trylock, read, check,
irqs_disabled_flags(flags), nest_lock, ip, 0, 0);
lockdep_recursion_finish();
@@ -5013,13 +5014,15 @@ void lock_release(struct lockdep_map *lock, unsigned long ip)
{
unsigned long flags;
+ trace_lock_release(lock, ip);
+
if (unlikely(current->lockdep_recursion))
return;
raw_local_irq_save(flags);
check_flags(flags);
+
current->lockdep_recursion++;
- trace_lock_release(lock, ip);
if (__lock_release(lock, ip))
check_chain_key(current);
lockdep_recursion_finish();
@@ -5205,8 +5208,6 @@ __lock_acquired(struct lockdep_map *lock, unsigned long ip)
hlock->holdtime_stamp = now;
}
- trace_lock_acquired(lock, ip);
-
stats = get_lock_stats(hlock_class(hlock));
if (waittime) {
if (hlock->read)
@@ -5225,6 +5226,8 @@ void lock_contended(struct lockdep_map *lock, unsigned long ip)
{
unsigned long flags;
+ trace_lock_acquired(lock, ip);
+
if (unlikely(!lock_stat || !debug_locks))
return;
@@ -5234,7 +5237,6 @@ void lock_contended(struct lockdep_map *lock, unsigned long ip)
raw_local_irq_save(flags);
check_flags(flags);
current->lockdep_recursion++;
- trace_lock_contended(lock, ip);
__lock_contended(lock, ip);
lockdep_recursion_finish();
raw_local_irq_restore(flags);
@@ -5245,6 +5247,8 @@ void lock_acquired(struct lockdep_map *lock, unsigned long ip)
{
unsigned long flags;
+ trace_lock_contended(lock, ip);
+
if (unlikely(!lock_stat || !debug_locks))
return;
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [tip: locking/core] arm64: Implement arch_irqs_disabled()
2020-08-21 8:47 ` [PATCH v2 09/11] arm64: " Peter Zijlstra
@ 2020-08-27 7:54 ` tip-bot2 for Peter Zijlstra
0 siblings, 0 replies; 53+ messages in thread
From: tip-bot2 for Peter Zijlstra @ 2020-08-27 7:54 UTC (permalink / raw)
To: linux-tip-commits
Cc: Catalin Marinas, Will Deacon, kernel test robot,
Peter Zijlstra (Intel),
Thomas Gleixner, Rafael J. Wysocki, x86, LKML
The following commit has been merged into the locking/core branch of tip:
Commit-ID: 021c109330ebc1f54b546c63a078ea3c31356ecb
Gitweb: https://git.kernel.org/tip/021c109330ebc1f54b546c63a078ea3c31356ecb
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Fri, 21 Aug 2020 10:40:49 +02:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:55 +02:00
arm64: Implement arch_irqs_disabled()
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Link: https://lkml.kernel.org/r/20200821085348.664425120@infradead.org
---
arch/arm64/include/asm/irqflags.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
index aa4b652..ff328e5 100644
--- a/arch/arm64/include/asm/irqflags.h
+++ b/arch/arm64/include/asm/irqflags.h
@@ -95,6 +95,11 @@ static inline int arch_irqs_disabled_flags(unsigned long flags)
return res;
}
+static inline int arch_irqs_disabled(void)
+{
+ return arch_irqs_disabled_flags(arch_local_save_flags());
+}
+
static inline unsigned long arch_local_irq_save(void)
{
unsigned long flags;
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [tip: locking/core] nds32: Implement arch_irqs_disabled()
2020-08-21 8:47 ` [PATCH v2 08/11] nds32: Implement arch_irqs_disabled() Peter Zijlstra
@ 2020-08-27 7:54 ` tip-bot2 for Peter Zijlstra
0 siblings, 0 replies; 53+ messages in thread
From: tip-bot2 for Peter Zijlstra @ 2020-08-27 7:54 UTC (permalink / raw)
To: linux-tip-commits
Cc: Nick Hu, Greentime Hu, Vincent Chen, kernel test robot,
Peter Zijlstra (Intel), Steven Rostedt (VMware),
Thomas Gleixner, Rafael J. Wysocki, Marco Elver, x86, LKML
The following commit has been merged into the locking/core branch of tip:
Commit-ID: 36206b588bc815e5f64e8da72d7ab79e00b76281
Gitweb: https://git.kernel.org/tip/36206b588bc815e5f64e8da72d7ab79e00b76281
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Thu, 20 Aug 2020 09:27:52 +02:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:55 +02:00
nds32: Implement arch_irqs_disabled()
Cc: Nick Hu <nickhu@andestech.com>
Cc: Greentime Hu <green.hu@gmail.com>
Cc: Vincent Chen <deanbo422@gmail.com>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Marco Elver <elver@google.com>
Link: https://lkml.kernel.org/r/20200821085348.604899379@infradead.org
---
arch/nds32/include/asm/irqflags.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/nds32/include/asm/irqflags.h b/arch/nds32/include/asm/irqflags.h
index fb45ec4..51ef800 100644
--- a/arch/nds32/include/asm/irqflags.h
+++ b/arch/nds32/include/asm/irqflags.h
@@ -34,3 +34,8 @@ static inline int arch_irqs_disabled_flags(unsigned long flags)
{
return !flags;
}
+
+static inline int arch_irqs_disabled(void)
+{
+ return arch_irqs_disabled_flags(arch_local_save_flags());
+}
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [tip: locking/core] mips: Implement arch_irqs_disabled()
2020-08-26 10:16 ` [PATCH v2 12/11] mips: Implement arch_irqs_disabled() peterz
@ 2020-08-27 7:54 ` tip-bot2 for Peter Zijlstra
0 siblings, 0 replies; 53+ messages in thread
From: tip-bot2 for Peter Zijlstra @ 2020-08-27 7:54 UTC (permalink / raw)
To: linux-tip-commits
Cc: Thomas Bogendoerfer, Paul Burton, kernel test robot,
Peter Zijlstra (Intel),
x86, LKML
The following commit has been merged into the locking/core branch of tip:
Commit-ID: 99dc56feb7932020502d40107a712fa302b32082
Gitweb: https://git.kernel.org/tip/99dc56feb7932020502d40107a712fa302b32082
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Sat, 22 Aug 2020 18:04:15 +02:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:55 +02:00
mips: Implement arch_irqs_disabled()
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20200826101653.GE1362448@hirez.programming.kicks-ass.net
---
arch/mips/include/asm/irqflags.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/mips/include/asm/irqflags.h b/arch/mips/include/asm/irqflags.h
index 47a8ffc..f5b8300 100644
--- a/arch/mips/include/asm/irqflags.h
+++ b/arch/mips/include/asm/irqflags.h
@@ -137,6 +137,11 @@ static inline int arch_irqs_disabled_flags(unsigned long flags)
return !(flags & 1);
}
+static inline int arch_irqs_disabled(void)
+{
+ return arch_irqs_disabled_flags(arch_local_save_flags());
+}
+
#endif /* #ifndef __ASSEMBLY__ */
/*
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [tip: locking/core] locking/lockdep: Cleanup
2020-08-21 8:47 ` [PATCH v2 07/11] locking/lockdep: Cleanup Peter Zijlstra
@ 2020-08-27 7:54 ` tip-bot2 for Peter Zijlstra
0 siblings, 0 replies; 53+ messages in thread
From: tip-bot2 for Peter Zijlstra @ 2020-08-27 7:54 UTC (permalink / raw)
To: linux-tip-commits
Cc: Peter Zijlstra (Intel), Steven Rostedt (VMware),
Thomas Gleixner, Rafael J. Wysocki, Marco Elver, x86, LKML
The following commit has been merged into the locking/core branch of tip:
Commit-ID: 00b0ed2d4997af6d0a93edef820386951fd66d94
Gitweb: https://git.kernel.org/tip/00b0ed2d4997af6d0a93edef820386951fd66d94
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 12 Aug 2020 19:28:06 +02:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:54 +02:00
locking/lockdep: Cleanup
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Marco Elver <elver@google.com>
Link: https://lkml.kernel.org/r/20200821085348.546087214@infradead.org
---
include/linux/irqflags.h | 54 +++++++++++++++++++++------------------
1 file changed, 30 insertions(+), 24 deletions(-)
diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index d7e50a2..00d553d 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -49,10 +49,11 @@ struct irqtrace_events {
DECLARE_PER_CPU(int, hardirqs_enabled);
DECLARE_PER_CPU(int, hardirq_context);
- extern void trace_hardirqs_on_prepare(void);
- extern void trace_hardirqs_off_finish(void);
- extern void trace_hardirqs_on(void);
- extern void trace_hardirqs_off(void);
+extern void trace_hardirqs_on_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() (raw_cpu_read(hardirq_context))
# define lockdep_softirq_context(p) ((p)->softirq_context)
# define lockdep_hardirqs_enabled() (this_cpu_read(hardirqs_enabled))
@@ -120,17 +121,17 @@ do { \
#else
# define trace_hardirqs_on_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
-# define lockdep_softirq_context(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)
-# define lockdep_hardirq_exit() do { } while (0)
-# define lockdep_softirq_enter() do { } while (0)
-# define lockdep_softirq_exit() do { } while (0)
+# define trace_hardirqs_on() do { } while (0)
+# define trace_hardirqs_off() do { } while (0)
+# define lockdep_hardirq_context() 0
+# define lockdep_softirq_context(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)
+# define lockdep_hardirq_exit() do { } while (0)
+# define lockdep_softirq_enter() do { } while (0)
+# define lockdep_softirq_exit() do { } while (0)
# define lockdep_hrtimer_enter(__hrtimer) false
# define lockdep_hrtimer_exit(__context) do { } while (0)
# define lockdep_posixtimer_enter() do { } while (0)
@@ -181,17 +182,25 @@ do { \
* if !TRACE_IRQFLAGS.
*/
#ifdef CONFIG_TRACE_IRQFLAGS
-#define local_irq_enable() \
- do { trace_hardirqs_on(); raw_local_irq_enable(); } while (0)
-#define local_irq_disable() \
- do { raw_local_irq_disable(); trace_hardirqs_off(); } while (0)
+
+#define local_irq_enable() \
+ do { \
+ trace_hardirqs_on(); \
+ raw_local_irq_enable(); \
+ } while (0)
+
+#define local_irq_disable() \
+ do { \
+ raw_local_irq_disable(); \
+ trace_hardirqs_off(); \
+ } while (0)
+
#define local_irq_save(flags) \
do { \
raw_local_irq_save(flags); \
trace_hardirqs_off(); \
} while (0)
-
#define local_irq_restore(flags) \
do { \
if (raw_irqs_disabled_flags(flags)) { \
@@ -214,10 +223,7 @@ do { \
#define local_irq_enable() do { raw_local_irq_enable(); } while (0)
#define local_irq_disable() do { raw_local_irq_disable(); } while (0)
-#define local_irq_save(flags) \
- do { \
- raw_local_irq_save(flags); \
- } while (0)
+#define local_irq_save(flags) do { raw_local_irq_save(flags); } while (0)
#define local_irq_restore(flags) do { raw_local_irq_restore(flags); } while (0)
#define safe_halt() do { raw_safe_halt(); } while (0)
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [tip: locking/core] x86/entry: Remove unused THUNKs
2020-08-21 8:47 ` [PATCH v2 06/11] x86/entry: Remove unused THUNKs Peter Zijlstra
@ 2020-08-27 7:54 ` tip-bot2 for Peter Zijlstra
0 siblings, 0 replies; 53+ messages in thread
From: tip-bot2 for Peter Zijlstra @ 2020-08-27 7:54 UTC (permalink / raw)
To: linux-tip-commits
Cc: Peter Zijlstra (Intel), Steven Rostedt (VMware),
Thomas Gleixner, Rafael J. Wysocki, Marco Elver, x86, LKML
The following commit has been merged into the locking/core branch of tip:
Commit-ID: 7da93f379330f2be1122ca7f54ab1eb44ef9aa59
Gitweb: https://git.kernel.org/tip/7da93f379330f2be1122ca7f54ab1eb44ef9aa59
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 12 Aug 2020 19:28:07 +02:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:54 +02:00
x86/entry: Remove unused THUNKs
Unused remnants
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Marco Elver <elver@google.com>
Link: https://lkml.kernel.org/r/20200821085348.487040689@infradead.org
---
arch/x86/entry/thunk_32.S | 5 -----
1 file changed, 5 deletions(-)
diff --git a/arch/x86/entry/thunk_32.S b/arch/x86/entry/thunk_32.S
index 3a07ce3..f1f96d4 100644
--- a/arch/x86/entry/thunk_32.S
+++ b/arch/x86/entry/thunk_32.S
@@ -29,11 +29,6 @@ SYM_CODE_START_NOALIGN(\name)
SYM_CODE_END(\name)
.endm
-#ifdef CONFIG_TRACE_IRQFLAGS
- THUNK trace_hardirqs_on_thunk,trace_hardirqs_on_caller,1
- THUNK trace_hardirqs_off_thunk,trace_hardirqs_off_caller,1
-#endif
-
#ifdef CONFIG_PREEMPTION
THUNK preempt_schedule_thunk, preempt_schedule
THUNK preempt_schedule_notrace_thunk, preempt_schedule_notrace
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [tip: locking/core] cpuidle: Make CPUIDLE_FLAG_TLB_FLUSHED generic
2020-08-21 8:47 ` [PATCH v2 04/11] cpuidle: Make CPUIDLE_FLAG_TLB_FLUSHED generic Peter Zijlstra
@ 2020-08-27 7:54 ` tip-bot2 for Peter Zijlstra
0 siblings, 0 replies; 53+ messages in thread
From: tip-bot2 for Peter Zijlstra @ 2020-08-27 7:54 UTC (permalink / raw)
To: linux-tip-commits
Cc: Peter Zijlstra (Intel), Steven Rostedt (VMware),
Thomas Gleixner, Rafael J. Wysocki, Marco Elver, x86, LKML
The following commit has been merged into the locking/core branch of tip:
Commit-ID: bf9282dc26e7fe2a0736edc568762f0f05d12416
Gitweb: https://git.kernel.org/tip/bf9282dc26e7fe2a0736edc568762f0f05d12416
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 12 Aug 2020 12:22:17 +02:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:53 +02:00
cpuidle: Make CPUIDLE_FLAG_TLB_FLUSHED generic
This allows moving the leave_mm() call into generic code before
rcu_idle_enter(). Gets rid of more trace_*_rcuidle() users.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Marco Elver <elver@google.com>
Link: https://lkml.kernel.org/r/20200821085348.369441600@infradead.org
---
arch/x86/include/asm/mmu.h | 1 +
arch/x86/mm/tlb.c | 13 ++-----------
drivers/cpuidle/cpuidle.c | 4 ++++
drivers/idle/intel_idle.c | 16 ----------------
include/linux/cpuidle.h | 13 +++++++------
include/linux/mmu_context.h | 5 +++++
6 files changed, 19 insertions(+), 33 deletions(-)
diff --git a/arch/x86/include/asm/mmu.h b/arch/x86/include/asm/mmu.h
index 0a301ad..9257667 100644
--- a/arch/x86/include/asm/mmu.h
+++ b/arch/x86/include/asm/mmu.h
@@ -59,5 +59,6 @@ typedef struct {
}
void leave_mm(int cpu);
+#define leave_mm leave_mm
#endif /* _ASM_X86_MMU_H */
diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index 1a3569b..0951b47 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -555,21 +555,12 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
this_cpu_write(cpu_tlbstate.ctxs[new_asid].tlb_gen, next_tlb_gen);
load_new_mm_cr3(next->pgd, new_asid, true);
- /*
- * NB: This gets called via leave_mm() in the idle path
- * where RCU functions differently. Tracing normally
- * uses RCU, so we need to use the _rcuidle variant.
- *
- * (There is no good reason for this. The idle code should
- * be rearranged to call this before rcu_idle_enter().)
- */
- trace_tlb_flush_rcuidle(TLB_FLUSH_ON_TASK_SWITCH, TLB_FLUSH_ALL);
+ trace_tlb_flush(TLB_FLUSH_ON_TASK_SWITCH, TLB_FLUSH_ALL);
} else {
/* The new ASID is already up to date. */
load_new_mm_cr3(next->pgd, new_asid, false);
- /* See above wrt _rcuidle. */
- trace_tlb_flush_rcuidle(TLB_FLUSH_ON_TASK_SWITCH, 0);
+ trace_tlb_flush(TLB_FLUSH_ON_TASK_SWITCH, 0);
}
/* Make sure we write CR3 before loaded_mm. */
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 9bcda41..04becd7 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -22,6 +22,7 @@
#include <linux/module.h>
#include <linux/suspend.h>
#include <linux/tick.h>
+#include <linux/mmu_context.h>
#include <trace/events/power.h>
#include "cpuidle.h"
@@ -228,6 +229,9 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
broadcast = false;
}
+ if (target_state->flags & CPUIDLE_FLAG_TLB_FLUSHED)
+ leave_mm(dev->cpu);
+
/* Take note of the planned idle state. */
sched_idle_set_state(target_state);
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 8e0fb1a..9a810e4 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -90,14 +90,6 @@ static unsigned int mwait_substates __initdata;
#define CPUIDLE_FLAG_ALWAYS_ENABLE BIT(15)
/*
- * Set this flag for states where the HW flushes the TLB for us
- * and so we don't need cross-calls to keep it consistent.
- * If this flag is set, SW flushes the TLB, so even if the
- * HW doesn't do the flushing, this flag is safe to use.
- */
-#define CPUIDLE_FLAG_TLB_FLUSHED BIT(16)
-
-/*
* MWAIT takes an 8-bit "hint" in EAX "suggesting"
* the C-state (top nibble) and sub-state (bottom nibble)
* 0x00 means "MWAIT(C1)", 0x10 means "MWAIT(C2)" etc.
@@ -131,14 +123,6 @@ static __cpuidle int intel_idle(struct cpuidle_device *dev,
unsigned long eax = flg2MWAIT(state->flags);
unsigned long ecx = 1; /* break on interrupt flag */
bool tick;
- int cpu = smp_processor_id();
-
- /*
- * leave_mm() to avoid costly and often unnecessary wakeups
- * for flushing the user TLB's associated with the active mm.
- */
- if (state->flags & CPUIDLE_FLAG_TLB_FLUSHED)
- leave_mm(cpu);
if (!static_cpu_has(X86_FEATURE_ARAT)) {
/*
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index b65909a..75895e6 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -75,12 +75,13 @@ struct cpuidle_state {
};
/* Idle State Flags */
-#define CPUIDLE_FLAG_NONE (0x00)
-#define CPUIDLE_FLAG_POLLING BIT(0) /* polling state */
-#define CPUIDLE_FLAG_COUPLED BIT(1) /* state applies to multiple cpus */
-#define CPUIDLE_FLAG_TIMER_STOP BIT(2) /* timer is stopped on this state */
-#define CPUIDLE_FLAG_UNUSABLE BIT(3) /* avoid using this state */
-#define CPUIDLE_FLAG_OFF BIT(4) /* disable this state by default */
+#define CPUIDLE_FLAG_NONE (0x00)
+#define CPUIDLE_FLAG_POLLING BIT(0) /* polling state */
+#define CPUIDLE_FLAG_COUPLED BIT(1) /* state applies to multiple cpus */
+#define CPUIDLE_FLAG_TIMER_STOP BIT(2) /* timer is stopped on this state */
+#define CPUIDLE_FLAG_UNUSABLE BIT(3) /* avoid using this state */
+#define CPUIDLE_FLAG_OFF BIT(4) /* disable this state by default */
+#define CPUIDLE_FLAG_TLB_FLUSHED BIT(5) /* idle-state flushes TLBs */
struct cpuidle_device_kobj;
struct cpuidle_state_kobj;
diff --git a/include/linux/mmu_context.h b/include/linux/mmu_context.h
index c51a841..03dee12 100644
--- a/include/linux/mmu_context.h
+++ b/include/linux/mmu_context.h
@@ -3,10 +3,15 @@
#define _LINUX_MMU_CONTEXT_H
#include <asm/mmu_context.h>
+#include <asm/mmu.h>
/* Architectures that care about IRQ state in switch_mm can override this. */
#ifndef switch_mm_irqs_off
# define switch_mm_irqs_off switch_mm
#endif
+#ifndef leave_mm
+static inline void leave_mm(int cpu) { }
+#endif
+
#endif
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [tip: locking/core] cpuidle: Move trace_cpu_idle() into generic code
2020-08-21 8:47 ` [PATCH v2 05/11] cpuidle: Move trace_cpu_idle() into generic code Peter Zijlstra
@ 2020-08-27 7:54 ` tip-bot2 for Peter Zijlstra
0 siblings, 0 replies; 53+ messages in thread
From: tip-bot2 for Peter Zijlstra @ 2020-08-27 7:54 UTC (permalink / raw)
To: linux-tip-commits
Cc: Peter Zijlstra (Intel), Steven Rostedt (VMware),
Thomas Gleixner, Rafael J. Wysocki, Marco Elver, x86, LKML
The following commit has been merged into the locking/core branch of tip:
Commit-ID: 9864f5b5943ab0f1f835f21dc3f9f068d06f5b52
Gitweb: https://git.kernel.org/tip/9864f5b5943ab0f1f835f21dc3f9f068d06f5b52
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 12 Aug 2020 12:27:10 +02:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:54 +02:00
cpuidle: Move trace_cpu_idle() into generic code
Remove trace_cpu_idle() from the arch_cpu_idle() implementations and
put it in the generic code, right before disabling RCU. Gets rid of
more trace_*_rcuidle() users.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Marco Elver <elver@google.com>
Link: https://lkml.kernel.org/r/20200821085348.428433395@infradead.org
---
arch/arm/mach-omap2/pm34xx.c | 4 ----
arch/arm64/kernel/process.c | 2 --
arch/s390/kernel/idle.c | 3 +--
arch/x86/kernel/process.c | 4 ----
kernel/sched/idle.c | 3 +++
5 files changed, 4 insertions(+), 12 deletions(-)
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 6df395f..f5dfddf 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -298,11 +298,7 @@ static void omap3_pm_idle(void)
if (omap_irq_pending())
return;
- trace_cpu_idle_rcuidle(1, smp_processor_id());
-
omap_sram_idle();
-
- trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
}
#ifdef CONFIG_SUSPEND
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index b63ce4c..f180449 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -123,10 +123,8 @@ void arch_cpu_idle(void)
* This should do all the clock switching and wait for interrupt
* tricks
*/
- trace_cpu_idle_rcuidle(1, smp_processor_id());
cpu_do_idle();
local_irq_enable();
- trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
}
#ifdef CONFIG_HOTPLUG_CPU
diff --git a/arch/s390/kernel/idle.c b/arch/s390/kernel/idle.c
index 88bb42c..c73f506 100644
--- a/arch/s390/kernel/idle.c
+++ b/arch/s390/kernel/idle.c
@@ -33,14 +33,13 @@ void enabled_wait(void)
PSW_MASK_IO | PSW_MASK_EXT | PSW_MASK_MCHECK;
clear_cpu_flag(CIF_NOHZ_DELAY);
- trace_cpu_idle_rcuidle(1, smp_processor_id());
local_irq_save(flags);
/* Call the assembler magic in entry.S */
psw_idle(idle, psw_mask);
local_irq_restore(flags);
- trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
/* Account time spent with enabled wait psw loaded as idle time. */
+ /* XXX seqcount has tracepoints that require RCU */
write_seqcount_begin(&idle->seqcount);
idle_time = idle->clock_idle_exit - idle->clock_idle_enter;
idle->clock_idle_enter = idle->clock_idle_exit = 0ULL;
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 994d839..13ce616 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -684,9 +684,7 @@ void arch_cpu_idle(void)
*/
void __cpuidle default_idle(void)
{
- trace_cpu_idle_rcuidle(1, smp_processor_id());
safe_halt();
- trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
}
#if defined(CONFIG_APM_MODULE) || defined(CONFIG_HALTPOLL_CPUIDLE_MODULE)
EXPORT_SYMBOL(default_idle);
@@ -792,7 +790,6 @@ static int prefer_mwait_c1_over_halt(const struct cpuinfo_x86 *c)
static __cpuidle void mwait_idle(void)
{
if (!current_set_polling_and_test()) {
- trace_cpu_idle_rcuidle(1, smp_processor_id());
if (this_cpu_has(X86_BUG_CLFLUSH_MONITOR)) {
mb(); /* quirk */
clflush((void *)¤t_thread_info()->flags);
@@ -804,7 +801,6 @@ static __cpuidle void mwait_idle(void)
__sti_mwait(0, 0);
else
local_irq_enable();
- trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
} else {
local_irq_enable();
}
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index ea3a098..f324dc3 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -91,11 +91,14 @@ void __cpuidle default_idle_call(void)
if (current_clr_polling_and_test()) {
local_irq_enable();
} else {
+
+ trace_cpu_idle(1, smp_processor_id());
stop_critical_timings();
rcu_idle_enter();
arch_cpu_idle();
rcu_idle_exit();
start_critical_timings();
+ trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
}
}
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [tip: locking/core] lockdep: Use raw_cpu_*() for per-cpu variables
2020-08-21 8:47 ` [PATCH v2 01/11] lockdep: Use raw_cpu_*() for per-cpu variables Peter Zijlstra
2020-08-27 1:05 ` Joel Fernandes
@ 2020-08-27 7:54 ` tip-bot2 for Peter Zijlstra
1 sibling, 0 replies; 53+ messages in thread
From: tip-bot2 for Peter Zijlstra @ 2020-08-27 7:54 UTC (permalink / raw)
To: linux-tip-commits
Cc: Sven Schnelle, Steven Rostedt (VMware),
Thomas Gleixner, Rafael J. Wysocki, Marco Elver,
Peter Zijlstra (Intel),
x86, LKML
The following commit has been merged into the locking/core branch of tip:
Commit-ID: fddf9055a60dfcc97bda5ef03c8fa4108ed555c5
Gitweb: https://git.kernel.org/tip/fddf9055a60dfcc97bda5ef03c8fa4108ed555c5
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Thu, 20 Aug 2020 09:13:30 +02:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:53 +02:00
lockdep: Use raw_cpu_*() for per-cpu variables
Sven reported that commit a21ee6055c30 ("lockdep: Change
hardirq{s_enabled,_context} to per-cpu variables") caused trouble on
s390 because their this_cpu_*() primitives disable preemption which
then lands back tracing.
On the one hand, per-cpu ops should use preempt_*able_notrace() and
raw_local_irq_*(), on the other hand, we can trivialy use raw_cpu_*()
ops for this.
Fixes: a21ee6055c30 ("lockdep: Change hardirq{s_enabled,_context} to per-cpu variables")
Reported-by: Sven Schnelle <svens@linux.ibm.com>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Marco Elver <elver@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20200821085348.192346882@infradead.org
---
include/linux/irqflags.h | 6 +++---
include/linux/lockdep.h | 18 +++++++++++++-----
kernel/locking/lockdep.c | 4 ++--
3 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index bd5c557..d7e50a2 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -53,13 +53,13 @@ DECLARE_PER_CPU(int, hardirq_context);
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))
+# define lockdep_hardirq_context() (raw_cpu_read(hardirq_context))
# define lockdep_softirq_context(p) ((p)->softirq_context)
# define lockdep_hardirqs_enabled() (this_cpu_read(hardirqs_enabled))
# define lockdep_softirqs_enabled(p) ((p)->softirqs_enabled)
# define lockdep_hardirq_enter() \
do { \
- if (this_cpu_inc_return(hardirq_context) == 1) \
+ if (__this_cpu_inc_return(hardirq_context) == 1)\
current->hardirq_threaded = 0; \
} while (0)
# define lockdep_hardirq_threaded() \
@@ -68,7 +68,7 @@ do { \
} while (0)
# define lockdep_hardirq_exit() \
do { \
- this_cpu_dec(hardirq_context); \
+ __this_cpu_dec(hardirq_context); \
} while (0)
# define lockdep_softirq_enter() \
do { \
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 62a382d..6a584b3 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -535,19 +535,27 @@ do { \
DECLARE_PER_CPU(int, hardirqs_enabled);
DECLARE_PER_CPU(int, hardirq_context);
+/*
+ * The below lockdep_assert_*() macros use raw_cpu_read() to access the above
+ * per-cpu variables. This is required because this_cpu_read() will potentially
+ * call into preempt/irq-disable and that obviously isn't right. This is also
+ * correct because when IRQs are enabled, it doesn't matter if we accidentally
+ * read the value from our previous CPU.
+ */
+
#define lockdep_assert_irqs_enabled() \
do { \
- WARN_ON_ONCE(debug_locks && !this_cpu_read(hardirqs_enabled)); \
+ WARN_ON_ONCE(debug_locks && !raw_cpu_read(hardirqs_enabled)); \
} while (0)
#define lockdep_assert_irqs_disabled() \
do { \
- WARN_ON_ONCE(debug_locks && this_cpu_read(hardirqs_enabled)); \
+ WARN_ON_ONCE(debug_locks && raw_cpu_read(hardirqs_enabled)); \
} while (0)
#define lockdep_assert_in_irq() \
do { \
- WARN_ON_ONCE(debug_locks && !this_cpu_read(hardirq_context)); \
+ WARN_ON_ONCE(debug_locks && !raw_cpu_read(hardirq_context)); \
} while (0)
#define lockdep_assert_preemption_enabled() \
@@ -555,7 +563,7 @@ do { \
WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_COUNT) && \
debug_locks && \
(preempt_count() != 0 || \
- !this_cpu_read(hardirqs_enabled))); \
+ !raw_cpu_read(hardirqs_enabled))); \
} while (0)
#define lockdep_assert_preemption_disabled() \
@@ -563,7 +571,7 @@ do { \
WARN_ON_ONCE(IS_ENABLED(CONFIG_PREEMPT_COUNT) && \
debug_locks && \
(preempt_count() == 0 && \
- this_cpu_read(hardirqs_enabled))); \
+ raw_cpu_read(hardirqs_enabled))); \
} while (0)
#else
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 2fad21d..c872e95 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -3756,7 +3756,7 @@ void noinstr lockdep_hardirqs_on(unsigned long ip)
skip_checks:
/* we'll do an OFF -> ON transition: */
- this_cpu_write(hardirqs_enabled, 1);
+ __this_cpu_write(hardirqs_enabled, 1);
trace->hardirq_enable_ip = ip;
trace->hardirq_enable_event = ++trace->irq_events;
debug_atomic_inc(hardirqs_on_events);
@@ -3795,7 +3795,7 @@ void noinstr lockdep_hardirqs_off(unsigned long ip)
/*
* We have done an ON -> OFF transition:
*/
- this_cpu_write(hardirqs_enabled, 0);
+ __this_cpu_write(hardirqs_enabled, 0);
trace->hardirq_disable_ip = ip;
trace->hardirq_disable_event = ++trace->irq_events;
debug_atomic_inc(hardirqs_off_events);
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [tip: locking/core] cpuidle: Fixup IRQ state
2020-08-21 8:47 ` [PATCH v2 02/11] cpuidle: Fixup IRQ state Peter Zijlstra
2020-08-21 17:36 ` Rafael J. Wysocki
@ 2020-08-27 7:54 ` tip-bot2 for Peter Zijlstra
1 sibling, 0 replies; 53+ messages in thread
From: tip-bot2 for Peter Zijlstra @ 2020-08-27 7:54 UTC (permalink / raw)
To: linux-tip-commits
Cc: Peter Zijlstra (Intel), Steven Rostedt (VMware),
Thomas Gleixner, Rafael J. Wysocki, Marco Elver, x86, LKML
The following commit has been merged into the locking/core branch of tip:
Commit-ID: 49d9c5936314e44d314c605c39cce0fd947f9c3a
Gitweb: https://git.kernel.org/tip/49d9c5936314e44d314c605c39cce0fd947f9c3a
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Thu, 20 Aug 2020 16:47:24 +02:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:53 +02:00
cpuidle: Fixup IRQ state
Match the pattern elsewhere in this file.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Marco Elver <elver@google.com>
Link: https://lkml.kernel.org/r/20200821085348.251340558@infradead.org
---
drivers/cpuidle/cpuidle.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 8719731..2fe4f3c 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -153,7 +153,8 @@ static void enter_s2idle_proper(struct cpuidle_driver *drv,
*/
stop_critical_timings();
drv->states[index].enter_s2idle(dev, drv, index);
- WARN_ON(!irqs_disabled());
+ if (WARN_ON_ONCE(!irqs_disabled()))
+ local_irq_disable();
/*
* timekeeping_resume() that will be called by tick_unfreeze() for the
* first CPU executing it calls functions containing RCU read-side
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [tip: locking/core] sched,idle,rcu: Push rcu_idle deeper into the idle path
2020-08-21 8:47 ` [PATCH v2 03/11] sched,idle,rcu: Push rcu_idle deeper into the idle path Peter Zijlstra
2020-08-27 1:18 ` Joel Fernandes
@ 2020-08-27 7:54 ` tip-bot2 for Peter Zijlstra
1 sibling, 0 replies; 53+ messages in thread
From: tip-bot2 for Peter Zijlstra @ 2020-08-27 7:54 UTC (permalink / raw)
To: linux-tip-commits
Cc: Peter Zijlstra (Intel), Steven Rostedt (VMware),
Thomas Gleixner, Rafael J. Wysocki, Marco Elver, x86, LKML
The following commit has been merged into the locking/core branch of tip:
Commit-ID: 1098582a0f6c4e8fd28da0a6305f9233d02c9c1d
Gitweb: https://git.kernel.org/tip/1098582a0f6c4e8fd28da0a6305f9233d02c9c1d
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Fri, 07 Aug 2020 20:50:19 +02:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 26 Aug 2020 12:41:53 +02:00
sched,idle,rcu: Push rcu_idle deeper into the idle path
Lots of things take locks, due to a wee bug, rcu_lockdep didn't notice
that the locking tracepoints were using RCU.
Push rcu_idle_{enter,exit}() as deep as possible into the idle paths,
this also resolves a lot of _rcuidle()/RCU_NONIDLE() usage.
Specifically, sched_clock_idle_wakeup_event() will use ktime which
will use seqlocks which will tickle lockdep, and
stop_critical_timings() uses lock.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Marco Elver <elver@google.com>
Link: https://lkml.kernel.org/r/20200821085348.310943801@infradead.org
---
drivers/cpuidle/cpuidle.c | 12 ++++++++----
kernel/sched/idle.c | 22 ++++++++--------------
2 files changed, 16 insertions(+), 18 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 2fe4f3c..9bcda41 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -145,13 +145,14 @@ static void enter_s2idle_proper(struct cpuidle_driver *drv,
* executing it contains RCU usage regarded as invalid in the idle
* context, so tell RCU about that.
*/
- RCU_NONIDLE(tick_freeze());
+ tick_freeze();
/*
* The state used here cannot be a "coupled" one, because the "coupled"
* cpuidle mechanism enables interrupts and doing that with timekeeping
* suspended is generally unsafe.
*/
stop_critical_timings();
+ rcu_idle_enter();
drv->states[index].enter_s2idle(dev, drv, index);
if (WARN_ON_ONCE(!irqs_disabled()))
local_irq_disable();
@@ -160,7 +161,8 @@ static void enter_s2idle_proper(struct cpuidle_driver *drv,
* first CPU executing it calls functions containing RCU read-side
* critical sections, so tell RCU about that.
*/
- RCU_NONIDLE(tick_unfreeze());
+ rcu_idle_exit();
+ tick_unfreeze();
start_critical_timings();
time_end = ns_to_ktime(local_clock());
@@ -229,16 +231,18 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
/* Take note of the planned idle state. */
sched_idle_set_state(target_state);
- trace_cpu_idle_rcuidle(index, dev->cpu);
+ trace_cpu_idle(index, dev->cpu);
time_start = ns_to_ktime(local_clock());
stop_critical_timings();
+ rcu_idle_enter();
entered_state = target_state->enter(dev, drv, index);
+ rcu_idle_exit();
start_critical_timings();
sched_clock_idle_wakeup_event();
time_end = ns_to_ktime(local_clock());
- trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
+ trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
/* The cpu is no longer idle or about to enter idle. */
sched_idle_set_state(NULL);
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index 6bf3498..ea3a098 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -54,17 +54,18 @@ __setup("hlt", cpu_idle_nopoll_setup);
static noinline int __cpuidle cpu_idle_poll(void)
{
+ trace_cpu_idle(0, smp_processor_id());
+ stop_critical_timings();
rcu_idle_enter();
- trace_cpu_idle_rcuidle(0, smp_processor_id());
local_irq_enable();
- stop_critical_timings();
while (!tif_need_resched() &&
- (cpu_idle_force_poll || tick_check_broadcast_expired()))
+ (cpu_idle_force_poll || tick_check_broadcast_expired()))
cpu_relax();
- start_critical_timings();
- trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
+
rcu_idle_exit();
+ start_critical_timings();
+ trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
return 1;
}
@@ -91,7 +92,9 @@ void __cpuidle default_idle_call(void)
local_irq_enable();
} else {
stop_critical_timings();
+ rcu_idle_enter();
arch_cpu_idle();
+ rcu_idle_exit();
start_critical_timings();
}
}
@@ -158,7 +161,6 @@ static void cpuidle_idle_call(void)
if (cpuidle_not_available(drv, dev)) {
tick_nohz_idle_stop_tick();
- rcu_idle_enter();
default_idle_call();
goto exit_idle;
@@ -178,21 +180,17 @@ static void cpuidle_idle_call(void)
u64 max_latency_ns;
if (idle_should_enter_s2idle()) {
- rcu_idle_enter();
entered_state = call_cpuidle_s2idle(drv, dev);
if (entered_state > 0)
goto exit_idle;
- rcu_idle_exit();
-
max_latency_ns = U64_MAX;
} else {
max_latency_ns = dev->forced_idle_latency_limit_ns;
}
tick_nohz_idle_stop_tick();
- rcu_idle_enter();
next_state = cpuidle_find_deepest_state(drv, dev, max_latency_ns);
call_cpuidle(drv, dev, next_state);
@@ -209,8 +207,6 @@ static void cpuidle_idle_call(void)
else
tick_nohz_idle_retain_tick();
- rcu_idle_enter();
-
entered_state = call_cpuidle(drv, dev, next_state);
/*
* Give the governor an opportunity to reflect on the outcome
@@ -226,8 +222,6 @@ exit_idle:
*/
if (WARN_ON_ONCE(irqs_disabled()))
local_irq_enable();
-
- rcu_idle_exit();
}
/*
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v2 03/11] sched,idle,rcu: Push rcu_idle deeper into the idle path
2020-08-27 7:47 ` peterz
2020-08-27 7:53 ` Thomas Gleixner
@ 2020-08-27 22:30 ` Joel Fernandes
2020-09-02 7:08 ` peterz
1 sibling, 1 reply; 53+ messages in thread
From: Joel Fernandes @ 2020-08-27 22:30 UTC (permalink / raw)
To: peterz
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, svens, tglx, mathieu.desnoyers
On Thu, Aug 27, 2020 at 09:47:48AM +0200, peterz@infradead.org wrote:
> On Wed, Aug 26, 2020 at 09:24:19PM -0400, Joel Fernandes wrote:
> > On Wed, Aug 26, 2020 at 09:18:26PM -0400, Joel Fernandes wrote:
> > > On Fri, Aug 21, 2020 at 10:47:41AM +0200, Peter Zijlstra wrote:
> > > > Lots of things take locks, due to a wee bug, rcu_lockdep didn't notice
> > > > that the locking tracepoints were using RCU.
> > > >
> > > > Push rcu_idle_{enter,exit}() as deep as possible into the idle paths,
> > > > this also resolves a lot of _rcuidle()/RCU_NONIDLE() usage.
> > > >
> > > > Specifically, sched_clock_idle_wakeup_event() will use ktime which
> > > > will use seqlocks which will tickle lockdep, and
> > > > stop_critical_timings() uses lock.
> > >
> > > I was wondering if those tracepoints should just use _rcuidle variant of the
> > > trace call. But that's a terrible idea considering that would add unwanted
> > > overhead I think.
> > >
> > > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> >
> > BTW, if tracepoint is converted to use RCU-trace flavor, then these kinds of
> > issues go away, no? That RCU flavor is always watching.
>
> All trace_*_rcuidle() and RCU_NONIDLE() usage is a bug IMO.
>
> Ideally RCU-trace goes away too.
I was thinking that unless the rcu_idle_enter/exit calls coincide with points
in the kernel where instrumentation is not allowed, there is always a chance
somebody wants to use tracepoints after rcu_idle_enter or before exit. In
this case, trace_*_rcuidle() is unavoidable unless you can move the
rcu_idle_enter/exit calls deeper as you are doing.
Maybe objtool can help with that?
The other solution is RCU-trace if you can't push the rcu_idle_enter/exit
calls any deeper and still want to get rid of trace_*_rcuidle thingies. Me
and Mathieu were talking at LPC about tracepoint conversion to RCU-trace and
we can work on it if that's the right direction.
thanks,
- Joel
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints
2020-08-21 8:47 ` [PATCH v2 11/11] lockdep,trace: Expose tracepoints Peter Zijlstra
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
@ 2020-09-02 3:51 ` Guenter Roeck
2020-09-02 7:24 ` peterz
2020-09-02 8:56 ` peterz
1 sibling, 2 replies; 53+ messages in thread
From: Guenter Roeck @ 2020-09-02 3:51 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, joel, svens, tglx
On Fri, Aug 21, 2020 at 10:47:49AM +0200, Peter Zijlstra wrote:
> The lockdep tracepoints are under the lockdep recursion counter, this
> has a bunch of nasty side effects:
>
> - TRACE_IRQFLAGS doesn't work across the entire tracepoint
>
> - RCU-lockdep doesn't see the tracepoints either, hiding numerous
> "suspicious RCU usage" warnings.
>
> Pull the trace_lock_*() tracepoints completely out from under the
> lockdep recursion handling and completely rely on the trace level
> recusion handling -- also, tracing *SHOULD* not be taking locks in any
> case.
>
Wonder what is worse - the problem or its fix. This patch results in
a number of WARNING backtraces for several archtectures/platforms.
Reverting it fixes the problems.
Guenter
---
arm:
[ 27.055084]
[ 27.056213] =============================
[ 27.056274] WARNING: suspicious RCU usage
[ 27.056335] 5.9.0-rc3 #1 Not tainted
[ 27.056396] -----------------------------
[ 27.056457] include/trace/events/lock.h:13 suspicious rcu_dereference_check() usage!
[ 27.056549]
[ 27.056549] other info that might help us debug this:
[ 27.056549]
[ 27.056640]
[ 27.056640] rcu_scheduler_active = 2, debug_locks = 1
[ 27.056732] RCU used illegally from extended quiescent state!
[ 27.056793] no locks held by swapper/0/0.
[ 27.056854]
[ 27.056854] stack backtrace:
[ 27.056915] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.9.0-rc3 #1
[ 27.057006] Hardware name: Generic OMAP3-GP (Flattened Device Tree)
[ 27.057098] [<c03125b4>] (unwind_backtrace) from [<c030c32c>] (show_stack+0x10/0x14)
[ 27.057189] [<c030c32c>] (show_stack) from [<c08e7a4c>] (dump_stack+0xd8/0xf8)
[ 27.057312] [<c08e7a4c>] (dump_stack) from [<c03b04bc>] (lock_acquire+0x4d8/0x4dc)
[ 27.057464] [<c03b04bc>] (lock_acquire) from [<c12457e8>] (_raw_spin_lock_irqsave+0x58/0x74)
[ 27.057617] [<c12457e8>] (_raw_spin_lock_irqsave) from [<c0338198>] (pwrdm_lock+0x10/0x18)
[ 27.057739] [<c0338198>] (pwrdm_lock) from [<c033a214>] (clkdm_deny_idle+0x10/0x24)
[ 27.057891] [<c033a214>] (clkdm_deny_idle) from [<c0332e10>] (omap3_enter_idle_bm+0xd4/0x1b8)
[ 27.058044] [<c0332e10>] (omap3_enter_idle_bm) from [<c0f740d4>] (cpuidle_enter_state+0x16c/0x620)
[ 27.058197] [<c0f740d4>] (cpuidle_enter_state) from [<c0f745ec>] (cpuidle_enter+0x50/0x54)
[ 27.058349] [<c0f745ec>] (cpuidle_enter) from [<c0383aac>] (do_idle+0x228/0x2b8)
[ 27.058471] [<c0383aac>] (do_idle) from [<c0383f34>] (cpu_startup_entry+0x18/0x1c)
[ 27.058624] [<c0383f34>] (cpu_startup_entry) from [<c1c00e8c>] (start_kernel+0x518/0x558)
[ 27.059692]
[ 27.059753] =============================
[ 27.059753] WARNING: suspicious RCU usage
[ 27.059753] 5.9.0-rc3 #1 Not tainted
[ 27.059753] -----------------------------
[ 27.059753] include/trace/events/lock.h:58 suspicious rcu_dereference_check() usage!
[ 27.059783]
[ 27.059783] other info that might help us debug this:
[ 27.059783]
[ 27.059783]
[ 27.059783] rcu_scheduler_active = 2, debug_locks = 1
[ 27.059783] RCU used illegally from extended quiescent state!
[ 27.059783] 1 lock held by swapper/0/0:
[ 27.059814] #0: c1e30f3c (logbuf_lock){-...}-{2:2}, at: vprintk_emit+0x60/0x38c
[ 27.059906]
[ 27.059936] stack backtrace:
[ 27.059936] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.9.0-rc3 #1
[ 27.059936] Hardware name: Generic OMAP3-GP (Flattened Device Tree)
[ 27.059936] [<c03125b4>] (unwind_backtrace) from [<c030c32c>] (show_stack+0x10/0x14)
[ 27.059936] [<c030c32c>] (show_stack) from [<c08e7a4c>] (dump_stack+0xd8/0xf8)
[ 27.059936] [<c08e7a4c>] (dump_stack) from [<c03b4e70>] (lock_release+0x41c/0x420)
[ 27.059936] [<c03b4e70>] (lock_release) from [<c124598c>] (_raw_spin_unlock+0x14/0x38)
[ 27.059967] [<c124598c>] (_raw_spin_unlock) from [<c03c34c8>] (vprintk_emit+0xb4/0x38c)
[ 27.059967] [<c03c34c8>] (vprintk_emit) from [<c03c37c0>] (vprintk_default+0x20/0x28)
[ 27.059967] [<c03c37c0>] (vprintk_default) from [<c03c4088>] (printk+0x30/0x5c)
[ 27.059967] [<c03c4088>] (printk) from [<c03b6c0c>] (lockdep_rcu_suspicious+0x2c/0xec)
[ 27.059967] [<c03b6c0c>] (lockdep_rcu_suspicious) from [<c03b04bc>] (lock_acquire+0x4d8/0x4dc)
[ 27.059967] [<c03b04bc>] (lock_acquire) from [<c12457e8>] (_raw_spin_lock_irqsave+0x58/0x74)
[ 27.059997] [<c12457e8>] (_raw_spin_lock_irqsave) from [<c0338198>] (pwrdm_lock+0x10/0x18)
[ 27.059997] [<c0338198>] (pwrdm_lock) from [<c033a214>] (clkdm_deny_idle+0x10/0x24)
[ 27.059997] [<c033a214>] (clkdm_deny_idle) from [<c0332e10>] (omap3_enter_idle_bm+0xd4/0x1b8)
[ 27.059997] [<c0332e10>] (omap3_enter_idle_bm) from [<c0f740d4>] (cpuidle_enter_state+0x16c/0x620)
[ 27.059997] [<c0f740d4>] (cpuidle_enter_state) from [<c0f745ec>] (cpuidle_enter+0x50/0x54)
[ 27.059997] [<c0f745ec>] (cpuidle_enter) from [<c0383aac>] (do_idle+0x228/0x2b8)
[ 27.059997] [<c0383aac>] (do_idle) from [<c0383f34>] (cpu_startup_entry+0x18/0x1c)
[ 27.060028] [<c0383f34>] (cpu_startup_entry) from [<c1c00e8c>] (start_kernel+0x518/0x558)
s390:
[ 19.490586] =============================
[ 19.490752] WARNING: suspicious RCU usage
[ 19.490921] 5.9.0-rc3 #1 Not tainted
[ 19.491086] -----------------------------
[ 19.491253] include/trace/events/lock.h:37 suspicious rcu_dereference_check() usage!
[ 19.491418]
[ 19.491418] other info that might help us debug this:
[ 19.491418]
[ 19.491613]
[ 19.491613] rcu_scheduler_active = 2, debug_locks = 1
[ 19.491779] RCU used illegally from extended quiescent state!
[ 19.491950] no locks held by swapper/0/0.
[ 19.492118]
[ 19.492118] stack backtrace:
[ 19.492292] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.9.0-rc3 #1
[ 19.492458] Hardware name: QEMU 2964 QEMU (KVM/Linux)
[ 19.492622] Call Trace:
[ 19.492798] [<00000000001164f2>] show_stack+0x9a/0x100
[ 19.492972] [<0000000000982b94>] dump_stack+0x9c/0xd0
[ 19.493147] [<00000000001d5de2>] lock_acquire+0x41a/0x498
[ 19.493320] [<0000000000103b72>] enabled_wait+0xca/0x198
[ 19.493493] [<0000000000103f80>] arch_cpu_idle+0x20/0x38
[ 19.493667] [<0000000000e03c30>] default_idle_call+0x98/0x300
[ 19.493847] [<000000000019958c>] do_idle+0xe4/0x190
[ 19.494021] [<00000000001998f4>] cpu_startup_entry+0x34/0x38
[ 19.494196] [<00000000013a297a>] arch_call_rest_init+0x92/0x98
[ 19.494362] no locks held by swapper/0/0.
[ 19.497570]
[ 19.497595] =============================
[ 19.497605] WARNING: suspicious RCU usage
[ 19.497614] 5.9.0-rc3 #1 Not tainted
[ 19.497624] -----------------------------
[ 19.497634] include/trace/events/lock.h:63 suspicious rcu_dereference_check() usage!
[ 19.497643]
[ 19.497652] other info that might help us debug this:
[ 19.497661]
[ 19.497669]
[ 19.497679] rcu_scheduler_active = 2, debug_locks = 1
[ 19.497689] RCU used illegally from extended quiescent state!
[ 19.497698] 1 lock held by swapper/0/0:
[ 19.497712] #0: 0000000001252750 (logbuf_lock){-...}-{2:2}, at: vprintk_emit+0x70/0x368
[ 19.497779]
[ 19.497788] stack backtrace:
[ 19.497797] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.9.0-rc3 #1
[ 19.497807] Hardware name: QEMU 2964 QEMU (KVM/Linux)
[ 19.497816] Call Trace:
[ 19.497825] [<00000000001164f2>] show_stack+0x9a/0x100
[ 19.497835] [<0000000000982b94>] dump_stack+0x9c/0xd0
[ 19.497844] [<00000000001d9914>] lock_release+0x334/0x350
[ 19.497854] [<0000000000e04354>] _raw_spin_unlock+0x2c/0x50
[ 19.497863] [<00000000001e6d5a>] vprintk_emit+0xb2/0x368
[ 19.497873] [<00000000001e704a>] vprintk_default+0x3a/0x48
[ 19.497882] [<00000000001e7b8c>] printk+0x4c/0x58
[ 19.497892] [<00000000001db3c2>] lockdep_rcu_suspicious+0x32/0x110
[ 19.497902] [<00000000001d5de2>] lock_acquire+0x41a/0x498
[ 19.497911] [<0000000000103b72>] enabled_wait+0xca/0x198
[ 19.497921] [<0000000000103f80>] arch_cpu_idle+0x20/0x38
[ 19.497932] [<0000000000e03c30>] default_idle_call+0x98/0x300
[ 19.497943] [<000000000019958c>] do_idle+0xe4/0x190
[ 19.497954] [<00000000001998f4>] cpu_startup_entry+0x34/0x38
[ 19.497964] [<00000000013a297a>] arch_call_rest_init+0x92/0x98
[ 19.497974] 1 lock held by swapper/0/0:
[ 19.497982] #0: 0000000001252750 (logbuf_lock){-...}-{2:2}, at: vprintk_emit+0x70/0x368
Bisect log (for arm):
# bad: [f75aef392f869018f78cfedf3c320a6b3fcfda6b] Linux 5.9-rc3
# good: [1127b219ce9481c84edad9711626d856127d5e51] Merge tag 'fallthrough-fixes-5.9-rc3' of git://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux
git bisect start 'f75aef392f86' '1127b219ce94'
# good: [8bb5021cc2ee5d5dd129a9f2f5ad2bb76eea297d] Merge tag 'powerpc-5.9-4' of git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux
git bisect good 8bb5021cc2ee5d5dd129a9f2f5ad2bb76eea297d
# good: [ceb2465c51195967f11f6507538579816ac67cb8] Merge tag 'irqchip-fixes-5.9-2' of git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms into irq/urgent
git bisect good ceb2465c51195967f11f6507538579816ac67cb8
# bad: [b69bea8a657b681442765b06be92a2607b1bd875] Merge tag 'locking-urgent-2020-08-30' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
git bisect bad b69bea8a657b681442765b06be92a2607b1bd875
# good: [00b0ed2d4997af6d0a93edef820386951fd66d94] locking/lockdep: Cleanup
git bisect good 00b0ed2d4997af6d0a93edef820386951fd66d94
# good: [044d0d6de9f50192f9697583504a382347ee95ca] lockdep: Only trace IRQ edges
git bisect good 044d0d6de9f50192f9697583504a382347ee95ca
# good: [3edd8db2d53fe6c96ad66248bb1479ae62807268] Merge tag '5.9-rc2-smb-fix' of git://git.samba.org/sfrench/cifs-2.6
git bisect good 3edd8db2d53fe6c96ad66248bb1479ae62807268
# bad: [eb1f00237aca2e368b93db79303f8433d1976d10] lockdep,trace: Expose tracepoints
git bisect bad eb1f00237aca2e368b93db79303f8433d1976d10
# first bad commit: [eb1f00237aca2e368b93db79303f8433d1976d10] lockdep,trace: Expose tracepoints
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 10/11] lockdep: Only trace IRQ edges
2020-08-21 8:47 ` [PATCH v2 10/11] lockdep: Only trace IRQ edges Peter Zijlstra
2020-08-26 0:47 ` Thomas Gleixner
@ 2020-09-02 4:21 ` Guenter Roeck
2020-09-02 9:09 ` peterz
1 sibling, 1 reply; 53+ messages in thread
From: Guenter Roeck @ 2020-09-02 4:21 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, joel, svens, tglx
On Fri, Aug 21, 2020 at 10:47:48AM +0200, Peter Zijlstra wrote:
> From: Nicholas Piggin <npiggin@gmail.com>
>
> Problem:
>
> raw_local_irq_save();
> local_irq_save();
> ...
> local_irq_restore();
> raw_local_irq_restore();
>
> existing instances:
>
> - lock_acquire()
> raw_local_irq_save()
> __lock_acquire()
> arch_spin_lock(&graph_lock)
> pv_wait() := kvm_wait() (same or worse for Xen/HyperV)
> local_irq_save()
>
> - trace_clock_global()
> raw_local_irq_save()
> arch_spin_lock()
> pv_wait() := kvm_wait()
> local_irq_save()
>
> - apic_retrigger_irq()
> raw_local_irq_save()
> apic->send_IPI() := default_send_IPI_single_phys()
> local_irq_save()
>
> Possible solutions:
>
> A) make it work by enabling the tracing inside raw_*()
> B) make it work by keeping tracing disabled inside raw_*()
>
> Now, given that the only reason to use the raw_* variant is because you don't
> want tracing, A) seems like a weird option (although it can be done), so we
> pick B) and declare any code that ends up doing:
>
> raw_local_irq_save()
> local_irq_save()
> lockdep_assert_irqs_disabled();
>
> broken. AFAICT this problem has existed forever, the only reason it came
> up is because I changed IRQ tracing vs lockdep recursion and the first
> instance is fairly common, the other cases hardly ever happen.
>
On sparc64, this patch results in the traceback below. The traceback is gone
after reverting the patch.
Guenter
---
[ 0.000000] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:4875 check_flags.part.39+0x280/0x2a0
[ 0.000000] DEBUG_LOCKS_WARN_ON(lockdep_hardirqs_enabled())
[ 0.000000] Modules linked in:
[ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.9.0-rc3 #1
[ 0.000000] Call Trace:
[ 0.000000] [<0000000000469890>] __warn+0xb0/0xe0
[ 0.000000] [<00000000004698fc>] warn_slowpath_fmt+0x3c/0x80
[ 0.000000] [<00000000004cfce0>] check_flags.part.39+0x280/0x2a0
[ 0.000000] [<00000000004cff18>] lock_acquire+0x218/0x4e0
[ 0.000000] [<0000000000d740c8>] _raw_spin_lock+0x28/0x40
[ 0.000000] [<00000000009870f4>] p1275_cmd_direct+0x14/0x60
[ 0.000000] [<00000000009872cc>] prom_getproplen+0x4c/0x60
[ 0.000000] [<0000000000987308>] prom_getproperty+0x8/0x80
[ 0.000000] [<0000000000987390>] prom_getint+0x10/0x40
[ 0.000000] [<00000000017df4b4>] prom_init+0x38/0x8c
[ 0.000000] [<0000000000d6b558>] tlb_fixup_done+0x44/0x6c
[ 0.000000] [<00000000ffd0e930>] 0xffd0e930
[ 0.000000] irq event stamp: 1
[ 0.000000] hardirqs last enabled at (1): [<0000000000987124>] p1275_cmd_direct+0x44/0x60
[ 0.000000] hardirqs last disabled at (0): [<0000000000000000>] 0x0
[ 0.000000] softirqs last enabled at (0): [<0000000000000000>] 0x0
[ 0.000000] softirqs last disabled at (0): [<0000000000000000>] 0x0
[ 0.000000] random: get_random_bytes called from print_oops_end_marker+0x30/0x60 with crng_init=0
[ 0.000000] ---[ end trace 0000000000000000 ]---
[ 0.000000] possible reason: unannotated irqs-off.
---
bisect log:
# bad: [f75aef392f869018f78cfedf3c320a6b3fcfda6b] Linux 5.9-rc3
# good: [1127b219ce9481c84edad9711626d856127d5e51] Merge tag 'fallthrough-fixes-5.9-rc3' of git://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux
git bisect start 'f75aef392f86' '1127b219ce94'
# good: [8bb5021cc2ee5d5dd129a9f2f5ad2bb76eea297d] Merge tag 'powerpc-5.9-4' of git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux
git bisect good 8bb5021cc2ee5d5dd129a9f2f5ad2bb76eea297d
# good: [ceb2465c51195967f11f6507538579816ac67cb8] Merge tag 'irqchip-fixes-5.9-2' of git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms into irq/urgent
git bisect good ceb2465c51195967f11f6507538579816ac67cb8
# bad: [b69bea8a657b681442765b06be92a2607b1bd875] Merge tag 'locking-urgent-2020-08-30' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
git bisect bad b69bea8a657b681442765b06be92a2607b1bd875
# good: [00b0ed2d4997af6d0a93edef820386951fd66d94] locking/lockdep: Cleanup
git bisect good 00b0ed2d4997af6d0a93edef820386951fd66d94
# bad: [044d0d6de9f50192f9697583504a382347ee95ca] lockdep: Only trace IRQ edges
git bisect bad 044d0d6de9f50192f9697583504a382347ee95ca
# good: [021c109330ebc1f54b546c63a078ea3c31356ecb] arm64: Implement arch_irqs_disabled()
git bisect good 021c109330ebc1f54b546c63a078ea3c31356ecb
# good: [99dc56feb7932020502d40107a712fa302b32082] mips: Implement arch_irqs_disabled()
git bisect good 99dc56feb7932020502d40107a712fa302b32082
# first bad commit: [044d0d6de9f50192f9697583504a382347ee95ca] lockdep: Only trace IRQ edges
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 03/11] sched,idle,rcu: Push rcu_idle deeper into the idle path
2020-08-27 22:30 ` Joel Fernandes
@ 2020-09-02 7:08 ` peterz
0 siblings, 0 replies; 53+ messages in thread
From: peterz @ 2020-09-02 7:08 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, svens, tglx, mathieu.desnoyers
On Thu, Aug 27, 2020 at 06:30:01PM -0400, Joel Fernandes wrote:
> On Thu, Aug 27, 2020 at 09:47:48AM +0200, peterz@infradead.org wrote:
> > All trace_*_rcuidle() and RCU_NONIDLE() usage is a bug IMO.
> >
> > Ideally RCU-trace goes away too.
>
> I was thinking that unless the rcu_idle_enter/exit calls coincide with points
> in the kernel where instrumentation is not allowed, there is always a chance
> somebody wants to use tracepoints after rcu_idle_enter or before exit. In
> this case, trace_*_rcuidle() is unavoidable unless you can move the
> rcu_idle_enter/exit calls deeper as you are doing.
>
> Maybe objtool can help with that?
The eventual goal is to mark idle code noinstr too. There's more work
before we can do that.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints
2020-09-02 3:51 ` [PATCH v2 11/11] " Guenter Roeck
@ 2020-09-02 7:24 ` peterz
2020-09-02 13:21 ` Guenter Roeck
2020-09-02 8:56 ` peterz
1 sibling, 1 reply; 53+ messages in thread
From: peterz @ 2020-09-02 7:24 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, joel, svens, tglx
On Tue, Sep 01, 2020 at 08:51:46PM -0700, Guenter Roeck wrote:
> On Fri, Aug 21, 2020 at 10:47:49AM +0200, Peter Zijlstra wrote:
> > The lockdep tracepoints are under the lockdep recursion counter, this
> > has a bunch of nasty side effects:
> >
> > - TRACE_IRQFLAGS doesn't work across the entire tracepoint
> >
> > - RCU-lockdep doesn't see the tracepoints either, hiding numerous
> > "suspicious RCU usage" warnings.
> >
> > Pull the trace_lock_*() tracepoints completely out from under the
> > lockdep recursion handling and completely rely on the trace level
> > recusion handling -- also, tracing *SHOULD* not be taking locks in any
> > case.
> >
>
> Wonder what is worse - the problem or its fix. This patch results in
> a number of WARNING backtraces for several archtectures/platforms.
> Reverting it fixes the problems.
Without all this there was a recursion that could crash. But yes,
tedious.
OTOH the warnings are about real bugs that were pre-existing, we now see
them and can fix them.
I'll reply to ARM separately, but let's have a peek at s390.
> s390:
>
> [ 19.490586] =============================
> [ 19.490752] WARNING: suspicious RCU usage
> [ 19.490921] 5.9.0-rc3 #1 Not tainted
> [ 19.491086] -----------------------------
> [ 19.491253] include/trace/events/lock.h:37 suspicious rcu_dereference_check() usage!
> [ 19.493147] [<00000000001d5de2>] lock_acquire+0x41a/0x498
> [ 19.493320] [<0000000000103b72>] enabled_wait+0xca/0x198
> [ 19.493493] [<0000000000103f80>] arch_cpu_idle+0x20/0x38
Does this help?
---
diff --git a/arch/s390/kernel/idle.c b/arch/s390/kernel/idle.c
index c73f50649e7e..f7f1e64e0d98 100644
--- a/arch/s390/kernel/idle.c
+++ b/arch/s390/kernel/idle.c
@@ -39,14 +39,13 @@ void enabled_wait(void)
local_irq_restore(flags);
/* Account time spent with enabled wait psw loaded as idle time. */
- /* XXX seqcount has tracepoints that require RCU */
- write_seqcount_begin(&idle->seqcount);
+ raw_write_seqcount_begin(&idle->seqcount);
idle_time = idle->clock_idle_exit - idle->clock_idle_enter;
idle->clock_idle_enter = idle->clock_idle_exit = 0ULL;
idle->idle_time += idle_time;
idle->idle_count++;
account_idle_time(cputime_to_nsecs(idle_time));
- write_seqcount_end(&idle->seqcount);
+ raw_write_seqcount_end(&idle->seqcount);
}
NOKPROBE_SYMBOL(enabled_wait);
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints
2020-09-02 3:51 ` [PATCH v2 11/11] " Guenter Roeck
2020-09-02 7:24 ` peterz
@ 2020-09-02 8:56 ` peterz
2020-09-02 13:57 ` Guenter Roeck
1 sibling, 1 reply; 53+ messages in thread
From: peterz @ 2020-09-02 8:56 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, joel, svens, tglx, ulf.hansson, viresh.kumar,
vincent.guittot
On Tue, Sep 01, 2020 at 08:51:46PM -0700, Guenter Roeck wrote:
> [ 27.056457] include/trace/events/lock.h:13 suspicious rcu_dereference_check() usage!
> [ 27.057006] Hardware name: Generic OMAP3-GP (Flattened Device Tree)
> [ 27.057098] [<c03125b4>] (unwind_backtrace) from [<c030c32c>] (show_stack+0x10/0x14)
> [ 27.057189] [<c030c32c>] (show_stack) from [<c08e7a4c>] (dump_stack+0xd8/0xf8)
> [ 27.057312] [<c08e7a4c>] (dump_stack) from [<c03b04bc>] (lock_acquire+0x4d8/0x4dc)
> [ 27.057464] [<c03b04bc>] (lock_acquire) from [<c12457e8>] (_raw_spin_lock_irqsave+0x58/0x74)
> [ 27.057617] [<c12457e8>] (_raw_spin_lock_irqsave) from [<c0338198>] (pwrdm_lock+0x10/0x18)
> [ 27.057739] [<c0338198>] (pwrdm_lock) from [<c033a214>] (clkdm_deny_idle+0x10/0x24)
> [ 27.057891] [<c033a214>] (clkdm_deny_idle) from [<c0332e10>] (omap3_enter_idle_bm+0xd4/0x1b8)
> [ 27.058044] [<c0332e10>] (omap3_enter_idle_bm) from [<c0f740d4>] (cpuidle_enter_state+0x16c/0x620)
ARM cpuidle is a trainwreck :/
So it looks like we have:
- clkdm_
- pwrdm_
- cpu_pm_
- pm_runtime_
In that approximate order, and then there's the coupled idle muck.
Sometimes cpuidle core calls cpu_pm_*(), but mostly it's sprinkled
around in drivers.
How about we unconditionally kill tracing when RCU is idle? Yes this is
a hack, and we really should turn it into a WARN in due time.
The thing is, we're shutting down clock/power domains and god knows
what, the CPU is in a crap state, we'll not take interrupts, but tracing
must happen! Hell no.
Let's make the rule that if you want something traced, you get to pull
it out of the cpuidle driver and stick it in the generic code with a
CPUIDLE_FLAG, before rcu_idle_enter().
Totally untested patch below..
---
diff --git a/include/linux/preempt.h b/include/linux/preempt.h
index 7d9c1c0e149c..878bac893e41 100644
--- a/include/linux/preempt.h
+++ b/include/linux/preempt.h
@@ -27,17 +27,20 @@
* SOFTIRQ_MASK: 0x0000ff00
* HARDIRQ_MASK: 0x000f0000
* NMI_MASK: 0x00f00000
+ * RCUIDLE_MASK: 0x01000000
* PREEMPT_NEED_RESCHED: 0x80000000
*/
#define PREEMPT_BITS 8
#define SOFTIRQ_BITS 8
#define HARDIRQ_BITS 4
#define NMI_BITS 4
+#define RCUIDLE_BITS 1
#define PREEMPT_SHIFT 0
#define SOFTIRQ_SHIFT (PREEMPT_SHIFT + PREEMPT_BITS)
#define HARDIRQ_SHIFT (SOFTIRQ_SHIFT + SOFTIRQ_BITS)
#define NMI_SHIFT (HARDIRQ_SHIFT + HARDIRQ_BITS)
+#define RCUIDLE_SHIFT (NMI_SHIFT + NMI_BITS)
#define __IRQ_MASK(x) ((1UL << (x))-1)
@@ -45,11 +48,13 @@
#define SOFTIRQ_MASK (__IRQ_MASK(SOFTIRQ_BITS) << SOFTIRQ_SHIFT)
#define HARDIRQ_MASK (__IRQ_MASK(HARDIRQ_BITS) << HARDIRQ_SHIFT)
#define NMI_MASK (__IRQ_MASK(NMI_BITS) << NMI_SHIFT)
+#define RCUIDLE_MASK (__IRQ_MASK(RCUIDLE_BITS) << RCUIDLE_SHIFT)
#define PREEMPT_OFFSET (1UL << PREEMPT_SHIFT)
#define SOFTIRQ_OFFSET (1UL << SOFTIRQ_SHIFT)
#define HARDIRQ_OFFSET (1UL << HARDIRQ_SHIFT)
#define NMI_OFFSET (1UL << NMI_SHIFT)
+#define RCUIDLE_OFFSET (1UL << RCUIDLE_SHIFT)
#define SOFTIRQ_DISABLE_OFFSET (2 * SOFTIRQ_OFFSET)
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 598fec9f9dbf..997472c662c4 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -164,7 +164,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
void *__data; \
int __maybe_unused __idx = 0; \
\
- if (!(cond)) \
+ if (!(cond) || (preempt_count() & RCUIDLE_MASK)) \
return; \
\
/* srcu can't be used from NMI */ \
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 8ce77d9ac716..ad9fb4f12c63 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -269,6 +269,8 @@ static noinstr void rcu_dynticks_eqs_enter(void)
/* Better not have special action (TLB flush) pending! */
WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
(seq & RCU_DYNTICK_CTRL_MASK));
+
+ __preempt_count_add(RCUIDLE_OFFSET);
}
/*
@@ -281,6 +283,8 @@ static noinstr void rcu_dynticks_eqs_exit(void)
struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
int seq;
+ __preempt_count_sub(RCUIDLE_OFFSET);
+
/*
* CPUs seeing atomic_add_return() must see prior idle sojourns,
* and we also must force ordering with the next RCU read-side
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v2 10/11] lockdep: Only trace IRQ edges
2020-09-02 4:21 ` Guenter Roeck
@ 2020-09-02 9:09 ` peterz
2020-09-02 9:12 ` peterz
0 siblings, 1 reply; 53+ messages in thread
From: peterz @ 2020-09-02 9:09 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, joel, svens, tglx
On Tue, Sep 01, 2020 at 09:21:37PM -0700, Guenter Roeck wrote:
> [ 0.000000] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:4875 check_flags.part.39+0x280/0x2a0
> [ 0.000000] DEBUG_LOCKS_WARN_ON(lockdep_hardirqs_enabled())
> [ 0.000000] [<00000000004cff18>] lock_acquire+0x218/0x4e0
> [ 0.000000] [<0000000000d740c8>] _raw_spin_lock+0x28/0x40
> [ 0.000000] [<00000000009870f4>] p1275_cmd_direct+0x14/0x60
Lol! yes, I can see that going side-ways... let me poke at that.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 10/11] lockdep: Only trace IRQ edges
2020-09-02 9:09 ` peterz
@ 2020-09-02 9:12 ` peterz
2020-09-02 13:48 ` Guenter Roeck
0 siblings, 1 reply; 53+ messages in thread
From: peterz @ 2020-09-02 9:12 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, joel, svens, tglx, davem
On Wed, Sep 02, 2020 at 11:09:35AM +0200, peterz@infradead.org wrote:
> On Tue, Sep 01, 2020 at 09:21:37PM -0700, Guenter Roeck wrote:
> > [ 0.000000] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:4875 check_flags.part.39+0x280/0x2a0
> > [ 0.000000] DEBUG_LOCKS_WARN_ON(lockdep_hardirqs_enabled())
>
> > [ 0.000000] [<00000000004cff18>] lock_acquire+0x218/0x4e0
> > [ 0.000000] [<0000000000d740c8>] _raw_spin_lock+0x28/0x40
> > [ 0.000000] [<00000000009870f4>] p1275_cmd_direct+0x14/0x60
>
> Lol! yes, I can see that going side-ways... let me poke at that.
I suspect this will do.
diff --git a/arch/sparc/prom/p1275.c b/arch/sparc/prom/p1275.c
index 889aa602f8d8..7cfe88e30b52 100644
--- a/arch/sparc/prom/p1275.c
+++ b/arch/sparc/prom/p1275.c
@@ -38,7 +38,7 @@ void p1275_cmd_direct(unsigned long *args)
unsigned long flags;
local_save_flags(flags);
- local_irq_restore((unsigned long)PIL_NMI);
+ arch_local_irq_restore((unsigned long)PIL_NMI);
raw_spin_lock(&prom_entry_lock);
prom_world(1);
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints
2020-09-02 7:24 ` peterz
@ 2020-09-02 13:21 ` Guenter Roeck
0 siblings, 0 replies; 53+ messages in thread
From: Guenter Roeck @ 2020-09-02 13:21 UTC (permalink / raw)
To: peterz
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, joel, svens, tglx
On 9/2/20 12:24 AM, peterz@infradead.org wrote:
> On Tue, Sep 01, 2020 at 08:51:46PM -0700, Guenter Roeck wrote:
>> On Fri, Aug 21, 2020 at 10:47:49AM +0200, Peter Zijlstra wrote:
>>> The lockdep tracepoints are under the lockdep recursion counter, this
>>> has a bunch of nasty side effects:
>>>
>>> - TRACE_IRQFLAGS doesn't work across the entire tracepoint
>>>
>>> - RCU-lockdep doesn't see the tracepoints either, hiding numerous
>>> "suspicious RCU usage" warnings.
>>>
>>> Pull the trace_lock_*() tracepoints completely out from under the
>>> lockdep recursion handling and completely rely on the trace level
>>> recusion handling -- also, tracing *SHOULD* not be taking locks in any
>>> case.
>>>
>>
>> Wonder what is worse - the problem or its fix. This patch results in
>> a number of WARNING backtraces for several archtectures/platforms.
>> Reverting it fixes the problems.
>
> Without all this there was a recursion that could crash. But yes,
> tedious.
>
> OTOH the warnings are about real bugs that were pre-existing, we now see
> them and can fix them.
>
> I'll reply to ARM separately, but let's have a peek at s390.
>
>> s390:
>>
>> [ 19.490586] =============================
>> [ 19.490752] WARNING: suspicious RCU usage
>> [ 19.490921] 5.9.0-rc3 #1 Not tainted
>> [ 19.491086] -----------------------------
>> [ 19.491253] include/trace/events/lock.h:37 suspicious rcu_dereference_check() usage!
>
>> [ 19.493147] [<00000000001d5de2>] lock_acquire+0x41a/0x498
>> [ 19.493320] [<0000000000103b72>] enabled_wait+0xca/0x198
>> [ 19.493493] [<0000000000103f80>] arch_cpu_idle+0x20/0x38
>
> Does this help?
>
> ---
>
> diff --git a/arch/s390/kernel/idle.c b/arch/s390/kernel/idle.c
> index c73f50649e7e..f7f1e64e0d98 100644
> --- a/arch/s390/kernel/idle.c
> +++ b/arch/s390/kernel/idle.c
> @@ -39,14 +39,13 @@ void enabled_wait(void)
> local_irq_restore(flags);
>
> /* Account time spent with enabled wait psw loaded as idle time. */
> - /* XXX seqcount has tracepoints that require RCU */
> - write_seqcount_begin(&idle->seqcount);
> + raw_write_seqcount_begin(&idle->seqcount);
> idle_time = idle->clock_idle_exit - idle->clock_idle_enter;
> idle->clock_idle_enter = idle->clock_idle_exit = 0ULL;
> idle->idle_time += idle_time;
> idle->idle_count++;
> account_idle_time(cputime_to_nsecs(idle_time));
> - write_seqcount_end(&idle->seqcount);
> + raw_write_seqcount_end(&idle->seqcount);
> }
> NOKPROBE_SYMBOL(enabled_wait);
>
Yes, it does.
Tested-by: Guenter Roeck <linux@roeck-us.net>
Guenter
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 10/11] lockdep: Only trace IRQ edges
2020-09-02 9:12 ` peterz
@ 2020-09-02 13:48 ` Guenter Roeck
2020-09-08 14:22 ` peterz
0 siblings, 1 reply; 53+ messages in thread
From: Guenter Roeck @ 2020-09-02 13:48 UTC (permalink / raw)
To: peterz
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, joel, svens, tglx, davem
On 9/2/20 2:12 AM, peterz@infradead.org wrote:
> On Wed, Sep 02, 2020 at 11:09:35AM +0200, peterz@infradead.org wrote:
>> On Tue, Sep 01, 2020 at 09:21:37PM -0700, Guenter Roeck wrote:
>>> [ 0.000000] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:4875 check_flags.part.39+0x280/0x2a0
>>> [ 0.000000] DEBUG_LOCKS_WARN_ON(lockdep_hardirqs_enabled())
>>
>>> [ 0.000000] [<00000000004cff18>] lock_acquire+0x218/0x4e0
>>> [ 0.000000] [<0000000000d740c8>] _raw_spin_lock+0x28/0x40
>>> [ 0.000000] [<00000000009870f4>] p1275_cmd_direct+0x14/0x60
>>
>> Lol! yes, I can see that going side-ways... let me poke at that.
>
> I suspect this will do.
>
> diff --git a/arch/sparc/prom/p1275.c b/arch/sparc/prom/p1275.c
> index 889aa602f8d8..7cfe88e30b52 100644
> --- a/arch/sparc/prom/p1275.c
> +++ b/arch/sparc/prom/p1275.c
> @@ -38,7 +38,7 @@ void p1275_cmd_direct(unsigned long *args)
> unsigned long flags;
>
> local_save_flags(flags);
> - local_irq_restore((unsigned long)PIL_NMI);
> + arch_local_irq_restore((unsigned long)PIL_NMI);
> raw_spin_lock(&prom_entry_lock);
>
> prom_world(1);
>
No, that doesn't help. Even removing that line entirely doesn't help.
The problem seems to be that interrupts are not enabled in the first
place. But why wasn't this a problem before ?
Guenter
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints
2020-09-02 8:56 ` peterz
@ 2020-09-02 13:57 ` Guenter Roeck
2020-09-03 14:00 ` peterz
0 siblings, 1 reply; 53+ messages in thread
From: Guenter Roeck @ 2020-09-02 13:57 UTC (permalink / raw)
To: peterz
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, joel, svens, tglx, ulf.hansson, viresh.kumar,
vincent.guittot
On 9/2/20 1:56 AM, peterz@infradead.org wrote:
> On Tue, Sep 01, 2020 at 08:51:46PM -0700, Guenter Roeck wrote:
>
>> [ 27.056457] include/trace/events/lock.h:13 suspicious rcu_dereference_check() usage!
>
>> [ 27.057006] Hardware name: Generic OMAP3-GP (Flattened Device Tree)
>> [ 27.057098] [<c03125b4>] (unwind_backtrace) from [<c030c32c>] (show_stack+0x10/0x14)
>> [ 27.057189] [<c030c32c>] (show_stack) from [<c08e7a4c>] (dump_stack+0xd8/0xf8)
>> [ 27.057312] [<c08e7a4c>] (dump_stack) from [<c03b04bc>] (lock_acquire+0x4d8/0x4dc)
>> [ 27.057464] [<c03b04bc>] (lock_acquire) from [<c12457e8>] (_raw_spin_lock_irqsave+0x58/0x74)
>> [ 27.057617] [<c12457e8>] (_raw_spin_lock_irqsave) from [<c0338198>] (pwrdm_lock+0x10/0x18)
>> [ 27.057739] [<c0338198>] (pwrdm_lock) from [<c033a214>] (clkdm_deny_idle+0x10/0x24)
>> [ 27.057891] [<c033a214>] (clkdm_deny_idle) from [<c0332e10>] (omap3_enter_idle_bm+0xd4/0x1b8)
>> [ 27.058044] [<c0332e10>] (omap3_enter_idle_bm) from [<c0f740d4>] (cpuidle_enter_state+0x16c/0x620)
>
> ARM cpuidle is a trainwreck :/
>
> So it looks like we have:
>
> - clkdm_
> - pwrdm_
> - cpu_pm_
> - pm_runtime_
>
> In that approximate order, and then there's the coupled idle muck.
> Sometimes cpuidle core calls cpu_pm_*(), but mostly it's sprinkled
> around in drivers.
>
> How about we unconditionally kill tracing when RCU is idle? Yes this is
> a hack, and we really should turn it into a WARN in due time.
>
> The thing is, we're shutting down clock/power domains and god knows
> what, the CPU is in a crap state, we'll not take interrupts, but tracing
> must happen! Hell no.
>
> Let's make the rule that if you want something traced, you get to pull
> it out of the cpuidle driver and stick it in the generic code with a
> CPUIDLE_FLAG, before rcu_idle_enter().
>
> Totally untested patch below..
>
Unfortunately, that patch does not make a difference; I still see the
same tracebacks with it applied.
Guenter
> ---
>
> diff --git a/include/linux/preempt.h b/include/linux/preempt.h
> index 7d9c1c0e149c..878bac893e41 100644
> --- a/include/linux/preempt.h
> +++ b/include/linux/preempt.h
> @@ -27,17 +27,20 @@
> * SOFTIRQ_MASK: 0x0000ff00
> * HARDIRQ_MASK: 0x000f0000
> * NMI_MASK: 0x00f00000
> + * RCUIDLE_MASK: 0x01000000
> * PREEMPT_NEED_RESCHED: 0x80000000
> */
> #define PREEMPT_BITS 8
> #define SOFTIRQ_BITS 8
> #define HARDIRQ_BITS 4
> #define NMI_BITS 4
> +#define RCUIDLE_BITS 1
>
> #define PREEMPT_SHIFT 0
> #define SOFTIRQ_SHIFT (PREEMPT_SHIFT + PREEMPT_BITS)
> #define HARDIRQ_SHIFT (SOFTIRQ_SHIFT + SOFTIRQ_BITS)
> #define NMI_SHIFT (HARDIRQ_SHIFT + HARDIRQ_BITS)
> +#define RCUIDLE_SHIFT (NMI_SHIFT + NMI_BITS)
>
> #define __IRQ_MASK(x) ((1UL << (x))-1)
>
> @@ -45,11 +48,13 @@
> #define SOFTIRQ_MASK (__IRQ_MASK(SOFTIRQ_BITS) << SOFTIRQ_SHIFT)
> #define HARDIRQ_MASK (__IRQ_MASK(HARDIRQ_BITS) << HARDIRQ_SHIFT)
> #define NMI_MASK (__IRQ_MASK(NMI_BITS) << NMI_SHIFT)
> +#define RCUIDLE_MASK (__IRQ_MASK(RCUIDLE_BITS) << RCUIDLE_SHIFT)
>
> #define PREEMPT_OFFSET (1UL << PREEMPT_SHIFT)
> #define SOFTIRQ_OFFSET (1UL << SOFTIRQ_SHIFT)
> #define HARDIRQ_OFFSET (1UL << HARDIRQ_SHIFT)
> #define NMI_OFFSET (1UL << NMI_SHIFT)
> +#define RCUIDLE_OFFSET (1UL << RCUIDLE_SHIFT)
>
> #define SOFTIRQ_DISABLE_OFFSET (2 * SOFTIRQ_OFFSET)
>
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 598fec9f9dbf..997472c662c4 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -164,7 +164,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> void *__data; \
> int __maybe_unused __idx = 0; \
> \
> - if (!(cond)) \
> + if (!(cond) || (preempt_count() & RCUIDLE_MASK)) \
> return; \
> \
> /* srcu can't be used from NMI */ \
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 8ce77d9ac716..ad9fb4f12c63 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -269,6 +269,8 @@ static noinstr void rcu_dynticks_eqs_enter(void)
> /* Better not have special action (TLB flush) pending! */
> WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> (seq & RCU_DYNTICK_CTRL_MASK));
> +
> + __preempt_count_add(RCUIDLE_OFFSET);
> }
>
> /*
> @@ -281,6 +283,8 @@ static noinstr void rcu_dynticks_eqs_exit(void)
> struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> int seq;
>
> + __preempt_count_sub(RCUIDLE_OFFSET);
> +
> /*
> * CPUs seeing atomic_add_return() must see prior idle sojourns,
> * and we also must force ordering with the next RCU read-side
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints
2020-09-02 13:57 ` Guenter Roeck
@ 2020-09-03 14:00 ` peterz
2020-09-03 14:13 ` peterz
2020-09-03 15:19 ` Guenter Roeck
0 siblings, 2 replies; 53+ messages in thread
From: peterz @ 2020-09-03 14:00 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, joel, svens, tglx, ulf.hansson, viresh.kumar,
vincent.guittot
On Wed, Sep 02, 2020 at 06:57:36AM -0700, Guenter Roeck wrote:
> On 9/2/20 1:56 AM, peterz@infradead.org wrote:
> > On Tue, Sep 01, 2020 at 08:51:46PM -0700, Guenter Roeck wrote:
> >
> >> [ 27.056457] include/trace/events/lock.h:13 suspicious rcu_dereference_check() usage!
> >
> >> [ 27.057006] Hardware name: Generic OMAP3-GP (Flattened Device Tree)
> >> [ 27.057098] [<c03125b4>] (unwind_backtrace) from [<c030c32c>] (show_stack+0x10/0x14)
> >> [ 27.057189] [<c030c32c>] (show_stack) from [<c08e7a4c>] (dump_stack+0xd8/0xf8)
> >> [ 27.057312] [<c08e7a4c>] (dump_stack) from [<c03b04bc>] (lock_acquire+0x4d8/0x4dc)
> >> [ 27.057464] [<c03b04bc>] (lock_acquire) from [<c12457e8>] (_raw_spin_lock_irqsave+0x58/0x74)
> >> [ 27.057617] [<c12457e8>] (_raw_spin_lock_irqsave) from [<c0338198>] (pwrdm_lock+0x10/0x18)
> >> [ 27.057739] [<c0338198>] (pwrdm_lock) from [<c033a214>] (clkdm_deny_idle+0x10/0x24)
> >> [ 27.057891] [<c033a214>] (clkdm_deny_idle) from [<c0332e10>] (omap3_enter_idle_bm+0xd4/0x1b8)
> >> [ 27.058044] [<c0332e10>] (omap3_enter_idle_bm) from [<c0f740d4>] (cpuidle_enter_state+0x16c/0x620)
> >
> > ARM cpuidle is a trainwreck :/
> >
> > So it looks like we have:
> >
> > - clkdm_
> > - pwrdm_
> > - cpu_pm_
> > - pm_runtime_
> >
> > In that approximate order, and then there's the coupled idle muck.
> > Sometimes cpuidle core calls cpu_pm_*(), but mostly it's sprinkled
> > around in drivers.
> >
> > How about we unconditionally kill tracing when RCU is idle? Yes this is
> > a hack, and we really should turn it into a WARN in due time.
> >
> > The thing is, we're shutting down clock/power domains and god knows
> > what, the CPU is in a crap state, we'll not take interrupts, but tracing
> > must happen! Hell no.
> >
> > Let's make the rule that if you want something traced, you get to pull
> > it out of the cpuidle driver and stick it in the generic code with a
> > CPUIDLE_FLAG, before rcu_idle_enter().
> >
> > Totally untested patch below..
> >
>
> Unfortunately, that patch does not make a difference; I still see the
> same tracebacks with it applied.
I stuck a tracepoint in intel_idle and had a rummage around. The below
seems to work for me now.
---
diff --git a/include/linux/preempt.h b/include/linux/preempt.h
index 7d9c1c0e149c..878bac893e41 100644
--- a/include/linux/preempt.h
+++ b/include/linux/preempt.h
@@ -27,17 +27,20 @@
* SOFTIRQ_MASK: 0x0000ff00
* HARDIRQ_MASK: 0x000f0000
* NMI_MASK: 0x00f00000
+ * RCUIDLE_MASK: 0x01000000
* PREEMPT_NEED_RESCHED: 0x80000000
*/
#define PREEMPT_BITS 8
#define SOFTIRQ_BITS 8
#define HARDIRQ_BITS 4
#define NMI_BITS 4
+#define RCUIDLE_BITS 1
#define PREEMPT_SHIFT 0
#define SOFTIRQ_SHIFT (PREEMPT_SHIFT + PREEMPT_BITS)
#define HARDIRQ_SHIFT (SOFTIRQ_SHIFT + SOFTIRQ_BITS)
#define NMI_SHIFT (HARDIRQ_SHIFT + HARDIRQ_BITS)
+#define RCUIDLE_SHIFT (NMI_SHIFT + NMI_BITS)
#define __IRQ_MASK(x) ((1UL << (x))-1)
@@ -45,11 +48,13 @@
#define SOFTIRQ_MASK (__IRQ_MASK(SOFTIRQ_BITS) << SOFTIRQ_SHIFT)
#define HARDIRQ_MASK (__IRQ_MASK(HARDIRQ_BITS) << HARDIRQ_SHIFT)
#define NMI_MASK (__IRQ_MASK(NMI_BITS) << NMI_SHIFT)
+#define RCUIDLE_MASK (__IRQ_MASK(RCUIDLE_BITS) << RCUIDLE_SHIFT)
#define PREEMPT_OFFSET (1UL << PREEMPT_SHIFT)
#define SOFTIRQ_OFFSET (1UL << SOFTIRQ_SHIFT)
#define HARDIRQ_OFFSET (1UL << HARDIRQ_SHIFT)
#define NMI_OFFSET (1UL << NMI_SHIFT)
+#define RCUIDLE_OFFSET (1UL << RCUIDLE_SHIFT)
#define SOFTIRQ_DISABLE_OFFSET (2 * SOFTIRQ_OFFSET)
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 3722a10fc46d..5bc45f6750f5 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -172,12 +172,18 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
int __maybe_unused __idx = 0; \
void *__data; \
\
- if (!(cond)) \
+ if (!(cond) || (preempt_count() & RCUIDLE_MASK)) \
return; \
\
/* srcu can't be used from NMI */ \
WARN_ON_ONCE(rcuidle && in_nmi()); \
\
+ if (IS_ENABLED(CONFIG_LOCKDEP) && !(rcuidle)) { \
+ rcu_read_lock_sched_notrace(); \
+ rcu_dereference_sched(__tracepoint_##name.funcs);\
+ rcu_read_unlock_sched_notrace(); \
+ } \
+ \
/* keep srcu and sched-rcu usage consistent */ \
preempt_disable_notrace(); \
\
@@ -242,11 +248,6 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
TP_PROTO(data_proto), \
TP_ARGS(data_args), \
TP_CONDITION(cond), 0); \
- if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) { \
- rcu_read_lock_sched_notrace(); \
- rcu_dereference_sched(__tracepoint_##name.funcs);\
- rcu_read_unlock_sched_notrace(); \
- } \
} \
__DECLARE_TRACE_RCU(name, PARAMS(proto), PARAMS(args), \
PARAMS(cond), PARAMS(data_proto), PARAMS(data_args)) \
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 8ce77d9ac716..ad9fb4f12c63 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -269,6 +269,8 @@ static noinstr void rcu_dynticks_eqs_enter(void)
/* Better not have special action (TLB flush) pending! */
WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
(seq & RCU_DYNTICK_CTRL_MASK));
+
+ __preempt_count_add(RCUIDLE_OFFSET);
}
/*
@@ -281,6 +283,8 @@ static noinstr void rcu_dynticks_eqs_exit(void)
struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
int seq;
+ __preempt_count_sub(RCUIDLE_OFFSET);
+
/*
* CPUs seeing atomic_add_return() must see prior idle sojourns,
* and we also must force ordering with the next RCU read-side
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints
2020-09-03 14:00 ` peterz
@ 2020-09-03 14:13 ` peterz
2020-09-03 15:19 ` Guenter Roeck
1 sibling, 0 replies; 53+ messages in thread
From: peterz @ 2020-09-03 14:13 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, joel, svens, tglx, ulf.hansson, viresh.kumar,
vincent.guittot
On Thu, Sep 03, 2020 at 04:00:47PM +0200, peterz@infradead.org wrote:
> I stuck a tracepoint in intel_idle and had a rummage around. The below
> seems to work for me now.
Note that this will insta-kill all trace_*_rcuidle() tracepoint that are
actually used in rcuidle.
A git-grep seems to suggest the remaining users are:
- arch/arm
- arch/arm64 -- IPI tracepoint that are ordered incorrectly in
their entry code,
- runtime pm
- common clk -- these both need re-ordering somehow
- trace_preemptirq -- this is going to be interesting for
those archs that idle with IRQs enabled
- printk -- we shouldn't be printk()-ing from idle, and if we are,
missing this is the least of our problems.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints
2020-09-03 14:00 ` peterz
2020-09-03 14:13 ` peterz
@ 2020-09-03 15:19 ` Guenter Roeck
2020-09-03 15:36 ` peterz
1 sibling, 1 reply; 53+ messages in thread
From: Guenter Roeck @ 2020-09-03 15:19 UTC (permalink / raw)
To: peterz
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, joel, svens, tglx, ulf.hansson, viresh.kumar,
vincent.guittot
On 9/3/20 7:00 AM, peterz@infradead.org wrote:
> On Wed, Sep 02, 2020 at 06:57:36AM -0700, Guenter Roeck wrote:
>> On 9/2/20 1:56 AM, peterz@infradead.org wrote:
>>> On Tue, Sep 01, 2020 at 08:51:46PM -0700, Guenter Roeck wrote:
>>>
>>>> [ 27.056457] include/trace/events/lock.h:13 suspicious rcu_dereference_check() usage!
>>>
>>>> [ 27.057006] Hardware name: Generic OMAP3-GP (Flattened Device Tree)
>>>> [ 27.057098] [<c03125b4>] (unwind_backtrace) from [<c030c32c>] (show_stack+0x10/0x14)
>>>> [ 27.057189] [<c030c32c>] (show_stack) from [<c08e7a4c>] (dump_stack+0xd8/0xf8)
>>>> [ 27.057312] [<c08e7a4c>] (dump_stack) from [<c03b04bc>] (lock_acquire+0x4d8/0x4dc)
>>>> [ 27.057464] [<c03b04bc>] (lock_acquire) from [<c12457e8>] (_raw_spin_lock_irqsave+0x58/0x74)
>>>> [ 27.057617] [<c12457e8>] (_raw_spin_lock_irqsave) from [<c0338198>] (pwrdm_lock+0x10/0x18)
>>>> [ 27.057739] [<c0338198>] (pwrdm_lock) from [<c033a214>] (clkdm_deny_idle+0x10/0x24)
>>>> [ 27.057891] [<c033a214>] (clkdm_deny_idle) from [<c0332e10>] (omap3_enter_idle_bm+0xd4/0x1b8)
>>>> [ 27.058044] [<c0332e10>] (omap3_enter_idle_bm) from [<c0f740d4>] (cpuidle_enter_state+0x16c/0x620)
>>>
>>> ARM cpuidle is a trainwreck :/
>>>
>>> So it looks like we have:
>>>
>>> - clkdm_
>>> - pwrdm_
>>> - cpu_pm_
>>> - pm_runtime_
>>>
>>> In that approximate order, and then there's the coupled idle muck.
>>> Sometimes cpuidle core calls cpu_pm_*(), but mostly it's sprinkled
>>> around in drivers.
>>>
>>> How about we unconditionally kill tracing when RCU is idle? Yes this is
>>> a hack, and we really should turn it into a WARN in due time.
>>>
>>> The thing is, we're shutting down clock/power domains and god knows
>>> what, the CPU is in a crap state, we'll not take interrupts, but tracing
>>> must happen! Hell no.
>>>
>>> Let's make the rule that if you want something traced, you get to pull
>>> it out of the cpuidle driver and stick it in the generic code with a
>>> CPUIDLE_FLAG, before rcu_idle_enter().
>>>
>>> Totally untested patch below..
>>>
>>
>> Unfortunately, that patch does not make a difference; I still see the
>> same tracebacks with it applied.
>
> I stuck a tracepoint in intel_idle and had a rummage around. The below
> seems to work for me now.
>
> ---
> diff --git a/include/linux/preempt.h b/include/linux/preempt.h
> index 7d9c1c0e149c..878bac893e41 100644
> --- a/include/linux/preempt.h
> +++ b/include/linux/preempt.h
> @@ -27,17 +27,20 @@
> * SOFTIRQ_MASK: 0x0000ff00
> * HARDIRQ_MASK: 0x000f0000
> * NMI_MASK: 0x00f00000
> + * RCUIDLE_MASK: 0x01000000
> * PREEMPT_NEED_RESCHED: 0x80000000
> */
> #define PREEMPT_BITS 8
> #define SOFTIRQ_BITS 8
> #define HARDIRQ_BITS 4
> #define NMI_BITS 4
> +#define RCUIDLE_BITS 1
>
> #define PREEMPT_SHIFT 0
> #define SOFTIRQ_SHIFT (PREEMPT_SHIFT + PREEMPT_BITS)
> #define HARDIRQ_SHIFT (SOFTIRQ_SHIFT + SOFTIRQ_BITS)
> #define NMI_SHIFT (HARDIRQ_SHIFT + HARDIRQ_BITS)
> +#define RCUIDLE_SHIFT (NMI_SHIFT + NMI_BITS)
>
> #define __IRQ_MASK(x) ((1UL << (x))-1)
>
> @@ -45,11 +48,13 @@
> #define SOFTIRQ_MASK (__IRQ_MASK(SOFTIRQ_BITS) << SOFTIRQ_SHIFT)
> #define HARDIRQ_MASK (__IRQ_MASK(HARDIRQ_BITS) << HARDIRQ_SHIFT)
> #define NMI_MASK (__IRQ_MASK(NMI_BITS) << NMI_SHIFT)
> +#define RCUIDLE_MASK (__IRQ_MASK(RCUIDLE_BITS) << RCUIDLE_SHIFT)
>
> #define PREEMPT_OFFSET (1UL << PREEMPT_SHIFT)
> #define SOFTIRQ_OFFSET (1UL << SOFTIRQ_SHIFT)
> #define HARDIRQ_OFFSET (1UL << HARDIRQ_SHIFT)
> #define NMI_OFFSET (1UL << NMI_SHIFT)
> +#define RCUIDLE_OFFSET (1UL << RCUIDLE_SHIFT)
>
> #define SOFTIRQ_DISABLE_OFFSET (2 * SOFTIRQ_OFFSET)
>
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 3722a10fc46d..5bc45f6750f5 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -172,12 +172,18 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> int __maybe_unused __idx = 0; \
> void *__data; \
> \
> - if (!(cond)) \
> + if (!(cond) || (preempt_count() & RCUIDLE_MASK)) \
> return; \
> \
> /* srcu can't be used from NMI */ \
> WARN_ON_ONCE(rcuidle && in_nmi()); \
> \
> + if (IS_ENABLED(CONFIG_LOCKDEP) && !(rcuidle)) { \
> + rcu_read_lock_sched_notrace(); \
> + rcu_dereference_sched(__tracepoint_##name.funcs);\
This doesn't compile for me - there is no "name" parameter in __DO_TRACE().
Error log:
In file included from ./include/linux/rculist.h:11,
from ./include/linux/pid.h:5,
from ./include/linux/sched.h:14,
from ./include/linux/sched/numa_balancing.h:10,
from ./include/trace/events/sched.h:8,
from kernel/sched/core.c:10:
./include/trace/events/sched.h: In function 'trace_sched_kthread_stop':
./include/linux/tracepoint.h:175:26: error: '__tracepoint_name' undeclared
I applied your patch on top of v5.9-rc3-75-gfc3abb53250a. Are you using
a different branch ?
Guenter
> + rcu_read_unlock_sched_notrace(); \
> + } \
> + \
> /* keep srcu and sched-rcu usage consistent */ \
> preempt_disable_notrace(); \
> \
> @@ -242,11 +248,6 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> TP_PROTO(data_proto), \
> TP_ARGS(data_args), \
> TP_CONDITION(cond), 0); \
> - if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) { \
> - rcu_read_lock_sched_notrace(); \
> - rcu_dereference_sched(__tracepoint_##name.funcs);\
> - rcu_read_unlock_sched_notrace(); \
> - } \
> } \
> __DECLARE_TRACE_RCU(name, PARAMS(proto), PARAMS(args), \
> PARAMS(cond), PARAMS(data_proto), PARAMS(data_args)) \
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 8ce77d9ac716..ad9fb4f12c63 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -269,6 +269,8 @@ static noinstr void rcu_dynticks_eqs_enter(void)
> /* Better not have special action (TLB flush) pending! */
> WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> (seq & RCU_DYNTICK_CTRL_MASK));
> +
> + __preempt_count_add(RCUIDLE_OFFSET);
> }
>
> /*
> @@ -281,6 +283,8 @@ static noinstr void rcu_dynticks_eqs_exit(void)
> struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> int seq;
>
> + __preempt_count_sub(RCUIDLE_OFFSET);
> +
> /*
> * CPUs seeing atomic_add_return() must see prior idle sojourns,
> * and we also must force ordering with the next RCU read-side
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints
2020-09-03 15:19 ` Guenter Roeck
@ 2020-09-03 15:36 ` peterz
2020-09-03 15:54 ` Paul E. McKenney
2020-09-03 18:11 ` Guenter Roeck
0 siblings, 2 replies; 53+ messages in thread
From: peterz @ 2020-09-03 15:36 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, joel, svens, tglx, ulf.hansson, viresh.kumar,
vincent.guittot
On Thu, Sep 03, 2020 at 08:19:38AM -0700, Guenter Roeck wrote:
> This doesn't compile for me - there is no "name" parameter in __DO_TRACE().
>
> Error log:
> In file included from ./include/linux/rculist.h:11,
> from ./include/linux/pid.h:5,
> from ./include/linux/sched.h:14,
> from ./include/linux/sched/numa_balancing.h:10,
> from ./include/trace/events/sched.h:8,
> from kernel/sched/core.c:10:
> ./include/trace/events/sched.h: In function 'trace_sched_kthread_stop':
> ./include/linux/tracepoint.h:175:26: error: '__tracepoint_name' undeclared
>
> I applied your patch on top of v5.9-rc3-75-gfc3abb53250a. Are you using
> a different branch ?
Argh, I was on tip/master... easy fix though.
---
diff --git a/include/linux/preempt.h b/include/linux/preempt.h
index 7d9c1c0e149c..878bac893e41 100644
--- a/include/linux/preempt.h
+++ b/include/linux/preempt.h
@@ -27,17 +27,20 @@
* SOFTIRQ_MASK: 0x0000ff00
* HARDIRQ_MASK: 0x000f0000
* NMI_MASK: 0x00f00000
+ * RCUIDLE_MASK: 0x01000000
* PREEMPT_NEED_RESCHED: 0x80000000
*/
#define PREEMPT_BITS 8
#define SOFTIRQ_BITS 8
#define HARDIRQ_BITS 4
#define NMI_BITS 4
+#define RCUIDLE_BITS 1
#define PREEMPT_SHIFT 0
#define SOFTIRQ_SHIFT (PREEMPT_SHIFT + PREEMPT_BITS)
#define HARDIRQ_SHIFT (SOFTIRQ_SHIFT + SOFTIRQ_BITS)
#define NMI_SHIFT (HARDIRQ_SHIFT + HARDIRQ_BITS)
+#define RCUIDLE_SHIFT (NMI_SHIFT + NMI_BITS)
#define __IRQ_MASK(x) ((1UL << (x))-1)
@@ -45,11 +48,13 @@
#define SOFTIRQ_MASK (__IRQ_MASK(SOFTIRQ_BITS) << SOFTIRQ_SHIFT)
#define HARDIRQ_MASK (__IRQ_MASK(HARDIRQ_BITS) << HARDIRQ_SHIFT)
#define NMI_MASK (__IRQ_MASK(NMI_BITS) << NMI_SHIFT)
+#define RCUIDLE_MASK (__IRQ_MASK(RCUIDLE_BITS) << RCUIDLE_SHIFT)
#define PREEMPT_OFFSET (1UL << PREEMPT_SHIFT)
#define SOFTIRQ_OFFSET (1UL << SOFTIRQ_SHIFT)
#define HARDIRQ_OFFSET (1UL << HARDIRQ_SHIFT)
#define NMI_OFFSET (1UL << NMI_SHIFT)
+#define RCUIDLE_OFFSET (1UL << RCUIDLE_SHIFT)
#define SOFTIRQ_DISABLE_OFFSET (2 * SOFTIRQ_OFFSET)
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 598fec9f9dbf..0469bc1c24fc 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -164,12 +164,18 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
void *__data; \
int __maybe_unused __idx = 0; \
\
- if (!(cond)) \
+ if (!(cond) || (preempt_count() & RCUIDLE_MASK)) \
return; \
\
/* srcu can't be used from NMI */ \
WARN_ON_ONCE(rcuidle && in_nmi()); \
\
+ if (IS_ENABLED(CONFIG_LOCKDEP) && !(rcuidle)) { \
+ rcu_read_lock_sched_notrace(); \
+ rcu_dereference_sched((tp)->funcs); \
+ rcu_read_unlock_sched_notrace(); \
+ } \
+ \
/* keep srcu and sched-rcu usage consistent */ \
preempt_disable_notrace(); \
\
@@ -235,11 +241,6 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
TP_PROTO(data_proto), \
TP_ARGS(data_args), \
TP_CONDITION(cond), 0); \
- if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) { \
- rcu_read_lock_sched_notrace(); \
- rcu_dereference_sched(__tracepoint_##name.funcs);\
- rcu_read_unlock_sched_notrace(); \
- } \
} \
__DECLARE_TRACE_RCU(name, PARAMS(proto), PARAMS(args), \
PARAMS(cond), PARAMS(data_proto), PARAMS(data_args)) \
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 8ce77d9ac716..ad9fb4f12c63 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -269,6 +269,8 @@ static noinstr void rcu_dynticks_eqs_enter(void)
/* Better not have special action (TLB flush) pending! */
WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
(seq & RCU_DYNTICK_CTRL_MASK));
+
+ __preempt_count_add(RCUIDLE_OFFSET);
}
/*
@@ -281,6 +283,8 @@ static noinstr void rcu_dynticks_eqs_exit(void)
struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
int seq;
+ __preempt_count_sub(RCUIDLE_OFFSET);
+
/*
* CPUs seeing atomic_add_return() must see prior idle sojourns,
* and we also must force ordering with the next RCU read-side
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints
2020-09-03 15:36 ` peterz
@ 2020-09-03 15:54 ` Paul E. McKenney
2020-09-03 18:11 ` Guenter Roeck
1 sibling, 0 replies; 53+ messages in thread
From: Paul E. McKenney @ 2020-09-03 15:54 UTC (permalink / raw)
To: peterz
Cc: Guenter Roeck, linux-kernel, mingo, will, npiggin, elver, jgross,
rostedt, rjw, joel, svens, tglx, ulf.hansson, viresh.kumar,
vincent.guittot
On Thu, Sep 03, 2020 at 05:36:49PM +0200, peterz@infradead.org wrote:
> On Thu, Sep 03, 2020 at 08:19:38AM -0700, Guenter Roeck wrote:
> > This doesn't compile for me - there is no "name" parameter in __DO_TRACE().
> >
> > Error log:
> > In file included from ./include/linux/rculist.h:11,
> > from ./include/linux/pid.h:5,
> > from ./include/linux/sched.h:14,
> > from ./include/linux/sched/numa_balancing.h:10,
> > from ./include/trace/events/sched.h:8,
> > from kernel/sched/core.c:10:
> > ./include/trace/events/sched.h: In function 'trace_sched_kthread_stop':
> > ./include/linux/tracepoint.h:175:26: error: '__tracepoint_name' undeclared
> >
> > I applied your patch on top of v5.9-rc3-75-gfc3abb53250a. Are you using
> > a different branch ?
>
> Argh, I was on tip/master... easy fix though.
>
> ---
> diff --git a/include/linux/preempt.h b/include/linux/preempt.h
> index 7d9c1c0e149c..878bac893e41 100644
> --- a/include/linux/preempt.h
> +++ b/include/linux/preempt.h
> @@ -27,17 +27,20 @@
> * SOFTIRQ_MASK: 0x0000ff00
> * HARDIRQ_MASK: 0x000f0000
> * NMI_MASK: 0x00f00000
> + * RCUIDLE_MASK: 0x01000000
> * PREEMPT_NEED_RESCHED: 0x80000000
> */
> #define PREEMPT_BITS 8
> #define SOFTIRQ_BITS 8
> #define HARDIRQ_BITS 4
> #define NMI_BITS 4
> +#define RCUIDLE_BITS 1
>
> #define PREEMPT_SHIFT 0
> #define SOFTIRQ_SHIFT (PREEMPT_SHIFT + PREEMPT_BITS)
> #define HARDIRQ_SHIFT (SOFTIRQ_SHIFT + SOFTIRQ_BITS)
> #define NMI_SHIFT (HARDIRQ_SHIFT + HARDIRQ_BITS)
> +#define RCUIDLE_SHIFT (NMI_SHIFT + NMI_BITS)
>
> #define __IRQ_MASK(x) ((1UL << (x))-1)
>
> @@ -45,11 +48,13 @@
> #define SOFTIRQ_MASK (__IRQ_MASK(SOFTIRQ_BITS) << SOFTIRQ_SHIFT)
> #define HARDIRQ_MASK (__IRQ_MASK(HARDIRQ_BITS) << HARDIRQ_SHIFT)
> #define NMI_MASK (__IRQ_MASK(NMI_BITS) << NMI_SHIFT)
> +#define RCUIDLE_MASK (__IRQ_MASK(RCUIDLE_BITS) << RCUIDLE_SHIFT)
>
> #define PREEMPT_OFFSET (1UL << PREEMPT_SHIFT)
> #define SOFTIRQ_OFFSET (1UL << SOFTIRQ_SHIFT)
> #define HARDIRQ_OFFSET (1UL << HARDIRQ_SHIFT)
> #define NMI_OFFSET (1UL << NMI_SHIFT)
> +#define RCUIDLE_OFFSET (1UL << RCUIDLE_SHIFT)
>
> #define SOFTIRQ_DISABLE_OFFSET (2 * SOFTIRQ_OFFSET)
>
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 598fec9f9dbf..0469bc1c24fc 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -164,12 +164,18 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> void *__data; \
> int __maybe_unused __idx = 0; \
> \
> - if (!(cond)) \
> + if (!(cond) || (preempt_count() & RCUIDLE_MASK)) \
> return; \
> \
> /* srcu can't be used from NMI */ \
> WARN_ON_ONCE(rcuidle && in_nmi()); \
> \
> + if (IS_ENABLED(CONFIG_LOCKDEP) && !(rcuidle)) { \
> + rcu_read_lock_sched_notrace(); \
> + rcu_dereference_sched((tp)->funcs); \
> + rcu_read_unlock_sched_notrace(); \
> + } \
> + \
> /* keep srcu and sched-rcu usage consistent */ \
> preempt_disable_notrace(); \
> \
> @@ -235,11 +241,6 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> TP_PROTO(data_proto), \
> TP_ARGS(data_args), \
> TP_CONDITION(cond), 0); \
> - if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) { \
> - rcu_read_lock_sched_notrace(); \
> - rcu_dereference_sched(__tracepoint_##name.funcs);\
> - rcu_read_unlock_sched_notrace(); \
> - } \
> } \
> __DECLARE_TRACE_RCU(name, PARAMS(proto), PARAMS(args), \
> PARAMS(cond), PARAMS(data_proto), PARAMS(data_args)) \
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 8ce77d9ac716..ad9fb4f12c63 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -269,6 +269,8 @@ static noinstr void rcu_dynticks_eqs_enter(void)
> /* Better not have special action (TLB flush) pending! */
> WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> (seq & RCU_DYNTICK_CTRL_MASK));
> +
> + __preempt_count_add(RCUIDLE_OFFSET);
But the rcu_data structure's ->dynticks field is still to be used in cases
where the RCU grace-period kthread must do off-CPU access to the CPU's
RCU-idle state, correct? (Otherwise, some memory barriers are needed.)
Thanx, Paul
> }
>
> /*
> @@ -281,6 +283,8 @@ static noinstr void rcu_dynticks_eqs_exit(void)
> struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> int seq;
>
> + __preempt_count_sub(RCUIDLE_OFFSET);
> +
> /*
> * CPUs seeing atomic_add_return() must see prior idle sojourns,
> * and we also must force ordering with the next RCU read-side
>
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 11/11] lockdep,trace: Expose tracepoints
2020-09-03 15:36 ` peterz
2020-09-03 15:54 ` Paul E. McKenney
@ 2020-09-03 18:11 ` Guenter Roeck
1 sibling, 0 replies; 53+ messages in thread
From: Guenter Roeck @ 2020-09-03 18:11 UTC (permalink / raw)
To: peterz
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, joel, svens, tglx, ulf.hansson, viresh.kumar,
vincent.guittot
On 9/3/20 8:36 AM, peterz@infradead.org wrote:
> On Thu, Sep 03, 2020 at 08:19:38AM -0700, Guenter Roeck wrote:
>> This doesn't compile for me - there is no "name" parameter in __DO_TRACE().
>>
>> Error log:
>> In file included from ./include/linux/rculist.h:11,
>> from ./include/linux/pid.h:5,
>> from ./include/linux/sched.h:14,
>> from ./include/linux/sched/numa_balancing.h:10,
>> from ./include/trace/events/sched.h:8,
>> from kernel/sched/core.c:10:
>> ./include/trace/events/sched.h: In function 'trace_sched_kthread_stop':
>> ./include/linux/tracepoint.h:175:26: error: '__tracepoint_name' undeclared
>>
>> I applied your patch on top of v5.9-rc3-75-gfc3abb53250a. Are you using
>> a different branch ?
>
> Argh, I was on tip/master... easy fix though.
That version silences the tracebacks on arm. I can't test on real hardware,
so I don't know if it also fixes the problem reported at [1] (which seems
related).
Guenter
---
[1] https://lore.kernel.org/lkml/CA+G9fYuiJwN1ad955Xw4ShamX2=373r+56KsbpeverEs+i_NAg@mail.gmail.com/t/#u
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 10/11] lockdep: Only trace IRQ edges
2020-09-02 13:48 ` Guenter Roeck
@ 2020-09-08 14:22 ` peterz
2020-09-08 14:40 ` Guenter Roeck
0 siblings, 1 reply; 53+ messages in thread
From: peterz @ 2020-09-08 14:22 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, joel, svens, tglx, davem
On Wed, Sep 02, 2020 at 06:48:30AM -0700, Guenter Roeck wrote:
> On 9/2/20 2:12 AM, peterz@infradead.org wrote:
> > On Wed, Sep 02, 2020 at 11:09:35AM +0200, peterz@infradead.org wrote:
> >> On Tue, Sep 01, 2020 at 09:21:37PM -0700, Guenter Roeck wrote:
> >>> [ 0.000000] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:4875 check_flags.part.39+0x280/0x2a0
> >>> [ 0.000000] DEBUG_LOCKS_WARN_ON(lockdep_hardirqs_enabled())
> >>
> >>> [ 0.000000] [<00000000004cff18>] lock_acquire+0x218/0x4e0
> >>> [ 0.000000] [<0000000000d740c8>] _raw_spin_lock+0x28/0x40
> >>> [ 0.000000] [<00000000009870f4>] p1275_cmd_direct+0x14/0x60
> >>
> >> Lol! yes, I can see that going side-ways... let me poke at that.
> >
> > I suspect this will do.
> >
> > diff --git a/arch/sparc/prom/p1275.c b/arch/sparc/prom/p1275.c
> > index 889aa602f8d8..7cfe88e30b52 100644
> > --- a/arch/sparc/prom/p1275.c
> > +++ b/arch/sparc/prom/p1275.c
> > @@ -38,7 +38,7 @@ void p1275_cmd_direct(unsigned long *args)
> > unsigned long flags;
> >
> > local_save_flags(flags);
> > - local_irq_restore((unsigned long)PIL_NMI);
> > + arch_local_irq_restore((unsigned long)PIL_NMI);
> > raw_spin_lock(&prom_entry_lock);
> >
> > prom_world(1);
> >
> No, that doesn't help. Even removing that line entirely doesn't help.
> The problem seems to be that interrupts are not enabled in the first
> place. But why wasn't this a problem before ?
Previously every interrupt opt would disable/enable things, now we only
update state when something actually changes.
Anyway, I'm struggling with qemu-system-sparc64, I've got a sparc64
cross booting to mount, but I'm not seeing this, could you get me your
specific qemu cmdline please?
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 10/11] lockdep: Only trace IRQ edges
2020-09-08 14:22 ` peterz
@ 2020-09-08 14:40 ` Guenter Roeck
2020-09-08 15:41 ` [PATCH] sparc64: Fix irqtrace warnings on Ultra-S peterz
0 siblings, 1 reply; 53+ messages in thread
From: Guenter Roeck @ 2020-09-08 14:40 UTC (permalink / raw)
To: peterz
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, joel, svens, tglx, davem
On 9/8/20 7:22 AM, peterz@infradead.org wrote:
> On Wed, Sep 02, 2020 at 06:48:30AM -0700, Guenter Roeck wrote:
>> On 9/2/20 2:12 AM, peterz@infradead.org wrote:
>>> On Wed, Sep 02, 2020 at 11:09:35AM +0200, peterz@infradead.org wrote:
>>>> On Tue, Sep 01, 2020 at 09:21:37PM -0700, Guenter Roeck wrote:
>>>>> [ 0.000000] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:4875 check_flags.part.39+0x280/0x2a0
>>>>> [ 0.000000] DEBUG_LOCKS_WARN_ON(lockdep_hardirqs_enabled())
>>>>
>>>>> [ 0.000000] [<00000000004cff18>] lock_acquire+0x218/0x4e0
>>>>> [ 0.000000] [<0000000000d740c8>] _raw_spin_lock+0x28/0x40
>>>>> [ 0.000000] [<00000000009870f4>] p1275_cmd_direct+0x14/0x60
>>>>
>>>> Lol! yes, I can see that going side-ways... let me poke at that.
>>>
>>> I suspect this will do.
>>>
>>> diff --git a/arch/sparc/prom/p1275.c b/arch/sparc/prom/p1275.c
>>> index 889aa602f8d8..7cfe88e30b52 100644
>>> --- a/arch/sparc/prom/p1275.c
>>> +++ b/arch/sparc/prom/p1275.c
>>> @@ -38,7 +38,7 @@ void p1275_cmd_direct(unsigned long *args)
>>> unsigned long flags;
>>>
>>> local_save_flags(flags);
>>> - local_irq_restore((unsigned long)PIL_NMI);
>>> + arch_local_irq_restore((unsigned long)PIL_NMI);
>>> raw_spin_lock(&prom_entry_lock);
>>>
>>> prom_world(1);
>>>
>> No, that doesn't help. Even removing that line entirely doesn't help.
>> The problem seems to be that interrupts are not enabled in the first
>> place. But why wasn't this a problem before ?
>
> Previously every interrupt opt would disable/enable things, now we only
> update state when something actually changes.
>
> Anyway, I'm struggling with qemu-system-sparc64, I've got a sparc64
> cross booting to mount, but I'm not seeing this, could you get me your
> specific qemu cmdline please?
>
initrd:
qemu-system-sparc64 -M sun4u -cpu "TI UltraSparc IIi" -m 512 \
-initrd rootfs.cpio \
-kernel arch/sparc/boot/image -no-reboot \
-append "panic=-1 slub_debug=FZPUA rdinit=/sbin/init console=ttyS0" \
-nographic -monitor none
root file system:
qemu-system-sparc64 -M sun4u -cpu "TI UltraSparc IIi" -m 512 \
-snapshot =drive file=rootfs.ext2,format=raw,if=ide \
-kernel arch/sparc/boot/image -no-reboot \
-append "panic=-1 slub_debug=FZPUA root=/dev/sda rootwait console=ttyS0" \
-nographic -monitor none
Some of it, like the CPU, should not be needed. qemu version is v5.1,
but v5.0 should do as well. Some older qemu versions won't accept the
kernel from the command line.
Did you enable lockdep debugging ? In my configuration I enable lots
of debug options on top of defconfig. See [1], function __setup_fragment(),
for details.
Some root file systems are at [2] if needed. The complete script used
to build and run the code is at [3].
Guenter
---
[1] https://github.com/groeck/linux-build-test/blob/master/rootfs/scripts/common.sh
[2] https://github.com/groeck/linux-build-test/tree/master/rootfs/sparc64
[3] https://github.com/groeck/linux-build-test/blob/master/rootfs/sparc64/run-qemu-sparc64.sh
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH] sparc64: Fix irqtrace warnings on Ultra-S
2020-09-08 14:40 ` Guenter Roeck
@ 2020-09-08 15:41 ` peterz
0 siblings, 0 replies; 53+ messages in thread
From: peterz @ 2020-09-08 15:41 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-kernel, mingo, will, npiggin, elver, jgross, paulmck,
rostedt, rjw, joel, svens, tglx, davem
On Tue, Sep 08, 2020 at 07:40:23AM -0700, Guenter Roeck wrote:
> qemu-system-sparc64 -M sun4u -cpu "TI UltraSparc IIi" -m 512 \
> -initrd rootfs.cpio \
> -kernel arch/sparc/boot/image -no-reboot \
> -append "panic=-1 slub_debug=FZPUA rdinit=/sbin/init console=ttyS0" \
> -nographic -monitor none
Thanks I got it. Also enabling DEBUG_LOCKDEP helps (-:
---
Subject: sparc64: Fix irqtrace warnings on Ultra-S
Recent changes in Lockdep's IRQTRACE broke Ultra-S.
In order avoid redundant IRQ state changes, local_irq_restore() lost the
ability to trace a disable. Change the code to use local_irq_save() to
disable IRQs and then use arch_local_irq_restore() to further disable
NMIs.
This result in slightly suboptimal code, but given this code uses a
global spinlock, performance cannot be its primary purpose.
Fixes: 044d0d6de9f5 ("lockdep: Only trace IRQ edges")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
A possible alternative would be:
local_save_flags(flags);
arch_local_irq_restore((unsigned long)PIL_NMI);
if (IS_ENABLED(CONFIG_TRACE_IRQFLAGS))
trace_hardirqs_off();
which generates optimal code, but is more verbose.
arch/sparc/prom/p1275.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/arch/sparc/prom/p1275.c b/arch/sparc/prom/p1275.c
index 889aa602f8d8..e22233fcf741 100644
--- a/arch/sparc/prom/p1275.c
+++ b/arch/sparc/prom/p1275.c
@@ -37,16 +37,15 @@ void p1275_cmd_direct(unsigned long *args)
{
unsigned long flags;
- local_save_flags(flags);
- local_irq_restore((unsigned long)PIL_NMI);
+ local_irq_save(flags);
+ arch_local_irq_restore((unsigned long)PIL_NMI);
raw_spin_lock(&prom_entry_lock);
prom_world(1);
prom_cif_direct(args);
prom_world(0);
- raw_spin_unlock(&prom_entry_lock);
- local_irq_restore(flags);
+ raw_spin_unlock_irqrestore(&prom_entry_lock, flags);
}
void prom_cif_init(void *cif_handler, void *cif_stack)
^ permalink raw reply related [flat|nested] 53+ messages in thread
end of thread, other threads:[~2020-09-08 20:17 UTC | newest]
Thread overview: 53+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-21 8:47 [PATCH v2 00/11] TRACE_IRQFLAGS wreckage Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 01/11] lockdep: Use raw_cpu_*() for per-cpu variables Peter Zijlstra
2020-08-27 1:05 ` Joel Fernandes
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 02/11] cpuidle: Fixup IRQ state Peter Zijlstra
2020-08-21 17:36 ` Rafael J. Wysocki
2020-08-27 1:06 ` Joel Fernandes
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 03/11] sched,idle,rcu: Push rcu_idle deeper into the idle path Peter Zijlstra
2020-08-27 1:18 ` Joel Fernandes
2020-08-27 1:24 ` Joel Fernandes
2020-08-27 7:47 ` peterz
2020-08-27 7:53 ` Thomas Gleixner
2020-08-27 22:30 ` Joel Fernandes
2020-09-02 7:08 ` peterz
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 04/11] cpuidle: Make CPUIDLE_FLAG_TLB_FLUSHED generic Peter Zijlstra
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 05/11] cpuidle: Move trace_cpu_idle() into generic code Peter Zijlstra
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 06/11] x86/entry: Remove unused THUNKs Peter Zijlstra
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 07/11] locking/lockdep: Cleanup Peter Zijlstra
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 08/11] nds32: Implement arch_irqs_disabled() Peter Zijlstra
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 09/11] arm64: " Peter Zijlstra
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
2020-08-21 8:47 ` [PATCH v2 10/11] lockdep: Only trace IRQ edges Peter Zijlstra
2020-08-26 0:47 ` Thomas Gleixner
2020-09-02 4:21 ` Guenter Roeck
2020-09-02 9:09 ` peterz
2020-09-02 9:12 ` peterz
2020-09-02 13:48 ` Guenter Roeck
2020-09-08 14:22 ` peterz
2020-09-08 14:40 ` Guenter Roeck
2020-09-08 15:41 ` [PATCH] sparc64: Fix irqtrace warnings on Ultra-S peterz
2020-08-21 8:47 ` [PATCH v2 11/11] lockdep,trace: Expose tracepoints Peter Zijlstra
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
2020-09-02 3:51 ` [PATCH v2 11/11] " Guenter Roeck
2020-09-02 7:24 ` peterz
2020-09-02 13:21 ` Guenter Roeck
2020-09-02 8:56 ` peterz
2020-09-02 13:57 ` Guenter Roeck
2020-09-03 14:00 ` peterz
2020-09-03 14:13 ` peterz
2020-09-03 15:19 ` Guenter Roeck
2020-09-03 15:36 ` peterz
2020-09-03 15:54 ` Paul E. McKenney
2020-09-03 18:11 ` Guenter Roeck
2020-08-21 12:58 ` [PATCH v2 00/11] TRACE_IRQFLAGS wreckage peterz
2020-08-26 10:16 ` [PATCH v2 12/11] mips: Implement arch_irqs_disabled() peterz
2020-08-27 7:54 ` [tip: locking/core] " tip-bot2 for Peter Zijlstra
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).