| Message ID | 20230309165603.6967197d@gandalf.local.home (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tracing: Make tracepoint lockdep check actually test something | expand |
On Thu, 9 Mar 2023 16:56:03 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > A while ago where the trace events had the following: > > rcu_read_lock_sched_notrace(); > rcu_dereference_sched(...); > rcu_read_unlock_sched_notrace(); > > If the tracepoint is enabled, it could trigger RCU issues if called in > the wrong place. And this warning was only triggered if lockdep was > enabled. If the tracepoint was never enabled with lockdep, the bug would > not be caught. To handle this, the above sequence was done when lockdep > was enabled regardless if the tracepoint was enabled or not (although the > always enabled code really didn't do anything, it would still trigger a > warning). > > But a lot has changed since that lockdep code was added. One is, that > sequence no longer triggers any warning. Another is, the tracepoint when > enabled doesn't even do that sequence anymore. > > The main check we care about today is whether RCU is "watching" or not. > So if lockdep is enabled, always check if rcu_is_watching() which will > trigger a warning if it is not (tracepoints require RCU to be watching). > > Note, that old sequence did add a bit of overhead when lockdep was enabled, > and with the latest kernel updates, would cause the system to slow down > enough to trigger kernel "stalled" warnings. > > Link: http://lore.kernel.org/lkml/20140806181801.GA4605@redhat.com > Link: http://lore.kernel.org/lkml/20140807175204.C257CAC5@viggo.jf.intel.com > Link: https://lore.kernel.org/lkml/20230307184645.521db5c9@gandalf.local.home/ > This check has been introduced by commit 3a630178fd5f ("tracing: generate RCU warnings even when tracepoints are disabled"), and it also added a comment above this macro. * When lockdep is enabled, we make sure to always do the RCU portions of * the tracepoint code, regardless of whether tracing is on. However, * don't check if the condition is false, due to interaction with idle * instrumentation. This lets us find RCU issues triggered with tracepoints * even when this tracepoint is off. This code has no purpose other than * poking RCU a bit. I think at least the last sentence will be outdated by this fix. Thank you, > Cc: stable@vger.kernel.org > Fixes: e6753f23d961 ("tracepoint: Make rcuidle tracepoint callers use SRCU") > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > --- > include/linux/tracepoint.h | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > 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), \ > -- > 2.39.1 >
On Fri, 10 Mar 2023 17:27:58 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > This check has been introduced by commit 3a630178fd5f ("tracing: generate RCU > warnings even when tracepoints are disabled"), and it also added a comment > above this macro. > > * When lockdep is enabled, we make sure to always do the RCU portions of > * the tracepoint code, regardless of whether tracing is on. However, > * don't check if the condition is false, due to interaction with idle > * instrumentation. This lets us find RCU issues triggered with tracepoints > * even when this tracepoint is off. This code has no purpose other than > * poking RCU a bit. > > I think at least the last sentence will be outdated by this fix. Ah thanks, I forgot to update that part. What about: * When lockdep is enabled, we make sure to always test if RCU is * "watching" regardless if the tracepoint is enabled or not. Tracepoints * require RCU to be active, and it should always warn at the tracepoint * site if it is not watching, as it will need to be active when the * tracepoint is enabled. ? -- Steve
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), \