On Mon, Apr 10, 2023 at 10:31 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Mon, 10 Apr 2023 10:12:24 -0400
> Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> > The disabling of preemption was just done because every place that used it
> > happened to also disable preemption. So it was just a clean up, not a
> > requirement. Although the documentation said it did disable preemption :-/
> >
> > See ce5e48036c9e7 ("ftrace: disable preemption when recursion locked")
> >
> > I think I can add a ftrace_test_recursion_try_aquire() and release() that
> > is does the same thing without preemption. That way, we don't need to
> > revert that patch, and use that instead.
>
> This patch adds a:
>
> test_recursion_try_acquire() and test_recursion_release()
>
> I called it "acquire" and "release" so that "lock" can imply preemption being
> disabled, and this only protects against recursion (and I removed "ftrace"
> in the name, as it is meant for non-ftrace uses, which I may give it a
> different set of recursion bits).
>
> Note, the reason to pass in ip, and parent_ip (_THIS_IP_ and _RET_IP_) is
> for debugging purposes. They *should* be optimized out, as everything is
> __always_inline or macros, and those are only used if
> CONFIG_FTRACE_RECORD_RECURSION is enabled.
>
> -- Steve
>
Thanks for the explanation.
>
> diff --git a/include/linux/trace_recursion.h b/include/linux/trace_recursion.h
> index d48cd92d2364..80de2ee7b4c3 100644
> --- a/include/linux/trace_recursion.h
> +++ b/include/linux/trace_recursion.h
> @@ -150,9 +150,6 @@ extern void ftrace_record_recursion(unsigned long ip, unsigned long parent_ip);
> # define trace_warn_on_no_rcu(ip) false
> #endif
>
> -/*
> - * Preemption is promised to be disabled when return bit >= 0.
> - */
> static __always_inline int trace_test_and_set_recursion(unsigned long ip, unsigned long pip,
> int start)
> {
> @@ -182,18 +179,11 @@ static __always_inline int trace_test_and_set_recursion(unsigned long ip, unsign
> val |= 1 << bit;
> current->trace_recursion = val;
> barrier();
> -
> - preempt_disable_notrace();
> -
> return bit;
> }
>
> -/*
> - * Preemption will be enabled (if it was previously enabled).
> - */
> static __always_inline void trace_clear_recursion(int bit)
> {
> - preempt_enable_notrace();
> barrier();
> trace_recursion_clear(bit);
> }
> @@ -205,12 +195,18 @@ static __always_inline void trace_clear_recursion(int bit)
> * tracing recursed in the same context (normal vs interrupt),
> *
> * Returns: -1 if a recursion happened.
> - * >= 0 if no recursion.
> + * >= 0 if no recursion and preemption will be disabled.
> */
> static __always_inline int ftrace_test_recursion_trylock(unsigned long ip,
> unsigned long parent_ip)
> {
> - return trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START);
> + int bit;
> +
> + bit = trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START);
> + if (unlikely(bit < 0))
> + return -1;
> + preempt_disable_notrace();
> + return bit;
> }
>
> /**
> @@ -220,6 +216,33 @@ static __always_inline int ftrace_test_recursion_trylock(unsigned long ip,
> * This is used at the end of a ftrace callback.
> */
> static __always_inline void ftrace_test_recursion_unlock(int bit)
> +{
> + preempt_enable_notrace();
> + trace_clear_recursion(bit);
> +}
> +
> +/**
> + * test_recursion_try_acquire - tests for recursion in same context
> + *
> + * This will detect recursion of a function.
> + *
> + * Returns: -1 if a recursion happened.
> + * >= 0 if no recursion
> + */
> +static __always_inline int test_recursion_try_acquire(unsigned long ip,
> + unsigned long parent_ip)
> +{
> + return trace_test_and_set_recursion(ip, parent_ip, TRACE_FTRACE_START);
> +}
> +
> +/**
> + * test_recursion_release - called after a success of test_recursion_try_acquire()
> + * @bit: The return of a successful test_recursion_try_acquire()
> + *
> + * This releases the recursion lock taken by a non-negative return call
> + * by test_recursion_try_acquire().
> + */
> +static __always_inline void test_recursion_release(int bit)
> {
> trace_clear_recursion(bit);
> }
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 0feea145bb29..ff8172ba48b0 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -7644,6 +7644,7 @@ __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
> if (bit < 0)
> return;
>
> + preempt_disable();
> do_for_each_ftrace_op(op, ftrace_ops_list) {
> /* Stub functions don't need to be called nor tested */
> if (op->flags & FTRACE_OPS_FL_STUB)
> @@ -7665,6 +7666,7 @@ __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
> }
> } while_for_each_ftrace_op(op);
> out:
> + preempt_enable();
> trace_clear_recursion(bit);
> }
>
Great!
--
Regards
Yafang
