Message ID | 169139092722.324433.16681957760325391475.stgit@devnote2 (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | bpf: fprobe: rethook: Use ftrace_regs instead of pt_regs | expand |
On Mon, Aug 7, 2023 at 8:48 AM Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h > index ce156c7704ee..3fb94a1a2461 100644 > --- a/include/linux/ftrace.h > +++ b/include/linux/ftrace.h > @@ -112,11 +112,11 @@ static inline int ftrace_mod_get_kallsym(unsigned int symnum, unsigned long *val > } > #endif > > -#ifdef CONFIG_FUNCTION_TRACER > - > -extern int ftrace_enabled; > - > -#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS > +/* > + * If the architecture doesn't support FTRACE_WITH_ARGS or disable function nit: disables* > + * tracer, define the default(pt_regs compatible) ftrace_regs. > + */ > +#if !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS) || !defined(CONFIG_FUNCTION_TRACER) I wonder if we should make things simpler with: #if defined(HAVE_PT_REGS_COMPAT_FTRACE_REGS) || !defined(CONFIG_FUNCTION_TRACER) And remove the ftrace_regs definitions that are copy-pastes of this block in arch specific headers. Then we can enforce in a single point that HAVE_PT_REGS_COMPAT_FTRACE_REGS holds. Maybe that's a question for Steven ?
On Wed, 9 Aug 2023 12:29:13 +0200 Florent Revest <revest@chromium.org> wrote: > On Mon, Aug 7, 2023 at 8:48 AM Masami Hiramatsu (Google) > <mhiramat@kernel.org> wrote: > > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h > > index ce156c7704ee..3fb94a1a2461 100644 > > --- a/include/linux/ftrace.h > > +++ b/include/linux/ftrace.h > > @@ -112,11 +112,11 @@ static inline int ftrace_mod_get_kallsym(unsigned int symnum, unsigned long *val > > } > > #endif > > > > -#ifdef CONFIG_FUNCTION_TRACER > > - > > -extern int ftrace_enabled; > > - > > -#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS > > +/* > > + * If the architecture doesn't support FTRACE_WITH_ARGS or disable function > > nit: disables* Thanks! > > > + * tracer, define the default(pt_regs compatible) ftrace_regs. > > + */ > > +#if !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS) || !defined(CONFIG_FUNCTION_TRACER) > > I wonder if we should make things simpler with: > > #if defined(HAVE_PT_REGS_COMPAT_FTRACE_REGS) || !defined(CONFIG_FUNCTION_TRACER) > > And remove the ftrace_regs definitions that are copy-pastes of this > block in arch specific headers. Then we can enforce in a single point > that HAVE_PT_REGS_COMPAT_FTRACE_REGS holds. Here, the "HAVE_PT_REGS_COMPAT_FTRACE_REGS" does not mean that the ftrace_regs is completely compatible with pt_regs, but on the memory it wraps struct pt_regs (thus we can just cast the type). - CONFIG_DYNAMIC_FTRACE_WITH_REGS ftrace_regs is completely compatible with pt_regs - CONFIG_DYNAMIC_FTRACE_WITH_ARGS | ftrace_regs may not compatible with pt_regs | +- CONFIG_HAVE_PT_REGS_COMPAT_FTRACE_REGS but on memory image, ftrace_regs includes pt_regs. Thank you, > > Maybe that's a question for Steven ?
On Wed, Aug 9, 2023 at 4:16 PM Masami Hiramatsu <mhiramat@kernel.org> wrote: > > On Wed, 9 Aug 2023 12:29:13 +0200 > Florent Revest <revest@chromium.org> wrote: > > > + * tracer, define the default(pt_regs compatible) ftrace_regs. > > > + */ > > > +#if !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS) || !defined(CONFIG_FUNCTION_TRACER) > > > > I wonder if we should make things simpler with: > > > > #if defined(HAVE_PT_REGS_COMPAT_FTRACE_REGS) || !defined(CONFIG_FUNCTION_TRACER) > > > > And remove the ftrace_regs definitions that are copy-pastes of this > > block in arch specific headers. Then we can enforce in a single point > > that HAVE_PT_REGS_COMPAT_FTRACE_REGS holds. > > Here, the "HAVE_PT_REGS_COMPAT_FTRACE_REGS" does not mean that the > ftrace_regs is completely compatible with pt_regs, but on the memory > it wraps struct pt_regs (thus we can just cast the type). But in practice I think that all architectures that chose to wrap a pt_regs in their ftrace_regs also do: +#define ftrace_regs_get_instruction_pointer(fregs) \ + instruction_pointer(ftrace_get_regs(fregs)) +#define ftrace_regs_get_argument(fregs, n) \ + regs_get_kernel_argument(ftrace_get_regs(fregs), n) +#define ftrace_regs_get_stack_pointer(fregs) \ + kernel_stack_pointer(ftrace_get_regs(fregs)) +#define ftrace_regs_return_value(fregs) \ + regs_return_value(ftrace_get_regs(fregs)) +#define ftrace_regs_set_return_value(fregs, ret) \ + regs_set_return_value(ftrace_get_regs(fregs), ret) +#define ftrace_override_function_with_return(fregs) \ + override_function_with_return(ftrace_get_regs(fregs)) +#define ftrace_regs_query_register_offset(name) \ + regs_query_register_offset(name) And are just careful to populate the fields that let these macros work. So maybe these could be factorized... But anyway, I'm not particularly a super fan of the idea and I don't think it should necessarily fit in that series. It's just something that crossed my mind, if you're not a fan then we should probably not do it ;)
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h index ce156c7704ee..3fb94a1a2461 100644 --- a/include/linux/ftrace.h +++ b/include/linux/ftrace.h @@ -112,11 +112,11 @@ static inline int ftrace_mod_get_kallsym(unsigned int symnum, unsigned long *val } #endif -#ifdef CONFIG_FUNCTION_TRACER - -extern int ftrace_enabled; - -#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS +/* + * If the architecture doesn't support FTRACE_WITH_ARGS or disable function + * tracer, define the default(pt_regs compatible) ftrace_regs. + */ +#if !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS) || !defined(CONFIG_FUNCTION_TRACER) struct ftrace_regs { struct pt_regs regs; @@ -129,6 +129,22 @@ struct ftrace_regs { * HAVE_DYNAMIC_FTRACE_WITH_ARGS is set and it supports live kernel patching. */ #define ftrace_regs_set_instruction_pointer(fregs, ip) do { } while (0) + +#define ftrace_regs_get_instruction_pointer(fregs) \ + instruction_pointer(ftrace_get_regs(fregs)) +#define ftrace_regs_get_argument(fregs, n) \ + regs_get_kernel_argument(ftrace_get_regs(fregs), n) +#define ftrace_regs_get_stack_pointer(fregs) \ + kernel_stack_pointer(ftrace_get_regs(fregs)) +#define ftrace_regs_return_value(fregs) \ + regs_return_value(ftrace_get_regs(fregs)) +#define ftrace_regs_set_return_value(fregs, ret) \ + regs_set_return_value(ftrace_get_regs(fregs), ret) +#define ftrace_override_function_with_return(fregs) \ + override_function_with_return(ftrace_get_regs(fregs)) +#define ftrace_regs_query_register_offset(name) \ + regs_query_register_offset(name) + #endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */ static __always_inline struct pt_regs *ftrace_get_regs(struct ftrace_regs *fregs) @@ -151,22 +167,9 @@ static __always_inline bool ftrace_regs_has_args(struct ftrace_regs *fregs) return ftrace_get_regs(fregs) != NULL; } -#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS -#define ftrace_regs_get_instruction_pointer(fregs) \ - instruction_pointer(ftrace_get_regs(fregs)) -#define ftrace_regs_get_argument(fregs, n) \ - regs_get_kernel_argument(ftrace_get_regs(fregs), n) -#define ftrace_regs_get_stack_pointer(fregs) \ - kernel_stack_pointer(ftrace_get_regs(fregs)) -#define ftrace_regs_return_value(fregs) \ - regs_return_value(ftrace_get_regs(fregs)) -#define ftrace_regs_set_return_value(fregs, ret) \ - regs_set_return_value(ftrace_get_regs(fregs), ret) -#define ftrace_override_function_with_return(fregs) \ - override_function_with_return(ftrace_get_regs(fregs)) -#define ftrace_regs_query_register_offset(name) \ - regs_query_register_offset(name) -#endif +#ifdef CONFIG_FUNCTION_TRACER + +extern int ftrace_enabled; typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip, struct ftrace_ops *op, struct ftrace_regs *fregs);