Re: [PATCH v4 2/2] arm64/ftrace: Make function graph use ftrace directly

Mark Rutland
2022-04-19T13:47:47Z

On Sat, Apr 09, 2022 at 11:35:54PM +0800, Chengming Zhou wrote:
> As we do in commit 0c0593b45c9b ("x86/ftrace: Make function graph
> use ftrace directly"), we don't need special hook for graph tracer,
> but instead we use graph_ops:func function to install return_hooker.
>
> Since commit 3b23e4991fb6 ("arm64: implement ftrace with regs") add
> implementation for FTRACE_WITH_REGS on arm64, we can easily adopt
> the same cleanup on arm64.
>
> And this cleanup only changes the FTRACE_WITH_REGS implementation,
> so the mcount-based implementation is unaffected.

Could you please say *why* we only do this for FTRACE_WITH_REGS? IIUC that's
possible, but would require more invasive refactoring of the core code; have I
understood correctly?

If so, could we please make this:

| While in theory it would be possible to make a similar cleanup for
| !FTRACE_WITH_REGS, this will require rework of the core code, and so for now
| we only change the FTRACE_WITH_REGS implementation.

It'd be quite nice if we could clean up the !FTRACE_WITH_REGS case similarly,
but as it appeass that would require far more invasive changes, I'm happy to
leave that for future work.

>
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> ---
> v3:
> - Add comments in ftrace_graph_func() as suggested by Steve. Thanks.
>
> v2:
> - Remove FTRACE_WITH_REGS ftrace_graph_caller asm, thanks Mark.
> ---
> arch/arm64/include/asm/ftrace.h | 7 +++++++
> arch/arm64/kernel/entry-ftrace.S | 17 -----------------
> arch/arm64/kernel/ftrace.c | 17 +++++++++++++++++
> 3 files changed, 24 insertions(+), 17 deletions(-)
>
> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index 1494cfa8639b..dbc45a4157fa 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -80,8 +80,15 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr)
>
> #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> struct dyn_ftrace;
> +struct ftrace_ops;
> +struct ftrace_regs;
> +
> int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
> #define ftrace_init_nop ftrace_init_nop
> +
> +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> + struct ftrace_ops *op, struct ftrace_regs *fregs);
> +#define ftrace_graph_func ftrace_graph_func
> #endif
>
> #define ftrace_return_address(n) return_address(n)
> diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
> index e535480a4069..d42a205ef625 100644
> --- a/arch/arm64/kernel/entry-ftrace.S
> +++ b/arch/arm64/kernel/entry-ftrace.S
> @@ -97,12 +97,6 @@ SYM_CODE_START(ftrace_common)
> SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
> bl ftrace_stub
>
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> -SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) // ftrace_graph_caller();
> - nop // If enabled, this will be replaced
> - // "b ftrace_graph_caller"
> -#endif
> -
> /*
> * At the callsite x0-x8 and x19-x30 were live. Any C code will have preserved
> * x19-x29 per the AAPCS, and we created frame records upon entry, so we need
> @@ -127,17 +121,6 @@ ftrace_common_return:
> ret x9
> SYM_CODE_END(ftrace_common)
>
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> -SYM_CODE_START(ftrace_graph_caller)
> - ldr x0, [sp, #S_PC]
> - sub x0, x0, #AARCH64_INSN_SIZE // ip (callsite's BL insn)
> - add x1, sp, #S_LR // parent_ip (callsite's LR)
> - ldr x2, [sp, #PT_REGS_SIZE] // parent fp (callsite's FP)
> - bl prepare_ftrace_return
> - b ftrace_common_return
> -SYM_CODE_END(ftrace_graph_caller)
> -#endif
> -
> #else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
>
> /*
> diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
> index 4506c4a90ac1..35eb7c9b5e53 100644
> --- a/arch/arm64/kernel/ftrace.c
> +++ b/arch/arm64/kernel/ftrace.c
> @@ -268,6 +268,22 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
> }
>
> #ifdef CONFIG_DYNAMIC_FTRACE
> +
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> + struct ftrace_ops *op, struct ftrace_regs *fregs)
> +{
> + /*
> + * Athough graph_ops doesn't have FTRACE_OPS_FL_SAVE_REGS set in flags,
> + * regs can't be NULL in DYNAMIC_FTRACE_WITH_REGS. By design, it should
> + * be fixed when DYNAMIC_FTRACE_WITH_ARGS is implemented.
> + */

This is a bit confusing, since it makes it sound like there's an bug in the
current implementation, rather than something that would need to change if
support for DYNAMIC_FTRACE_WITH_ARGS is added.

Could we please make this:

/*
* When DYNAMIC_FTRACE_WITH_REGS is selected, `fregs` can never be NULL
* and arch_ftrace_get_regs(fregs) will always give a non-NULL pt_regs
* in which we can safely modify the LR.
*/

Other than that, this looks good to me. I gave it a spin under QEMU atop
v5.18-rc3. The CONFIG_FTRACE_STARTUP_TEST tests all pass, and I played with the
graph tracer with:

| # echo do_el0_svc > /sys/kernel/tracing/set_graph_function
| # echo function_graph > /sys/kernel/tracing/current_tracer

... for which the resutls looks sane.

To make sure this didn't adversely affect the return address rewriting, I also
concurrently ran perf with:

| # perf record -g -e raw_syscalls:sys_enter:k /bin/true
| # perf report

... for which the results also looked fine.

I also tested the !DYNAMIC_FTRACE_WITH_REGS modes by building with an older
compiler and also building with !DYNAMIC_FTRACE, which all looked good.

So FWIW:

Tested-by: Mark Rutland <mark.rutland@arm.com>

... and if you make the changes I requested above:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

If you could spin a v5 with that folded in, that would be great.

Thanks,
Mark.