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

Chengming Zhou
2022-04-19T14:00:44Z

On 2022/4/19 20:55, Mark Rutland wrote:
> 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?

Yes, I think so. The static mcount-based implementation should also be changed
in this way, but I haven't look too deep into that asm implementation yet.

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

Ok, will add it in the commit message. And leave this 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.
> */
>

Ok, will do. This expression is nicer, the previous comment maybe make people
think it's an bug to be fixed.

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

Of course, will do in v5.

Thanks.

>
> Thanks,
> Mark.