[PATCH v2 0/3] ftrace: Add sample code with dynamic ftrace_ops

[PATCH v2 0/3] ftrace: Add sample code with dynamic ftrace_ops

[Mark Rutland]

This series adds sample code to manipulate dynamic ftrace_ops, which
I've been using to benchmark/test some changes I've been making in this
area for arm64.

In the process of writing that I spotted a couple of minor issues,
addressed by the first two patches.

I'm not sure whether this should be a sample or something under lib/;
I'm happy to change that if folk have strong opinions.

Since v1 [1]:
* Rebase to v6.2-rc2 (trivial)
* Fix typos
* Fix commit title style
* Apply Steve's Reviewed-by to patch 2
* Fix "save_regs" module parameter name
* Add example output from sample module

[1] https://lore.kernel.org/lkml/20221103170907.931465-1-mark.rutland@arm.com/

Thanks,
Mark.

Mark Rutland (3):
  ftrace: Maintain samples/ftrace
  ftrace: Export ftrace_free_filter() to modules
  ftrace: Add sample with custom ops

 MAINTAINERS                                 |   1 +
 kernel/trace/ftrace.c                       |  23 +-
 samples/Kconfig                             |   7 +
 samples/Makefile                            |   1 +
 samples/ftrace/Makefile                     |   1 +
 samples/ftrace/ftrace-direct-multi-modify.c |   1 +
 samples/ftrace/ftrace-direct-multi.c        |   1 +
 samples/ftrace/ftrace-ops.c                 | 252 ++++++++++++++++++++
 8 files changed, 286 insertions(+), 1 deletion(-)
 create mode 100644 samples/ftrace/ftrace-ops.c

-- 
2.30.2

[PATCH v2 1/3] ftrace: Maintain samples/ftrace

[Mark Rutland]

There's no entry in MAINTAINERS for samples/ftrace. Add one so that the
FTRACE maintainers are kept in the loop.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7f86d02cb427a..739006e245fbd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8571,6 +8571,7 @@ F:	kernel/trace/fgraph.c
 F:	arch/*/*/*/*ftrace*
 F:	arch/*/*/*ftrace*
 F:	include/*/ftrace.h
+F:	samples/ftrace
 
 FUNGIBLE ETHERNET DRIVERS
 M:	Dimitris Michailidis <dmichail@fungible.com>
-- 
2.30.2

[PATCH v2 2/3] ftrace: Export ftrace_free_filter() to modules

[Mark Rutland]

Setting filters on an ftrace ops results in some memory being allocated
for the filter hashes, which must be freed before the ops can be freed.
This can be done by removing every individual element of the hash by
calling ftrace_set_filter_ip() or ftrace_set_filter_ips() with `remove`
set, but this is somewhat error prone as it's easy to forget to remove
an element.

Make it easier to clean this up by exporting ftrace_free_filter(), which
can be used to clean up all of the filter hashes after an ftrace_ops has
been unregistered.

Using this, fix the ftrace-direct* samples to free hashes prior to being
unloaded. All other code either removes individual filters explicitly or
is built-in and already calls ftrace_free_filter().

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Cc: Florent Revest <revest@chromium.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/trace/ftrace.c                       | 23 ++++++++++++++++++++-
 samples/ftrace/ftrace-direct-multi-modify.c |  1 +
 samples/ftrace/ftrace-direct-multi.c        |  1 +
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 442438b93fe98..750aa3f08b25a 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1248,12 +1248,17 @@ static void free_ftrace_hash_rcu(struct ftrace_hash *hash)
 	call_rcu(&hash->rcu, __free_ftrace_hash_rcu);
 }
 
+/**
+ * ftrace_free_filter - remove all filters for an ftrace_ops
+ * @ops - the ops to remove the filters from
+ */
 void ftrace_free_filter(struct ftrace_ops *ops)
 {
 	ftrace_ops_init(ops);
 	free_ftrace_hash(ops->func_hash->filter_hash);
 	free_ftrace_hash(ops->func_hash->notrace_hash);
 }
+EXPORT_SYMBOL_GPL(ftrace_free_filter);
 
 static struct ftrace_hash *alloc_ftrace_hash(int size_bits)
 {
@@ -5839,6 +5844,10 @@ EXPORT_SYMBOL_GPL(modify_ftrace_direct_multi);
  *
  * Filters denote which functions should be enabled when tracing is enabled
  * If @ip is NULL, it fails to update filter.
+ *
+ * This can allocate memory which must be freed before @ops can be freed,
+ * either by removing each filtered addr or by using
+ * ftrace_free_filter(@ops).
  */
 int ftrace_set_filter_ip(struct ftrace_ops *ops, unsigned long ip,
 			 int remove, int reset)
@@ -5858,7 +5867,11 @@ EXPORT_SYMBOL_GPL(ftrace_set_filter_ip);
  *
  * Filters denote which functions should be enabled when tracing is enabled
  * If @ips array or any ip specified within is NULL , it fails to update filter.
- */
+ *
+ * This can allocate memory which must be freed before @ops can be freed,
+ * either by removing each filtered addr or by using
+ * ftrace_free_filter(@ops).
+*/
 int ftrace_set_filter_ips(struct ftrace_ops *ops, unsigned long *ips,
 			  unsigned int cnt, int remove, int reset)
 {
@@ -5900,6 +5913,10 @@ ftrace_set_regex(struct ftrace_ops *ops, unsigned char *buf, int len,
  *
  * Filters denote which functions should be enabled when tracing is enabled.
  * If @buf is NULL and reset is set, all functions will be enabled for tracing.
+ *
+ * This can allocate memory which must be freed before @ops can be freed,
+ * either by removing each filtered addr or by using
+ * ftrace_free_filter(@ops).
  */
 int ftrace_set_filter(struct ftrace_ops *ops, unsigned char *buf,
 		       int len, int reset)
@@ -5919,6 +5936,10 @@ EXPORT_SYMBOL_GPL(ftrace_set_filter);
  * Notrace Filters denote which functions should not be enabled when tracing
  * is enabled. If @buf is NULL and reset is set, all functions will be enabled
  * for tracing.
+ *
+ * This can allocate memory which must be freed before @ops can be freed,
+ * either by removing each filtered addr or by using
+ * ftrace_free_filter(@ops).
  */
 int ftrace_set_notrace(struct ftrace_ops *ops, unsigned char *buf,
 			int len, int reset)
diff --git a/samples/ftrace/ftrace-direct-multi-modify.c b/samples/ftrace/ftrace-direct-multi-modify.c
index d52370cad0b6e..a825dbd2c9cfd 100644
--- a/samples/ftrace/ftrace-direct-multi-modify.c
+++ b/samples/ftrace/ftrace-direct-multi-modify.c
@@ -152,6 +152,7 @@ static void __exit ftrace_direct_multi_exit(void)
 {
 	kthread_stop(simple_tsk);
 	unregister_ftrace_direct_multi(&direct, my_tramp);
+	ftrace_free_filter(&direct);
 }
 
 module_init(ftrace_direct_multi_init);
diff --git a/samples/ftrace/ftrace-direct-multi.c b/samples/ftrace/ftrace-direct-multi.c
index ec1088922517d..d955a26506053 100644
--- a/samples/ftrace/ftrace-direct-multi.c
+++ b/samples/ftrace/ftrace-direct-multi.c
@@ -79,6 +79,7 @@ static int __init ftrace_direct_multi_init(void)
 static void __exit ftrace_direct_multi_exit(void)
 {
 	unregister_ftrace_direct_multi(&direct, (unsigned long) my_tramp);
+	ftrace_free_filter(&direct);
 }
 
 module_init(ftrace_direct_multi_init);
-- 
2.30.2

[PATCH v2 3/3] ftrace: Add sample with custom ops

[Mark Rutland]

When reworking core ftrace code or architectural ftrace code, it's often
necessary to test/analyse/benchmark a number of ftrace_ops
configurations. This patch adds a module which can be used to explore
some of those configurations.

I'm using this to benchmark various options for changing the way
trampolines and handling of ftrace_ops work on arm64, and ensuring other
architectures aren't adversely affected.

For example, in a QEMU+KVM VM running on a 2GHz Xeon E5-2660
workstation, loading the module in various configurations produces:

| # insmod ftrace-ops.ko
| ftrace_ops: registering:
|   relevant ops: 1
|     tracee: tracee_relevant [ftrace_ops]
|     tracer: ops_func_nop [ftrace_ops]
|   irrelevant ops: 0
|     tracee: tracee_irrelevant [ftrace_ops]
|     tracer: ops_func_nop [ftrace_ops]
|   saving registers: NO
|   assist recursion: NO
|   assist RCU: NO
| ftrace_ops: Attempted 100000 calls to tracee_relevant [ftrace_ops] in 1681558ns (16ns / call)

| # insmod ftrace-ops.ko nr_ops_irrelevant=5
| ftrace_ops: registering:
|   relevant ops: 1
|     tracee: tracee_relevant [ftrace_ops]
|     tracer: ops_func_nop [ftrace_ops]
|   irrelevant ops: 5
|     tracee: tracee_irrelevant [ftrace_ops]
|     tracer: ops_func_nop [ftrace_ops]
|   saving registers: NO
|   assist recursion: NO
|   assist RCU: NO
| ftrace_ops: Attempted 100000 calls to tracee_relevant [ftrace_ops] in 1693042ns (16ns / call)

| # insmod ftrace-ops.ko nr_ops_relevant=2
| ftrace_ops: registering:
|   relevant ops: 2
|     tracee: tracee_relevant [ftrace_ops]
|     tracer: ops_func_nop [ftrace_ops]
|   irrelevant ops: 0
|     tracee: tracee_irrelevant [ftrace_ops]
|     tracer: ops_func_nop [ftrace_ops]
|   saving registers: NO
|   assist recursion: NO
|   assist RCU: NO
| ftrace_ops: Attempted 100000 calls to tracee_relevant [ftrace_ops] in 11965582ns (119ns / call)

| # insmod ftrace-ops.ko save_regs=true
| ftrace_ops: registering:
|   relevant ops: 1
|     tracee: tracee_relevant [ftrace_ops]
|     tracer: ops_func_nop [ftrace_ops]
|   irrelevant ops: 0
|     tracee: tracee_irrelevant [ftrace_ops]
|     tracer: ops_func_nop [ftrace_ops]
|   saving registers: YES
|   assist recursion: NO
|   assist RCU: NO
| ftrace_ops: Attempted 100000 calls to tracee_relevant [ftrace_ops] in 4459624ns (44ns / call)

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Florent Revest <revest@chromium.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 samples/Kconfig             |   7 +
 samples/Makefile            |   1 +
 samples/ftrace/Makefile     |   1 +
 samples/ftrace/ftrace-ops.c | 252 ++++++++++++++++++++++++++++++++++++
 4 files changed, 261 insertions(+)
 create mode 100644 samples/ftrace/ftrace-ops.c

diff --git a/samples/Kconfig b/samples/Kconfig
index 0d81c00289ee3..daf14c35f071d 100644
--- a/samples/Kconfig
+++ b/samples/Kconfig
@@ -46,6 +46,13 @@ config SAMPLE_FTRACE_DIRECT_MULTI
 	  that hooks to wake_up_process and schedule, and prints
 	  the function addresses.
 
+config SAMPLE_FTRACE_OPS
+	tristate "Build custom ftrace ops example"
+	depends on FUNCTION_TRACER
+	help
+	  This builds an ftrace ops example that hooks two functions and
+	  measures the time taken to invoke one function a number of times.
+
 config SAMPLE_TRACE_ARRAY
         tristate "Build sample module for kernel access to Ftrace instancess"
 	depends on EVENT_TRACING && m
diff --git a/samples/Makefile b/samples/Makefile
index 9832ef3f8fcba..7cb632ef88eeb 100644
--- a/samples/Makefile
+++ b/samples/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_SAMPLE_TRACE_CUSTOM_EVENTS) += trace_events/
 obj-$(CONFIG_SAMPLE_TRACE_PRINTK)	+= trace_printk/
 obj-$(CONFIG_SAMPLE_FTRACE_DIRECT)	+= ftrace/
 obj-$(CONFIG_SAMPLE_FTRACE_DIRECT_MULTI) += ftrace/
+obj-$(CONFIG_SAMPLE_FTRACE_OPS)		+= ftrace/
 obj-$(CONFIG_SAMPLE_TRACE_ARRAY)	+= ftrace/
 subdir-$(CONFIG_SAMPLE_UHID)		+= uhid
 obj-$(CONFIG_VIDEO_PCI_SKELETON)	+= v4l/
diff --git a/samples/ftrace/Makefile b/samples/ftrace/Makefile
index faf8cdb79c5f4..589baf2ec4e3d 100644
--- a/samples/ftrace/Makefile
+++ b/samples/ftrace/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_SAMPLE_FTRACE_DIRECT) += ftrace-direct-too.o
 obj-$(CONFIG_SAMPLE_FTRACE_DIRECT) += ftrace-direct-modify.o
 obj-$(CONFIG_SAMPLE_FTRACE_DIRECT_MULTI) += ftrace-direct-multi.o
 obj-$(CONFIG_SAMPLE_FTRACE_DIRECT_MULTI) += ftrace-direct-multi-modify.o
+obj-$(CONFIG_SAMPLE_FTRACE_OPS) += ftrace-ops.o
 
 CFLAGS_sample-trace-array.o := -I$(src)
 obj-$(CONFIG_SAMPLE_TRACE_ARRAY) += sample-trace-array.o
diff --git a/samples/ftrace/ftrace-ops.c b/samples/ftrace/ftrace-ops.c
new file mode 100644
index 0000000000000..24deb51c72618
--- /dev/null
+++ b/samples/ftrace/ftrace-ops.c
@@ -0,0 +1,252 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#define pr_fmt(fmt)       KBUILD_MODNAME ": " fmt
+
+#include <linux/ftrace.h>
+#include <linux/ktime.h>
+#include <linux/module.h>
+
+#include <asm/barrier.h>
+
+/*
+ * Arbitrary large value chosen to be sufficiently large to minimize noise but
+ * sufficiently small to complete quickly.
+ */
+unsigned int nr_function_calls = 100000;
+module_param(nr_function_calls, uint, 0);
+MODULE_PARM_DESC(nr_function_calls, "How many times to call the relevant tracee");
+
+/*
+ * The number of ops associated with a call site affects whether a tracer can
+ * be called directly or whether it's necessary to go via the list func, which
+ * can be significantly more expensive.
+ */
+unsigned int nr_ops_relevant = 1;
+module_param(nr_ops_relevant, uint, 0);
+MODULE_PARM_DESC(nr_ops_relevant, "How many ftrace_ops to associate with the relevant tracee");
+
+/*
+ * On architectures where all call sites share the same trampoline, having
+ * tracers enabled for distinct functions can force the use of the list func
+ * and incur overhead for all call sites.
+ */
+unsigned int nr_ops_irrelevant = 0;
+module_param(nr_ops_irrelevant, uint, 0);
+MODULE_PARM_DESC(nr_ops_irrelevant, "How many ftrace_ops to associate with the irrelevant tracee");
+
+/*
+ * On architectures with DYNAMIC_FTRACE_WITH_REGS, saving the full pt_regs can
+ * be more expensive than only saving the minimal necessary regs.
+ */
+bool save_regs = false;
+module_param(save_regs, bool, 0);
+MODULE_PARM_DESC(save_regs, "Register ops with FTRACE_OPS_FL_SAVE_REGS (save all registers in the trampoline)");
+
+bool assist_recursion = false;
+module_param(assist_recursion, bool, 0);
+MODULE_PARM_DESC(assist_reursion, "Register ops with FTRACE_OPS_FL_RECURSION");
+
+bool assist_rcu = false;
+module_param(assist_rcu, bool, 0);
+MODULE_PARM_DESC(assist_reursion, "Register ops with FTRACE_OPS_FL_RCU");
+
+/*
+ * By default, a trivial tracer is used which immediately returns to mimimize
+ * overhead. Sometimes a consistency check using a more expensive tracer is
+ * desireable.
+ */
+bool check_count = false;
+module_param(check_count, bool, 0);
+MODULE_PARM_DESC(check_count, "Check that tracers are called the expected number of times\n");
+
+/*
+ * Usually it's not interesting to leave the ops registered after the test
+ * runs, but sometimes it can be useful to leave them registered so that they
+ * can be inspected through the tracefs 'enabled_functions' file.
+ */
+bool persist = false;
+module_param(persist, bool, 0);
+MODULE_PARM_DESC(persist, "Successfully load module and leave ftrace ops registered after test completes\n");
+
+/*
+ * Marked as noinline to ensure that an out-of-line traceable copy is
+ * generated by the compiler.
+ *
+ * The barrier() ensures the compiler won't elide calls by determining there
+ * are no side-effects.
+ */
+static noinline void tracee_relevant(void)
+{
+	barrier();
+}
+
+/*
+ * Marked as noinline to ensure that an out-of-line traceable copy is
+ * generated by the compiler.
+ *
+ * The barrier() ensures the compiler won't elide calls by determining there
+ * are no side-effects.
+ */
+static noinline void tracee_irrelevant(void)
+{
+	barrier();
+}
+
+struct sample_ops {
+	struct ftrace_ops ops;
+	unsigned int count;
+};
+
+static void ops_func_nop(unsigned long ip, unsigned long parent_ip,
+			 struct ftrace_ops *op,
+			 struct ftrace_regs *fregs)
+{
+	/* do nothing */
+}
+
+static void ops_func_count(unsigned long ip, unsigned long parent_ip,
+			   struct ftrace_ops *op,
+			   struct ftrace_regs *fregs)
+{
+	struct sample_ops *self;
+
+	self = container_of(op, struct sample_ops, ops);
+	self->count++;
+}
+
+struct sample_ops *ops_relevant;
+struct sample_ops *ops_irrelevant;
+
+static struct sample_ops *ops_alloc_init(void *tracee, ftrace_func_t func,
+					 unsigned long flags, int nr)
+{
+	struct sample_ops *ops;
+
+	ops = kcalloc(nr, sizeof(*ops), GFP_KERNEL);
+	if (WARN_ON_ONCE(!ops))
+		return NULL;
+
+	for (unsigned int i = 0; i < nr; i++) {
+		ops[i].ops.func = func;
+		ops[i].ops.flags = flags;
+		WARN_ON_ONCE(ftrace_set_filter_ip(&ops[i].ops, (unsigned long)tracee, 0, 0));
+		WARN_ON_ONCE(register_ftrace_function(&ops[i].ops));
+	}
+
+	return ops;
+}
+
+static void ops_destroy(struct sample_ops *ops, int nr)
+{
+	if (!ops)
+		return;
+
+	for (unsigned int i = 0; i < nr; i++) {
+		WARN_ON_ONCE(unregister_ftrace_function(&ops[i].ops));
+		ftrace_free_filter(&ops[i].ops);
+	}
+
+	kfree(ops);
+}
+
+static void ops_check(struct sample_ops *ops, int nr,
+		      unsigned int expected_count)
+{
+	if (!ops || !check_count)
+		return;
+
+	for (unsigned int i = 0; i < nr; i++) {
+		if (ops->count == expected_count)
+			continue;
+		pr_warn("Counter called %u times (expected %u)\n",
+			ops->count, expected_count);
+	}
+}
+
+ftrace_func_t tracer_relevant = ops_func_nop;
+ftrace_func_t tracer_irrelevant = ops_func_nop;
+
+static int __init ftrace_ops_sample_init(void)
+{
+	unsigned long flags = 0;
+	ktime_t start, end;
+	u64 period;
+
+	if (!IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_REGS) && save_regs) {
+		pr_info("this kernel does not support saving registers\n");
+		save_regs = false;
+	} else if (save_regs) {
+		flags |= FTRACE_OPS_FL_SAVE_REGS;
+	}
+
+	if (assist_recursion)
+		flags |= FTRACE_OPS_FL_RECURSION;
+
+	if (assist_rcu)
+		flags |= FTRACE_OPS_FL_RCU;
+
+	if (check_count) {
+		tracer_relevant = ops_func_count;
+		tracer_irrelevant = ops_func_count;
+	}
+
+	pr_info("registering:\n"
+		"  relevant ops: %u\n"
+		"    tracee: %ps\n"
+		"    tracer: %ps\n"
+		"  irrelevant ops: %u\n"
+		"    tracee: %ps\n"
+		"    tracer: %ps\n"
+		"  saving registers: %s\n"
+		"  assist recursion: %s\n"
+		"  assist RCU: %s\n",
+		nr_ops_relevant, tracee_relevant, tracer_relevant,
+		nr_ops_irrelevant, tracee_irrelevant, tracer_irrelevant,
+		save_regs ? "YES" : "NO",
+		assist_recursion ? "YES" : "NO",
+		assist_rcu ? "YES" : "NO");
+
+	ops_relevant = ops_alloc_init(tracee_relevant, tracer_relevant,
+				      flags, nr_ops_relevant);
+	ops_irrelevant = ops_alloc_init(tracee_irrelevant, tracer_irrelevant,
+					flags, nr_ops_irrelevant);
+
+	start = ktime_get();
+	for (unsigned int i = 0; i < nr_function_calls; i++)
+		tracee_relevant();
+	end = ktime_get();
+
+	ops_check(ops_relevant, nr_ops_relevant, nr_function_calls);
+	ops_check(ops_irrelevant, nr_ops_irrelevant, 0);
+
+	period = ktime_to_ns(ktime_sub(end, start));
+
+	pr_info("Attempted %u calls to %ps in %lluns (%lluns / call)\n",
+		nr_function_calls, tracee_relevant,
+		period, period / nr_function_calls);
+
+	if (persist)
+		return 0;
+
+	ops_destroy(ops_relevant, nr_ops_relevant);
+	ops_destroy(ops_irrelevant, nr_ops_irrelevant);
+
+	/*
+	 * The benchmark completed sucessfully, but there's no reason to keep
+	 * the module around. Return an error do the user doesn't have to
+	 * manually unload the module.
+	 */
+	return -EINVAL;
+}
+module_init(ftrace_ops_sample_init);
+
+static void __exit ftrace_ops_sample_exit(void)
+{
+	ops_destroy(ops_relevant, nr_ops_relevant);
+	ops_destroy(ops_irrelevant, nr_ops_irrelevant);
+}
+module_exit(ftrace_ops_sample_exit);
+
+MODULE_AUTHOR("Mark Rutland");
+MODULE_DESCRIPTION("Example of using custom ftrace_ops");
+MODULE_LICENSE("GPL");
-- 
2.30.2

Re: [PATCH v2 1/3] ftrace: Maintain samples/ftrace

[Masami Hiramatsu]

On Tue,  3 Jan 2023 12:49:10 +0000
Mark Rutland <mark.rutland@arm.com> wrote:

> There's no entry in MAINTAINERS for samples/ftrace. Add one so that the
> FTRACE maintainers are kept in the loop.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>

This looks good to me.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thanks!

> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7f86d02cb427a..739006e245fbd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8571,6 +8571,7 @@ F:	kernel/trace/fgraph.c
>  F:	arch/*/*/*/*ftrace*
>  F:	arch/*/*/*ftrace*
>  F:	include/*/ftrace.h
> +F:	samples/ftrace
>  
>  FUNGIBLE ETHERNET DRIVERS
>  M:	Dimitris Michailidis <dmichail@fungible.com>
> -- 
> 2.30.2
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v2 2/3] ftrace: Export ftrace_free_filter() to modules

[Masami Hiramatsu]

On Tue,  3 Jan 2023 12:49:11 +0000
Mark Rutland <mark.rutland@arm.com> wrote:

> Setting filters on an ftrace ops results in some memory being allocated
> for the filter hashes, which must be freed before the ops can be freed.
> This can be done by removing every individual element of the hash by
> calling ftrace_set_filter_ip() or ftrace_set_filter_ips() with `remove`
> set, but this is somewhat error prone as it's easy to forget to remove
> an element.
> 
> Make it easier to clean this up by exporting ftrace_free_filter(), which
> can be used to clean up all of the filter hashes after an ftrace_ops has
> been unregistered.
> 
> Using this, fix the ftrace-direct* samples to free hashes prior to being
> unloaded. All other code either removes individual filters explicitly or
> is built-in and already calls ftrace_free_filter().

So, it seems to fix memory leaks. Then, it may need to go to stable.

Fixes: e1067a07cfbc ("ftrace/samples: Add module to test multi direct modify interface")
Fixes: 5fae941b9a6f ("ftrace/samples: Add multi direct interface test module")
Cc: stable@vger.kernel.org

And 

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thanks!

> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> Cc: Florent Revest <revest@chromium.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  kernel/trace/ftrace.c                       | 23 ++++++++++++++++++++-
>  samples/ftrace/ftrace-direct-multi-modify.c |  1 +
>  samples/ftrace/ftrace-direct-multi.c        |  1 +
>  3 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 442438b93fe98..750aa3f08b25a 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1248,12 +1248,17 @@ static void free_ftrace_hash_rcu(struct ftrace_hash *hash)
>  	call_rcu(&hash->rcu, __free_ftrace_hash_rcu);
>  }
>  
> +/**
> + * ftrace_free_filter - remove all filters for an ftrace_ops
> + * @ops - the ops to remove the filters from
> + */
>  void ftrace_free_filter(struct ftrace_ops *ops)
>  {
>  	ftrace_ops_init(ops);
>  	free_ftrace_hash(ops->func_hash->filter_hash);
>  	free_ftrace_hash(ops->func_hash->notrace_hash);
>  }
> +EXPORT_SYMBOL_GPL(ftrace_free_filter);
>  
>  static struct ftrace_hash *alloc_ftrace_hash(int size_bits)
>  {
> @@ -5839,6 +5844,10 @@ EXPORT_SYMBOL_GPL(modify_ftrace_direct_multi);
>   *
>   * Filters denote which functions should be enabled when tracing is enabled
>   * If @ip is NULL, it fails to update filter.
> + *
> + * This can allocate memory which must be freed before @ops can be freed,
> + * either by removing each filtered addr or by using
> + * ftrace_free_filter(@ops).
>   */
>  int ftrace_set_filter_ip(struct ftrace_ops *ops, unsigned long ip,
>  			 int remove, int reset)
> @@ -5858,7 +5867,11 @@ EXPORT_SYMBOL_GPL(ftrace_set_filter_ip);
>   *
>   * Filters denote which functions should be enabled when tracing is enabled
>   * If @ips array or any ip specified within is NULL , it fails to update filter.
> - */
> + *
> + * This can allocate memory which must be freed before @ops can be freed,
> + * either by removing each filtered addr or by using
> + * ftrace_free_filter(@ops).
> +*/
>  int ftrace_set_filter_ips(struct ftrace_ops *ops, unsigned long *ips,
>  			  unsigned int cnt, int remove, int reset)
>  {
> @@ -5900,6 +5913,10 @@ ftrace_set_regex(struct ftrace_ops *ops, unsigned char *buf, int len,
>   *
>   * Filters denote which functions should be enabled when tracing is enabled.
>   * If @buf is NULL and reset is set, all functions will be enabled for tracing.
> + *
> + * This can allocate memory which must be freed before @ops can be freed,
> + * either by removing each filtered addr or by using
> + * ftrace_free_filter(@ops).
>   */
>  int ftrace_set_filter(struct ftrace_ops *ops, unsigned char *buf,
>  		       int len, int reset)
> @@ -5919,6 +5936,10 @@ EXPORT_SYMBOL_GPL(ftrace_set_filter);
>   * Notrace Filters denote which functions should not be enabled when tracing
>   * is enabled. If @buf is NULL and reset is set, all functions will be enabled
>   * for tracing.
> + *
> + * This can allocate memory which must be freed before @ops can be freed,
> + * either by removing each filtered addr or by using
> + * ftrace_free_filter(@ops).
>   */
>  int ftrace_set_notrace(struct ftrace_ops *ops, unsigned char *buf,
>  			int len, int reset)
> diff --git a/samples/ftrace/ftrace-direct-multi-modify.c b/samples/ftrace/ftrace-direct-multi-modify.c
> index d52370cad0b6e..a825dbd2c9cfd 100644
> --- a/samples/ftrace/ftrace-direct-multi-modify.c
> +++ b/samples/ftrace/ftrace-direct-multi-modify.c
> @@ -152,6 +152,7 @@ static void __exit ftrace_direct_multi_exit(void)
>  {
>  	kthread_stop(simple_tsk);
>  	unregister_ftrace_direct_multi(&direct, my_tramp);
> +	ftrace_free_filter(&direct);
>  }
>  
>  module_init(ftrace_direct_multi_init);
> diff --git a/samples/ftrace/ftrace-direct-multi.c b/samples/ftrace/ftrace-direct-multi.c
> index ec1088922517d..d955a26506053 100644
> --- a/samples/ftrace/ftrace-direct-multi.c
> +++ b/samples/ftrace/ftrace-direct-multi.c
> @@ -79,6 +79,7 @@ static int __init ftrace_direct_multi_init(void)
>  static void __exit ftrace_direct_multi_exit(void)
>  {
>  	unregister_ftrace_direct_multi(&direct, (unsigned long) my_tramp);
> +	ftrace_free_filter(&direct);
>  }
>  
>  module_init(ftrace_direct_multi_init);
> -- 
> 2.30.2
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v2 3/3] ftrace: Add sample with custom ops

[Masami Hiramatsu]

On Tue,  3 Jan 2023 12:49:12 +0000
Mark Rutland <mark.rutland@arm.com> wrote:

> When reworking core ftrace code or architectural ftrace code, it's often
> necessary to test/analyse/benchmark a number of ftrace_ops
> configurations. This patch adds a module which can be used to explore
> some of those configurations.
> 
> I'm using this to benchmark various options for changing the way
> trampolines and handling of ftrace_ops work on arm64, and ensuring other
> architectures aren't adversely affected.
> 
> For example, in a QEMU+KVM VM running on a 2GHz Xeon E5-2660
> workstation, loading the module in various configurations produces:
> 
> | # insmod ftrace-ops.ko
> | ftrace_ops: registering:
> |   relevant ops: 1
> |     tracee: tracee_relevant [ftrace_ops]
> |     tracer: ops_func_nop [ftrace_ops]
> |   irrelevant ops: 0
> |     tracee: tracee_irrelevant [ftrace_ops]
> |     tracer: ops_func_nop [ftrace_ops]
> |   saving registers: NO
> |   assist recursion: NO
> |   assist RCU: NO
> | ftrace_ops: Attempted 100000 calls to tracee_relevant [ftrace_ops] in 1681558ns (16ns / call)
> 
> | # insmod ftrace-ops.ko nr_ops_irrelevant=5
> | ftrace_ops: registering:
> |   relevant ops: 1
> |     tracee: tracee_relevant [ftrace_ops]
> |     tracer: ops_func_nop [ftrace_ops]
> |   irrelevant ops: 5
> |     tracee: tracee_irrelevant [ftrace_ops]
> |     tracer: ops_func_nop [ftrace_ops]
> |   saving registers: NO
> |   assist recursion: NO
> |   assist RCU: NO
> | ftrace_ops: Attempted 100000 calls to tracee_relevant [ftrace_ops] in 1693042ns (16ns / call)
> 
> | # insmod ftrace-ops.ko nr_ops_relevant=2
> | ftrace_ops: registering:
> |   relevant ops: 2
> |     tracee: tracee_relevant [ftrace_ops]
> |     tracer: ops_func_nop [ftrace_ops]
> |   irrelevant ops: 0
> |     tracee: tracee_irrelevant [ftrace_ops]
> |     tracer: ops_func_nop [ftrace_ops]
> |   saving registers: NO
> |   assist recursion: NO
> |   assist RCU: NO
> | ftrace_ops: Attempted 100000 calls to tracee_relevant [ftrace_ops] in 11965582ns (119ns / call)
> 
> | # insmod ftrace-ops.ko save_regs=true
> | ftrace_ops: registering:
> |   relevant ops: 1
> |     tracee: tracee_relevant [ftrace_ops]
> |     tracer: ops_func_nop [ftrace_ops]
> |   irrelevant ops: 0
> |     tracee: tracee_irrelevant [ftrace_ops]
> |     tracer: ops_func_nop [ftrace_ops]
> |   saving registers: YES
> |   assist recursion: NO
> |   assist RCU: NO
> | ftrace_ops: Attempted 100000 calls to tracee_relevant [ftrace_ops] in 4459624ns (44ns / call)
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Florent Revest <revest@chromium.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>

Thanks for this useful micro benchmark module :)
This looks good to me.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>


> ---
>  samples/Kconfig             |   7 +
>  samples/Makefile            |   1 +
>  samples/ftrace/Makefile     |   1 +
>  samples/ftrace/ftrace-ops.c | 252 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 261 insertions(+)
>  create mode 100644 samples/ftrace/ftrace-ops.c
> 
> diff --git a/samples/Kconfig b/samples/Kconfig
> index 0d81c00289ee3..daf14c35f071d 100644
> --- a/samples/Kconfig
> +++ b/samples/Kconfig
> @@ -46,6 +46,13 @@ config SAMPLE_FTRACE_DIRECT_MULTI
>  	  that hooks to wake_up_process and schedule, and prints
>  	  the function addresses.
>  
> +config SAMPLE_FTRACE_OPS
> +	tristate "Build custom ftrace ops example"
> +	depends on FUNCTION_TRACER
> +	help
> +	  This builds an ftrace ops example that hooks two functions and
> +	  measures the time taken to invoke one function a number of times.
> +
>  config SAMPLE_TRACE_ARRAY
>          tristate "Build sample module for kernel access to Ftrace instancess"
>  	depends on EVENT_TRACING && m
> diff --git a/samples/Makefile b/samples/Makefile
> index 9832ef3f8fcba..7cb632ef88eeb 100644
> --- a/samples/Makefile
> +++ b/samples/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_SAMPLE_TRACE_CUSTOM_EVENTS) += trace_events/
>  obj-$(CONFIG_SAMPLE_TRACE_PRINTK)	+= trace_printk/
>  obj-$(CONFIG_SAMPLE_FTRACE_DIRECT)	+= ftrace/
>  obj-$(CONFIG_SAMPLE_FTRACE_DIRECT_MULTI) += ftrace/
> +obj-$(CONFIG_SAMPLE_FTRACE_OPS)		+= ftrace/
>  obj-$(CONFIG_SAMPLE_TRACE_ARRAY)	+= ftrace/
>  subdir-$(CONFIG_SAMPLE_UHID)		+= uhid
>  obj-$(CONFIG_VIDEO_PCI_SKELETON)	+= v4l/
> diff --git a/samples/ftrace/Makefile b/samples/ftrace/Makefile
> index faf8cdb79c5f4..589baf2ec4e3d 100644
> --- a/samples/ftrace/Makefile
> +++ b/samples/ftrace/Makefile
> @@ -5,6 +5,7 @@ obj-$(CONFIG_SAMPLE_FTRACE_DIRECT) += ftrace-direct-too.o
>  obj-$(CONFIG_SAMPLE_FTRACE_DIRECT) += ftrace-direct-modify.o
>  obj-$(CONFIG_SAMPLE_FTRACE_DIRECT_MULTI) += ftrace-direct-multi.o
>  obj-$(CONFIG_SAMPLE_FTRACE_DIRECT_MULTI) += ftrace-direct-multi-modify.o
> +obj-$(CONFIG_SAMPLE_FTRACE_OPS) += ftrace-ops.o
>  
>  CFLAGS_sample-trace-array.o := -I$(src)
>  obj-$(CONFIG_SAMPLE_TRACE_ARRAY) += sample-trace-array.o
> diff --git a/samples/ftrace/ftrace-ops.c b/samples/ftrace/ftrace-ops.c
> new file mode 100644
> index 0000000000000..24deb51c72618
> --- /dev/null
> +++ b/samples/ftrace/ftrace-ops.c
> @@ -0,0 +1,252 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#define pr_fmt(fmt)       KBUILD_MODNAME ": " fmt
> +
> +#include <linux/ftrace.h>
> +#include <linux/ktime.h>
> +#include <linux/module.h>
> +
> +#include <asm/barrier.h>
> +
> +/*
> + * Arbitrary large value chosen to be sufficiently large to minimize noise but
> + * sufficiently small to complete quickly.
> + */
> +unsigned int nr_function_calls = 100000;
> +module_param(nr_function_calls, uint, 0);
> +MODULE_PARM_DESC(nr_function_calls, "How many times to call the relevant tracee");
> +
> +/*
> + * The number of ops associated with a call site affects whether a tracer can
> + * be called directly or whether it's necessary to go via the list func, which
> + * can be significantly more expensive.
> + */
> +unsigned int nr_ops_relevant = 1;
> +module_param(nr_ops_relevant, uint, 0);
> +MODULE_PARM_DESC(nr_ops_relevant, "How many ftrace_ops to associate with the relevant tracee");
> +
> +/*
> + * On architectures where all call sites share the same trampoline, having
> + * tracers enabled for distinct functions can force the use of the list func
> + * and incur overhead for all call sites.
> + */
> +unsigned int nr_ops_irrelevant = 0;
> +module_param(nr_ops_irrelevant, uint, 0);
> +MODULE_PARM_DESC(nr_ops_irrelevant, "How many ftrace_ops to associate with the irrelevant tracee");
> +
> +/*
> + * On architectures with DYNAMIC_FTRACE_WITH_REGS, saving the full pt_regs can
> + * be more expensive than only saving the minimal necessary regs.
> + */
> +bool save_regs = false;
> +module_param(save_regs, bool, 0);
> +MODULE_PARM_DESC(save_regs, "Register ops with FTRACE_OPS_FL_SAVE_REGS (save all registers in the trampoline)");
> +
> +bool assist_recursion = false;
> +module_param(assist_recursion, bool, 0);
> +MODULE_PARM_DESC(assist_reursion, "Register ops with FTRACE_OPS_FL_RECURSION");
> +
> +bool assist_rcu = false;
> +module_param(assist_rcu, bool, 0);
> +MODULE_PARM_DESC(assist_reursion, "Register ops with FTRACE_OPS_FL_RCU");
> +
> +/*
> + * By default, a trivial tracer is used which immediately returns to mimimize
> + * overhead. Sometimes a consistency check using a more expensive tracer is
> + * desireable.
> + */
> +bool check_count = false;
> +module_param(check_count, bool, 0);
> +MODULE_PARM_DESC(check_count, "Check that tracers are called the expected number of times\n");
> +
> +/*
> + * Usually it's not interesting to leave the ops registered after the test
> + * runs, but sometimes it can be useful to leave them registered so that they
> + * can be inspected through the tracefs 'enabled_functions' file.
> + */
> +bool persist = false;
> +module_param(persist, bool, 0);
> +MODULE_PARM_DESC(persist, "Successfully load module and leave ftrace ops registered after test completes\n");
> +
> +/*
> + * Marked as noinline to ensure that an out-of-line traceable copy is
> + * generated by the compiler.
> + *
> + * The barrier() ensures the compiler won't elide calls by determining there
> + * are no side-effects.
> + */
> +static noinline void tracee_relevant(void)
> +{
> +	barrier();
> +}
> +
> +/*
> + * Marked as noinline to ensure that an out-of-line traceable copy is
> + * generated by the compiler.
> + *
> + * The barrier() ensures the compiler won't elide calls by determining there
> + * are no side-effects.
> + */
> +static noinline void tracee_irrelevant(void)
> +{
> +	barrier();
> +}
> +
> +struct sample_ops {
> +	struct ftrace_ops ops;
> +	unsigned int count;
> +};
> +
> +static void ops_func_nop(unsigned long ip, unsigned long parent_ip,
> +			 struct ftrace_ops *op,
> +			 struct ftrace_regs *fregs)
> +{
> +	/* do nothing */
> +}
> +
> +static void ops_func_count(unsigned long ip, unsigned long parent_ip,
> +			   struct ftrace_ops *op,
> +			   struct ftrace_regs *fregs)
> +{
> +	struct sample_ops *self;
> +
> +	self = container_of(op, struct sample_ops, ops);
> +	self->count++;
> +}
> +
> +struct sample_ops *ops_relevant;
> +struct sample_ops *ops_irrelevant;
> +
> +static struct sample_ops *ops_alloc_init(void *tracee, ftrace_func_t func,
> +					 unsigned long flags, int nr)
> +{
> +	struct sample_ops *ops;
> +
> +	ops = kcalloc(nr, sizeof(*ops), GFP_KERNEL);
> +	if (WARN_ON_ONCE(!ops))
> +		return NULL;
> +
> +	for (unsigned int i = 0; i < nr; i++) {
> +		ops[i].ops.func = func;
> +		ops[i].ops.flags = flags;
> +		WARN_ON_ONCE(ftrace_set_filter_ip(&ops[i].ops, (unsigned long)tracee, 0, 0));
> +		WARN_ON_ONCE(register_ftrace_function(&ops[i].ops));
> +	}
> +
> +	return ops;
> +}
> +
> +static void ops_destroy(struct sample_ops *ops, int nr)
> +{
> +	if (!ops)
> +		return;
> +
> +	for (unsigned int i = 0; i < nr; i++) {
> +		WARN_ON_ONCE(unregister_ftrace_function(&ops[i].ops));
> +		ftrace_free_filter(&ops[i].ops);
> +	}
> +
> +	kfree(ops);
> +}
> +
> +static void ops_check(struct sample_ops *ops, int nr,
> +		      unsigned int expected_count)
> +{
> +	if (!ops || !check_count)
> +		return;
> +
> +	for (unsigned int i = 0; i < nr; i++) {
> +		if (ops->count == expected_count)
> +			continue;
> +		pr_warn("Counter called %u times (expected %u)\n",
> +			ops->count, expected_count);
> +	}
> +}
> +
> +ftrace_func_t tracer_relevant = ops_func_nop;
> +ftrace_func_t tracer_irrelevant = ops_func_nop;
> +
> +static int __init ftrace_ops_sample_init(void)
> +{
> +	unsigned long flags = 0;
> +	ktime_t start, end;
> +	u64 period;
> +
> +	if (!IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_REGS) && save_regs) {
> +		pr_info("this kernel does not support saving registers\n");
> +		save_regs = false;
> +	} else if (save_regs) {
> +		flags |= FTRACE_OPS_FL_SAVE_REGS;
> +	}
> +
> +	if (assist_recursion)
> +		flags |= FTRACE_OPS_FL_RECURSION;
> +
> +	if (assist_rcu)
> +		flags |= FTRACE_OPS_FL_RCU;
> +
> +	if (check_count) {
> +		tracer_relevant = ops_func_count;
> +		tracer_irrelevant = ops_func_count;
> +	}
> +
> +	pr_info("registering:\n"
> +		"  relevant ops: %u\n"
> +		"    tracee: %ps\n"
> +		"    tracer: %ps\n"
> +		"  irrelevant ops: %u\n"
> +		"    tracee: %ps\n"
> +		"    tracer: %ps\n"
> +		"  saving registers: %s\n"
> +		"  assist recursion: %s\n"
> +		"  assist RCU: %s\n",
> +		nr_ops_relevant, tracee_relevant, tracer_relevant,
> +		nr_ops_irrelevant, tracee_irrelevant, tracer_irrelevant,
> +		save_regs ? "YES" : "NO",
> +		assist_recursion ? "YES" : "NO",
> +		assist_rcu ? "YES" : "NO");
> +
> +	ops_relevant = ops_alloc_init(tracee_relevant, tracer_relevant,
> +				      flags, nr_ops_relevant);
> +	ops_irrelevant = ops_alloc_init(tracee_irrelevant, tracer_irrelevant,
> +					flags, nr_ops_irrelevant);
> +
> +	start = ktime_get();
> +	for (unsigned int i = 0; i < nr_function_calls; i++)
> +		tracee_relevant();
> +	end = ktime_get();
> +
> +	ops_check(ops_relevant, nr_ops_relevant, nr_function_calls);
> +	ops_check(ops_irrelevant, nr_ops_irrelevant, 0);
> +
> +	period = ktime_to_ns(ktime_sub(end, start));
> +
> +	pr_info("Attempted %u calls to %ps in %lluns (%lluns / call)\n",
> +		nr_function_calls, tracee_relevant,
> +		period, period / nr_function_calls);
> +
> +	if (persist)
> +		return 0;
> +
> +	ops_destroy(ops_relevant, nr_ops_relevant);
> +	ops_destroy(ops_irrelevant, nr_ops_irrelevant);
> +
> +	/*
> +	 * The benchmark completed sucessfully, but there's no reason to keep
> +	 * the module around. Return an error do the user doesn't have to
> +	 * manually unload the module.
> +	 */
> +	return -EINVAL;
> +}
> +module_init(ftrace_ops_sample_init);
> +
> +static void __exit ftrace_ops_sample_exit(void)
> +{
> +	ops_destroy(ops_relevant, nr_ops_relevant);
> +	ops_destroy(ops_irrelevant, nr_ops_irrelevant);
> +}
> +module_exit(ftrace_ops_sample_exit);
> +
> +MODULE_AUTHOR("Mark Rutland");
> +MODULE_DESCRIPTION("Example of using custom ftrace_ops");
> +MODULE_LICENSE("GPL");
> -- 
> 2.30.2
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v2 2/3] ftrace: Export ftrace_free_filter() to modules

[Mark Rutland]

On Wed, Jan 04, 2023 at 11:51:49PM +0900, Masami Hiramatsu wrote:
> On Tue,  3 Jan 2023 12:49:11 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > Setting filters on an ftrace ops results in some memory being allocated
> > for the filter hashes, which must be freed before the ops can be freed.
> > This can be done by removing every individual element of the hash by
> > calling ftrace_set_filter_ip() or ftrace_set_filter_ips() with `remove`
> > set, but this is somewhat error prone as it's easy to forget to remove
> > an element.
> > 
> > Make it easier to clean this up by exporting ftrace_free_filter(), which
> > can be used to clean up all of the filter hashes after an ftrace_ops has
> > been unregistered.
> > 
> > Using this, fix the ftrace-direct* samples to free hashes prior to being
> > unloaded. All other code either removes individual filters explicitly or
> > is built-in and already calls ftrace_free_filter().
> 
> So, it seems to fix memory leaks. Then, it may need to go to stable.
> 
> Fixes: e1067a07cfbc ("ftrace/samples: Add module to test multi direct modify interface")
> Fixes: 5fae941b9a6f ("ftrace/samples: Add multi direct interface test module")
> Cc: stable@vger.kernel.org
> 
> And 
> 
> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Thanks!

Thanks!

Assuming Steve is also happy with the series, I assume one of you two will pick
this up and fold those in.

I've folded all those tags in my local branch (and pushed that to my
ftrace/ops-sample branch on kernel.org), so if you'd prefer I post a v3 with
those I'm quite happy to do so.

Thanks,
Mark.

> 
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> > Cc: Florent Revest <revest@chromium.org>
> > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  kernel/trace/ftrace.c                       | 23 ++++++++++++++++++++-
> >  samples/ftrace/ftrace-direct-multi-modify.c |  1 +
> >  samples/ftrace/ftrace-direct-multi.c        |  1 +
> >  3 files changed, 24 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 442438b93fe98..750aa3f08b25a 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -1248,12 +1248,17 @@ static void free_ftrace_hash_rcu(struct ftrace_hash *hash)
> >  	call_rcu(&hash->rcu, __free_ftrace_hash_rcu);
> >  }
> >  
> > +/**
> > + * ftrace_free_filter - remove all filters for an ftrace_ops
> > + * @ops - the ops to remove the filters from
> > + */
> >  void ftrace_free_filter(struct ftrace_ops *ops)
> >  {
> >  	ftrace_ops_init(ops);
> >  	free_ftrace_hash(ops->func_hash->filter_hash);
> >  	free_ftrace_hash(ops->func_hash->notrace_hash);
> >  }
> > +EXPORT_SYMBOL_GPL(ftrace_free_filter);
> >  
> >  static struct ftrace_hash *alloc_ftrace_hash(int size_bits)
> >  {
> > @@ -5839,6 +5844,10 @@ EXPORT_SYMBOL_GPL(modify_ftrace_direct_multi);
> >   *
> >   * Filters denote which functions should be enabled when tracing is enabled
> >   * If @ip is NULL, it fails to update filter.
> > + *
> > + * This can allocate memory which must be freed before @ops can be freed,
> > + * either by removing each filtered addr or by using
> > + * ftrace_free_filter(@ops).
> >   */
> >  int ftrace_set_filter_ip(struct ftrace_ops *ops, unsigned long ip,
> >  			 int remove, int reset)
> > @@ -5858,7 +5867,11 @@ EXPORT_SYMBOL_GPL(ftrace_set_filter_ip);
> >   *
> >   * Filters denote which functions should be enabled when tracing is enabled
> >   * If @ips array or any ip specified within is NULL , it fails to update filter.
> > - */
> > + *
> > + * This can allocate memory which must be freed before @ops can be freed,
> > + * either by removing each filtered addr or by using
> > + * ftrace_free_filter(@ops).
> > +*/
> >  int ftrace_set_filter_ips(struct ftrace_ops *ops, unsigned long *ips,
> >  			  unsigned int cnt, int remove, int reset)
> >  {
> > @@ -5900,6 +5913,10 @@ ftrace_set_regex(struct ftrace_ops *ops, unsigned char *buf, int len,
> >   *
> >   * Filters denote which functions should be enabled when tracing is enabled.
> >   * If @buf is NULL and reset is set, all functions will be enabled for tracing.
> > + *
> > + * This can allocate memory which must be freed before @ops can be freed,
> > + * either by removing each filtered addr or by using
> > + * ftrace_free_filter(@ops).
> >   */
> >  int ftrace_set_filter(struct ftrace_ops *ops, unsigned char *buf,
> >  		       int len, int reset)
> > @@ -5919,6 +5936,10 @@ EXPORT_SYMBOL_GPL(ftrace_set_filter);
> >   * Notrace Filters denote which functions should not be enabled when tracing
> >   * is enabled. If @buf is NULL and reset is set, all functions will be enabled
> >   * for tracing.
> > + *
> > + * This can allocate memory which must be freed before @ops can be freed,
> > + * either by removing each filtered addr or by using
> > + * ftrace_free_filter(@ops).
> >   */
> >  int ftrace_set_notrace(struct ftrace_ops *ops, unsigned char *buf,
> >  			int len, int reset)
> > diff --git a/samples/ftrace/ftrace-direct-multi-modify.c b/samples/ftrace/ftrace-direct-multi-modify.c
> > index d52370cad0b6e..a825dbd2c9cfd 100644
> > --- a/samples/ftrace/ftrace-direct-multi-modify.c
> > +++ b/samples/ftrace/ftrace-direct-multi-modify.c
> > @@ -152,6 +152,7 @@ static void __exit ftrace_direct_multi_exit(void)
> >  {
> >  	kthread_stop(simple_tsk);
> >  	unregister_ftrace_direct_multi(&direct, my_tramp);
> > +	ftrace_free_filter(&direct);
> >  }
> >  
> >  module_init(ftrace_direct_multi_init);
> > diff --git a/samples/ftrace/ftrace-direct-multi.c b/samples/ftrace/ftrace-direct-multi.c
> > index ec1088922517d..d955a26506053 100644
> > --- a/samples/ftrace/ftrace-direct-multi.c
> > +++ b/samples/ftrace/ftrace-direct-multi.c
> > @@ -79,6 +79,7 @@ static int __init ftrace_direct_multi_init(void)
> >  static void __exit ftrace_direct_multi_exit(void)
> >  {
> >  	unregister_ftrace_direct_multi(&direct, (unsigned long) my_tramp);
> > +	ftrace_free_filter(&direct);
> >  }
> >  
> >  module_init(ftrace_direct_multi_init);
> > -- 
> > 2.30.2
> > 
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v2 2/3] ftrace: Export ftrace_free_filter() to modules

[Steven Rostedt]

On Wed, 4 Jan 2023 15:30:16 +0000
Mark Rutland <mark.rutland@arm.com> wrote:

> > Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Thanks!  
> 
> Thanks!
> 
> Assuming Steve is also happy with the series, I assume one of you two will pick
> this up and fold those in.
> 
> I've folded all those tags in my local branch (and pushed that to my
> ftrace/ops-sample branch on kernel.org), so if you'd prefer I post a v3 with
> those I'm quite happy to do so.

I was planning on looking at this today.

No need to post new patches for tags, that's easy to add. I only care about
code changes for new patches.

-- Steve

Re: [PATCH v2 3/3] ftrace: Add sample with custom ops

[Guenter Roeck]

On Tue, Jan 03, 2023 at 12:49:12PM +0000, Mark Rutland wrote:
> When reworking core ftrace code or architectural ftrace code, it's often
> necessary to test/analyse/benchmark a number of ftrace_ops
> configurations. This patch adds a module which can be used to explore
> some of those configurations.
> 
> I'm using this to benchmark various options for changing the way
> trampolines and handling of ftrace_ops work on arm64, and ensuring other
> architectures aren't adversely affected.
> 
> For example, in a QEMU+KVM VM running on a 2GHz Xeon E5-2660
> workstation, loading the module in various configurations produces:
> 
...

> +++ b/samples/ftrace/ftrace-ops.c

...

> +static int __init ftrace_ops_sample_init(void)
> +{
> +	unsigned long flags = 0;
> +	ktime_t start, end;
> +	u64 period;
> +
...
> +
> +	period = ktime_to_ns(ktime_sub(end, start));
> +
> +	pr_info("Attempted %u calls to %ps in %lluns (%lluns / call)\n",
> +		nr_function_calls, tracee_relevant,
> +		period, period / nr_function_calls);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^

Building csky:allmodconfig ... failed
--------------
Error log:
ERROR: modpost: "__udivdi3" [samples/ftrace/ftrace-ops.ko] undefined!

Guenter

Re: [PATCH v2 3/3] ftrace: Add sample with custom ops

[Mark Rutland]

On Mon, Feb 06, 2023 at 02:25:16PM -0800, Guenter Roeck wrote:
> On Tue, Jan 03, 2023 at 12:49:12PM +0000, Mark Rutland wrote:
> > +	pr_info("Attempted %u calls to %ps in %lluns (%lluns / call)\n",
> > +		nr_function_calls, tracee_relevant,
> > +		period, period / nr_function_calls);
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Building csky:allmodconfig ... failed
> --------------
> Error log:
> ERROR: modpost: "__udivdi3" [samples/ftrace/ftrace-ops.ko] undefined!

Arnd sent a fix for this a week ago:

  https://lore.kernel.org/lkml/20230130130246.247537-1-arnd@kernel.org/

I believe Steve has been travelling and hasn't had the chance to pick that up
yet.

Thanks,
Mark.