Re: [PATCH v3 1/2] ftrace: disable preemption between ftrace_test_recursion_trylock/unlock()
From: 王贇 <hidden>
Date: 2021-10-15 03:13:18
Also in:
linux-riscv, linuxppc-dev, lkml
On 2021/10/14 下午11:14, Petr Mladek wrote: [snip]
quoted
- return trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START, TRACE_FTRACE_MAX); + int bit; + + bit = trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START, TRACE_FTRACE_MAX); + /* + * The zero bit indicate we are nested + * in another trylock(), which means the + * preemption already disabled. + */ + if (bit > 0) + preempt_disable_notrace();Is this safe? The preemption is disabled only when trace_test_and_set_recursion() was called by ftrace_test_recursion_trylock(). We must either always disable the preemtion when bit >= 0. Or we have to disable the preemtion already in trace_test_and_set_recursion().
Internal calling of trace_test_and_set_recursion() will disable preemption on succeed, it should be safe. We can also consider move the logical into trace_test_and_set_recursion() and trace_clear_recursion(), but I'm not very sure about that... ftrace internally already make sure preemption disabled, what uncovered is those users who call API trylock/unlock, isn't it?
Finally, the comment confused me a lot. The difference between nesting and recursion is far from clear. And the code is tricky liky like hell :-) I propose to add some comments, see below for a proposal.
The comments do confusing, I'll make it something like: The zero bit indicate trace recursion happened, whatever the recursively call was made by ftrace handler or ftrace itself, the preemption already disabled. Will this one looks better to you?
quoted
+ + return bit; } /**@@ -222,9 +233,13 @@ static __always_inline int ftrace_test_recursion_trylock(unsigned long ip, * @bit: The return of a successful ftrace_test_recursion_trylock() * * This is used at the end of a ftrace callback. + * + * Preemption will be enabled (if it was previously enabled). */ static __always_inline void ftrace_test_recursion_unlock(int bit) { + if (bit)This is not symetric with trylock(). It should be: if (bit > 0) Anyway, trace_clear_recursion() quiently ignores bit != 0
Yes, bit == 0 should not happen in here.
quoted hunk ↗ jump to hunk
quoted
+ preempt_enable_notrace(); trace_clear_recursion(bit); }Below is my proposed patch that tries to better explain the recursion check: From 20d69f11e2683262fa0043b803999061cbac543f Mon Sep 17 00:00:00 2001 From: Petr Mladek <pmladek@suse.com> Date: Thu, 14 Oct 2021 16:57:39 +0200 Subject: [PATCH] trace: Better describe the recursion check return values The trace recursion check might be called recursively by different layers of the tracing code. It is safe recursion and the check is even optimized for this case. The problematic recursion is when the traced function is called by the tracing code. This is properly detected. Try to explain this difference a better way. Signed-off-by: Petr Mladek <pmladek@suse.com> --- include/linux/trace_recursion.h | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)diff --git a/include/linux/trace_recursion.h b/include/linux/trace_recursion.h index a9f9c5714e65..b5efb804efdf 100644 --- a/include/linux/trace_recursion.h +++ b/include/linux/trace_recursion.h@@ -159,13 +159,27 @@ extern void ftrace_record_recursion(unsigned long ip, unsigned long parent_ip); # define do_ftrace_record_recursion(ip, pip) do { } while (0) #endif +/* + * trace_test_and_set_recursion() is called on several layers + * of the ftrace code when handling the same ftrace entry. + * These calls might be nested/recursive. + * + * It uses TRACE_LIST_*BITs to distinguish between this + * internal recursion and recursion caused by calling + * the traced function by the ftrace code. + * + * Returns: > 0 when no recursion + * 0 when called recursively internally (safe)
The 0 can also happened when ftrace handler recursively called trylock() under the same context, or not? Regards, Michael Wang
+ * -1 when the traced function was called recursively from
+ * the ftrace handler (unsafe)
+ */
static __always_inline int trace_test_and_set_recursion(unsigned long ip, unsigned long pip,
int start, int max)
{
unsigned int val = READ_ONCE(current->trace_recursion);
int bit;
- /* A previous recursion check was made */
+ /* Called recursively internally by different ftrace code layers? */
if ((val & TRACE_CONTEXT_MASK) > max)
return 0;