| Message ID | 20230307184645.521db5c9@gandalf.local.home (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tracing, hardirq: Do not test lockdep on irq trace points when disabled | expand |
On Tue, Mar 07, 2023 at 06:46:45PM -0500, Steven Rostedt wrote: > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > When CONFIG_LOCKDEP is enabled, the trace points have: > > static inline void trace_##name(proto) \ > { \ > if (static_key_false(&__tracepoint_##name.key)) \ > __DO_TRACE(name, \ > TP_ARGS(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(); \ > } \ > } \ > Where it will test lockdep for trace points even when they are not > enabled, to make sure they do not cause RCU issues, and lockdep will > trigger even when the trace points are not enabled. I'm confused what that's actually trying to do.. You're not tickling the rcu_is_watching() check, because rcu_read_lock_sched_notrace() doesn't have that. You're not tickling RCU_LOCKDEP_WARN() because you did rcu_read_lock_sched_notrace(). So what?!?
On Wed, 8 Mar 2023 15:39:25 +0100 Peter Zijlstra <peterz@infradead.org> wrote: > > if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) { \ > > rcu_read_lock_sched_notrace(); \ > > rcu_dereference_sched(__tracepoint_##name.funcs);\ > > rcu_read_unlock_sched_notrace(); \ > > } \ > > } \ > > > Where it will test lockdep for trace points even when they are not > > enabled, to make sure they do not cause RCU issues, and lockdep will > > trigger even when the trace points are not enabled. > > I'm confused what that's actually trying to do.. > > You're not tickling the rcu_is_watching() check, because > rcu_read_lock_sched_notrace() doesn't have that. You're not tickling > RCU_LOCKDEP_WARN() because you did rcu_read_lock_sched_notrace(). > > So what?!? Actually, I think the proper changes is to just add "rcu_is_watching()" warning here? That code is from 2014 where it simulated what was done in DO_TRACE() and I think back then, those calls did trigger warnings. But this code has not kept up with the changes in DO_TRACE. So something like this instead? diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h index e299f29375bb..d3a221158ab1 100644 --- a/include/linux/tracepoint.h +++ b/include/linux/tracepoint.h @@ -260,9 +260,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p) TP_ARGS(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(); \ + WARN_ON_ONCE(!rcu_is_watching()); \ } \ } \ __DECLARE_TRACE_RCU(name, PARAMS(proto), PARAMS(args), \ -- Steve
On Wed, 8 Mar 2023 11:56:06 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h > index e299f29375bb..d3a221158ab1 100644 > --- a/include/linux/tracepoint.h > +++ b/include/linux/tracepoint.h > @@ -260,9 +260,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p) > TP_ARGS(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(); \ > + WARN_ON_ONCE(!rcu_is_watching()); \ > } \ > } \ > __DECLARE_TRACE_RCU(name, PARAMS(proto), PARAMS(args), \ This fixes the hung tasks as well. I'll push this through instead, and mark it for stable. As the old check was incorrect for a long time. -- Steve
diff --git a/kernel/trace/trace_preemptirq.c b/kernel/trace/trace_preemptirq.c index f992444a0b1f..4765e1a6c7f4 100644 --- a/kernel/trace/trace_preemptirq.c +++ b/kernel/trace/trace_preemptirq.c @@ -26,9 +26,17 @@ static DEFINE_PER_CPU(int, tracing_irq_cpu); * * On older architectures, use the rcuidle tracing methods (which * aren't NMI-safe - so exclude NMI contexts): + * + * Note, when LOCKDEP is enabled, the default checks for the trace point + * can cause a noticeable slowdown even when the trace point is not + * enabled. By only calling the trace point when the trace point is + * enabled, will keep the lockdep checks from being always triggered + * and slowing down the system. */ #ifdef CONFIG_ARCH_WANTS_NO_INSTR -#define trace(point) trace_##point +#define trace(point) \ + if (!IS_ENABLED(CONFIG_LOCKDEP) || trace_##point##_enabled()) \ + trace_##point #else #define trace(point) if (!in_nmi()) trace_##point##_rcuidle #endif