Message ID | 20210605111034.1810858-4-jolsa@kernel.org |
---|---|
State | New |
Headers | show |
Series | x86/ftrace/bpf: Add batch support for direct/tracing attach | expand |
On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa <jolsa@kernel.org> wrote: > > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> > > We don't need special hook for graph tracer entry point, > but instead we can use graph_ops::func function to install > the return_hooker. > > This moves the graph tracing setup _before_ the direct > trampoline prepares the stack, so the return_hooker will > be called when the direct trampoline is finished. > > This simplifies the code, because we don't need to take into > account the direct trampoline setup when preparing the graph > tracer hooker and we can allow function graph tracer on entries > registered with direct trampoline. > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > arch/x86/include/asm/ftrace.h | 9 +++++++-- > arch/x86/kernel/ftrace.c | 37 ++++++++++++++++++++++++++++++++--- > arch/x86/kernel/ftrace_64.S | 29 +-------------------------- > include/linux/ftrace.h | 6 ++++++ > kernel/trace/fgraph.c | 8 +++++--- > 5 files changed, 53 insertions(+), 36 deletions(-) > > diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h > index 9f3130f40807..024d9797646e 100644 > --- a/arch/x86/include/asm/ftrace.h > +++ b/arch/x86/include/asm/ftrace.h > @@ -57,6 +57,13 @@ arch_ftrace_get_regs(struct ftrace_regs *fregs) > > #define ftrace_instruction_pointer_set(fregs, _ip) \ > do { (fregs)->regs.ip = (_ip); } while (0) > + > +struct ftrace_ops; > +#define ftrace_graph_func ftrace_graph_func > +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip, > + struct ftrace_ops *op, struct ftrace_regs *fregs); > +#else > +#define FTRACE_GRAPH_TRAMP_ADDR FTRACE_GRAPH_ADDR > #endif > > #ifdef CONFIG_DYNAMIC_FTRACE > @@ -65,8 +72,6 @@ struct dyn_arch_ftrace { > /* No extra data needed for x86 */ > }; > > -#define FTRACE_GRAPH_TRAMP_ADDR FTRACE_GRAPH_ADDR > - > #endif /* CONFIG_DYNAMIC_FTRACE */ > #endif /* __ASSEMBLY__ */ > #endif /* CONFIG_FUNCTION_TRACER */ > diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c > index c555624da989..804fcc6ef2c7 100644 > --- a/arch/x86/kernel/ftrace.c > +++ b/arch/x86/kernel/ftrace.c > @@ -527,7 +527,7 @@ static void *addr_from_call(void *ptr) > return ptr + CALL_INSN_SIZE + call.disp; > } > > -void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent, > +void prepare_ftrace_return(unsigned long ip, unsigned long *parent, > unsigned long frame_pointer); > > /* > @@ -541,7 +541,8 @@ static void *static_tramp_func(struct ftrace_ops *ops, struct dyn_ftrace *rec) > void *ptr; > > if (ops && ops->trampoline) { > -#ifdef CONFIG_FUNCTION_GRAPH_TRACER > +#if !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS) && \ > + defined(CONFIG_FUNCTION_GRAPH_TRACER) > /* > * We only know about function graph tracer setting as static > * trampoline. > @@ -589,8 +590,9 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops) > #ifdef CONFIG_FUNCTION_GRAPH_TRACER > > #ifdef CONFIG_DYNAMIC_FTRACE > -extern void ftrace_graph_call(void); > > +#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS > +extern void ftrace_graph_call(void); > static const char *ftrace_jmp_replace(unsigned long ip, unsigned long addr) > { > return text_gen_insn(JMP32_INSN_OPCODE, (void *)ip, (void *)addr); > @@ -618,7 +620,17 @@ int ftrace_disable_ftrace_graph_caller(void) > > return ftrace_mod_jmp(ip, &ftrace_stub); > } > +#else /* !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */ > +int ftrace_enable_ftrace_graph_caller(void) > +{ > + return 0; > +} > > +int ftrace_disable_ftrace_graph_caller(void) > +{ > + return 0; > +} > +#endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */ > #endif /* !CONFIG_DYNAMIC_FTRACE */ > > /* > @@ -629,6 +641,7 @@ void prepare_ftrace_return(unsigned long ip, unsigned long *parent, > unsigned long frame_pointer) > { > unsigned long return_hooker = (unsigned long)&return_to_handler; > + int bit; > > /* > * When resuming from suspend-to-ram, this function can be indirectly > @@ -648,7 +661,25 @@ void prepare_ftrace_return(unsigned long ip, unsigned long *parent, > if (unlikely(atomic_read(¤t->tracing_graph_pause))) > return; > > + bit = ftrace_test_recursion_trylock(ip, *parent); > + if (bit < 0) > + return; > + > if (!function_graph_enter(*parent, ip, frame_pointer, parent)) > *parent = return_hooker; > + > + ftrace_test_recursion_unlock(bit); > +} > + > +#ifdef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS > +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip, > + struct ftrace_ops *op, struct ftrace_regs *fregs) > +{ > + struct pt_regs *regs = &fregs->regs; > + unsigned long *stack = (unsigned long *)kernel_stack_pointer(regs); > + > + prepare_ftrace_return(ip, (unsigned long *)stack, 0); > } > +#endif > + > #endif /* CONFIG_FUNCTION_GRAPH_TRACER */ > diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S > index a8eb084a7a9a..7a879901f103 100644 > --- a/arch/x86/kernel/ftrace_64.S > +++ b/arch/x86/kernel/ftrace_64.S > @@ -174,11 +174,6 @@ SYM_INNER_LABEL(ftrace_caller_end, SYM_L_GLOBAL) > SYM_FUNC_END(ftrace_caller); > > SYM_FUNC_START(ftrace_epilogue) > -#ifdef CONFIG_FUNCTION_GRAPH_TRACER > -SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) > - jmp ftrace_stub > -#endif > - > /* > * This is weak to keep gas from relaxing the jumps. > * It is also used to copy the retq for trampolines. > @@ -288,15 +283,6 @@ SYM_FUNC_START(__fentry__) > cmpq $ftrace_stub, ftrace_trace_function > jnz trace > > -fgraph_trace: > -#ifdef CONFIG_FUNCTION_GRAPH_TRACER > - cmpq $ftrace_stub, ftrace_graph_return > - jnz ftrace_graph_caller > - > - cmpq $ftrace_graph_entry_stub, ftrace_graph_entry > - jnz ftrace_graph_caller > -#endif > - > SYM_INNER_LABEL(ftrace_stub, SYM_L_GLOBAL) > retq > > @@ -314,25 +300,12 @@ trace: > CALL_NOSPEC r8 > restore_mcount_regs > > - jmp fgraph_trace > + jmp ftrace_stub > SYM_FUNC_END(__fentry__) > EXPORT_SYMBOL(__fentry__) > #endif /* CONFIG_DYNAMIC_FTRACE */ > > #ifdef CONFIG_FUNCTION_GRAPH_TRACER > -SYM_FUNC_START(ftrace_graph_caller) > - /* Saves rbp into %rdx and fills first parameter */ > - save_mcount_regs > - > - leaq MCOUNT_REG_SIZE+8(%rsp), %rsi > - movq $0, %rdx /* No framepointers needed */ > - call prepare_ftrace_return > - > - restore_mcount_regs > - > - retq > -SYM_FUNC_END(ftrace_graph_caller) > - > SYM_FUNC_START(return_to_handler) > subq $24, %rsp > > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h > index a69f363b61bf..40b493908f09 100644 > --- a/include/linux/ftrace.h > +++ b/include/linux/ftrace.h > @@ -614,6 +614,12 @@ void ftrace_modify_all_code(int command); > extern void ftrace_graph_caller(void); > extern int ftrace_enable_ftrace_graph_caller(void); > extern int ftrace_disable_ftrace_graph_caller(void); > +#ifndef ftrace_graph_func > +#define ftrace_graph_func ftrace_stub > +#define FTRACE_OPS_GRAPH_STUB | FTRACE_OPS_FL_STUB > +#else > +#define FTRACE_OPS_GRAPH_STUB > +#endif > #else > static inline int ftrace_enable_ftrace_graph_caller(void) { return 0; } > static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; } > diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c > index b8a0d1d564fb..58e96b45e9da 100644 > --- a/kernel/trace/fgraph.c > +++ b/kernel/trace/fgraph.c > @@ -115,6 +115,7 @@ int function_graph_enter(unsigned long ret, unsigned long func, > { > struct ftrace_graph_ent trace; > > +#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS > /* > * Skip graph tracing if the return location is served by direct trampoline, > * since call sequence and return addresses are unpredictable anyway. > @@ -124,6 +125,7 @@ int function_graph_enter(unsigned long ret, unsigned long func, > if (ftrace_direct_func_count && > ftrace_find_rec_direct(ret - MCOUNT_INSN_SIZE)) > return -EBUSY; > +#endif > trace.func = func; > trace.depth = ++current->curr_ret_depth; > > @@ -333,10 +335,10 @@ unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx, > #endif /* HAVE_FUNCTION_GRAPH_RET_ADDR_PTR */ > > static struct ftrace_ops graph_ops = { > - .func = ftrace_stub, > + .func = ftrace_graph_func, > .flags = FTRACE_OPS_FL_INITIALIZED | > - FTRACE_OPS_FL_PID | > - FTRACE_OPS_FL_STUB, > + FTRACE_OPS_FL_PID > + FTRACE_OPS_GRAPH_STUB, nit: this looks so weird... Why not define FTRACE_OPS_GRAPH_STUB as zero in case of #ifdef ftrace_graph_func? Then it will be natural and correctly looking | FTRACE_OPS_GRAPH_STUB? > #ifdef FTRACE_GRAPH_TRAMP_ADDR > .trampoline = FTRACE_GRAPH_TRAMP_ADDR, > /* trampoline_size is only needed for dynamically allocated tramps */ > -- > 2.31.1 >
On Tue, Jun 08, 2021 at 11:35:58AM -0700, Andrii Nakryiko wrote: SNIP > > diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c > > index b8a0d1d564fb..58e96b45e9da 100644 > > --- a/kernel/trace/fgraph.c > > +++ b/kernel/trace/fgraph.c > > @@ -115,6 +115,7 @@ int function_graph_enter(unsigned long ret, unsigned long func, > > { > > struct ftrace_graph_ent trace; > > > > +#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS > > /* > > * Skip graph tracing if the return location is served by direct trampoline, > > * since call sequence and return addresses are unpredictable anyway. > > @@ -124,6 +125,7 @@ int function_graph_enter(unsigned long ret, unsigned long func, > > if (ftrace_direct_func_count && > > ftrace_find_rec_direct(ret - MCOUNT_INSN_SIZE)) > > return -EBUSY; > > +#endif > > trace.func = func; > > trace.depth = ++current->curr_ret_depth; > > > > @@ -333,10 +335,10 @@ unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx, > > #endif /* HAVE_FUNCTION_GRAPH_RET_ADDR_PTR */ > > > > static struct ftrace_ops graph_ops = { > > - .func = ftrace_stub, > > + .func = ftrace_graph_func, > > .flags = FTRACE_OPS_FL_INITIALIZED | > > - FTRACE_OPS_FL_PID | > > - FTRACE_OPS_FL_STUB, > > + FTRACE_OPS_FL_PID > > + FTRACE_OPS_GRAPH_STUB, > > nit: this looks so weird... Why not define FTRACE_OPS_GRAPH_STUB as > zero in case of #ifdef ftrace_graph_func? Then it will be natural and > correctly looking | FTRACE_OPS_GRAPH_STUB? ok, I can change that thanks, jirka > > > #ifdef FTRACE_GRAPH_TRAMP_ADDR > > .trampoline = FTRACE_GRAPH_TRAMP_ADDR, > > /* trampoline_size is only needed for dynamically allocated tramps */ > > -- > > 2.31.1 > > >
On Tue, 8 Jun 2021 20:51:25 +0200 Jiri Olsa <jolsa@redhat.com> wrote: > > > + FTRACE_OPS_FL_PID > > > + FTRACE_OPS_GRAPH_STUB, > > > > nit: this looks so weird... Why not define FTRACE_OPS_GRAPH_STUB as > > zero in case of #ifdef ftrace_graph_func? Then it will be natural and > > correctly looking | FTRACE_OPS_GRAPH_STUB? I have no idea why I did that :-/ But it was a while ago when I wrote this code. I think there was a reason for it, but with various updates, that reason disappeared. > > ok, I can change that Yes, please do. Thanks, -- Steve
diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h index 9f3130f40807..024d9797646e 100644 --- a/arch/x86/include/asm/ftrace.h +++ b/arch/x86/include/asm/ftrace.h @@ -57,6 +57,13 @@ arch_ftrace_get_regs(struct ftrace_regs *fregs) #define ftrace_instruction_pointer_set(fregs, _ip) \ do { (fregs)->regs.ip = (_ip); } while (0) + +struct ftrace_ops; +#define ftrace_graph_func ftrace_graph_func +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip, + struct ftrace_ops *op, struct ftrace_regs *fregs); +#else +#define FTRACE_GRAPH_TRAMP_ADDR FTRACE_GRAPH_ADDR #endif #ifdef CONFIG_DYNAMIC_FTRACE @@ -65,8 +72,6 @@ struct dyn_arch_ftrace { /* No extra data needed for x86 */ }; -#define FTRACE_GRAPH_TRAMP_ADDR FTRACE_GRAPH_ADDR - #endif /* CONFIG_DYNAMIC_FTRACE */ #endif /* __ASSEMBLY__ */ #endif /* CONFIG_FUNCTION_TRACER */ diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c index c555624da989..804fcc6ef2c7 100644 --- a/arch/x86/kernel/ftrace.c +++ b/arch/x86/kernel/ftrace.c @@ -527,7 +527,7 @@ static void *addr_from_call(void *ptr) return ptr + CALL_INSN_SIZE + call.disp; } -void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent, +void prepare_ftrace_return(unsigned long ip, unsigned long *parent, unsigned long frame_pointer); /* @@ -541,7 +541,8 @@ static void *static_tramp_func(struct ftrace_ops *ops, struct dyn_ftrace *rec) void *ptr; if (ops && ops->trampoline) { -#ifdef CONFIG_FUNCTION_GRAPH_TRACER +#if !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS) && \ + defined(CONFIG_FUNCTION_GRAPH_TRACER) /* * We only know about function graph tracer setting as static * trampoline. @@ -589,8 +590,9 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops) #ifdef CONFIG_FUNCTION_GRAPH_TRACER #ifdef CONFIG_DYNAMIC_FTRACE -extern void ftrace_graph_call(void); +#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS +extern void ftrace_graph_call(void); static const char *ftrace_jmp_replace(unsigned long ip, unsigned long addr) { return text_gen_insn(JMP32_INSN_OPCODE, (void *)ip, (void *)addr); @@ -618,7 +620,17 @@ int ftrace_disable_ftrace_graph_caller(void) return ftrace_mod_jmp(ip, &ftrace_stub); } +#else /* !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */ +int ftrace_enable_ftrace_graph_caller(void) +{ + return 0; +} +int ftrace_disable_ftrace_graph_caller(void) +{ + return 0; +} +#endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */ #endif /* !CONFIG_DYNAMIC_FTRACE */ /* @@ -629,6 +641,7 @@ void prepare_ftrace_return(unsigned long ip, unsigned long *parent, unsigned long frame_pointer) { unsigned long return_hooker = (unsigned long)&return_to_handler; + int bit; /* * When resuming from suspend-to-ram, this function can be indirectly @@ -648,7 +661,25 @@ void prepare_ftrace_return(unsigned long ip, unsigned long *parent, if (unlikely(atomic_read(¤t->tracing_graph_pause))) return; + bit = ftrace_test_recursion_trylock(ip, *parent); + if (bit < 0) + return; + if (!function_graph_enter(*parent, ip, frame_pointer, parent)) *parent = return_hooker; + + ftrace_test_recursion_unlock(bit); +} + +#ifdef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip, + struct ftrace_ops *op, struct ftrace_regs *fregs) +{ + struct pt_regs *regs = &fregs->regs; + unsigned long *stack = (unsigned long *)kernel_stack_pointer(regs); + + prepare_ftrace_return(ip, (unsigned long *)stack, 0); } +#endif + #endif /* CONFIG_FUNCTION_GRAPH_TRACER */ diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S index a8eb084a7a9a..7a879901f103 100644 --- a/arch/x86/kernel/ftrace_64.S +++ b/arch/x86/kernel/ftrace_64.S @@ -174,11 +174,6 @@ SYM_INNER_LABEL(ftrace_caller_end, SYM_L_GLOBAL) SYM_FUNC_END(ftrace_caller); SYM_FUNC_START(ftrace_epilogue) -#ifdef CONFIG_FUNCTION_GRAPH_TRACER -SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) - jmp ftrace_stub -#endif - /* * This is weak to keep gas from relaxing the jumps. * It is also used to copy the retq for trampolines. @@ -288,15 +283,6 @@ SYM_FUNC_START(__fentry__) cmpq $ftrace_stub, ftrace_trace_function jnz trace -fgraph_trace: -#ifdef CONFIG_FUNCTION_GRAPH_TRACER - cmpq $ftrace_stub, ftrace_graph_return - jnz ftrace_graph_caller - - cmpq $ftrace_graph_entry_stub, ftrace_graph_entry - jnz ftrace_graph_caller -#endif - SYM_INNER_LABEL(ftrace_stub, SYM_L_GLOBAL) retq @@ -314,25 +300,12 @@ trace: CALL_NOSPEC r8 restore_mcount_regs - jmp fgraph_trace + jmp ftrace_stub SYM_FUNC_END(__fentry__) EXPORT_SYMBOL(__fentry__) #endif /* CONFIG_DYNAMIC_FTRACE */ #ifdef CONFIG_FUNCTION_GRAPH_TRACER -SYM_FUNC_START(ftrace_graph_caller) - /* Saves rbp into %rdx and fills first parameter */ - save_mcount_regs - - leaq MCOUNT_REG_SIZE+8(%rsp), %rsi - movq $0, %rdx /* No framepointers needed */ - call prepare_ftrace_return - - restore_mcount_regs - - retq -SYM_FUNC_END(ftrace_graph_caller) - SYM_FUNC_START(return_to_handler) subq $24, %rsp diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h index a69f363b61bf..40b493908f09 100644 --- a/include/linux/ftrace.h +++ b/include/linux/ftrace.h @@ -614,6 +614,12 @@ void ftrace_modify_all_code(int command); extern void ftrace_graph_caller(void); extern int ftrace_enable_ftrace_graph_caller(void); extern int ftrace_disable_ftrace_graph_caller(void); +#ifndef ftrace_graph_func +#define ftrace_graph_func ftrace_stub +#define FTRACE_OPS_GRAPH_STUB | FTRACE_OPS_FL_STUB +#else +#define FTRACE_OPS_GRAPH_STUB +#endif #else static inline int ftrace_enable_ftrace_graph_caller(void) { return 0; } static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; } diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c index b8a0d1d564fb..58e96b45e9da 100644 --- a/kernel/trace/fgraph.c +++ b/kernel/trace/fgraph.c @@ -115,6 +115,7 @@ int function_graph_enter(unsigned long ret, unsigned long func, { struct ftrace_graph_ent trace; +#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS /* * Skip graph tracing if the return location is served by direct trampoline, * since call sequence and return addresses are unpredictable anyway. @@ -124,6 +125,7 @@ int function_graph_enter(unsigned long ret, unsigned long func, if (ftrace_direct_func_count && ftrace_find_rec_direct(ret - MCOUNT_INSN_SIZE)) return -EBUSY; +#endif trace.func = func; trace.depth = ++current->curr_ret_depth; @@ -333,10 +335,10 @@ unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx, #endif /* HAVE_FUNCTION_GRAPH_RET_ADDR_PTR */ static struct ftrace_ops graph_ops = { - .func = ftrace_stub, + .func = ftrace_graph_func, .flags = FTRACE_OPS_FL_INITIALIZED | - FTRACE_OPS_FL_PID | - FTRACE_OPS_FL_STUB, + FTRACE_OPS_FL_PID + FTRACE_OPS_GRAPH_STUB, #ifdef FTRACE_GRAPH_TRAMP_ADDR .trampoline = FTRACE_GRAPH_TRAMP_ADDR, /* trampoline_size is only needed for dynamically allocated tramps */