Re: [RFC PATCH v3 2/3] tracing: Introduce tracepoint_is_syscall()

[Steven Rostedt <rostedt@goodmis.org>]

On Sat, 26 Oct 2024 11:46:28 -0400
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> Introduce a "syscall" flag within the extended structure to know whether
> a tracepoint needs rcu tasks trace grace period before reclaim.
> This can be queried using tracepoint_is_syscall().
> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Michael Jeanson <mjeanson@efficios.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Yonghong Song <yhs@fb.com>
> Cc: Paul E. McKenney <paulmck@kernel.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Cc: bpf@vger.kernel.org
> Cc: Joel Fernandes <joel@joelfernandes.org>
> Cc: Jordan Rife <jrife@google.com>
> ---
>  include/linux/tracepoint-defs.h |  2 ++
>  include/linux/tracepoint.h      | 24 ++++++++++++++++++++++++
>  include/trace/define_trace.h    |  2 +-
>  3 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
> index 967c08d9da84..53119e074c87 100644
> --- a/include/linux/tracepoint-defs.h
> +++ b/include/linux/tracepoint-defs.h
> @@ -32,6 +32,8 @@ struct tracepoint_func {
>  struct tracepoint_ext {
>  	int (*regfunc)(void);
>  	void (*unregfunc)(void);
> +	/* Flags. */
> +	unsigned int syscall:1;

I wonder if we should call it "sleepable" instead? For this patch set
do we really care if it's a system call or not? It's really if the
tracepoint is sleepable or not that's the issue. System calls are just
one user of it, there may be more in the future, and the changes to BPF
will still be needed.

Other than that, I think this could work.

-- Steve


>  };
>  
>  struct tracepoint {
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 83dc24ee8b13..93e70bc64533 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -104,6 +104,12 @@ void for_each_tracepoint_in_module(struct module *mod,
>   * tracepoint_synchronize_unregister must be called between the last tracepoint
>   * probe unregistration and the end of module exit to make sure there is no
>   * caller executing a probe when it is freed.
> + *
> + * An alternative is to use the following for batch reclaim associated
> + * with a given tracepoint:
> + *
> + * - tracepoint_is_syscall() == false: call_rcu()
> + * - tracepoint_is_syscall() == true:  call_rcu_tasks_trace()
>   */
>  #ifdef CONFIG_TRACEPOINTS
>  static inline void tracepoint_synchronize_unregister(void)
> @@ -111,9 +117,17 @@ static inline void tracepoint_synchronize_unregister(void)
>  	synchronize_rcu_tasks_trace();
>  	synchronize_rcu();
>  }
> +static inline bool tracepoint_is_syscall(struct tracepoint *tp)
> +{
> +	return tp->ext && tp->ext->syscall;
> +}
>  #else
>  static inline void tracepoint_synchronize_unregister(void)
>  { }
> +static inline bool tracepoint_is_syscall(struct tracepoint *tp)
> +{
> +	return false;
> +}
>  #endif
>  
>  #ifdef CONFIG_HAVE_SYSCALL_TRACEPOINTS
> @@ -345,6 +359,15 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>  	struct tracepoint_ext __tracepoint_ext_##_name = {		\
>  		.regfunc = _reg,					\
>  		.unregfunc = _unreg,					\
> +		.syscall = false,					\
> +	};								\
> +	__DEFINE_TRACE_EXT(_name, &__tracepoint_ext_##_name, PARAMS(_proto), PARAMS(_args));
> +
> +#define DEFINE_TRACE_SYSCALL(_name, _reg, _unreg, _proto, _args)	\
> +	struct tracepoint_ext __tracepoint_ext_##_name = {		\
> +		.regfunc = _reg,					\
> +		.unregfunc = _unreg,					\
> +		.syscall = true,					\
>  	};								\
>  	__DEFINE_TRACE_EXT(_name, &__tracepoint_ext_##_name, PARAMS(_proto), PARAMS(_args));
>  
> @@ -389,6 +412,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>  #define __DECLARE_TRACE_SYSCALL	__DECLARE_TRACE
>  
>  #define DEFINE_TRACE_FN(name, reg, unreg, proto, args)
> +#define DEFINE_TRACE_SYSCALL(name, reg, unreg, proto, args)
>  #define DEFINE_TRACE(name, proto, args)
>  #define EXPORT_TRACEPOINT_SYMBOL_GPL(name)
>  #define EXPORT_TRACEPOINT_SYMBOL(name)
> diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
> index ff5fa17a6259..63fea2218afa 100644
> --- a/include/trace/define_trace.h
> +++ b/include/trace/define_trace.h
> @@ -48,7 +48,7 @@
>  
>  #undef TRACE_EVENT_SYSCALL
>  #define TRACE_EVENT_SYSCALL(name, proto, args, struct, assign, print, reg, unreg) \
> -	DEFINE_TRACE_FN(name, reg, unreg, PARAMS(proto), PARAMS(args))
> +	DEFINE_TRACE_SYSCALL(name, reg, unreg, PARAMS(proto), PARAMS(args))
>  
>  #undef TRACE_EVENT_NOP
>  #define TRACE_EVENT_NOP(name, proto, args, struct, assign, print)