diff mbox series

[v2] tracing: Make tracepoint lockdep check actually test something

Message ID 20230310172856.77406446@gandalf.local.home (mailing list archive)
State Accepted
Commit c2679254b9c9980d9045f0f722cf093a2b1f7590
Headers show
Series [v2] tracing: Make tracepoint lockdep check actually test something | expand

Commit Message

Steven Rostedt March 10, 2023, 10:28 p.m. UTC 
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/

Cc: stable@vger.kernel.org
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Joel Fernandes <joel@joelfernandes.org>
Fixes: e6753f23d961 ("tracepoint: Make rcuidle tracepoint callers use SRCU")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
Changes since v1: https://lore.kernel.org/linux-trace-kernel/20230309165603.6967197d@gandalf.local.home

 - Updated the comment above the code (Masami Hiramatsu)

 include/linux/tracepoint.h | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

Comments

Peter Zijlstra March 10, 2023, 11:56 p.m. UTC | #1 
On Fri, Mar 10, 2023 at 05:28:56PM -0500, Steven Rostedt wrote:

> @@ -249,9 +248,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),		\

Yep, that makes heaps more sense. If you so care you can save one more
line and make the {} go away too.

In any case,

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Steven Rostedt March 11, 2023, 12:40 a.m. UTC | #2 
On Sat, 11 Mar 2023 00:56:53 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Mar 10, 2023 at 05:28:56PM -0500, Steven Rostedt wrote:
> 
> > @@ -249,9 +248,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),		\  
> 
> Yep, that makes heaps more sense. If you so care you can save one more
> line and make the {} go away too.

I thought about that, but I kinda prefer the brackets still.

> 
> In any case,
> 
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Thanks!

-- Steve
Steven Rostedt March 13, 2023, 1:46 p.m. UTC | #3 
On Fri, 10 Mar 2023 17:28:56 -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/
> 
> Cc: stable@vger.kernel.org
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Joel Fernandes <joel@joelfernandes.org>
> Fixes: e6753f23d961 ("tracepoint: Make rcuidle tracepoint callers use SRCU")
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
> Changes since v1: https://lore.kernel.org/linux-trace-kernel/20230309165603.6967197d@gandalf.local.home
> 
>  - Updated the comment above the code (Masami Hiramatsu)
> 
>  include/linux/tracepoint.h | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index fa1004fcf810..2083f2d2f05b 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -231,12 +231,11 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>   * not add unwanted padding between the beginning of the section and the
>   * structure. Force alignment to the same alignment as the section start.
>   *
> - * 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.
> + * 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.
>   */

Masami,

Are you OK with this version of the patch?

-- Steve

>  #define __DECLARE_TRACE(name, proto, args, cond, data_proto)		\
>  	extern int __traceiter_##name(data_proto);			\
> @@ -249,9 +248,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),		\
diff mbox series

Patch

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index fa1004fcf810..2083f2d2f05b 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -231,12 +231,11 @@  static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
  * not add unwanted padding between the beginning of the section and the
  * structure. Force alignment to the same alignment as the section start.
  *
- * 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.
+ * 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.
  */
 #define __DECLARE_TRACE(name, proto, args, cond, data_proto)		\
 	extern int __traceiter_##name(data_proto);			\
@@ -249,9 +248,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),		\