Re: [PATCH v2 10/16] tracing: Guard __DECLARE_TRACE() use of __DO_TRACE_CALL() with SRCU-fast

[Steven Rostedt <rostedt@goodmis.org>]

On Wed,  5 Nov 2025 12:32:10 -0800
"Paul E. McKenney" <paulmck@kernel.org> wrote:
> 
> The current commit can be thought of as an approximate revert of that
> commit, with some compensating additions of preemption disabling pointed
> out by Steven Rostedt (thank you, Steven!).  This preemption disabling

> uses guard(preempt_notrace)(), and while in the area a couple of other
> use cases were also converted to guards.

Actually, please don't do any conversions. That code is unrelated to
this work and I may be touching it. I don't need unneeded conflicts.

> ---
>  include/linux/tracepoint.h   | 45 ++++++++++++++++++++++--------------
>  include/trace/perf.h         |  4 ++--
>  include/trace/trace_events.h |  4 ++--
>  kernel/tracepoint.c          | 21 ++++++++++++++++-
>  4 files changed, 52 insertions(+), 22 deletions(-)
> 
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 826ce3f8e1f8..9f8b19cd303a 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -33,6 +33,8 @@ struct trace_eval_map {
>  
>  #define TRACEPOINT_DEFAULT_PRIO	10
>  
> +extern struct srcu_struct tracepoint_srcu;
> +
>  extern int
>  tracepoint_probe_register(struct tracepoint *tp, void *probe, void *data);
>  extern int
> @@ -115,7 +117,10 @@ void for_each_tracepoint_in_module(struct module *mod,
>  static inline void tracepoint_synchronize_unregister(void)
>  {
>  	synchronize_rcu_tasks_trace();
> -	synchronize_rcu();
> +	if (IS_ENABLED(CONFIG_PREEMPT_RT))
> +		synchronize_srcu(&tracepoint_srcu);
> +	else
> +		synchronize_rcu();
>  }

Instead of using the IS_ENABLED(CONFIG_PREEMPT_RT) I think it would be
somewhat cleaner to add macros (all of this is untested):

#ifdef CONFIG_PREEMPT_RT
extern struct srcu_struct tracepoint_srcu;
# define tracepoint_sync() synchronizes_srcu(&tracepoint_srcu)
# define tracepoint_guard() \
     guard(srcu_fast_notrace)(&tracepoint_srcu); \
     guard(migrate)()
#else
# define tracepoint_sync() synchronize_rcu();
# define tracepoint_guard() guard(preempt_notrace)
#endif

And then the above can be:

static inline void tracepoint_synchronize_unregister(void)
{
 	synchronize_rcu_tasks_trace();
	tracepoint_sync();
}

and the below:

	static inline void __do_trace_##name(proto)			\
	{								\
		if (cond) {						\
			tracepoint_guard();				\
			__DO_TRACE_CALL(name, TP_ARGS(args));		\
		}							\
	}								\

And not have to duplicate all that code.

>  static inline bool tracepoint_is_faultable(struct tracepoint *tp)
>  {
> @@ -266,23 +271,29 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>  		return static_branch_unlikely(&__tracepoint_##name.key);\
>  	}
>  
> -#define __DECLARE_TRACE(name, proto, args, cond, data_proto)		\
> +#define __DECLARE_TRACE(name, proto, args, cond, data_proto)			\
>  	__DECLARE_TRACE_COMMON(name, PARAMS(proto), PARAMS(args), PARAMS(data_proto)) \
> -	static inline void __do_trace_##name(proto)			\
> -	{								\
> -		if (cond) {						\
> -			guard(preempt_notrace)();			\
> -			__DO_TRACE_CALL(name, TP_ARGS(args));		\
> -		}							\
> -	}								\
> -	static inline void trace_##name(proto)				\
> -	{								\
> -		if (static_branch_unlikely(&__tracepoint_##name.key))	\
> -			__do_trace_##name(args);			\
> -		if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) {		\
> -			WARN_ONCE(!rcu_is_watching(),			\
> -				  "RCU not watching for tracepoint");	\
> -		}							\
> +	static inline void __do_trace_##name(proto)				\
> +	{									\
> +		if (cond) {							\
> +			if (IS_ENABLED(CONFIG_PREEMPT_RT) && preemptible()) {	\
> +				guard(srcu_fast_notrace)(&tracepoint_srcu);	\
> +				guard(migrate)();				\
> +				__DO_TRACE_CALL(name, TP_ARGS(args));		\
> +			} else {						\
> +				guard(preempt_notrace)();			\
> +				__DO_TRACE_CALL(name, TP_ARGS(args));		\
> +			}							\
> +		}								\
> +	}									\
> +	static inline void trace_##name(proto)					\
> +	{									\
> +		if (static_branch_unlikely(&__tracepoint_##name.key))		\
> +			__do_trace_##name(args);				\
> +		if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) {			\
> +			WARN_ONCE(!rcu_is_watching(),				\
> +				  "RCU not watching for tracepoint");		\
> +		}								\
>  	
>  

>  /*
> diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h
> index 4f22136fd465..fbc07d353be6 100644
> --- a/include/trace/trace_events.h
> +++ b/include/trace/trace_events.h
> @@ -436,6 +436,7 @@ __DECLARE_EVENT_CLASS(call, PARAMS(proto), PARAMS(args), PARAMS(tstruct), \
>  static notrace void							\
>  trace_event_raw_event_##call(void *__data, proto)			\
>  {									\
> +	guard(preempt_notrace)();					\

Note, the tracepoint code expects that there's only one level of
preemption done, as it records the preempt_count and needs to subtract
what tracing added. Just calling preempt_notrace here if it had already
disabled preemption will break that code.

It should only disable preemption if it hasn't already done that (when
PREEMPT_RT is enabled).

>  	do_trace_event_raw_event_##call(__data, args);			\
>  }
>  
> @@ -447,9 +448,8 @@ static notrace void							\
>  trace_event_raw_event_##call(void *__data, proto)			\
>  {									\
>  	might_fault();							\
> -	preempt_disable_notrace();					\
> +	guard(preempt_notrace)();					\
>  	do_trace_event_raw_event_##call(__data, args);			\
> -	preempt_enable_notrace();					\

I may be modifying the above, so I would leave it alone.

Thanks,

-- Steve