mbox series

[v20,00/19] tracing: fprobe: function_graph: Multi-function graph and fprobe on fgraph

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

Message

Masami Hiramatsu (Google) Dec. 6, 2024, 12:08 a.m. UTC
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>

Comments

Masami Hiramatsu (Google) Dec. 6, 2024, 8:20 a.m. UTC | #1
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>
>
Heiko Carstens Dec. 6, 2024, 9:35 a.m. UTC | #2
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?
Steven Rostedt Dec. 6, 2024, 2:52 p.m. UTC | #3
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
Masami Hiramatsu (Google) Dec. 7, 2024, 6:51 a.m. UTC | #4
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(&current->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,
Steven Rostedt Dec. 7, 2024, 2:47 p.m. UTC | #5
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
Masami Hiramatsu (Google) Dec. 9, 2024, 6:36 a.m. UTC | #6
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
Masami Hiramatsu (Google) Dec. 9, 2024, 6:36 a.m. UTC | #7
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