From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C3A1BC61DA4 for ; Wed, 15 Mar 2023 23:34:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=uwP/NVVacjTDEAXDcH2sQcQk7kqLM5YXhcCSgVxFhHc=; b=mhTejPI2pf5mQn 5yTQ789rXszzp/XMVRkUHYZBwxsH3VUQTjFpwjJ06h1dmo/V5ot++qNIYQ2F4dhpgcWNLKWbuRFzQ On/5QLMUbpujkWGjwfC19sqD6tebRN2MXNHN+qXkdNcjUBCyM7I47ld9hg4MpAaC8fjsuwmvPbveM ExRwq8i1HJhFypKh/+js1Ds8dLtKc/DMl9ut+GBlsSkD7K3qsptXEbDtimbSuj0ImuiL35CrhE+A8 q2uMb+hxkDJydPYaweDPOXiVRTCS171AFlOA/J8YK4DtfY0MyE7c9zyYjwcw/F1bDQ8lXTtWK9xX0 gUyl/Pkph2wHSXwDPiNw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pcadA-00Ej49-2L; Wed, 15 Mar 2023 23:34:04 +0000 Received: from ams.source.kernel.org ([2604:1380:4601:e00::1]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1pcad6-00Ej2d-0h for linux-arm-kernel@lists.infradead.org; Wed, 15 Mar 2023 23:34:03 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id C2BA5B81E8A; Wed, 15 Mar 2023 23:33:58 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id B8B08C433D2; Wed, 15 Mar 2023 23:33:56 +0000 (UTC) Date: Wed, 15 Mar 2023 19:33:55 -0400 From: Steven Rostedt To: Florent Revest Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org, bpf@vger.kernel.org, catalin.marinas@arm.com, will@kernel.org, mhiramat@kernel.org, mark.rutland@arm.com, ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org, kpsingh@kernel.org, jolsa@kernel.org, xukuohai@huaweicloud.com, lihuafei1@huawei.com Subject: Re: [PATCH v2 01/10] ftrace: Replace uses of _ftrace_direct APIs with _ftrace_direct_multi Message-ID: <20230315193355.30c48764@gandalf.local.home> In-Reply-To: <20230207182135.2671106-2-revest@chromium.org> References: <20230207182135.2671106-1-revest@chromium.org> <20230207182135.2671106-2-revest@chromium.org> X-Mailer: Claws Mail 3.17.8 (GTK+ 2.24.33; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230315_163400_538798_A1CEA01F X-CRM114-Status: GOOD ( 30.28 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, 7 Feb 2023 19:21:26 +0100 Florent Revest 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 > Acked-by: Mark Rutland > Tested-by: Mark Rutland > --- > 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"); _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel