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

[Mark Rutland <mark.rutland@arm.com>]

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.