On Mon, Mar 25, 2024 at 3:11 PM Steven Rostedt [off-list ref] wrote:

On Mon, 25 Mar 2024 11:38:48 +0900
Masami Hiramatsu (Google) [off-list ref] wrote:

quoted

On Fri, 22 Mar 2024 09:03:23 -0700
Andrii Nakryiko [off-list ref] wrote:

quoted

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.

Really, it's for debugging. I would argue that it should *not* be default y.
Peter added this to find all the locations that could be called where RCU
is not watching. But the issue I have is that this is that it *does cause
overhead* with function tracing.

I believe we found pretty much all locations that were an issue, and we
should now just make it an option for developers.

It's no different than lockdep. Test boxes should have it enabled, but
there's no reason to have this enabled in a production system.

I tend to agree with Steven here (which is why I sent this patch as it
is), but I'm happy to do it as an opt-out, if Masami insists. Please
do let me know if I need to send v2 or this one is actually the one
we'll end up using. Thanks!

-- Steve

quoted

quoted

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Paul E. McKenney <paulmck@kernel.org>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 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?

quoted

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

quoted

+   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.

quoted

+
+     If unsure, say N

      If unsure, say Y

Thank you,

quoted

+
 config RING_BUFFER_RECORD_RECURSION
    bool "Record functions that recurse in the ring buffer"
    depends on FTRACE_RECORD_RECURSION
--
2.43.0