On Fri, 22 Mar 2024 09:03:23 -0700
Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> Introduce CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING config option to
> control whether ftrace low-level code performs additional
> rcu_is_watching()-based validation logic in an attempt to catch noinstr
> violations.
>
> This check is expected to never be true in practice and would be best
> controlled with extra config to let users decide if they are willing to
> pay the price.
Hmm, for me, it sounds like "WARN_ON(something) never be true in practice
so disable it by default". I think CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING
is OK, but tht should be set to Y by default. If you have already verified
that your system never make it true and you want to optimize your ftrace
path, you can manually set it to N at your own risk.
>
> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Cc: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> Cc: Paul E. McKenney <paulmck@xxxxxxxxxx>
> Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> ---
> include/linux/trace_recursion.h | 2 +-
> kernel/trace/Kconfig | 13 +++++++++++++
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/trace_recursion.h b/include/linux/trace_recursion.h
> index d48cd92d2364..24ea8ac049b4 100644
> --- a/include/linux/trace_recursion.h
> +++ b/include/linux/trace_recursion.h
> @@ -135,7 +135,7 @@ extern void ftrace_record_recursion(unsigned long ip, unsigned long parent_ip);
> # define do_ftrace_record_recursion(ip, pip) do { } while (0)
> #endif
>
> -#ifdef CONFIG_ARCH_WANTS_NO_INSTR
> +#ifdef CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING
> # define trace_warn_on_no_rcu(ip) \
> ({ \
> bool __ret = !rcu_is_watching(); \
BTW, maybe we can add "unlikely" in the next "if" line?
> diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> index 61c541c36596..19bce4e217d6 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -974,6 +974,19 @@ config FTRACE_RECORD_RECURSION_SIZE
> This file can be reset, but the limit can not change in
> size at runtime.
>
> +config FTRACE_VALIDATE_RCU_IS_WATCHING
> + bool "Validate RCU is on during ftrace recursion check"
> + depends on FUNCTION_TRACER
> + depends on ARCH_WANTS_NO_INSTR
default y
> + help
> + All callbacks that attach to the function tracing have some sort
> + of protection against recursion. This option performs additional
> + checks to make sure RCU is on when ftrace callbacks recurse.
> +
> + This will add more overhead to all ftrace-based invocations.
... invocations, but keep it safe.
> +
> + If unsure, say N
If unsure, say Y
Thank you,
> +
> config RING_BUFFER_RECORD_RECURSION
> bool "Record functions that recurse in the ring buffer"
> depends on FTRACE_RECORD_RECURSION
> --
> 2.43.0
>
--
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
