| Message ID | 20230201163420.1579014-2-revest@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Add ftrace direct call for arm64 | expand |
On Wed, Feb 01, 2023 at 05:34:13PM +0100, Florent Revest wrote: > The _multi API requires that users keep their own ops and can enforce > that an op is only associated to one direct call. > > Signed-off-by: Florent Revest <revest@chromium.org> > --- > kernel/trace/trace_selftest.c | 9 ++++++--- > samples/ftrace/ftrace-direct-modify.c | 11 +++++++---- > samples/ftrace/ftrace-direct-too.c | 12 +++++++----- > samples/ftrace/ftrace-direct.c | 12 +++++++----- Looking at samples/ftrace/, as of this patch we have a few samples that are almost identical, modulo the function being traced, and some different register shuffling for arguments: * ftrace-direct.c and ftrace-direct-multi.c * ftrace-direct-modify.c and ftrace-direct-modify ... perhaps it would be better to just delete the !multi versions ? Regardless, the changes below look functionally correct to me. I gave it a spin on x86_64, both with the FTRACE_STARTUP_TEST, and loading the modules dynamically, and those were all happy: | # mount -t tracefs none /sys/kernel/tracing/ | # for KO in *.ko; do | > echo $KO; | > insmod $KO; | > cat /sys/kernel/tracing/enabled_functions | > rmmod $KO; | > done | ftrace-direct-modify.ko | [ 91.890089] | [ 91.890600] ********************************************************** | [ 91.892277] ** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE ** | [ 91.893912] ** ** | [ 91.895569] ** trace_printk() being used. Allocating extra memory. ** | [ 91.897215] ** ** | [ 91.898861] ** This means that this is a DEBUG kernel and it is ** | [ 91.900517] ** unsafe for production use. ** | [ 91.902138] ** ** | [ 91.903755] ** If you see this message and you are not debugging ** | [ 91.905378] ** the kernel, report this immediately to your vendor! ** | [ 91.907010] ** ** | [ 91.908625] ** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE ** | [ 91.910221] ********************************************************** | schedule (1) R D tramp: ftrace_regs_caller+0x0/0x66 (call_direct_funcs+0x0/0x20) | direct-->my_tramp1+0x0/0x1d [ftrace_direct_modify] | ftrace-direct-multi-modify.ko | wake_up_process (1) R D tramp: ftrace_regs_caller+0x0/0x66 (call_direct_funcs+0x0/0x20) | direct-->my_tramp1+0x0/0x23 [ftrace_direct_multi_modify] | schedule (1) R D tramp: ftrace_regs_caller+0x0/0x66 (call_direct_funcs+0x0/0x20) | direct-->my_tramp1+0x0/0x23 [ftrace_direct_multi_modify] | ftrace-direct-multi.ko | wake_up_process (1) R D tramp: ftrace_regs_caller+0x0/0x66 (call_direct_funcs+0x0/0x20) | direct-->my_tramp+0x0/0x30 [ftrace_direct_multi] | schedule (1) R D tramp: ftrace_regs_caller+0x0/0x66 (call_direct_funcs+0x0/0x20) | direct-->my_tramp+0x0/0x30 [ftrace_direct_multi] | ftrace-direct-too.ko | handle_mm_fault (1) R D tramp: ftrace_regs_caller+0x0/0x66 (call_direct_funcs+0x0/0x20) | direct-->my_tramp+0x0/0x30 [ftrace_direct_too] | ftrace-direct.ko | wake_up_process (1) R D tramp: ftrace_regs_caller+0x0/0x66 (call_direct_funcs+0x0/0x20) | direct-->my_tramp+0x0/0x20 [ftrace_direct] So FWIW: Acked-by: Mark Rutland <mark.rutland@arm.com> Tested-by: Mark Rutland <mark.rutland@arm.com> Mark. > 4 files changed, 27 insertions(+), 17 deletions(-) > > diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c > index ff0536cea968..9b7f10cbc51d 100644 > --- a/kernel/trace/trace_selftest.c > +++ b/kernel/trace/trace_selftest.c > @@ -806,6 +806,9 @@ trace_selftest_startup_function_graph(struct tracer *trace, > int ret; > unsigned long count; > char *func_name __maybe_unused; > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS > + struct ftrace_ops direct = {}; > +#endif > > #ifdef CONFIG_DYNAMIC_FTRACE > if (ftrace_filter_param) { > @@ -870,8 +873,8 @@ trace_selftest_startup_function_graph(struct tracer *trace, > * Register direct function together with graph tracer > * and make sure we get graph trace. > */ > - ret = register_ftrace_direct((unsigned long) DYN_FTRACE_TEST_NAME, > - (unsigned long) trace_direct_tramp); > + ftrace_set_filter_ip(&direct, (unsigned long)DYN_FTRACE_TEST_NAME, 0, 0); > + ret = register_ftrace_direct_multi(&direct, (unsigned long)trace_direct_tramp); > if (ret) > goto out; > > @@ -891,7 +894,7 @@ trace_selftest_startup_function_graph(struct tracer *trace, > > unregister_ftrace_graph(&fgraph_ops); > > - ret = unregister_ftrace_direct((unsigned long) DYN_FTRACE_TEST_NAME, > + ret = unregister_ftrace_direct_multi(&direct, > (unsigned long) trace_direct_tramp); > if (ret) > goto out; > diff --git a/samples/ftrace/ftrace-direct-modify.c b/samples/ftrace/ftrace-direct-modify.c > index de5a0f67f320..e1e6c286970c 100644 > --- a/samples/ftrace/ftrace-direct-modify.c > +++ b/samples/ftrace/ftrace-direct-modify.c > @@ -96,6 +96,8 @@ asm ( > > #endif /* CONFIG_S390 */ > > +static struct ftrace_ops direct; > + > static unsigned long my_tramp = (unsigned long)my_tramp1; > static unsigned long tramps[2] = { > (unsigned long)my_tramp1, > @@ -114,7 +116,7 @@ static int simple_thread(void *arg) > if (ret) > continue; > t ^= 1; > - ret = modify_ftrace_direct(my_ip, my_tramp, tramps[t]); > + ret = modify_ftrace_direct_multi(&direct, tramps[t]); > if (!ret) > my_tramp = tramps[t]; > WARN_ON_ONCE(ret); > @@ -129,7 +131,8 @@ static int __init ftrace_direct_init(void) > { > int ret; > > - ret = register_ftrace_direct(my_ip, my_tramp); > + ftrace_set_filter_ip(&direct, (unsigned long) my_ip, 0, 0); > + ret = register_ftrace_direct_multi(&direct, my_tramp); > if (!ret) > simple_tsk = kthread_run(simple_thread, NULL, "event-sample-fn"); > return ret; > @@ -138,12 +141,12 @@ static int __init ftrace_direct_init(void) > static void __exit ftrace_direct_exit(void) > { > kthread_stop(simple_tsk); > - unregister_ftrace_direct(my_ip, my_tramp); > + unregister_ftrace_direct_multi(&direct, my_tramp); > } > > module_init(ftrace_direct_init); > module_exit(ftrace_direct_exit); > > MODULE_AUTHOR("Steven Rostedt"); > -MODULE_DESCRIPTION("Example use case of using modify_ftrace_direct()"); > +MODULE_DESCRIPTION("Example use case of using modify_ftrace_direct_multi()"); > MODULE_LICENSE("GPL"); > diff --git a/samples/ftrace/ftrace-direct-too.c b/samples/ftrace/ftrace-direct-too.c > index e13fb59a2b47..0e907092e2c0 100644 > --- a/samples/ftrace/ftrace-direct-too.c > +++ b/samples/ftrace/ftrace-direct-too.c > @@ -70,21 +70,23 @@ asm ( > > #endif /* CONFIG_S390 */ > > +static struct ftrace_ops direct; > + > static int __init ftrace_direct_init(void) > { > - return register_ftrace_direct((unsigned long)handle_mm_fault, > - (unsigned long)my_tramp); > + ftrace_set_filter_ip(&direct, (unsigned long) handle_mm_fault, 0, 0); > + > + return register_ftrace_direct_multi(&direct, (unsigned long) my_tramp); > } > > static void __exit ftrace_direct_exit(void) > { > - unregister_ftrace_direct((unsigned long)handle_mm_fault, > - (unsigned long)my_tramp); > + unregister_ftrace_direct_multi(&direct, (unsigned long)my_tramp); > } > > module_init(ftrace_direct_init); > module_exit(ftrace_direct_exit); > > MODULE_AUTHOR("Steven Rostedt"); > -MODULE_DESCRIPTION("Another example use case of using register_ftrace_direct()"); > +MODULE_DESCRIPTION("Another example use case of using register_ftrace_direct_multi()"); > MODULE_LICENSE("GPL"); > diff --git a/samples/ftrace/ftrace-direct.c b/samples/ftrace/ftrace-direct.c > index 1f769d0db20f..e446c38f6b58 100644 > --- a/samples/ftrace/ftrace-direct.c > +++ b/samples/ftrace/ftrace-direct.c > @@ -63,21 +63,23 @@ asm ( > > #endif /* CONFIG_S390 */ > > +static struct ftrace_ops direct; > + > static int __init ftrace_direct_init(void) > { > - return register_ftrace_direct((unsigned long)wake_up_process, > - (unsigned long)my_tramp); > + ftrace_set_filter_ip(&direct, (unsigned long) wake_up_process, 0, 0); > + > + return register_ftrace_direct_multi(&direct, (unsigned long) my_tramp); > } > > static void __exit ftrace_direct_exit(void) > { > - unregister_ftrace_direct((unsigned long)wake_up_process, > - (unsigned long)my_tramp); > + unregister_ftrace_direct_multi(&direct, (unsigned long)my_tramp); > } > > module_init(ftrace_direct_init); > module_exit(ftrace_direct_exit); > > MODULE_AUTHOR("Steven Rostedt"); > -MODULE_DESCRIPTION("Example use case of using register_ftrace_direct()"); > +MODULE_DESCRIPTION("Example use case of using register_ftrace_direct_multi()"); > MODULE_LICENSE("GPL"); > -- > 2.39.1.519.gcb327c4b5f-goog >
On Thu, Feb 2, 2023 at 4:02 PM Mark Rutland <mark.rutland@arm.com> wrote: > Looking at samples/ftrace/, as of this patch we have a few samples that are > almost identical, modulo the function being traced, and some different register > shuffling for arguments: > > * ftrace-direct.c and ftrace-direct-multi.c > * ftrace-direct-modify.c and ftrace-direct-modify > > ... perhaps it would be better to just delete the !multi versions ? The multi versions hook two functions and the !multi hook just one but I agree that this granularity in coverage is probably just a maintenance burden and doesn't help with much! :) I'll delete the !multi in v2, as part of the patch 2 and patch 1 will just migrate the selftest to use the multi API.
On Thu, Feb 2, 2023 at 6:37 PM Florent Revest <revest@chromium.org> wrote: > > On Thu, Feb 2, 2023 at 4:02 PM Mark Rutland <mark.rutland@arm.com> wrote: > > Looking at samples/ftrace/, as of this patch we have a few samples that are > > almost identical, modulo the function being traced, and some different register > > shuffling for arguments: > > > > * ftrace-direct.c and ftrace-direct-multi.c > > * ftrace-direct-modify.c and ftrace-direct-modify > > > > ... perhaps it would be better to just delete the !multi versions ? > > The multi versions hook two functions and the !multi hook just one but > I agree that this granularity in coverage is probably just a > maintenance burden and doesn't help with much! :) > I'll delete the !multi in v2, as part of the patch 2 and patch 1 will > just migrate the selftest to use the multi API. Actually, I'm not sure anymore if we should delete the !multi samples... I realized that they are also used as part of the ftrace selftests in: - tools/testing/selftests/ftrace/test.d/direct/ftrace-direct.tc - tools/testing/selftests/ftrace/test.d/direct/kprobe-direct.tc It does not really make sense to use the ftrace-direct-muti sample as a drop-in replacement for the ftrace-direct sample there since they don't really do the same thing so we would either need to change the test a bit or the multi sample. Also, we would still need to adapt the ftrace-direct-too sample since it has no multi equivalent and is required there. It's certainly doable but now it feels to me like going one step too far with the refactoring within the scope of this series. Do you think it's worth it ? I have 70% of that work done already so I'm happy to finish it but I thought I should check back before sending a v2 that's more complex than anticipated.
On Tue, 7 Feb 2023 16:21:49 +0100 Florent Revest <revest@chromium.org> wrote: > Actually, I'm not sure anymore if we should delete the !multi samples... > > I realized that they are also used as part of the ftrace selftests in: > - tools/testing/selftests/ftrace/test.d/direct/ftrace-direct.tc > - tools/testing/selftests/ftrace/test.d/direct/kprobe-direct.tc > > It does not really make sense to use the ftrace-direct-muti sample as > a drop-in replacement for the ftrace-direct sample there since they > don't really do the same thing so we would either need to change the > test a bit or the multi sample. > Also, we would still need to adapt the ftrace-direct-too sample since > it has no multi equivalent and is required there. Let's not delete the samples, and they do test slightly different use cases (although the code may be somewhat the same). I rather still keep that test coverage. -- Steve
On Tue, Feb 7, 2023 at 4:35 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Tue, 7 Feb 2023 16:21:49 +0100 > Florent Revest <revest@chromium.org> wrote: > > > Actually, I'm not sure anymore if we should delete the !multi samples... > > > > I realized that they are also used as part of the ftrace selftests in: > > - tools/testing/selftests/ftrace/test.d/direct/ftrace-direct.tc > > - tools/testing/selftests/ftrace/test.d/direct/kprobe-direct.tc > > > > It does not really make sense to use the ftrace-direct-muti sample as > > a drop-in replacement for the ftrace-direct sample there since they > > don't really do the same thing so we would either need to change the > > test a bit or the multi sample. > > Also, we would still need to adapt the ftrace-direct-too sample since > > it has no multi equivalent and is required there. > > Let's not delete the samples, and they do test slightly different use cases > (although the code may be somewhat the same). I rather still keep that test > coverage. > > -- Steve Ack :) Thanks Steve! I'll undo the work I've started to do on this and send a v2 shortly afterward.
diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c index ff0536cea968..9b7f10cbc51d 100644 --- a/kernel/trace/trace_selftest.c +++ b/kernel/trace/trace_selftest.c @@ -806,6 +806,9 @@ trace_selftest_startup_function_graph(struct tracer *trace, int ret; unsigned long count; char *func_name __maybe_unused; +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS + struct ftrace_ops direct = {}; +#endif #ifdef CONFIG_DYNAMIC_FTRACE if (ftrace_filter_param) { @@ -870,8 +873,8 @@ trace_selftest_startup_function_graph(struct tracer *trace, * Register direct function together with graph tracer * and make sure we get graph trace. */ - ret = register_ftrace_direct((unsigned long) DYN_FTRACE_TEST_NAME, - (unsigned long) trace_direct_tramp); + ftrace_set_filter_ip(&direct, (unsigned long)DYN_FTRACE_TEST_NAME, 0, 0); + ret = register_ftrace_direct_multi(&direct, (unsigned long)trace_direct_tramp); if (ret) goto out; @@ -891,7 +894,7 @@ trace_selftest_startup_function_graph(struct tracer *trace, unregister_ftrace_graph(&fgraph_ops); - ret = unregister_ftrace_direct((unsigned long) DYN_FTRACE_TEST_NAME, + ret = unregister_ftrace_direct_multi(&direct, (unsigned long) trace_direct_tramp); if (ret) goto out; diff --git a/samples/ftrace/ftrace-direct-modify.c b/samples/ftrace/ftrace-direct-modify.c index de5a0f67f320..e1e6c286970c 100644 --- a/samples/ftrace/ftrace-direct-modify.c +++ b/samples/ftrace/ftrace-direct-modify.c @@ -96,6 +96,8 @@ asm ( #endif /* CONFIG_S390 */ +static struct ftrace_ops direct; + static unsigned long my_tramp = (unsigned long)my_tramp1; static unsigned long tramps[2] = { (unsigned long)my_tramp1, @@ -114,7 +116,7 @@ static int simple_thread(void *arg) if (ret) continue; t ^= 1; - ret = modify_ftrace_direct(my_ip, my_tramp, tramps[t]); + ret = modify_ftrace_direct_multi(&direct, tramps[t]); if (!ret) my_tramp = tramps[t]; WARN_ON_ONCE(ret); @@ -129,7 +131,8 @@ static int __init ftrace_direct_init(void) { int ret; - ret = register_ftrace_direct(my_ip, my_tramp); + ftrace_set_filter_ip(&direct, (unsigned long) my_ip, 0, 0); + ret = register_ftrace_direct_multi(&direct, my_tramp); if (!ret) simple_tsk = kthread_run(simple_thread, NULL, "event-sample-fn"); return ret; @@ -138,12 +141,12 @@ static int __init ftrace_direct_init(void) static void __exit ftrace_direct_exit(void) { kthread_stop(simple_tsk); - unregister_ftrace_direct(my_ip, my_tramp); + unregister_ftrace_direct_multi(&direct, my_tramp); } module_init(ftrace_direct_init); module_exit(ftrace_direct_exit); MODULE_AUTHOR("Steven Rostedt"); -MODULE_DESCRIPTION("Example use case of using modify_ftrace_direct()"); +MODULE_DESCRIPTION("Example use case of using modify_ftrace_direct_multi()"); MODULE_LICENSE("GPL"); diff --git a/samples/ftrace/ftrace-direct-too.c b/samples/ftrace/ftrace-direct-too.c index e13fb59a2b47..0e907092e2c0 100644 --- a/samples/ftrace/ftrace-direct-too.c +++ b/samples/ftrace/ftrace-direct-too.c @@ -70,21 +70,23 @@ asm ( #endif /* CONFIG_S390 */ +static struct ftrace_ops direct; + static int __init ftrace_direct_init(void) { - return register_ftrace_direct((unsigned long)handle_mm_fault, - (unsigned long)my_tramp); + ftrace_set_filter_ip(&direct, (unsigned long) handle_mm_fault, 0, 0); + + return register_ftrace_direct_multi(&direct, (unsigned long) my_tramp); } static void __exit ftrace_direct_exit(void) { - unregister_ftrace_direct((unsigned long)handle_mm_fault, - (unsigned long)my_tramp); + unregister_ftrace_direct_multi(&direct, (unsigned long)my_tramp); } module_init(ftrace_direct_init); module_exit(ftrace_direct_exit); MODULE_AUTHOR("Steven Rostedt"); -MODULE_DESCRIPTION("Another example use case of using register_ftrace_direct()"); +MODULE_DESCRIPTION("Another example use case of using register_ftrace_direct_multi()"); MODULE_LICENSE("GPL"); diff --git a/samples/ftrace/ftrace-direct.c b/samples/ftrace/ftrace-direct.c index 1f769d0db20f..e446c38f6b58 100644 --- a/samples/ftrace/ftrace-direct.c +++ b/samples/ftrace/ftrace-direct.c @@ -63,21 +63,23 @@ asm ( #endif /* CONFIG_S390 */ +static struct ftrace_ops direct; + static int __init ftrace_direct_init(void) { - return register_ftrace_direct((unsigned long)wake_up_process, - (unsigned long)my_tramp); + ftrace_set_filter_ip(&direct, (unsigned long) wake_up_process, 0, 0); + + return register_ftrace_direct_multi(&direct, (unsigned long) my_tramp); } static void __exit ftrace_direct_exit(void) { - unregister_ftrace_direct((unsigned long)wake_up_process, - (unsigned long)my_tramp); + unregister_ftrace_direct_multi(&direct, (unsigned long)my_tramp); } module_init(ftrace_direct_init); module_exit(ftrace_direct_exit); MODULE_AUTHOR("Steven Rostedt"); -MODULE_DESCRIPTION("Example use case of using register_ftrace_direct()"); +MODULE_DESCRIPTION("Example use case of using register_ftrace_direct_multi()"); MODULE_LICENSE("GPL");
The _multi API requires that users keep their own ops and can enforce that an op is only associated to one direct call. Signed-off-by: Florent Revest <revest@chromium.org> --- kernel/trace/trace_selftest.c | 9 ++++++--- samples/ftrace/ftrace-direct-modify.c | 11 +++++++---- samples/ftrace/ftrace-direct-too.c | 12 +++++++----- samples/ftrace/ftrace-direct.c | 12 +++++++----- 4 files changed, 27 insertions(+), 17 deletions(-)