| Message ID | 20230207182135.2671106-2-revest@chromium.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF |
| Headers | show |
| Series | Add ftrace direct call for arm64 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Tue, 7 Feb 2023 19:21:26 +0100 Florent Revest <revest@chromium.org> wrote: > The _multi API requires that users keep their own ops but can enforce > that an op is only associated to one direct call. > > Signed-off-by: Florent Revest <revest@chromium.org> > Acked-by: Mark Rutland <mark.rutland@arm.com> > Tested-by: Mark Rutland <mark.rutland@arm.com> > --- > kernel/trace/trace_selftest.c | 11 +++++++---- > samples/ftrace/ftrace-direct-modify.c | 12 ++++++++---- > samples/ftrace/ftrace-direct-too.c | 12 +++++++----- > samples/ftrace/ftrace-direct.c | 12 +++++++----- > 4 files changed, 29 insertions(+), 18 deletions(-) > > diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c > index ff0536cea968..57221f69a33b 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 = {}; Make this static to the file and move it above to where there's already an #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS block. Less #ifdef is better, and also, I don't like having as an example an ftrace_ops structure allocated on the stack. It can become increasingly larger as time goes by. I don't want to encourage placing it on stacks. > +#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; I had to rebase this code on top of the latest tree because of updates. Which is good, because this patch requires those same. When using ftrace_set_filter_ip() one needs to call ftrace_free_filter(&direct); Because the ftrace_set_filter_ip() allocates hashs on the ftrace_ops and those need to be freed. > > @@ -891,8 +894,8 @@ trace_selftest_startup_function_graph(struct tracer *trace, > > unregister_ftrace_graph(&fgraph_ops); > > - ret = unregister_ftrace_direct((unsigned long) DYN_FTRACE_TEST_NAME, > - (unsigned long) trace_direct_tramp); > + 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..ecd76f75cb80 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,9 @@ 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 +142,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); Same here. > } > > 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); And here. > } > > 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); And here. Maybe we could add a parameter to unregister_ftrace_direct_multi(): bool free_filters when set, it will call the ftrace_free_filter(), as it looks like there's a common code path here: ftrace_set_filter_ip(); register_ftrace_direct_multi(); [..] unregister_ftrace_direct_multi(); ftrace_free_filter(); Add the option will save people from having to do that last step, and also remind people that the filters need to be freed. -- Steve > } > > 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");
On Thu, Mar 16, 2023 at 12:33 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Tue, 7 Feb 2023 19:21:26 +0100 > Florent Revest <revest@chromium.org> wrote: > > > The _multi API requires that users keep their own ops but can enforce > > that an op is only associated to one direct call. > > > > Signed-off-by: Florent Revest <revest@chromium.org> > > Acked-by: Mark Rutland <mark.rutland@arm.com> > > Tested-by: Mark Rutland <mark.rutland@arm.com> > > --- > > kernel/trace/trace_selftest.c | 11 +++++++---- > > samples/ftrace/ftrace-direct-modify.c | 12 ++++++++---- > > samples/ftrace/ftrace-direct-too.c | 12 +++++++----- > > samples/ftrace/ftrace-direct.c | 12 +++++++----- > > 4 files changed, 29 insertions(+), 18 deletions(-) > > > > diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c > > index ff0536cea968..57221f69a33b 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 = {}; > > Make this static to the file and move it above to where there's already an > #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS block. > > Less #ifdef is better, and also, I don't like having as an example an > ftrace_ops structure allocated on the stack. It can become increasingly > larger as time goes by. I don't want to encourage placing it on stacks. Agreed. > > +#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; > > I had to rebase this code on top of the latest tree because of updates. > Which is good, because this patch requires those same. When using > ftrace_set_filter_ip() one needs to call > > ftrace_free_filter(&direct); > > Because the ftrace_set_filter_ip() allocates hashs on the ftrace_ops and > those need to be freed. Ah yes, good catch. > > > > > @@ -891,8 +894,8 @@ trace_selftest_startup_function_graph(struct tracer *trace, > > > > unregister_ftrace_graph(&fgraph_ops); > > > > - ret = unregister_ftrace_direct((unsigned long) DYN_FTRACE_TEST_NAME, > > - (unsigned long) trace_direct_tramp); > > + 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..ecd76f75cb80 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,9 @@ 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 +142,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); > > Same here. > > > } > > > > 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); > > And here. > > > } > > > > 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); > > And here. > > Maybe we could add a parameter to unregister_ftrace_direct_multi(): > > bool free_filters > > when set, it will call the ftrace_free_filter(), as it looks like there's a > common code path here: > > ftrace_set_filter_ip(); > register_ftrace_direct_multi(); > [..] > unregister_ftrace_direct_multi(); > ftrace_free_filter(); > > Add the option will save people from having to do that last step, and also > remind people that the filters need to be freed. I agree, it's otherwise quite easy to miss the ftrace_free_filter. :) I'll add a patch introducing this "free_filters" arg in the new subset series that I'll send you directly.
diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c index ff0536cea968..57221f69a33b 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,8 +894,8 @@ trace_selftest_startup_function_graph(struct tracer *trace, unregister_ftrace_graph(&fgraph_ops); - ret = unregister_ftrace_direct((unsigned long) DYN_FTRACE_TEST_NAME, - (unsigned long) trace_direct_tramp); + 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..ecd76f75cb80 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,9 @@ 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 +142,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");