Message ID | 173344373580.50709.5332611753907139634.stgit@devnote2 (mailing list archive) |
---|---|
Headers | show |
Series | tracing: fprobe: function_graph: Multi-function graph and fprobe on fgraph | expand |
Hi, Sorry, I found a problem on arm64 on qemu. Let me recheck it. [ 592.422044] # test_fprobe_entry: EXPECTATION FAILED at lib/test_fprobe.c:38 [ 592.422044] Expected (preempt_count() == 0 && !({ unsigned long _flags; do { ({ unsigned long __dummy; typeof(_flags) __dummy2; (void)(&__dummy == &__dummy2); 1; }); _flags = arch_local_save_flags(); } while (0); ({ ({ unsigned long __dummy; typeof(_flags) __dummy2; (void)(&__dummy == &__dummy2); 1; }); arch_irqs_disabled_flags(_flags); }); })) to be false, but is true [ 592.427029] # test_fprobe_entry: EXPECTATION FAILED at lib/test_fprobe.c:38 [ 592.427029] Expected (preempt_count() == 0 && !({ unsigned long _flags; do { ({ unsigned long __dummy; typeof(_flags) __dummy2; (void)(&__dummy == &__dummy2); 1; }); _flags = arch_local_save_flags(); } while (0); ({ ({ unsigned long __dummy; typeof(_flags) __dummy2; (void)(&__dummy == &__dummy2); 1; }); arch_irqs_disabled_flags(_flags); }); })) to be false, but is true [ 592.461632] not ok 1 test_fprobe_entry [ 592.564126] # test_fprobe: EXPECTATION FAILED at lib/test_fprobe.c:38 [ 592.564126] Expected (preempt_count() == 0 && !({ unsigned long _flags; do { ({ unsigned long __dummy; typeof(_flags) __dummy2; (void)(&__dummy == &__dummy2); 1; }); _flags = arch_local_save_flags(); } while (0); ({ ({ unsigned long __dummy; typeof(_flags) __dummy2; (void)(&__dummy == &__dummy2); 1; }); arch_irqs_disabled_flags(_flags); }); })) to be false, but is true [ 592.568100] # test_fprobe: EXPECTATION FAILED at lib/test_fprobe.c:38 [ 592.568100] Expected (preempt_count() == 0 && !({ unsigned long _flags; do { ({ unsigned long __dummy; typeof(_flags) __dummy2; (void)(&__dummy == &__dummy2); 1; }); _flags = arch_local_save_flags(); } while (0); ({ ({ unsigned long __dummy; typeof(_flags) __dummy2; (void)(&__dummy == &__dummy2); 1; }); arch_irqs_disabled_flags(_flags); }); })) to be false, but is true [ 592.603844] not ok 2 test_fprobe [ 592.650501] # test_fprobe_syms: EXPECTATION FAILED at lib/test_fprobe.c:38 [ 592.650501] Expected (preempt_count() == 0 && !({ unsigned long _flags; do { ({ unsigned long __dummy; typeof(_flags) __dummy2; (void)(&__dummy == &__dummy2); 1; }); _flags = arch_local_save_flags(); } while (0); ({ ({ unsigned long __dummy; typeof(_flags) __dummy2; (void)(&__dummy == &__dummy2); 1; }); arch_irqs_disabled_flags(_flags); }); })) to be false, but is true [ 592.654706] # test_fprobe_syms: EXPECTATION FAILED at lib/test_fprobe.c:38 [ 592.654706] Expected (preempt_count() == 0 && !({ unsigned long _flags; do { ({ unsigned long __dummy; typeof(_flags) __dummy2; (void)(&__dummy == &__dummy2); 1; }); _flags = arch_local_save_flags(); } while (0); ({ ({ unsigned long __dummy; typeof(_flags) __dummy2; (void)(&__dummy == &__dummy2); 1; }); arch_irqs_disabled_flags(_flags); }); })) to be false, but is true [ 592.699596] not ok 3 test_fprobe_syms [ 592.802046] # test_fprobe_data: EXPECTATION FAILED at lib/test_fprobe.c:38 [ 592.802046] Expected (preempt_count() == 0 && !({ unsigned long _flags; do { ({ unsigned long __dummy; typeof(_flags) __dummy2; (void)(&__dummy == &__dummy2); 1; }); _flags = arch_local_save_flags(); } while (0); ({ ({ unsigned long __dummy; typeof(_flags) __dummy2; (void)(&__dummy == &__dummy2); 1; }); arch_irqs_disabled_flags(_flags); }); })) to be false, but is true [ 592.839950] not ok 4 test_fprobe_data [ 592.945481] # test_fprobe_skip: EXPECTATION FAILED at lib/test_fprobe.c:38 [ 592.945481] Expected (preempt_count() == 0 && !({ unsigned long _flags; do { ({ unsigned long __dummy; typeof(_flags) __dummy2; (void)(&__dummy == &__dummy2); 1; }); _flags = arch_local_save_flags(); } while (0); ({ ({ unsigned long __dummy; typeof(_flags) __dummy2; (void)(&__dummy == &__dummy2); 1; }); arch_irqs_disabled_flags(_flags); }); })) to be false, but is true [ 592.979938] not ok 5 test_fprobe_skip [ 592.980046] # fprobe_test: pass:0 fail:5 skip:0 total:5 [ 592.980388] # Totals: pass:0 fail:5 skip:0 total:5 Thanks, On Fri, 6 Dec 2024 09:08:56 +0900 "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote: > Hi, > > Here is the 20th version of the series to re-implement the fprobe on > function-graph tracer. The previous version is; > > https://lore.kernel.org/all/173125372214.172790.6929368952404083802.stgit@devnote2/ > > This version is rebased on v6.13-rc1 and fixes to make CONFIG_FPROBE > "n" by default, so that it does not enable function graph tracer by > default. > > Overview > -------- > This series rewrites the fprobe on this function-graph. > The purposes of this change are; > > 1) Remove dependency of the rethook from fprobe so that we can reduce > the return hook code and shadow stack. > > 2) Make 'ftrace_regs' the common trace interface for the function > boundary. > > 1) Currently we have 2(or 3) different function return hook codes, > the function-graph tracer and rethook (and legacy kretprobe). > But since this is redundant and needs double maintenance cost, > I would like to unify those. From the user's viewpoint, function- > graph tracer is very useful to grasp the execution path. For this > purpose, it is hard to use the rethook in the function-graph > tracer, but the opposite is possible. (Strictly speaking, kretprobe > can not use it because it requires 'pt_regs' for historical reasons.) > > 2) Now the fprobe provides the 'pt_regs' for its handler, but that is > wrong for the function entry and exit. Moreover, depending on the > architecture, there is no way to accurately reproduce 'pt_regs' > outside of interrupt or exception handlers. This means fprobe should > not use 'pt_regs' because it does not use such exceptions. > (Conversely, kprobe should use 'pt_regs' because it is an abstract > interface of the software breakpoint exception.) > > This series changes fprobe to use function-graph tracer for tracing > function entry and exit, instead of mixture of ftrace and rethook. > Unlike the rethook which is a per-task list of system-wide allocated > nodes, the function graph's ret_stack is a per-task shadow stack. > Thus it does not need to set 'nr_maxactive' (which is the number of > pre-allocated nodes). > Also the handlers will get the 'ftrace_regs' instead of 'pt_regs'. > Since eBPF mulit_kprobe/multi_kretprobe events still use 'pt_regs' as > their register interface, this changes it to convert 'ftrace_regs' to > 'pt_regs'. Of course this conversion makes an incomplete 'pt_regs', > so users must access only registers for function parameters or > return value. > > Design > ------ > Instead of using ftrace's function entry hook directly, the new fprobe > is built on top of the function-graph's entry and return callbacks > with 'ftrace_regs'. > > Since the fprobe requires access to 'ftrace_regs', the architecture > must support CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS and > CONFIG_HAVE_FTRACE_GRAPH_FUNC, which enables to call function-graph > entry callback with 'ftrace_regs', and also > CONFIG_HAVE_FUNCTION_GRAPH_FREGS, which passes the ftrace_regs to > return_to_handler. > > All fprobes share a single function-graph ops (means shares a common > ftrace filter) similar to the kprobe-on-ftrace. This needs another > layer to find corresponding fprobe in the common function-graph > callbacks, but has much better scalability, since the number of > registered function-graph ops is limited. > > In the entry callback, the fprobe runs its entry_handler and saves the > address of 'fprobe' on the function-graph's shadow stack as data. The > return callback decodes the data to get the 'fprobe' address, and runs > the exit_handler. > > The fprobe introduces two hash-tables, one is for entry callback which > searches fprobes related to the given function address passed by entry > callback. The other is for a return callback which checks if the given > 'fprobe' data structure pointer is still valid. Note that it is > possible to unregister fprobe before the return callback runs. Thus > the address validation must be done before using it in the return > callback. > > Download > -------- > This series can be applied against the ftrace/for-next branch in > linux-trace tree. > > This series can also be found below branch. > > https://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git/log/?h=topic/fprobe-on-fgraph > > Thank you, > > --- > > Masami Hiramatsu (Google) (18): > fgraph: Pass ftrace_regs to entryfunc > fgraph: Replace fgraph_ret_regs with ftrace_regs > fgraph: Pass ftrace_regs to retfunc > fprobe: Use ftrace_regs in fprobe entry handler > fprobe: Use ftrace_regs in fprobe exit handler > tracing: Add ftrace_partial_regs() for converting ftrace_regs to pt_regs > tracing: Add ftrace_fill_perf_regs() for perf event > tracing/fprobe: Enable fprobe events with CONFIG_DYNAMIC_FTRACE_WITH_ARGS > bpf: Enable kprobe_multi feature if CONFIG_FPROBE is enabled > ftrace: Add CONFIG_HAVE_FTRACE_GRAPH_FUNC > fprobe: Rewrite fprobe on function-graph tracer > fprobe: Add fprobe_header encoding feature > tracing/fprobe: Remove nr_maxactive from fprobe > selftests: ftrace: Remove obsolate maxactive syntax check > selftests/ftrace: Add a test case for repeating register/unregister fprobe > Documentation: probes: Update fprobe on function-graph tracer > ftrace: Add ftrace_get_symaddr to convert fentry_ip to symaddr > bpf: Use ftrace_get_symaddr() in get_entry_ip() > > Sven Schnelle (1): > s390/tracing: Enable HAVE_FTRACE_GRAPH_FUNC > > > Documentation/trace/fprobe.rst | 42 + > arch/arm64/Kconfig | 2 > arch/arm64/include/asm/Kbuild | 1 > arch/arm64/include/asm/ftrace.h | 51 +- > arch/arm64/kernel/asm-offsets.c | 12 > arch/arm64/kernel/entry-ftrace.S | 32 + > arch/arm64/kernel/ftrace.c | 78 ++ > arch/loongarch/Kconfig | 4 > arch/loongarch/include/asm/fprobe.h | 12 > arch/loongarch/include/asm/ftrace.h | 32 - > arch/loongarch/kernel/asm-offsets.c | 12 > arch/loongarch/kernel/ftrace_dyn.c | 10 > arch/loongarch/kernel/mcount.S | 17 - > arch/loongarch/kernel/mcount_dyn.S | 14 > arch/powerpc/Kconfig | 1 > arch/powerpc/include/asm/ftrace.h | 13 > arch/powerpc/kernel/trace/ftrace.c | 2 > arch/powerpc/kernel/trace/ftrace_64_pg.c | 10 > arch/riscv/Kconfig | 3 > arch/riscv/include/asm/Kbuild | 1 > arch/riscv/include/asm/ftrace.h | 45 + > arch/riscv/kernel/ftrace.c | 17 - > arch/riscv/kernel/mcount.S | 24 - > arch/s390/Kconfig | 4 > arch/s390/include/asm/fprobe.h | 10 > arch/s390/include/asm/ftrace.h | 37 + > arch/s390/kernel/asm-offsets.c | 6 > arch/s390/kernel/entry.h | 1 > arch/s390/kernel/ftrace.c | 48 - > arch/s390/kernel/mcount.S | 23 - > arch/x86/Kconfig | 4 > arch/x86/include/asm/Kbuild | 1 > arch/x86/include/asm/ftrace.h | 54 +- > arch/x86/kernel/ftrace.c | 50 +- > arch/x86/kernel/ftrace_32.S | 13 > arch/x86/kernel/ftrace_64.S | 17 - > include/asm-generic/fprobe.h | 46 + > include/linux/fprobe.h | 62 +- > include/linux/ftrace.h | 116 +++ > include/linux/ftrace_regs.h | 2 > kernel/trace/Kconfig | 22 - > kernel/trace/bpf_trace.c | 38 - > kernel/trace/fgraph.c | 57 +- > kernel/trace/fprobe.c | 664 +++++++++++++++----- > kernel/trace/ftrace.c | 6 > kernel/trace/trace.h | 6 > kernel/trace/trace_fprobe.c | 146 ++-- > kernel/trace/trace_functions_graph.c | 10 > kernel/trace/trace_irqsoff.c | 6 > kernel/trace/trace_probe_tmpl.h | 2 > kernel/trace/trace_sched_wakeup.c | 6 > kernel/trace/trace_selftest.c | 11 > lib/test_fprobe.c | 51 -- > samples/fprobe/fprobe_example.c | 4 > .../test.d/dynevent/add_remove_fprobe_repeat.tc | 19 + > .../ftrace/test.d/dynevent/fprobe_syntax_errors.tc | 4 > 56 files changed, 1312 insertions(+), 669 deletions(-) > create mode 100644 arch/loongarch/include/asm/fprobe.h > create mode 100644 arch/s390/include/asm/fprobe.h > create mode 100644 include/asm-generic/fprobe.h > create mode 100644 tools/testing/selftests/ftrace/test.d/dynevent/add_remove_fprobe_repeat.tc > > -- > Masami Hiramatsu (Google) <mhiramat@kernel.org> >
On Fri, Dec 06, 2024 at 09:08:56AM +0900, Masami Hiramatsu (Google) wrote: > Hi, > > Here is the 20th version of the series to re-implement the fprobe on > function-graph tracer. The previous version is; > > https://lore.kernel.org/all/173125372214.172790.6929368952404083802.stgit@devnote2/ > > This version is rebased on v6.13-rc1 and fixes to make CONFIG_FPROBE > "n" by default, so that it does not enable function graph tracer by > default. Is there a reason why you didn't add the ACKs I provided for s390 related patches for v19 of this series?
On Fri, 6 Dec 2024 10:35:56 +0100 Heiko Carstens <hca@linux.ibm.com> wrote: > On Fri, Dec 06, 2024 at 09:08:56AM +0900, Masami Hiramatsu (Google) wrote: > > Hi, > > > > Here is the 20th version of the series to re-implement the fprobe on > > function-graph tracer. The previous version is; > > > > https://lore.kernel.org/all/173125372214.172790.6929368952404083802.stgit@devnote2/ > > > > This version is rebased on v6.13-rc1 and fixes to make CONFIG_FPROBE > > "n" by default, so that it does not enable function graph tracer by > > default. > > Is there a reason why you didn't add the ACKs I provided for s390 > related patches for v19 of this series? Probably just missed it. Masami, One thing I usually do when I rebase to a new series is to take my older patch series from Patchwork and reapply them. Because patchwork will pick up any acks, reviewed-bys or tested-bys. I then only drop the tags if the patch needs significant changes. You can also use b4 to do the same. -- Steve
On Fri, 6 Dec 2024 17:20:03 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > Hi, > > Sorry, I found a problem on arm64 on qemu. Let me recheck it. > > [ 592.422044] # test_fprobe_entry: EXPECTATION FAILED at lib/test_fprobe.c:38 > [ 592.422044] Expected (preempt_count() == 0 && !({ unsigned long _flags; do { ({ unsigned long __dummy; typeof(_flags) __dummy2; (void)(&__dummy == &__dummy2); 1; }); _flags = arch_local_save_flags(); } while (0); ({ ({ unsigned long __dummy; typeof(_flags) __dummy2; (void)(&__dummy == &__dummy2); 1; }); arch_irqs_disabled_flags(_flags); }); })) to be false, but is true This checks the handler is called with preempt off. On x86_64, the ftrace_graph_func calls function_graph_enter_regs() with ftrace_test_recursion_trylock() as below; void ftrace_graph_func(unsigned long ip, unsigned long parent_ip, struct ftrace_ops *op, struct ftrace_regs *fregs) { struct pt_regs *regs = &arch_ftrace_regs(fregs)->regs; unsigned long *stack = (unsigned long *)kernel_stack_pointer(regs); unsigned long return_hooker = (unsigned long)&return_to_handler; unsigned long *parent = (unsigned long *)stack; int bit; if (unlikely(skip_ftrace_return())) return; bit = ftrace_test_recursion_trylock(ip, *parent); if (bit < 0) return; if (!function_graph_enter_regs(*parent, ip, 0, parent, fregs)) *parent = return_hooker; ftrace_test_recursion_unlock(bit); } However, arm64 version does not; void ftrace_graph_func(unsigned long ip, unsigned long parent_ip, struct ftrace_ops *op, struct ftrace_regs *fregs) { unsigned long return_hooker = (unsigned long)&return_to_handler; unsigned long frame_pointer = arch_ftrace_regs(fregs)->fp; unsigned long *parent = &arch_ftrace_regs(fregs)->lr; unsigned long old; if (unlikely(atomic_read(¤t->tracing_graph_pause))) return; old = *parent; if (!function_graph_enter_regs(old, ip, frame_pointer, (void *)frame_pointer, fregs)) { *parent = return_hooker; } } Is it a bug or intended? Thank you,
On Sat, 7 Dec 2024 15:51:36 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > This checks the handler is called with preempt off. > > On x86_64, the ftrace_graph_func calls function_graph_enter_regs() with > ftrace_test_recursion_trylock() as below; > > void ftrace_graph_func(unsigned long ip, unsigned long parent_ip, > struct ftrace_ops *op, struct ftrace_regs *fregs) > { > struct pt_regs *regs = &arch_ftrace_regs(fregs)->regs; > unsigned long *stack = (unsigned long *)kernel_stack_pointer(regs); > unsigned long return_hooker = (unsigned long)&return_to_handler; > unsigned long *parent = (unsigned long *)stack; > int bit; > > if (unlikely(skip_ftrace_return())) > return; > > bit = ftrace_test_recursion_trylock(ip, *parent); > if (bit < 0) > return; > > if (!function_graph_enter_regs(*parent, ip, 0, parent, fregs)) > *parent = return_hooker; > > ftrace_test_recursion_unlock(bit); > } > > However, arm64 version does not; Hmm, I think we can move that recursion check out of the arch/x86 code and into ftrace_graph_enter_regs(). -- Steve
On Fri, 6 Dec 2024 09:52:47 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > On Fri, 6 Dec 2024 10:35:56 +0100 > Heiko Carstens <hca@linux.ibm.com> wrote: > > > On Fri, Dec 06, 2024 at 09:08:56AM +0900, Masami Hiramatsu (Google) wrote: > > > Hi, > > > > > > Here is the 20th version of the series to re-implement the fprobe on > > > function-graph tracer. The previous version is; > > > > > > https://lore.kernel.org/all/173125372214.172790.6929368952404083802.stgit@devnote2/ > > > > > > This version is rebased on v6.13-rc1 and fixes to make CONFIG_FPROBE > > > "n" by default, so that it does not enable function graph tracer by > > > default. > > > > Is there a reason why you didn't add the ACKs I provided for s390 > > related patches for v19 of this series? > > Probably just missed it. > > Masami, > > One thing I usually do when I rebase to a new series is to take my older > patch series from Patchwork and reapply them. Because patchwork will pick > up any acks, reviewed-bys or tested-bys. I then only drop the tags if the > patch needs significant changes. Oops, sorry, I missed those tags on v19. Let me fix that. Thanks. > > You can also use b4 to do the same. > > -- Steve
On Sat, 7 Dec 2024 09:47:10 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > On Sat, 7 Dec 2024 15:51:36 +0900 > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > > This checks the handler is called with preempt off. > > > > On x86_64, the ftrace_graph_func calls function_graph_enter_regs() with > > ftrace_test_recursion_trylock() as below; > > > > void ftrace_graph_func(unsigned long ip, unsigned long parent_ip, > > struct ftrace_ops *op, struct ftrace_regs *fregs) > > { > > struct pt_regs *regs = &arch_ftrace_regs(fregs)->regs; > > unsigned long *stack = (unsigned long *)kernel_stack_pointer(regs); > > unsigned long return_hooker = (unsigned long)&return_to_handler; > > unsigned long *parent = (unsigned long *)stack; > > int bit; > > > > if (unlikely(skip_ftrace_return())) > > return; > > > > bit = ftrace_test_recursion_trylock(ip, *parent); > > if (bit < 0) > > return; > > > > if (!function_graph_enter_regs(*parent, ip, 0, parent, fregs)) > > *parent = return_hooker; > > > > ftrace_test_recursion_unlock(bit); > > } > > > > However, arm64 version does not; > > Hmm, I think we can move that recursion check out of the arch/x86 code > and into ftrace_graph_enter_regs(). OK, that's reasonable. Thanks, > > -- Steve