| Message ID | 20221129033230.255947-7-guoren@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Palmer Dabbelt |
| Headers | show |
| Series | riscv: ftrace: Fixup ftrace detour code & Optimization | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/patch_count | success | Link |
| conchuod/cover_letter | success | Series has a cover letter |
| conchuod/tree_selection | success | Guessed tree name to be fixes |
| conchuod/fixes_present | success | Fixes tag present in non-next series |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/build_warn_rv64 | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 36 lines checked |
| conchuod/source_inline | success | Was 0 now: 0 |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
<guoren@kernel.org> 于2022年11月29日周二 03:33写道： > > From: Song Shuai <suagrfillet@gmail.com> > > This patch adds DYNAMIC_FTRACE_WITH_DIRECT_CALLS support for RISC-V. > > select the DYNAMIC_FTRACE_WITH_DIRECT_CALLS to provide the > register_ftrace_direct[_multi] interfaces allowing users to register > the customed trampoline (direct_caller) as the mcount for one or > more target functions. And modify_ftrace_direct[_multi] are also > provided for modifying direct_caller. > > To make the direct_caller and the other ftrace hooks (eg. function/fgraph > tracer, k[ret]probes) co-exist, a temporary register is nominated to > store the address of direct_caller in ftrace_regs_caller. After the > setting of the address direct_caller by direct_ops->func and the > RESTORE_REGS in ftrace_regs_caller, direct_caller will be jumped to > by the `jr` inst. > > Signed-off-by: Song Shuai <suagrfillet@gmail.com> > Tested-by: Guo Ren <guoren@kernel.org> > Signed-off-by: Guo Ren <guoren@kernel.org> > --- > arch/riscv/Kconfig | 1 + > arch/riscv/include/asm/ftrace.h | 6 ++++++ > arch/riscv/kernel/mcount-dyn.S | 4 ++++ > 3 files changed, 11 insertions(+) > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > index 1d0e5838b11b..2828537abfcd 100644 > --- a/arch/riscv/Kconfig > +++ b/arch/riscv/Kconfig > @@ -278,6 +278,7 @@ config ARCH_RV64I > select ARCH_SUPPORTS_INT128 if CC_HAS_INT128 > select HAVE_DYNAMIC_FTRACE if !XIP_KERNEL && MMU && $(cc-option,-fpatchable-function-entry=8) I noticed this cc-option uses the '-fpatchable-function-entry=8' to judge if the compiler supports this option. Should we keep up with the CC_FLAGS_FTRACE modified in ("riscv: ftrace: Reduce the detour code size to half"), or follow the parisc architecture to set the value as '1,1' in the case of the CC_FLAGS_FTRACE is not constant. ``` ./arch/parisc/Makefile:75:CC_FLAGS_FTRACE := -fpatchable-function-entry=$(NOP_COUNT),$(shell echo $$(($(NOP_COUNT)-1))) ./arch/parisc/Kconfig:70: select HAVE_DYNAMIC_FTRACE if $(cc-option,-fpatchable-function-entry=1,1) ``` > select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE > + select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS > select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL > select HAVE_FUNCTION_GRAPH_TRACER > select HAVE_FUNCTION_TRACER if !XIP_KERNEL && !PREEMPTION > diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h > index 84f856a3286e..4539f10fea56 100644 > --- a/arch/riscv/include/asm/ftrace.h > +++ b/arch/riscv/include/asm/ftrace.h > @@ -114,6 +114,12 @@ struct ftrace_regs; > 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 > + > +static inline void arch_ftrace_set_direct_caller(struct pt_regs *regs, unsigned long addr) > +{ > + regs->t1 = addr; > +} > + > #endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */ > > #endif /* __ASSEMBLY__ */ > diff --git a/arch/riscv/kernel/mcount-dyn.S b/arch/riscv/kernel/mcount-dyn.S > index 466c6ef217b1..fef7c460f991 100644 > --- a/arch/riscv/kernel/mcount-dyn.S > +++ b/arch/riscv/kernel/mcount-dyn.S > @@ -232,6 +232,7 @@ ENDPROC(ftrace_caller) > > #else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */ > ENTRY(ftrace_regs_caller) > + move t1, zero > SAVE_ABI_REGS 1 > PREPARE_ARGS > > @@ -241,7 +242,10 @@ ftrace_regs_call: > > > RESTORE_ABI_REGS 1 > + bnez t1,.Ldirect > jr t0 > +.Ldirect: > + jr t1 > ENDPROC(ftrace_regs_caller) > > ENTRY(ftrace_caller) > -- > 2.36.1 >
On Tue, Nov 29, 2022 at 8:03 PM Song Shuai <suagrfillet@gmail.com> wrote: > > <guoren@kernel.org> 于2022年11月29日周二 03:33写道： > > > > From: Song Shuai <suagrfillet@gmail.com> > > > > This patch adds DYNAMIC_FTRACE_WITH_DIRECT_CALLS support for RISC-V. > > > > select the DYNAMIC_FTRACE_WITH_DIRECT_CALLS to provide the > > register_ftrace_direct[_multi] interfaces allowing users to register > > the customed trampoline (direct_caller) as the mcount for one or > > more target functions. And modify_ftrace_direct[_multi] are also > > provided for modifying direct_caller. > > > > To make the direct_caller and the other ftrace hooks (eg. function/fgraph > > tracer, k[ret]probes) co-exist, a temporary register is nominated to > > store the address of direct_caller in ftrace_regs_caller. After the > > setting of the address direct_caller by direct_ops->func and the > > RESTORE_REGS in ftrace_regs_caller, direct_caller will be jumped to > > by the `jr` inst. > > > > Signed-off-by: Song Shuai <suagrfillet@gmail.com> > > Tested-by: Guo Ren <guoren@kernel.org> > > Signed-off-by: Guo Ren <guoren@kernel.org> > > --- > > arch/riscv/Kconfig | 1 + > > arch/riscv/include/asm/ftrace.h | 6 ++++++ > > arch/riscv/kernel/mcount-dyn.S | 4 ++++ > > 3 files changed, 11 insertions(+) > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > > index 1d0e5838b11b..2828537abfcd 100644 > > --- a/arch/riscv/Kconfig > > +++ b/arch/riscv/Kconfig > > @@ -278,6 +278,7 @@ config ARCH_RV64I > > select ARCH_SUPPORTS_INT128 if CC_HAS_INT128 > > select HAVE_DYNAMIC_FTRACE if !XIP_KERNEL && MMU && $(cc-option,-fpatchable-function-entry=8) > I noticed this cc-option uses the '-fpatchable-function-entry=8' to > judge if the compiler supports this option. > Should we keep up with the CC_FLAGS_FTRACE modified in ("riscv: > ftrace: Reduce the detour code size to half"), > or follow the parisc architecture to set the value as '1,1' in the > case of the CC_FLAGS_FTRACE is not constant. > ``` > ./arch/parisc/Makefile:75:CC_FLAGS_FTRACE := > -fpatchable-function-entry=$(NOP_COUNT),$(shell echo > $$(($(NOP_COUNT)-1))) > ./arch/parisc/Kconfig:70: select HAVE_DYNAMIC_FTRACE if > $(cc-option,-fpatchable-function-entry=1,1) The -fpatchable-function-entry=8 is still okay for riscv because it's big enough. Your proposal could be another optimization patch, not a fixup (No bug happens :). > ``` > > select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE > > + select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS > > select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL > > select HAVE_FUNCTION_GRAPH_TRACER > > select HAVE_FUNCTION_TRACER if !XIP_KERNEL && !PREEMPTION > > diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h > > index 84f856a3286e..4539f10fea56 100644 > > --- a/arch/riscv/include/asm/ftrace.h > > +++ b/arch/riscv/include/asm/ftrace.h > > @@ -114,6 +114,12 @@ struct ftrace_regs; > > 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 > > + > > +static inline void arch_ftrace_set_direct_caller(struct pt_regs *regs, unsigned long addr) > > +{ > > + regs->t1 = addr; > > +} > > + > > #endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */ > > > > #endif /* __ASSEMBLY__ */ > > diff --git a/arch/riscv/kernel/mcount-dyn.S b/arch/riscv/kernel/mcount-dyn.S > > index 466c6ef217b1..fef7c460f991 100644 > > --- a/arch/riscv/kernel/mcount-dyn.S > > +++ b/arch/riscv/kernel/mcount-dyn.S > > @@ -232,6 +232,7 @@ ENDPROC(ftrace_caller) > > > > #else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */ > > ENTRY(ftrace_regs_caller) > > + move t1, zero > > SAVE_ABI_REGS 1 > > PREPARE_ARGS > > > > @@ -241,7 +242,10 @@ ftrace_regs_call: > > > > > > RESTORE_ABI_REGS 1 > > + bnez t1,.Ldirect > > jr t0 > > +.Ldirect: > > + jr t1 > > ENDPROC(ftrace_regs_caller) > > > > ENTRY(ftrace_caller) > > -- > > 2.36.1 > > > > > -- > Thanks, > Song
On Mon, Nov 28, 2022 at 10:32:29PM -0500, guoren@kernel.org wrote: > From: Song Shuai <suagrfillet@gmail.com> > > This patch adds DYNAMIC_FTRACE_WITH_DIRECT_CALLS support for RISC-V. > > select the DYNAMIC_FTRACE_WITH_DIRECT_CALLS to provide the > register_ftrace_direct[_multi] interfaces allowing users to register > the customed trampoline (direct_caller) as the mcount for one or > more target functions. And modify_ftrace_direct[_multi] are also > provided for modifying direct_caller. > > To make the direct_caller and the other ftrace hooks (eg. function/fgraph > tracer, k[ret]probes) co-exist, a temporary register is nominated to > store the address of direct_caller in ftrace_regs_caller. After the > setting of the address direct_caller by direct_ops->func and the > RESTORE_REGS in ftrace_regs_caller, direct_caller will be jumped to > by the `jr` inst. > > Signed-off-by: Song Shuai <suagrfillet@gmail.com> > Tested-by: Guo Ren <guoren@kernel.org> > Signed-off-by: Guo Ren <guoren@kernel.org> > --- > arch/riscv/Kconfig | 1 + > arch/riscv/include/asm/ftrace.h | 6 ++++++ > arch/riscv/kernel/mcount-dyn.S | 4 ++++ > 3 files changed, 11 insertions(+) > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > index 1d0e5838b11b..2828537abfcd 100644 > --- a/arch/riscv/Kconfig > +++ b/arch/riscv/Kconfig > @@ -278,6 +278,7 @@ config ARCH_RV64I > select ARCH_SUPPORTS_INT128 if CC_HAS_INT128 > select HAVE_DYNAMIC_FTRACE if !XIP_KERNEL && MMU && $(cc-option,-fpatchable-function-entry=8) > select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE > + select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS > select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL > select HAVE_FUNCTION_GRAPH_TRACER > select HAVE_FUNCTION_TRACER if !XIP_KERNEL && !PREEMPTION Please sort new entries here in alphabetical order, so move the new entry up by one line :) Thanks, Conor.
On Thu, 01 Dec 2022 12:02:56 PST (-0800), Conor Dooley wrote: > On Mon, Nov 28, 2022 at 10:32:29PM -0500, guoren@kernel.org wrote: >> From: Song Shuai <suagrfillet@gmail.com> >> >> This patch adds DYNAMIC_FTRACE_WITH_DIRECT_CALLS support for RISC-V. >> >> select the DYNAMIC_FTRACE_WITH_DIRECT_CALLS to provide the >> register_ftrace_direct[_multi] interfaces allowing users to register >> the customed trampoline (direct_caller) as the mcount for one or >> more target functions. And modify_ftrace_direct[_multi] are also >> provided for modifying direct_caller. >> >> To make the direct_caller and the other ftrace hooks (eg. function/fgraph >> tracer, k[ret]probes) co-exist, a temporary register is nominated to >> store the address of direct_caller in ftrace_regs_caller. After the >> setting of the address direct_caller by direct_ops->func and the >> RESTORE_REGS in ftrace_regs_caller, direct_caller will be jumped to >> by the `jr` inst. >> >> Signed-off-by: Song Shuai <suagrfillet@gmail.com> >> Tested-by: Guo Ren <guoren@kernel.org> >> Signed-off-by: Guo Ren <guoren@kernel.org> >> --- >> arch/riscv/Kconfig | 1 + >> arch/riscv/include/asm/ftrace.h | 6 ++++++ >> arch/riscv/kernel/mcount-dyn.S | 4 ++++ >> 3 files changed, 11 insertions(+) >> >> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig >> index 1d0e5838b11b..2828537abfcd 100644 >> --- a/arch/riscv/Kconfig >> +++ b/arch/riscv/Kconfig >> @@ -278,6 +278,7 @@ config ARCH_RV64I >> select ARCH_SUPPORTS_INT128 if CC_HAS_INT128 >> select HAVE_DYNAMIC_FTRACE if !XIP_KERNEL && MMU && $(cc-option,-fpatchable-function-entry=8) >> select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE >> + select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS >> select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL >> select HAVE_FUNCTION_GRAPH_TRACER >> select HAVE_FUNCTION_TRACER if !XIP_KERNEL && !PREEMPTION > > Please sort new entries here in alphabetical order, so move the new > entry up by one line :) IIRC whomever sorted these entrties orignially posted a script that does that. Maybe that should be integrated into either checkpatch or one of the patchwork robots so we don't have to manually remember the alphabet?
On Thu, Dec 01, 2022 at 12:06:39PM -0800, Palmer Dabbelt wrote: > On Thu, 01 Dec 2022 12:02:56 PST (-0800), Conor Dooley wrote: > > On Mon, Nov 28, 2022 at 10:32:29PM -0500, guoren@kernel.org wrote: > > > From: Song Shuai <suagrfillet@gmail.com> > > > > > > This patch adds DYNAMIC_FTRACE_WITH_DIRECT_CALLS support for RISC-V. > > > > > > select the DYNAMIC_FTRACE_WITH_DIRECT_CALLS to provide the > > > register_ftrace_direct[_multi] interfaces allowing users to register > > > the customed trampoline (direct_caller) as the mcount for one or > > > more target functions. And modify_ftrace_direct[_multi] are also > > > provided for modifying direct_caller. > > > > > > To make the direct_caller and the other ftrace hooks (eg. function/fgraph > > > tracer, k[ret]probes) co-exist, a temporary register is nominated to > > > store the address of direct_caller in ftrace_regs_caller. After the > > > setting of the address direct_caller by direct_ops->func and the > > > RESTORE_REGS in ftrace_regs_caller, direct_caller will be jumped to > > > by the `jr` inst. > > > > > > Signed-off-by: Song Shuai <suagrfillet@gmail.com> > > > Tested-by: Guo Ren <guoren@kernel.org> > > > Signed-off-by: Guo Ren <guoren@kernel.org> > > > --- > > > arch/riscv/Kconfig | 1 + > > > arch/riscv/include/asm/ftrace.h | 6 ++++++ > > > arch/riscv/kernel/mcount-dyn.S | 4 ++++ > > > 3 files changed, 11 insertions(+) > > > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > > > index 1d0e5838b11b..2828537abfcd 100644 > > > --- a/arch/riscv/Kconfig > > > +++ b/arch/riscv/Kconfig > > > @@ -278,6 +278,7 @@ config ARCH_RV64I > > > select ARCH_SUPPORTS_INT128 if CC_HAS_INT128 > > > select HAVE_DYNAMIC_FTRACE if !XIP_KERNEL && MMU && $(cc-option,-fpatchable-function-entry=8) > > > select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE > > > + select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS > > > select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL > > > select HAVE_FUNCTION_GRAPH_TRACER > > > select HAVE_FUNCTION_TRACER if !XIP_KERNEL && !PREEMPTION > > > > Please sort new entries here in alphabetical order, so move the new > > entry up by one line :) > > IIRC whomever sorted these entrties orignially posted a script that does > that. Maybe that should be integrated into either checkpatch or one of the > patchwork robots so we don't have to manually remember the alphabet? If it actually does *sorting* I am not sure it's a suitable inclusion as-is that'd seem better as a pre-commit hook. But maybe it can be co-opted somehow. I'll try to dig it up off of lore tomorrow. Thanks, Conor.
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 1d0e5838b11b..2828537abfcd 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -278,6 +278,7 @@ config ARCH_RV64I select ARCH_SUPPORTS_INT128 if CC_HAS_INT128 select HAVE_DYNAMIC_FTRACE if !XIP_KERNEL && MMU && $(cc-option,-fpatchable-function-entry=8) select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE + select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL select HAVE_FUNCTION_GRAPH_TRACER select HAVE_FUNCTION_TRACER if !XIP_KERNEL && !PREEMPTION diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h index 84f856a3286e..4539f10fea56 100644 --- a/arch/riscv/include/asm/ftrace.h +++ b/arch/riscv/include/asm/ftrace.h @@ -114,6 +114,12 @@ struct ftrace_regs; 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 + +static inline void arch_ftrace_set_direct_caller(struct pt_regs *regs, unsigned long addr) +{ + regs->t1 = addr; +} + #endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */ #endif /* __ASSEMBLY__ */ diff --git a/arch/riscv/kernel/mcount-dyn.S b/arch/riscv/kernel/mcount-dyn.S index 466c6ef217b1..fef7c460f991 100644 --- a/arch/riscv/kernel/mcount-dyn.S +++ b/arch/riscv/kernel/mcount-dyn.S @@ -232,6 +232,7 @@ ENDPROC(ftrace_caller) #else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */ ENTRY(ftrace_regs_caller) + move t1, zero SAVE_ABI_REGS 1 PREPARE_ARGS @@ -241,7 +242,10 @@ ftrace_regs_call: RESTORE_ABI_REGS 1 + bnez t1,.Ldirect jr t0 +.Ldirect: + jr t1 ENDPROC(ftrace_regs_caller) ENTRY(ftrace_caller)