[PATCH 0/8] x86/ftrace: Add direct batch interface

[PATCH 0/8] x86/ftrace: Add direct batch interface

[Jiri Olsa]

hi,
adding interface to maintain multiple direct functions
within single calls. It's a base for follow up bpf batch
attach functionality.

New interface:

  int register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
  int unregister_ftrace_direct_multi(struct ftrace_ops *ops)
  int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)

that allows to register/unregister/modify direct function 'addr'
with struct ftrace_ops object. The ops filter can be updated
before with ftrace_set_filter_ip calls

  1) patches (1-4) that fix the ftrace graph tracing over the function
     with direct trampolines attached
  2) patches (5-8) that add batch interface for ftrace direct function
     register/unregister/modify

Also available at (based on Steven's ftrace/core branch):
  https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
  ftrace/direct

thanks,
jirka


---
Jiri Olsa (6):
      x86/ftrace: Remove extra orig rax move
      tracing: Add trampoline/graph selftest
      ftrace: Add ftrace_add_rec_direct function
      ftrace: Add multi direct register/unregister interface
      ftrace: Add multi direct modify interface
      ftrace/samples: Add multi direct interface test module

Steven Rostedt (VMware) (2):
      x86/ftrace: Remove fault protection code in prepare_ftrace_return
      x86/ftrace: Make function graph use ftrace directly

 arch/x86/include/asm/ftrace.h        |   9 +++--
 arch/x86/kernel/ftrace.c             |  71 +++++++++++++++++++-------------------
 arch/x86/kernel/ftrace_64.S          |  30 +---------------
 include/linux/ftrace.h               |  26 ++++++++++++++
 kernel/trace/fgraph.c                |   6 ++--
 kernel/trace/ftrace.c                | 214 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------
 kernel/trace/trace_selftest.c        |  49 +++++++++++++++++++++++++-
 samples/ftrace/Makefile              |   1 +
 samples/ftrace/ftrace-direct-multi.c |  52 ++++++++++++++++++++++++++++
 9 files changed, 364 insertions(+), 94 deletions(-)
 create mode 100644 samples/ftrace/ftrace-direct-multi.c

[PATCH 1/8] x86/ftrace: Remove extra orig rax move

[Jiri Olsa]

There's identical move 2 lines earlier.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 arch/x86/kernel/ftrace_64.S | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index 7c273846c687..a8eb084a7a9a 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -251,7 +251,6 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
 	 * If ORIG_RAX is anything but zero, make this a call to that.
 	 * See arch_ftrace_set_direct_caller().
 	 */
-	movq ORIG_RAX(%rsp), %rax
 	testq	%rax, %rax
 SYM_INNER_LABEL(ftrace_regs_caller_jmp, SYM_L_GLOBAL)
 	jnz	1f
-- 
2.31.1

[PATCH 2/8] x86/ftrace: Remove fault protection code in prepare_ftrace_return

[Jiri Olsa]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Removing the fault protection code when writing return_hooker
to stack. As Steven noted:

> That protection was there from the beginning due to being "paranoid",
> considering ftrace was bricking network cards. But that protection
> would not have even protected against that.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 arch/x86/kernel/ftrace.c | 38 +++-----------------------------------
 1 file changed, 3 insertions(+), 35 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 1b3ce3b4a2a2..c555624da989 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -625,12 +625,10 @@ int ftrace_disable_ftrace_graph_caller(void)
  * Hook the return address and push it in the stack of return addrs
  * in current thread info.
  */
-void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
+void prepare_ftrace_return(unsigned long ip, unsigned long *parent,
 			   unsigned long frame_pointer)
 {
 	unsigned long return_hooker = (unsigned long)&return_to_handler;
-	unsigned long old;
-	int faulted;
 
 	/*
 	 * When resuming from suspend-to-ram, this function can be indirectly
@@ -650,37 +648,7 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
 	if (unlikely(atomic_read(&current->tracing_graph_pause)))
 		return;
 
-	/*
-	 * Protect against fault, even if it shouldn't
-	 * happen. This tool is too much intrusive to
-	 * ignore such a protection.
-	 */
-	asm volatile(
-		"1: " _ASM_MOV " (%[parent]), %[old]\n"
-		"2: " _ASM_MOV " %[return_hooker], (%[parent])\n"
-		"   movl $0, %[faulted]\n"
-		"3:\n"
-
-		".section .fixup, \"ax\"\n"
-		"4: movl $1, %[faulted]\n"
-		"   jmp 3b\n"
-		".previous\n"
-
-		_ASM_EXTABLE(1b, 4b)
-		_ASM_EXTABLE(2b, 4b)
-
-		: [old] "=&r" (old), [faulted] "=r" (faulted)
-		: [parent] "r" (parent), [return_hooker] "r" (return_hooker)
-		: "memory"
-	);
-
-	if (unlikely(faulted)) {
-		ftrace_graph_stop();
-		WARN_ON(1);
-		return;
-	}
-
-	if (function_graph_enter(old, self_addr, frame_pointer, parent))
-		*parent = old;
+	if (!function_graph_enter(*parent, ip, frame_pointer, parent))
+		*parent = return_hooker;
 }
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
-- 
2.31.1

[PATCH 3/8] x86/ftrace: Make function graph use ftrace directly

[Jiri Olsa]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

We don't need special hook for graph tracer entry point,
but instead we can use graph_ops::func function to install
the return_hooker.

This moves the graph tracing setup _before_ the direct
trampoline prepares the stack, so the return_hooker will
be called when the direct trampoline is finished.

This simplifies the code, because we don't need to take into
account the direct trampoline setup when preparing the graph
tracer hooker and we can allow function graph tracer on entries
registered with direct trampoline.

[fixed compile error reported by kernel test robot <lkp@intel.com>]
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 arch/x86/include/asm/ftrace.h |  9 +++++++--
 arch/x86/kernel/ftrace.c      | 37 ++++++++++++++++++++++++++++++++---
 arch/x86/kernel/ftrace_64.S   | 29 +--------------------------
 include/linux/ftrace.h        |  9 +++++++++
 kernel/trace/fgraph.c         |  6 ++++--
 5 files changed, 55 insertions(+), 35 deletions(-)

diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index 9f3130f40807..024d9797646e 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -57,6 +57,13 @@ arch_ftrace_get_regs(struct ftrace_regs *fregs)
 
 #define ftrace_instruction_pointer_set(fregs, _ip)	\
 	do { (fregs)->regs.ip = (_ip); } while (0)
+
+struct ftrace_ops;
+#define ftrace_graph_func ftrace_graph_func
+void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
+		       struct ftrace_ops *op, struct ftrace_regs *fregs);
+#else
+#define FTRACE_GRAPH_TRAMP_ADDR FTRACE_GRAPH_ADDR
 #endif
 
 #ifdef CONFIG_DYNAMIC_FTRACE
@@ -65,8 +72,6 @@ struct dyn_arch_ftrace {
 	/* No extra data needed for x86 */
 };
 
-#define FTRACE_GRAPH_TRAMP_ADDR FTRACE_GRAPH_ADDR
-
 #endif /*  CONFIG_DYNAMIC_FTRACE */
 #endif /* __ASSEMBLY__ */
 #endif /* CONFIG_FUNCTION_TRACER */
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index c555624da989..804fcc6ef2c7 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -527,7 +527,7 @@ static void *addr_from_call(void *ptr)
 	return ptr + CALL_INSN_SIZE + call.disp;
 }
 
-void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
+void prepare_ftrace_return(unsigned long ip, unsigned long *parent,
 			   unsigned long frame_pointer);
 
 /*
@@ -541,7 +541,8 @@ static void *static_tramp_func(struct ftrace_ops *ops, struct dyn_ftrace *rec)
 	void *ptr;
 
 	if (ops && ops->trampoline) {
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+#if !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS) && \
+	defined(CONFIG_FUNCTION_GRAPH_TRACER)
 		/*
 		 * We only know about function graph tracer setting as static
 		 * trampoline.
@@ -589,8 +590,9 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops)
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 
 #ifdef CONFIG_DYNAMIC_FTRACE
-extern void ftrace_graph_call(void);
 
+#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
+extern void ftrace_graph_call(void);
 static const char *ftrace_jmp_replace(unsigned long ip, unsigned long addr)
 {
 	return text_gen_insn(JMP32_INSN_OPCODE, (void *)ip, (void *)addr);
@@ -618,7 +620,17 @@ int ftrace_disable_ftrace_graph_caller(void)
 
 	return ftrace_mod_jmp(ip, &ftrace_stub);
 }
+#else /* !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
+int ftrace_enable_ftrace_graph_caller(void)
+{
+	return 0;
+}
 
+int ftrace_disable_ftrace_graph_caller(void)
+{
+	return 0;
+}
+#endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
 #endif /* !CONFIG_DYNAMIC_FTRACE */
 
 /*
@@ -629,6 +641,7 @@ void prepare_ftrace_return(unsigned long ip, unsigned long *parent,
 			   unsigned long frame_pointer)
 {
 	unsigned long return_hooker = (unsigned long)&return_to_handler;
+	int bit;
 
 	/*
 	 * When resuming from suspend-to-ram, this function can be indirectly
@@ -648,7 +661,25 @@ void prepare_ftrace_return(unsigned long ip, unsigned long *parent,
 	if (unlikely(atomic_read(&current->tracing_graph_pause)))
 		return;
 
+	bit = ftrace_test_recursion_trylock(ip, *parent);
+	if (bit < 0)
+		return;
+
 	if (!function_graph_enter(*parent, ip, frame_pointer, parent))
 		*parent = return_hooker;
+
+	ftrace_test_recursion_unlock(bit);
+}
+
+#ifdef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
+void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
+		       struct ftrace_ops *op, struct ftrace_regs *fregs)
+{
+	struct pt_regs *regs = &fregs->regs;
+	unsigned long *stack = (unsigned long *)kernel_stack_pointer(regs);
+
+	prepare_ftrace_return(ip, (unsigned long *)stack, 0);
 }
+#endif
+
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index a8eb084a7a9a..7a879901f103 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -174,11 +174,6 @@ SYM_INNER_LABEL(ftrace_caller_end, SYM_L_GLOBAL)
 SYM_FUNC_END(ftrace_caller);
 
 SYM_FUNC_START(ftrace_epilogue)
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL)
-	jmp ftrace_stub
-#endif
-
 /*
  * This is weak to keep gas from relaxing the jumps.
  * It is also used to copy the retq for trampolines.
@@ -288,15 +283,6 @@ SYM_FUNC_START(__fentry__)
 	cmpq $ftrace_stub, ftrace_trace_function
 	jnz trace
 
-fgraph_trace:
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-	cmpq $ftrace_stub, ftrace_graph_return
-	jnz ftrace_graph_caller
-
-	cmpq $ftrace_graph_entry_stub, ftrace_graph_entry
-	jnz ftrace_graph_caller
-#endif
-
 SYM_INNER_LABEL(ftrace_stub, SYM_L_GLOBAL)
 	retq
 
@@ -314,25 +300,12 @@ trace:
 	CALL_NOSPEC r8
 	restore_mcount_regs
 
-	jmp fgraph_trace
+	jmp ftrace_stub
 SYM_FUNC_END(__fentry__)
 EXPORT_SYMBOL(__fentry__)
 #endif /* CONFIG_DYNAMIC_FTRACE */
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
-SYM_FUNC_START(ftrace_graph_caller)
-	/* Saves rbp into %rdx and fills first parameter  */
-	save_mcount_regs
-
-	leaq MCOUNT_REG_SIZE+8(%rsp), %rsi
-	movq $0, %rdx	/* No framepointers needed */
-	call	prepare_ftrace_return
-
-	restore_mcount_regs
-
-	retq
-SYM_FUNC_END(ftrace_graph_caller)
-
 SYM_FUNC_START(return_to_handler)
 	subq  $24, %rsp
 
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index a69f363b61bf..d399621a67ee 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -779,6 +779,15 @@ static inline bool is_ftrace_trampoline(unsigned long addr)
 }
 #endif /* CONFIG_DYNAMIC_FTRACE */
 
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+#ifndef ftrace_graph_func
+#define ftrace_graph_func ftrace_stub
+#define FTRACE_OPS_GRAPH_STUB FTRACE_OPS_FL_STUB
+#else
+#define FTRACE_OPS_GRAPH_STUB 0
+#endif
+#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
+
 /* totally disable ftrace - can not re-enable after this */
 void ftrace_kill(void);
 
diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
index b8a0d1d564fb..22061d38fc00 100644
--- a/kernel/trace/fgraph.c
+++ b/kernel/trace/fgraph.c
@@ -115,6 +115,7 @@ int function_graph_enter(unsigned long ret, unsigned long func,
 {
 	struct ftrace_graph_ent trace;
 
+#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
 	/*
 	 * Skip graph tracing if the return location is served by direct trampoline,
 	 * since call sequence and return addresses are unpredictable anyway.
@@ -124,6 +125,7 @@ int function_graph_enter(unsigned long ret, unsigned long func,
 	if (ftrace_direct_func_count &&
 	    ftrace_find_rec_direct(ret - MCOUNT_INSN_SIZE))
 		return -EBUSY;
+#endif
 	trace.func = func;
 	trace.depth = ++current->curr_ret_depth;
 
@@ -333,10 +335,10 @@ unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx,
 #endif /* HAVE_FUNCTION_GRAPH_RET_ADDR_PTR */
 
 static struct ftrace_ops graph_ops = {
-	.func			= ftrace_stub,
+	.func			= ftrace_graph_func,
 	.flags			= FTRACE_OPS_FL_INITIALIZED |
 				   FTRACE_OPS_FL_PID |
-				   FTRACE_OPS_FL_STUB,
+				   FTRACE_OPS_GRAPH_STUB,
 #ifdef FTRACE_GRAPH_TRAMP_ADDR
 	.trampoline		= FTRACE_GRAPH_TRAMP_ADDR,
 	/* trampoline_size is only needed for dynamically allocated tramps */
-- 
2.31.1

[PATCH 4/8] tracing: Add trampoline/graph selftest

[Jiri Olsa]

Adding selftest for checking that direct trampoline can
co-exist together with graph tracer on same function.

This is supported for CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
config option, which is defined only for x86_64 for now.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/trace/trace_selftest.c | 49 ++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c
index adf7ef194005..f8e55b949cdd 100644
--- a/kernel/trace/trace_selftest.c
+++ b/kernel/trace/trace_selftest.c
@@ -750,6 +750,8 @@ static struct fgraph_ops fgraph_ops __initdata  = {
 	.retfunc		= &trace_graph_return,
 };
 
+noinline __noclone static void trace_direct_tramp(void) { }
+
 /*
  * Pretty much the same than for the function tracer from which the selftest
  * has been borrowed.
@@ -760,6 +762,7 @@ trace_selftest_startup_function_graph(struct tracer *trace,
 {
 	int ret;
 	unsigned long count;
+	char *func_name __maybe_unused;
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 	if (ftrace_filter_param) {
@@ -808,8 +811,52 @@ trace_selftest_startup_function_graph(struct tracer *trace,
 		goto out;
 	}
 
-	/* Don't test dynamic tracing, the function tracer already did */
+#ifdef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
+	tracing_reset_online_cpus(&tr->array_buffer);
+	set_graph_array(tr);
 
+	/*
+	 * Some archs *cough*PowerPC*cough* add characters to the
+	 * start of the function names. We simply put a '*' to
+	 * accommodate them.
+	 */
+	func_name = "*" __stringify(DYN_FTRACE_TEST_NAME);
+	ftrace_set_global_filter(func_name, strlen(func_name), 1);
+
+	/*
+	 * 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);
+	if (ret)
+		goto out;
+
+	ret = register_ftrace_graph(&fgraph_ops);
+	if (ret) {
+		warn_failed_init_tracer(trace, ret);
+		goto out;
+	}
+
+	DYN_FTRACE_TEST_NAME();
+
+	count = 0;
+
+	tracing_stop();
+	/* check the trace buffer */
+	ret = trace_test_buffer(&tr->array_buffer, &count);
+
+	unregister_ftrace_graph(&fgraph_ops);
+
+	tracing_start();
+
+	if (!ret && !count) {
+		ret = -1;
+		goto out;
+	}
+#endif
+
+	/* Don't test dynamic tracing, the function tracer already did */
 out:
 	/* Stop it if we failed */
 	if (ret)
-- 
2.31.1

[PATCH 5/8] ftrace: Add ftrace_add_rec_direct function

[Jiri Olsa]

Factor out the code that adds (ip, addr) tuple to direct_functions
hash in new ftrace_add_rec_direct function. It will be used in
following patches.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/trace/ftrace.c | 60 ++++++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 24 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 7b180f61e6d3..c60217d81040 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2394,6 +2394,39 @@ unsigned long ftrace_find_rec_direct(unsigned long ip)
 	return entry->direct;
 }
 
+static struct ftrace_func_entry*
+ftrace_add_rec_direct(unsigned long ip, unsigned long addr,
+		      struct ftrace_hash **free_hash)
+{
+	struct ftrace_func_entry *entry;
+
+	if (ftrace_hash_empty(direct_functions) ||
+	    direct_functions->count > 2 * (1 << direct_functions->size_bits)) {
+		struct ftrace_hash *new_hash;
+		int size = ftrace_hash_empty(direct_functions) ? 0 :
+			direct_functions->count + 1;
+
+		if (size < 32)
+			size = 32;
+
+		new_hash = dup_hash(direct_functions, size);
+		if (!new_hash)
+			return NULL;
+
+		*free_hash = direct_functions;
+		direct_functions = new_hash;
+	}
+
+	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return NULL;
+
+	entry->ip = ip;
+	entry->direct = addr;
+	__add_hash_entry(direct_functions, entry);
+	return entry;
+}
+
 static void call_direct_funcs(unsigned long ip, unsigned long pip,
 			      struct ftrace_ops *ops, struct ftrace_regs *fregs)
 {
@@ -5110,27 +5143,6 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)
 	}
 
 	ret = -ENOMEM;
-	if (ftrace_hash_empty(direct_functions) ||
-	    direct_functions->count > 2 * (1 << direct_functions->size_bits)) {
-		struct ftrace_hash *new_hash;
-		int size = ftrace_hash_empty(direct_functions) ? 0 :
-			direct_functions->count + 1;
-
-		if (size < 32)
-			size = 32;
-
-		new_hash = dup_hash(direct_functions, size);
-		if (!new_hash)
-			goto out_unlock;
-
-		free_hash = direct_functions;
-		direct_functions = new_hash;
-	}
-
-	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
-	if (!entry)
-		goto out_unlock;
-
 	direct = ftrace_find_direct_func(addr);
 	if (!direct) {
 		direct = ftrace_alloc_direct_func(addr);
@@ -5140,9 +5152,9 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)
 		}
 	}
 
-	entry->ip = ip;
-	entry->direct = addr;
-	__add_hash_entry(direct_functions, entry);
+	entry = ftrace_add_rec_direct(ip, addr, &free_hash);
+	if (!entry)
+		goto out_unlock;
 
 	ret = ftrace_set_filter_ip(&direct_ops, ip, 0, 0);
 	if (ret)
-- 
2.31.1

[PATCH 6/8] ftrace: Add multi direct register/unregister interface

[Jiri Olsa]

Adding interface to register multiple direct functions
within single call. Adding following functions:

  register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
  unregister_ftrace_direct_multi(struct ftrace_ops *ops)

The register_ftrace_direct_multi registers direct function (addr)
with all functions in ops filter. The ops filter can be updated
before with ftrace_set_filter_ip calls.

All requested functions must not have direct function currently
registered, otherwise register_ftrace_direct_multi will fail.

The unregister_ftrace_direct_multi unregisters ops related direct
functions.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/ftrace.h |  11 ++++
 kernel/trace/ftrace.c  | 111 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 122 insertions(+)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index d399621a67ee..e40b5201c16e 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -316,7 +316,10 @@ int ftrace_modify_direct_caller(struct ftrace_func_entry *entry,
 				unsigned long old_addr,
 				unsigned long new_addr);
 unsigned long ftrace_find_rec_direct(unsigned long ip);
+int register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr);
+int unregister_ftrace_direct_multi(struct ftrace_ops *ops);
 #else
+struct ftrace_ops;
 # define ftrace_direct_func_count 0
 static inline int register_ftrace_direct(unsigned long ip, unsigned long addr)
 {
@@ -346,6 +349,14 @@ static inline unsigned long ftrace_find_rec_direct(unsigned long ip)
 {
 	return 0;
 }
+static inline int register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
+{
+	return -ENODEV;
+}
+static inline int unregister_ftrace_direct_multi(struct ftrace_ops *ops)
+{
+	return -ENODEV;
+}
 #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
 
 #ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index c60217d81040..7243769493c9 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5407,6 +5407,117 @@ int modify_ftrace_direct(unsigned long ip,
 	return ret;
 }
 EXPORT_SYMBOL_GPL(modify_ftrace_direct);
+
+#define MULTI_FLAGS (FTRACE_OPS_FL_IPMODIFY | FTRACE_OPS_FL_DIRECT | \
+		     FTRACE_OPS_FL_SAVE_REGS)
+
+static int check_direct_multi(struct ftrace_ops *ops)
+{
+	if (!(ops->flags & FTRACE_OPS_FL_INITIALIZED))
+		return -EINVAL;
+	if ((ops->flags & MULTI_FLAGS) != MULTI_FLAGS)
+		return -EINVAL;
+	return 0;
+}
+
+int register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
+{
+	struct ftrace_hash *hash, *free_hash = NULL;
+	struct ftrace_func_entry *entry, *new;
+	int err = -EBUSY, size, i;
+
+	if (ops->func || ops->trampoline)
+		return -EINVAL;
+	if (!(ops->flags & FTRACE_OPS_FL_INITIALIZED))
+		return -EINVAL;
+	if (ops->flags & FTRACE_OPS_FL_ENABLED)
+		return -EINVAL;
+
+	hash = ops->func_hash->filter_hash;
+	if (ftrace_hash_empty(hash))
+		return -EINVAL;
+
+	mutex_lock(&direct_mutex);
+
+	/* Make sure requested entries are not already registered.. */
+	size = 1 << hash->size_bits;
+	for (i = 0; i < size; i++) {
+		hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
+			if (ftrace_find_rec_direct(entry->ip))
+				goto out_unlock;
+		}
+	}
+
+	/* ... and insert them to direct_functions hash. */
+	err = -ENOMEM;
+	for (i = 0; i < size; i++) {
+		hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
+			new = ftrace_add_rec_direct(entry->ip, addr, &free_hash);
+			if (!new)
+				goto out_remove;
+			entry->direct = addr;
+		}
+	}
+
+	ops->func = call_direct_funcs;
+	ops->flags = MULTI_FLAGS;
+	ops->trampoline = FTRACE_REGS_ADDR;
+
+	err = register_ftrace_function(ops);
+
+ out_remove:
+	if (err) {
+		for (i = 0; i < size; i++) {
+			hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
+				new = __ftrace_lookup_ip(direct_functions, entry->ip);
+				if (new) {
+					remove_hash_entry(direct_functions, new);
+					kfree(new);
+				}
+			}
+		}
+	}
+
+ out_unlock:
+	mutex_unlock(&direct_mutex);
+
+	if (free_hash) {
+		synchronize_rcu_tasks();
+		free_ftrace_hash(free_hash);
+	}
+	return err;
+}
+EXPORT_SYMBOL_GPL(register_ftrace_direct_multi);
+
+int unregister_ftrace_direct_multi(struct ftrace_ops *ops)
+{
+	struct ftrace_hash *hash = ops->func_hash->filter_hash;
+	struct ftrace_func_entry *entry, *new;
+	int err, size, i;
+
+	if (check_direct_multi(ops))
+		return -EINVAL;
+	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
+		return -EINVAL;
+
+	mutex_lock(&direct_mutex);
+	err = unregister_ftrace_function(ops);
+
+	size = 1 << hash->size_bits;
+	for (i = 0; i < size; i++) {
+		hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
+			new = __ftrace_lookup_ip(direct_functions, entry->ip);
+			if (new) {
+				remove_hash_entry(direct_functions, new);
+				kfree(new);
+			}
+		}
+	}
+
+	mutex_unlock(&direct_mutex);
+	return err;
+}
+EXPORT_SYMBOL_GPL(unregister_ftrace_direct_multi);
 #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
 
 /**
-- 
2.31.1

[PATCH 7/8] ftrace: Add multi direct modify interface

[Jiri Olsa]

Adding interface to modify registered direct function
for ftrace_ops. Adding following function:

   modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)

The function changes the currently registered direct
function for all attached functions.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/ftrace.h |  6 ++++++
 kernel/trace/ftrace.c  | 43 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index e40b5201c16e..f3ba6366f7af 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -318,6 +318,8 @@ int ftrace_modify_direct_caller(struct ftrace_func_entry *entry,
 unsigned long ftrace_find_rec_direct(unsigned long ip);
 int register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr);
 int unregister_ftrace_direct_multi(struct ftrace_ops *ops);
+int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr);
+
 #else
 struct ftrace_ops;
 # define ftrace_direct_func_count 0
@@ -357,6 +359,10 @@ static inline int unregister_ftrace_direct_multi(struct ftrace_ops *ops)
 {
 	return -ENODEV;
 }
+static inline int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
+{
+	return -ENODEV;
+}
 #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
 
 #ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 7243769493c9..59940a6a907c 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5518,6 +5518,49 @@ int unregister_ftrace_direct_multi(struct ftrace_ops *ops)
 	return err;
 }
 EXPORT_SYMBOL_GPL(unregister_ftrace_direct_multi);
+
+int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
+{
+	struct ftrace_hash *hash = ops->func_hash->filter_hash;
+	struct ftrace_func_entry *entry, *iter;
+	int i, size;
+	int err;
+
+	if (check_direct_multi(ops))
+		return -EINVAL;
+	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
+		return -EINVAL;
+
+	mutex_lock(&direct_mutex);
+	mutex_lock(&ftrace_lock);
+
+	/*
+	 * Shutdown the ops, change 'direct' pointer for each
+	 * ops entry in direct_functions hash and startup the
+	 * ops back again.
+	 */
+	err = ftrace_shutdown(ops, 0);
+	if (err)
+		goto out_unlock;
+
+	size = 1 << hash->size_bits;
+	for (i = 0; i < size; i++) {
+		hlist_for_each_entry(iter, &hash->buckets[i], hlist) {
+			entry = __ftrace_lookup_ip(direct_functions, iter->ip);
+			if (!entry)
+				continue;
+			entry->direct = addr;
+		}
+	}
+
+	err = ftrace_startup(ops, 0);
+
+ out_unlock:
+	mutex_unlock(&ftrace_lock);
+	mutex_unlock(&direct_mutex);
+	return err;
+}
+EXPORT_SYMBOL_GPL(modify_ftrace_direct_multi);
 #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
 
 /**
-- 
2.31.1

[PATCH 8/8] ftrace/samples: Add multi direct interface test module

[Jiri Olsa]

Adding simple module that uses multi direct interface:

  register_ftrace_direct_multi
  unregister_ftrace_direct_multi

The init function registers trampoline for 2 functions,
and exit function unregisters them.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 samples/ftrace/Makefile              |  1 +
 samples/ftrace/ftrace-direct-multi.c | 52 ++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)
 create mode 100644 samples/ftrace/ftrace-direct-multi.c

diff --git a/samples/ftrace/Makefile b/samples/ftrace/Makefile
index 4ce896e10b2e..ab1d1c05c288 100644
--- a/samples/ftrace/Makefile
+++ b/samples/ftrace/Makefile
@@ -3,6 +3,7 @@
 obj-$(CONFIG_SAMPLE_FTRACE_DIRECT) += ftrace-direct.o
 obj-$(CONFIG_SAMPLE_FTRACE_DIRECT) += ftrace-direct-too.o
 obj-$(CONFIG_SAMPLE_FTRACE_DIRECT) += ftrace-direct-modify.o
+obj-$(CONFIG_SAMPLE_FTRACE_DIRECT) += ftrace-direct-multi.o
 
 CFLAGS_sample-trace-array.o := -I$(src)
 obj-$(CONFIG_SAMPLE_TRACE_ARRAY) += sample-trace-array.o
diff --git a/samples/ftrace/ftrace-direct-multi.c b/samples/ftrace/ftrace-direct-multi.c
new file mode 100644
index 000000000000..76b34d46d11c
--- /dev/null
+++ b/samples/ftrace/ftrace-direct-multi.c
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/module.h>
+
+#include <linux/mm.h> /* for handle_mm_fault() */
+#include <linux/ftrace.h>
+#include <linux/sched/stat.h>
+
+void my_direct_func(unsigned long ip)
+{
+	trace_printk("ip %lx\n", ip);
+}
+
+extern void my_tramp(void *);
+
+asm (
+"	.pushsection    .text, \"ax\", @progbits\n"
+"	.type		my_tramp, @function\n"
+"	.globl		my_tramp\n"
+"   my_tramp:"
+"	pushq %rbp\n"
+"	movq %rsp, %rbp\n"
+"	pushq %rdi\n"
+"	movq 8(%rbp), %rdi\n"
+"	call my_direct_func\n"
+"	popq %rdi\n"
+"	leave\n"
+"	ret\n"
+"	.size		my_tramp, .-my_tramp\n"
+"	.popsection\n"
+);
+
+static struct ftrace_ops direct;
+
+static int __init ftrace_direct_multi_init(void)
+{
+	ftrace_set_filter_ip(&direct, (unsigned long) wake_up_process, 0, 0);
+	ftrace_set_filter_ip(&direct, (unsigned long) schedule, 0, 0);
+
+	return register_ftrace_direct_multi(&direct, (unsigned long) my_tramp);
+}
+
+static void __exit ftrace_direct_multi_exit(void)
+{
+	unregister_ftrace_direct_multi(&direct);
+}
+
+module_init(ftrace_direct_multi_init);
+module_exit(ftrace_direct_multi_exit);
+
+MODULE_AUTHOR("Jiri Olsa");
+MODULE_DESCRIPTION("Example use case of using register_ftrace_direct_multi()");
+MODULE_LICENSE("GPL");
-- 
2.31.1

Re: [PATCH 0/8] x86/ftrace: Add direct batch interface

[Alexei Starovoitov]

On Tue, Aug 31, 2021 at 2:50 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> hi,
> adding interface to maintain multiple direct functions
> within single calls. It's a base for follow up bpf batch
> attach functionality.
>
> New interface:
>
>   int register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
>   int unregister_ftrace_direct_multi(struct ftrace_ops *ops)
>   int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
>
> that allows to register/unregister/modify direct function 'addr'
> with struct ftrace_ops object. The ops filter can be updated
> before with ftrace_set_filter_ip calls
>
>   1) patches (1-4) that fix the ftrace graph tracing over the function
>      with direct trampolines attached
>   2) patches (5-8) that add batch interface for ftrace direct function
>      register/unregister/modify
>
> Also available at (based on Steven's ftrace/core branch):
>   https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
>   ftrace/direct

Steven,

Could you review and merge this set for this merge window,
so we can process related bpf bits for the next cycle?

Re: [PATCH 0/8] x86/ftrace: Add direct batch interface

[Jiri Olsa]

On Wed, Sep 01, 2021 at 08:23:38AM -0700, Alexei Starovoitov wrote:
> On Tue, Aug 31, 2021 at 2:50 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > hi,
> > adding interface to maintain multiple direct functions
> > within single calls. It's a base for follow up bpf batch
> > attach functionality.
> >
> > New interface:
> >
> >   int register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
> >   int unregister_ftrace_direct_multi(struct ftrace_ops *ops)
> >   int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
> >
> > that allows to register/unregister/modify direct function 'addr'
> > with struct ftrace_ops object. The ops filter can be updated
> > before with ftrace_set_filter_ip calls
> >
> >   1) patches (1-4) that fix the ftrace graph tracing over the function
> >      with direct trampolines attached
> >   2) patches (5-8) that add batch interface for ftrace direct function
> >      register/unregister/modify
> >
> > Also available at (based on Steven's ftrace/core branch):
> >   https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> >   ftrace/direct
> 
> Steven,
> 
> Could you review and merge this set for this merge window,
> so we can process related bpf bits for the next cycle?

actually I might have sent it out too early, there's still
bpf part review discussion that might end up in interface
change

review would be great, but please hold on with the merge

thanks,
jirka

Re: [PATCH 0/8] x86/ftrace: Add direct batch interface

[Steven Rostedt]

On Wed, 1 Sep 2021 21:06:45 +0200 Jiri Olsa <jolsa@redhat.com> wrote:
> On Wed, Sep 01, 2021 at 08:23:38AM -0700, Alexei Starovoitov wrote:

> > Steven,
> > 
> > Could you review and merge this set for this merge window,
> > so we can process related bpf bits for the next cycle?  
> 
> actually I might have sent it out too early, there's still
> bpf part review discussion that might end up in interface
> change

Regardless, it is way too late to apply this to the current merge
window. I don't think Linus would appreciate me pulling in a complex
patch set 4 days into the merge window, review it, and then push it to
him without much faith that this wont cause any major issues in use
cases we did not think about. Not to mention, I would have to drop
everything I am responsible for to do that.

I would never ask another maintainer to do such an irresponsible act.

> 
> review would be great, but please hold on with the merge

I just got back from an 8 day vacation, and my inbox is way out of
hand, and I still need to put together the changes that have been in
linux-next for some time, and get that to Linus.

Hopefully I'll get to looking at this sometime next week.

-- Steve

Re: [PATCH 6/8] ftrace: Add multi direct register/unregister interface

[Steven Rostedt]

On Tue, 31 Aug 2021 11:50:15 +0200
Jiri Olsa <jolsa@redhat.com> wrote:

> Adding interface to register multiple direct functions
> within single call. Adding following functions:
> 
>   register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
>   unregister_ftrace_direct_multi(struct ftrace_ops *ops)
> 
> The register_ftrace_direct_multi registers direct function (addr)
> with all functions in ops filter. The ops filter can be updated
> before with ftrace_set_filter_ip calls.
> 
> All requested functions must not have direct function currently
> registered, otherwise register_ftrace_direct_multi will fail.
> 
> The unregister_ftrace_direct_multi unregisters ops related direct
> functions.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/linux/ftrace.h |  11 ++++
>  kernel/trace/ftrace.c  | 111 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 122 insertions(+)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index d399621a67ee..e40b5201c16e 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -316,7 +316,10 @@ int ftrace_modify_direct_caller(struct ftrace_func_entry *entry,
>  				unsigned long old_addr,
>  				unsigned long new_addr);
>  unsigned long ftrace_find_rec_direct(unsigned long ip);
> +int register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr);
> +int unregister_ftrace_direct_multi(struct ftrace_ops *ops);
>  #else
> +struct ftrace_ops;
>  # define ftrace_direct_func_count 0
>  static inline int register_ftrace_direct(unsigned long ip, unsigned long addr)
>  {
> @@ -346,6 +349,14 @@ static inline unsigned long ftrace_find_rec_direct(unsigned long ip)
>  {
>  	return 0;
>  }
> +static inline int register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
> +{
> +	return -ENODEV;
> +}
> +static inline int unregister_ftrace_direct_multi(struct ftrace_ops *ops)
> +{
> +	return -ENODEV;
> +}
>  #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
>  
>  #ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index c60217d81040..7243769493c9 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -5407,6 +5407,117 @@ int modify_ftrace_direct(unsigned long ip,
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(modify_ftrace_direct);
> +
> +#define MULTI_FLAGS (FTRACE_OPS_FL_IPMODIFY | FTRACE_OPS_FL_DIRECT | \
> +		     FTRACE_OPS_FL_SAVE_REGS)
> +
> +static int check_direct_multi(struct ftrace_ops *ops)
> +{
> +	if (!(ops->flags & FTRACE_OPS_FL_INITIALIZED))
> +		return -EINVAL;
> +	if ((ops->flags & MULTI_FLAGS) != MULTI_FLAGS)
> +		return -EINVAL;
> +	return 0;
> +}
> +

Needs kernel doc comments as this is an interface outside this file.

> +int register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
> +{
> +	struct ftrace_hash *hash, *free_hash = NULL;
> +	struct ftrace_func_entry *entry, *new;
> +	int err = -EBUSY, size, i;
> +
> +	if (ops->func || ops->trampoline)
> +		return -EINVAL;
> +	if (!(ops->flags & FTRACE_OPS_FL_INITIALIZED))
> +		return -EINVAL;
> +	if (ops->flags & FTRACE_OPS_FL_ENABLED)
> +		return -EINVAL;
> +
> +	hash = ops->func_hash->filter_hash;
> +	if (ftrace_hash_empty(hash))
> +		return -EINVAL;
> +
> +	mutex_lock(&direct_mutex);
> +
> +	/* Make sure requested entries are not already registered.. */
> +	size = 1 << hash->size_bits;
> +	for (i = 0; i < size; i++) {
> +		hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
> +			if (ftrace_find_rec_direct(entry->ip))
> +				goto out_unlock;
> +		}
> +	}
> +
> +	/* ... and insert them to direct_functions hash. */
> +	err = -ENOMEM;
> +	for (i = 0; i < size; i++) {
> +		hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
> +			new = ftrace_add_rec_direct(entry->ip, addr, &free_hash);
> +			if (!new)
> +				goto out_remove;
> +			entry->direct = addr;
> +		}
> +	}
> +
> +	ops->func = call_direct_funcs;
> +	ops->flags = MULTI_FLAGS;
> +	ops->trampoline = FTRACE_REGS_ADDR;
> +
> +	err = register_ftrace_function(ops);
> +
> + out_remove:
> +	if (err) {

The below code:

> +		for (i = 0; i < size; i++) {
> +			hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
> +				new = __ftrace_lookup_ip(direct_functions, entry->ip);
> +				if (new) {
> +					remove_hash_entry(direct_functions, new);
> +					kfree(new);
> +				}
> +			}
> +		}

is identical to code below.

> +	}
> +
> + out_unlock:
> +	mutex_unlock(&direct_mutex);
> +
> +	if (free_hash) {
> +		synchronize_rcu_tasks();
> +		free_ftrace_hash(free_hash);
> +	}
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(register_ftrace_direct_multi);
> +

Should have kernel doc as well.

> +int unregister_ftrace_direct_multi(struct ftrace_ops *ops)
> +{
> +	struct ftrace_hash *hash = ops->func_hash->filter_hash;
> +	struct ftrace_func_entry *entry, *new;
> +	int err, size, i;
> +
> +	if (check_direct_multi(ops))
> +		return -EINVAL;
> +	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
> +		return -EINVAL;
> +
> +	mutex_lock(&direct_mutex);
> +	err = unregister_ftrace_function(ops);
> +
> +	size = 1 << hash->size_bits;


> +	for (i = 0; i < size; i++) {
> +		hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
> +			new = __ftrace_lookup_ip(direct_functions, entry->ip);
> +			if (new) {
> +				remove_hash_entry(direct_functions, new);
> +				kfree(new);
> +			}
> +		}
> +	}

Would probably make sense to turn this into a static inline helper.

-- Steve


> +
> +	mutex_unlock(&direct_mutex);
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(unregister_ftrace_direct_multi);
>  #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
>  
>  /**

Re: [PATCH 7/8] ftrace: Add multi direct modify interface

[Steven Rostedt]

On Tue, 31 Aug 2021 11:50:16 +0200
Jiri Olsa <jolsa@redhat.com> wrote:

> Adding interface to modify registered direct function
> for ftrace_ops. Adding following function:
> 
>    modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
> 
> The function changes the currently registered direct
> function for all attached functions.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/linux/ftrace.h |  6 ++++++
>  kernel/trace/ftrace.c  | 43 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index e40b5201c16e..f3ba6366f7af 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -318,6 +318,8 @@ int ftrace_modify_direct_caller(struct ftrace_func_entry *entry,
>  unsigned long ftrace_find_rec_direct(unsigned long ip);
>  int register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr);
>  int unregister_ftrace_direct_multi(struct ftrace_ops *ops);
> +int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr);
> +
>  #else
>  struct ftrace_ops;
>  # define ftrace_direct_func_count 0
> @@ -357,6 +359,10 @@ static inline int unregister_ftrace_direct_multi(struct ftrace_ops *ops)
>  {
>  	return -ENODEV;
>  }
> +static inline int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
> +{
> +	return -ENODEV;
> +}
>  #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
>  
>  #ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 7243769493c9..59940a6a907c 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -5518,6 +5518,49 @@ int unregister_ftrace_direct_multi(struct ftrace_ops *ops)
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(unregister_ftrace_direct_multi);
> +

Needs kernel doc comments.

> +int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
> +{
> +	struct ftrace_hash *hash = ops->func_hash->filter_hash;
> +	struct ftrace_func_entry *entry, *iter;
> +	int i, size;
> +	int err;
> +
> +	if (check_direct_multi(ops))
> +		return -EINVAL;
> +	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
> +		return -EINVAL;
> +
> +	mutex_lock(&direct_mutex);
> +	mutex_lock(&ftrace_lock);
> +
> +	/*
> +	 * Shutdown the ops, change 'direct' pointer for each
> +	 * ops entry in direct_functions hash and startup the
> +	 * ops back again.
> +	 */
> +	err = ftrace_shutdown(ops, 0);

This needs to be commented that there's going to be a rather large time
frame that there will be no callbacks happening while this update occurs.

A better solution, that prevents having to do this, is to first change
the function fentry's to call the ftrace list loop function, that calls
the ftrace_ops list, and will call the direct call via the ops in the
loop. Have the ops->func call the new direct function (all will be
immediately affected). Update the entries, and then switch from the
loop back to the direct caller.

-- Steve



> +	if (err)
> +		goto out_unlock;
> +
> +	size = 1 << hash->size_bits;
> +	for (i = 0; i < size; i++) {
> +		hlist_for_each_entry(iter, &hash->buckets[i], hlist) {
> +			entry = __ftrace_lookup_ip(direct_functions, iter->ip);
> +			if (!entry)
> +				continue;
> +			entry->direct = addr;
> +		}
> +	}
> +
> +	err = ftrace_startup(ops, 0);
> +
> + out_unlock:
> +	mutex_unlock(&ftrace_lock);
> +	mutex_unlock(&direct_mutex);
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(modify_ftrace_direct_multi);
>  #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
>  
>  /**

Re: [PATCH 7/8] ftrace: Add multi direct modify interface

[Steven Rostedt]

On Tue, 14 Sep 2021 17:41:34 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> A better solution, that prevents having to do this, is to first change
> the function fentry's to call the ftrace list loop function, that calls
> the ftrace_ops list, and will call the direct call via the ops in the
> loop. Have the ops->func call the new direct function (all will be
> immediately affected). Update the entries, and then switch from the
> loop back to the direct caller.

An easy way to force the loop function to be called instead of the direct
trampoline, is to register a stub ftrace_ops to each of the functions that
the direct function attaches to. You can even share the hash in doing so.

Having the ftrace_ops attached in the same locations as the direct
trampoline, will force the loop function to be called (to call the stub
ftrace_ops as well as the direct trampoline ftrace_ops helper).

Then change the direct trampoline address, which will have the ftrace_ops
helper use that direct trampoline immediately*. Then when you remove the
ftrace_ops stub, it will update all the call sites to call the new direct
trampoline directly.

(*) not quite immediately, as there's no read memory barrier with the
direct helper, so it may still be calling the old trampoline. But this
shouldn't be an issue. If it is, then you would need to include some memory
barrier synchronization.

I'm curious to what the use case is for the multi direct modify interface
is?

-- Steve

Re: [PATCH 6/8] ftrace: Add multi direct register/unregister interface

[Jiri Olsa]

On Tue, Sep 14, 2021 at 05:35:55PM -0400, Steven Rostedt wrote:
> On Tue, 31 Aug 2021 11:50:15 +0200
> Jiri Olsa <jolsa@redhat.com> wrote:
> 
> > Adding interface to register multiple direct functions
> > within single call. Adding following functions:
> > 
> >   register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
> >   unregister_ftrace_direct_multi(struct ftrace_ops *ops)
> > 
> > The register_ftrace_direct_multi registers direct function (addr)
> > with all functions in ops filter. The ops filter can be updated
> > before with ftrace_set_filter_ip calls.
> > 
> > All requested functions must not have direct function currently
> > registered, otherwise register_ftrace_direct_multi will fail.
> > 
> > The unregister_ftrace_direct_multi unregisters ops related direct
> > functions.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  include/linux/ftrace.h |  11 ++++
> >  kernel/trace/ftrace.c  | 111 +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 122 insertions(+)
> > 
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index d399621a67ee..e40b5201c16e 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -316,7 +316,10 @@ int ftrace_modify_direct_caller(struct ftrace_func_entry *entry,
> >  				unsigned long old_addr,
> >  				unsigned long new_addr);
> >  unsigned long ftrace_find_rec_direct(unsigned long ip);
> > +int register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr);
> > +int unregister_ftrace_direct_multi(struct ftrace_ops *ops);
> >  #else
> > +struct ftrace_ops;
> >  # define ftrace_direct_func_count 0
> >  static inline int register_ftrace_direct(unsigned long ip, unsigned long addr)
> >  {
> > @@ -346,6 +349,14 @@ static inline unsigned long ftrace_find_rec_direct(unsigned long ip)
> >  {
> >  	return 0;
> >  }
> > +static inline int register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
> > +{
> > +	return -ENODEV;
> > +}
> > +static inline int unregister_ftrace_direct_multi(struct ftrace_ops *ops)
> > +{
> > +	return -ENODEV;
> > +}
> >  #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
> >  
> >  #ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index c60217d81040..7243769493c9 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -5407,6 +5407,117 @@ int modify_ftrace_direct(unsigned long ip,
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(modify_ftrace_direct);
> > +
> > +#define MULTI_FLAGS (FTRACE_OPS_FL_IPMODIFY | FTRACE_OPS_FL_DIRECT | \
> > +		     FTRACE_OPS_FL_SAVE_REGS)
> > +
> > +static int check_direct_multi(struct ftrace_ops *ops)
> > +{
> > +	if (!(ops->flags & FTRACE_OPS_FL_INITIALIZED))
> > +		return -EINVAL;
> > +	if ((ops->flags & MULTI_FLAGS) != MULTI_FLAGS)
> > +		return -EINVAL;
> > +	return 0;
> > +}
> > +
> 
> Needs kernel doc comments as this is an interface outside this file.

right, will add

> 
> > +int register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
> > +{
> > +	struct ftrace_hash *hash, *free_hash = NULL;
> > +	struct ftrace_func_entry *entry, *new;
> > +	int err = -EBUSY, size, i;
> > +
> > +	if (ops->func || ops->trampoline)
> > +		return -EINVAL;
> > +	if (!(ops->flags & FTRACE_OPS_FL_INITIALIZED))
> > +		return -EINVAL;
> > +	if (ops->flags & FTRACE_OPS_FL_ENABLED)
> > +		return -EINVAL;
> > +
> > +	hash = ops->func_hash->filter_hash;
> > +	if (ftrace_hash_empty(hash))
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&direct_mutex);
> > +
> > +	/* Make sure requested entries are not already registered.. */
> > +	size = 1 << hash->size_bits;
> > +	for (i = 0; i < size; i++) {
> > +		hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
> > +			if (ftrace_find_rec_direct(entry->ip))
> > +				goto out_unlock;
> > +		}
> > +	}
> > +
> > +	/* ... and insert them to direct_functions hash. */
> > +	err = -ENOMEM;
> > +	for (i = 0; i < size; i++) {
> > +		hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
> > +			new = ftrace_add_rec_direct(entry->ip, addr, &free_hash);
> > +			if (!new)
> > +				goto out_remove;
> > +			entry->direct = addr;
> > +		}
> > +	}
> > +
> > +	ops->func = call_direct_funcs;
> > +	ops->flags = MULTI_FLAGS;
> > +	ops->trampoline = FTRACE_REGS_ADDR;
> > +
> > +	err = register_ftrace_function(ops);
> > +
> > + out_remove:
> > +	if (err) {
> 
> The below code:
> 
> > +		for (i = 0; i < size; i++) {
> > +			hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
> > +				new = __ftrace_lookup_ip(direct_functions, entry->ip);
> > +				if (new) {
> > +					remove_hash_entry(direct_functions, new);
> > +					kfree(new);
> > +				}
> > +			}
> > +		}
> 
> is identical to code below.
> 
> > +	}
> > +
> > + out_unlock:
> > +	mutex_unlock(&direct_mutex);
> > +
> > +	if (free_hash) {
> > +		synchronize_rcu_tasks();
> > +		free_ftrace_hash(free_hash);
> > +	}
> > +	return err;
> > +}
> > +EXPORT_SYMBOL_GPL(register_ftrace_direct_multi);
> > +
> 
> Should have kernel doc as well.

ok

> 
> > +int unregister_ftrace_direct_multi(struct ftrace_ops *ops)
> > +{
> > +	struct ftrace_hash *hash = ops->func_hash->filter_hash;
> > +	struct ftrace_func_entry *entry, *new;
> > +	int err, size, i;
> > +
> > +	if (check_direct_multi(ops))
> > +		return -EINVAL;
> > +	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&direct_mutex);
> > +	err = unregister_ftrace_function(ops);
> > +
> > +	size = 1 << hash->size_bits;
> 
> 
> > +	for (i = 0; i < size; i++) {
> > +		hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
> > +			new = __ftrace_lookup_ip(direct_functions, entry->ip);
> > +			if (new) {
> > +				remove_hash_entry(direct_functions, new);
> > +				kfree(new);
> > +			}
> > +		}
> > +	}
> 
> Would probably make sense to turn this into a static inline helper.

ok

thanks,
jirka

> 
> -- Steve
> 
> 
> > +
> > +	mutex_unlock(&direct_mutex);
> > +	return err;
> > +}
> > +EXPORT_SYMBOL_GPL(unregister_ftrace_direct_multi);
> >  #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
> >  
> >  /**
> 

Re: [PATCH 7/8] ftrace: Add multi direct modify interface

[Jiri Olsa]

On Wed, Sep 15, 2021 at 05:47:18PM -0400, Steven Rostedt wrote:
> On Tue, 14 Sep 2021 17:41:34 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > A better solution, that prevents having to do this, is to first change
> > the function fentry's to call the ftrace list loop function, that calls
> > the ftrace_ops list, and will call the direct call via the ops in the
> > loop. Have the ops->func call the new direct function (all will be
> > immediately affected). Update the entries, and then switch from the
> > loop back to the direct caller.
> 
> An easy way to force the loop function to be called instead of the direct
> trampoline, is to register a stub ftrace_ops to each of the functions that
> the direct function attaches to. You can even share the hash in doing so.
> 
> Having the ftrace_ops attached in the same locations as the direct
> trampoline, will force the loop function to be called (to call the stub
> ftrace_ops as well as the direct trampoline ftrace_ops helper).
> 
> Then change the direct trampoline address, which will have the ftrace_ops
> helper use that direct trampoline immediately*. Then when you remove the
> ftrace_ops stub, it will update all the call sites to call the new direct
> trampoline directly.

ok, that's the way the current direct modify interface is using, right?
I thought it'd be not so easy to adopt for multiple functions, I'll check
on that again and come for help ;-)

> 
> (*) not quite immediately, as there's no read memory barrier with the
> direct helper, so it may still be calling the old trampoline. But this
> shouldn't be an issue. If it is, then you would need to include some memory
> barrier synchronization.
> 
> I'm curious to what the use case is for the multi direct modify interface
> is?

when the trampoline is re-generated by adding or removing program,
we have same functions to trace and new trampoline to attach

thanks,
jirka

> 
> -- Steve
> 

Re: [PATCH 7/8] ftrace: Add multi direct modify interface

[Steven Rostedt]

On Thu, 16 Sep 2021 21:49:37 +0200
Jiri Olsa <jolsa@redhat.com> wrote:

> > I'm curious to what the use case is for the multi direct modify interface
> > is?  
> 
> when the trampoline is re-generated by adding or removing program,
> we have same functions to trace and new trampoline to attach
> 

Then it probably doesn't matter for the slight delay in synchronization.

-- Steve

Re: [PATCH 7/8] ftrace: Add multi direct modify interface

[Jiri Olsa]

On Tue, Oct 19, 2021 at 10:44:11AM -0400, Steven Rostedt wrote:
> On Tue, 19 Oct 2021 16:03:03 +0200
> Jiri Olsa <jolsa@redhat.com> wrote:
> 
> > > You can make sure the patches in there have your latest version, as you can
> > > review my patch. I'll update the tags if you give me one.  
> > 
> > I'm getting error when compiling:
> > 
> >   CC      kernel/trace/ftrace.o
> > kernel/trace/ftrace.c: In function ‘modify_ftrace_direct_multi’:
> > kernel/trace/ftrace.c:5608:2: error: label ‘out_unlock’ defined but not used [-Werror=unused-label]
> 
> Ah, I don't think I've been hit by the "-Werror" yet ;-)
> 
> 
> >  5608 |  out_unlock:
> >       |  ^~~~~~~~~~
> > 
> > looks like out_unlock is nolonger needed, I removed it
> 
> My tests would have found this, as it has a check for "new warnings".
> 
> Anyway, was this in your latest patch, or did I pull in and older one?
> 
> That is, should I expect a v2 from you?

it's on top of your ftrace/core, in your change:
  e62d91d8206e ftrace/direct: Do not disable when switching direct callers

just removing the label will fix it

also you can add my ack

Acked-by: Jiri Olsa <jolsa@redhat.com>

thanks,
jirka

Re: [PATCH 7/8] ftrace: Add multi direct modify interface

[Steven Rostedt]

On Tue, 19 Oct 2021 16:03:03 +0200
Jiri Olsa <jolsa@redhat.com> wrote:

> > You can make sure the patches in there have your latest version, as you can
> > review my patch. I'll update the tags if you give me one.  
> 
> I'm getting error when compiling:
> 
>   CC      kernel/trace/ftrace.o
> kernel/trace/ftrace.c: In function ‘modify_ftrace_direct_multi’:
> kernel/trace/ftrace.c:5608:2: error: label ‘out_unlock’ defined but not used [-Werror=unused-label]

Ah, I don't think I've been hit by the "-Werror" yet ;-)


>  5608 |  out_unlock:
>       |  ^~~~~~~~~~
> 
> looks like out_unlock is nolonger needed, I removed it

My tests would have found this, as it has a check for "new warnings".

Anyway, was this in your latest patch, or did I pull in and older one?

That is, should I expect a v2 from you?

-- Steve

Re: [PATCH 7/8] ftrace: Add multi direct modify interface

[Jiri Olsa]

On Tue, Oct 19, 2021 at 09:32:16AM -0400, Steven Rostedt wrote:
> On Tue, 19 Oct 2021 15:26:21 +0200
> Jiri Olsa <jolsa@redhat.com> wrote:
> 
> > > when trying to apply on top of my changes  
> > 
> > I updated my ftrace/direct branch, it actually still had the previous
> > version.. sorry, perhaps this is the cause of fuzz
> 
> I just pushed it (including your patches) here:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
> 
>   ftrace/core
> 
> 
> This is where I keep my WIP code. It should not be used to base anything
> off of, as I rebase it constantly. But it has the current version I plan on
> testing.
> 
> You can make sure the patches in there have your latest version, as you can
> review my patch. I'll update the tags if you give me one.

I'm getting error when compiling:

  CC      kernel/trace/ftrace.o
kernel/trace/ftrace.c: In function ‘modify_ftrace_direct_multi’:
kernel/trace/ftrace.c:5608:2: error: label ‘out_unlock’ defined but not used [-Werror=unused-label]
 5608 |  out_unlock:
      |  ^~~~~~~~~~

looks like out_unlock is nolonger needed, I removed it

jirka

Re: [PATCH 7/8] ftrace: Add multi direct modify interface

[Steven Rostedt]

On Tue, 19 Oct 2021 15:26:21 +0200
Jiri Olsa <jolsa@redhat.com> wrote:

> > when trying to apply on top of my changes  
> 
> I updated my ftrace/direct branch, it actually still had the previous
> version.. sorry, perhaps this is the cause of fuzz

I just pushed it (including your patches) here:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git

  ftrace/core


This is where I keep my WIP code. It should not be used to base anything
off of, as I rebase it constantly. But it has the current version I plan on
testing.

You can make sure the patches in there have your latest version, as you can
review my patch. I'll update the tags if you give me one.

-- Steve

Re: [PATCH 7/8] ftrace: Add multi direct modify interface

[Jiri Olsa]

On Tue, Oct 19, 2021 at 03:19:48PM +0200, Jiri Olsa wrote:
> On Mon, Oct 18, 2021 at 10:10:15PM -0400, Steven Rostedt wrote:
> > On Sat, 16 Oct 2021 13:39:55 +0200
> > Jiri Olsa <jolsa@redhat.com> wrote:
> > 
> > > On Fri, Oct 15, 2021 at 10:05:09AM -0400, Steven Rostedt wrote:
> > > > On Fri, 15 Oct 2021 14:05:25 +0200
> > > > Jiri Olsa <jolsa@redhat.com> wrote:
> > > >   
> > > > > ATM I'm bit stuck on the bpf side of this whole change, I'll test
> > > > > it with my other changes when I unstuck myself ;-)  
> > > > 
> > > > If you want, I'll apply this as a separate change on top of your patch set.
> > > > As I don't see anything wrong with your current code.
> > > > 
> > > > And when you are satisfied with this, just give me a "tested-by" and I'll
> > > > push it too.  
> > > 
> > > sounds great, thanks
> > > jirka
> > 
> > Would you want to ack/review this?
> 
> hum, do you have it in some branch already? I'm getting:
> 
> patching file kernel/trace/ftrace.c
> Hunk #1 succeeded at 5521 with fuzz 1 (offset -40 lines).
> Hunk #2 FAILED at 5576.
> Hunk #3 succeeded at 5557 (offset -44 lines).
> 1 out of 3 hunks FAILED -- saving rejects to file kernel/trace/ftrace.c.rej
> 
> 
> when trying to apply on top of my changes

I updated my ftrace/direct branch, it actually still had the previous
version.. sorry, perhaps this is the cause of fuzz

jirka

> 
> thanks,
> jirka
> 
> > 
> > -- Steve
> > 
> > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > Subject: [PATCH] ftrace/direct: Do not disable when switching direct callers
> > 
> > Currently to switch a set of "multi" direct trampolines from one
> > trampoline to another, a full shutdown of the current set needs to be
> > done, followed by an update to what trampoline the direct callers would
> > call, and then re-enabling the callers. This leaves a time when the
> > functions will not be calling anything, and events may be missed.
> > 
> > Instead, use a trick to allow all the functions with direct trampolines
> > attached will always call either the new or old trampoline while the
> > switch is happening. To do this, first attach a "dummy" callback via
> > ftrace to all the functions that the current direct trampoline is attached
> > to. This will cause the functions to call the "list func" instead of the
> > direct trampoline. The list function will call the direct trampoline
> > "helper" that will set the function it should call as it returns back to
> > the ftrace trampoline.
> > 
> > At this moment, the direct caller descriptor can safely update the direct
> > call trampoline. The list function will pick either the new or old
> > function (depending on the memory coherency model of the architecture).
> > 
> > Now removing the dummy function from each of the locations of the direct
> > trampoline caller, will put back the direct call, but now to the new
> > trampoline.
> > 
> > A better visual is:
> > 
> > [ Changing direct call from my_direct_1 to my_direct_2 ]
> > 
> >   <traced_func>:
> >      call my_direct_1
> > 
> >  ||||||||||||||||||||
> >  vvvvvvvvvvvvvvvvvvvv
> > 
> >   <traced_func>:
> >      call ftrace_caller
> > 
> >   <ftrace_caller>:
> >     [..]
> >     call ftrace_ops_list_func
> > 
> > 	ftrace_ops_list_func()
> > 	{
> > 		ops->func() -> direct_helper -> set rax to my_direct_1 or my_direct_2
> > 	}
> > 
> >    call rax (to either my_direct_1 or my_direct_2
> > 
> >  ||||||||||||||||||||
> >  vvvvvvvvvvvvvvvvvvvv
> > 
> >   <traced_func>:
> >      call my_direct_2
> > 
> > Link: https://lore.kernel.org/all/20211014162819.5c85618b@gandalf.local.home/
> > 
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > ---
> >  kernel/trace/ftrace.c | 33 ++++++++++++++++++++-------------
> >  1 file changed, 20 insertions(+), 13 deletions(-)
> > 
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 30120342176e..7ad1e8ae5855 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -5561,8 +5561,12 @@ EXPORT_SYMBOL_GPL(unregister_ftrace_direct_multi);
> >   */
> >  int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
> >  {
> > -	struct ftrace_hash *hash = ops->func_hash->filter_hash;
> > +	struct ftrace_hash *hash;
> >  	struct ftrace_func_entry *entry, *iter;
> > +	static struct ftrace_ops tmp_ops = {
> > +		.func		= ftrace_stub,
> > +		.flags		= FTRACE_OPS_FL_STUB,
> > +	};
> >  	int i, size;
> >  	int err;
> >  
> > @@ -5572,21 +5576,22 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
> >  		return -EINVAL;
> >  
> >  	mutex_lock(&direct_mutex);
> > -	mutex_lock(&ftrace_lock);
> > +
> > +	/* Enable the tmp_ops to have the same functions as the direct ops */
> > +	ftrace_ops_init(&tmp_ops);
> > +	tmp_ops.func_hash = ops->func_hash;
> > +
> > +	err = register_ftrace_function(&tmp_ops);
> > +	if (err)
> > +		goto out_direct;
> >  
> >  	/*
> > -	 * Shutdown the ops, change 'direct' pointer for each
> > -	 * ops entry in direct_functions hash and startup the
> > -	 * ops back again.
> > -	 *
> > -	 * Note there is no callback called for @ops object after
> > -	 * this ftrace_shutdown call until ftrace_startup is called
> > -	 * later on.
> > +	 * Now the ftrace_ops_list_func() is called to do the direct callers.
> > +	 * We can safely change the direct functions attached to each entry.
> >  	 */
> > -	err = ftrace_shutdown(ops, 0);
> > -	if (err)
> > -		goto out_unlock;
> > +	mutex_lock(&ftrace_lock);
> >  
> > +	hash = ops->func_hash->filter_hash;
> >  	size = 1 << hash->size_bits;
> >  	for (i = 0; i < size; i++) {
> >  		hlist_for_each_entry(iter, &hash->buckets[i], hlist) {
> > @@ -5597,10 +5602,12 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
> >  		}
> >  	}
> >  
> > -	err = ftrace_startup(ops, 0);
> > +	/* Removing the tmp_ops will add the updated direct callers to the functions */
> > +	unregister_ftrace_function(&tmp_ops);
> >  
> >   out_unlock:
> >  	mutex_unlock(&ftrace_lock);
> > + out_direct:
> >  	mutex_unlock(&direct_mutex);
> >  	return err;
> >  }
> > -- 
> > 2.31.1
> > 

Re: [PATCH 7/8] ftrace: Add multi direct modify interface

[Jiri Olsa]

On Mon, Oct 18, 2021 at 10:10:15PM -0400, Steven Rostedt wrote:
> On Sat, 16 Oct 2021 13:39:55 +0200
> Jiri Olsa <jolsa@redhat.com> wrote:
> 
> > On Fri, Oct 15, 2021 at 10:05:09AM -0400, Steven Rostedt wrote:
> > > On Fri, 15 Oct 2021 14:05:25 +0200
> > > Jiri Olsa <jolsa@redhat.com> wrote:
> > >   
> > > > ATM I'm bit stuck on the bpf side of this whole change, I'll test
> > > > it with my other changes when I unstuck myself ;-)  
> > > 
> > > If you want, I'll apply this as a separate change on top of your patch set.
> > > As I don't see anything wrong with your current code.
> > > 
> > > And when you are satisfied with this, just give me a "tested-by" and I'll
> > > push it too.  
> > 
> > sounds great, thanks
> > jirka
> 
> Would you want to ack/review this?

hum, do you have it in some branch already? I'm getting:

patching file kernel/trace/ftrace.c
Hunk #1 succeeded at 5521 with fuzz 1 (offset -40 lines).
Hunk #2 FAILED at 5576.
Hunk #3 succeeded at 5557 (offset -44 lines).
1 out of 3 hunks FAILED -- saving rejects to file kernel/trace/ftrace.c.rej


when trying to apply on top of my changes

thanks,
jirka

> 
> -- Steve
> 
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> Subject: [PATCH] ftrace/direct: Do not disable when switching direct callers
> 
> Currently to switch a set of "multi" direct trampolines from one
> trampoline to another, a full shutdown of the current set needs to be
> done, followed by an update to what trampoline the direct callers would
> call, and then re-enabling the callers. This leaves a time when the
> functions will not be calling anything, and events may be missed.
> 
> Instead, use a trick to allow all the functions with direct trampolines
> attached will always call either the new or old trampoline while the
> switch is happening. To do this, first attach a "dummy" callback via
> ftrace to all the functions that the current direct trampoline is attached
> to. This will cause the functions to call the "list func" instead of the
> direct trampoline. The list function will call the direct trampoline
> "helper" that will set the function it should call as it returns back to
> the ftrace trampoline.
> 
> At this moment, the direct caller descriptor can safely update the direct
> call trampoline. The list function will pick either the new or old
> function (depending on the memory coherency model of the architecture).
> 
> Now removing the dummy function from each of the locations of the direct
> trampoline caller, will put back the direct call, but now to the new
> trampoline.
> 
> A better visual is:
> 
> [ Changing direct call from my_direct_1 to my_direct_2 ]
> 
>   <traced_func>:
>      call my_direct_1
> 
>  ||||||||||||||||||||
>  vvvvvvvvvvvvvvvvvvvv
> 
>   <traced_func>:
>      call ftrace_caller
> 
>   <ftrace_caller>:
>     [..]
>     call ftrace_ops_list_func
> 
> 	ftrace_ops_list_func()
> 	{
> 		ops->func() -> direct_helper -> set rax to my_direct_1 or my_direct_2
> 	}
> 
>    call rax (to either my_direct_1 or my_direct_2
> 
>  ||||||||||||||||||||
>  vvvvvvvvvvvvvvvvvvvv
> 
>   <traced_func>:
>      call my_direct_2
> 
> Link: https://lore.kernel.org/all/20211014162819.5c85618b@gandalf.local.home/
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  kernel/trace/ftrace.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 30120342176e..7ad1e8ae5855 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -5561,8 +5561,12 @@ EXPORT_SYMBOL_GPL(unregister_ftrace_direct_multi);
>   */
>  int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
>  {
> -	struct ftrace_hash *hash = ops->func_hash->filter_hash;
> +	struct ftrace_hash *hash;
>  	struct ftrace_func_entry *entry, *iter;
> +	static struct ftrace_ops tmp_ops = {
> +		.func		= ftrace_stub,
> +		.flags		= FTRACE_OPS_FL_STUB,
> +	};
>  	int i, size;
>  	int err;
>  
> @@ -5572,21 +5576,22 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
>  		return -EINVAL;
>  
>  	mutex_lock(&direct_mutex);
> -	mutex_lock(&ftrace_lock);
> +
> +	/* Enable the tmp_ops to have the same functions as the direct ops */
> +	ftrace_ops_init(&tmp_ops);
> +	tmp_ops.func_hash = ops->func_hash;
> +
> +	err = register_ftrace_function(&tmp_ops);
> +	if (err)
> +		goto out_direct;
>  
>  	/*
> -	 * Shutdown the ops, change 'direct' pointer for each
> -	 * ops entry in direct_functions hash and startup the
> -	 * ops back again.
> -	 *
> -	 * Note there is no callback called for @ops object after
> -	 * this ftrace_shutdown call until ftrace_startup is called
> -	 * later on.
> +	 * Now the ftrace_ops_list_func() is called to do the direct callers.
> +	 * We can safely change the direct functions attached to each entry.
>  	 */
> -	err = ftrace_shutdown(ops, 0);
> -	if (err)
> -		goto out_unlock;
> +	mutex_lock(&ftrace_lock);
>  
> +	hash = ops->func_hash->filter_hash;
>  	size = 1 << hash->size_bits;
>  	for (i = 0; i < size; i++) {
>  		hlist_for_each_entry(iter, &hash->buckets[i], hlist) {
> @@ -5597,10 +5602,12 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
>  		}
>  	}
>  
> -	err = ftrace_startup(ops, 0);
> +	/* Removing the tmp_ops will add the updated direct callers to the functions */
> +	unregister_ftrace_function(&tmp_ops);
>  
>   out_unlock:
>  	mutex_unlock(&ftrace_lock);
> + out_direct:
>  	mutex_unlock(&direct_mutex);
>  	return err;
>  }
> -- 
> 2.31.1
> 

Re: [PATCH 7/8] ftrace: Add multi direct modify interface

[Steven Rostedt]

On Sat, 16 Oct 2021 13:39:55 +0200
Jiri Olsa <jolsa@redhat.com> wrote:

> On Fri, Oct 15, 2021 at 10:05:09AM -0400, Steven Rostedt wrote:
> > On Fri, 15 Oct 2021 14:05:25 +0200
> > Jiri Olsa <jolsa@redhat.com> wrote:
> >   
> > > ATM I'm bit stuck on the bpf side of this whole change, I'll test
> > > it with my other changes when I unstuck myself ;-)  
> > 
> > If you want, I'll apply this as a separate change on top of your patch set.
> > As I don't see anything wrong with your current code.
> > 
> > And when you are satisfied with this, just give me a "tested-by" and I'll
> > push it too.  
> 
> sounds great, thanks
> jirka

Would you want to ack/review this?

-- Steve

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Subject: [PATCH] ftrace/direct: Do not disable when switching direct callers

Currently to switch a set of "multi" direct trampolines from one
trampoline to another, a full shutdown of the current set needs to be
done, followed by an update to what trampoline the direct callers would
call, and then re-enabling the callers. This leaves a time when the
functions will not be calling anything, and events may be missed.

Instead, use a trick to allow all the functions with direct trampolines
attached will always call either the new or old trampoline while the
switch is happening. To do this, first attach a "dummy" callback via
ftrace to all the functions that the current direct trampoline is attached
to. This will cause the functions to call the "list func" instead of the
direct trampoline. The list function will call the direct trampoline
"helper" that will set the function it should call as it returns back to
the ftrace trampoline.

At this moment, the direct caller descriptor can safely update the direct
call trampoline. The list function will pick either the new or old
function (depending on the memory coherency model of the architecture).

Now removing the dummy function from each of the locations of the direct
trampoline caller, will put back the direct call, but now to the new
trampoline.

A better visual is:

[ Changing direct call from my_direct_1 to my_direct_2 ]

  <traced_func>:
     call my_direct_1

 ||||||||||||||||||||
 vvvvvvvvvvvvvvvvvvvv

  <traced_func>:
     call ftrace_caller

  <ftrace_caller>:
    [..]
    call ftrace_ops_list_func

	ftrace_ops_list_func()
	{
		ops->func() -> direct_helper -> set rax to my_direct_1 or my_direct_2
	}

   call rax (to either my_direct_1 or my_direct_2

 ||||||||||||||||||||
 vvvvvvvvvvvvvvvvvvvv

  <traced_func>:
     call my_direct_2

Link: https://lore.kernel.org/all/20211014162819.5c85618b@gandalf.local.home/

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 30120342176e..7ad1e8ae5855 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5561,8 +5561,12 @@ EXPORT_SYMBOL_GPL(unregister_ftrace_direct_multi);
  */
 int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
 {
-	struct ftrace_hash *hash = ops->func_hash->filter_hash;
+	struct ftrace_hash *hash;
 	struct ftrace_func_entry *entry, *iter;
+	static struct ftrace_ops tmp_ops = {
+		.func		= ftrace_stub,
+		.flags		= FTRACE_OPS_FL_STUB,
+	};
 	int i, size;
 	int err;
 
@@ -5572,21 +5576,22 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
 		return -EINVAL;
 
 	mutex_lock(&direct_mutex);
-	mutex_lock(&ftrace_lock);
+
+	/* Enable the tmp_ops to have the same functions as the direct ops */
+	ftrace_ops_init(&tmp_ops);
+	tmp_ops.func_hash = ops->func_hash;
+
+	err = register_ftrace_function(&tmp_ops);
+	if (err)
+		goto out_direct;
 
 	/*
-	 * Shutdown the ops, change 'direct' pointer for each
-	 * ops entry in direct_functions hash and startup the
-	 * ops back again.
-	 *
-	 * Note there is no callback called for @ops object after
-	 * this ftrace_shutdown call until ftrace_startup is called
-	 * later on.
+	 * Now the ftrace_ops_list_func() is called to do the direct callers.
+	 * We can safely change the direct functions attached to each entry.
 	 */
-	err = ftrace_shutdown(ops, 0);
-	if (err)
-		goto out_unlock;
+	mutex_lock(&ftrace_lock);
 
+	hash = ops->func_hash->filter_hash;
 	size = 1 << hash->size_bits;
 	for (i = 0; i < size; i++) {
 		hlist_for_each_entry(iter, &hash->buckets[i], hlist) {
@@ -5597,10 +5602,12 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
 		}
 	}
 
-	err = ftrace_startup(ops, 0);
+	/* Removing the tmp_ops will add the updated direct callers to the functions */
+	unregister_ftrace_function(&tmp_ops);
 
  out_unlock:
 	mutex_unlock(&ftrace_lock);
+ out_direct:
 	mutex_unlock(&direct_mutex);
 	return err;
 }
-- 
2.31.1

Re: [PATCH 7/8] ftrace: Add multi direct modify interface

[Jiri Olsa]

On Fri, Oct 15, 2021 at 10:05:09AM -0400, Steven Rostedt wrote:
> On Fri, 15 Oct 2021 14:05:25 +0200
> Jiri Olsa <jolsa@redhat.com> wrote:
> 
> > ATM I'm bit stuck on the bpf side of this whole change, I'll test
> > it with my other changes when I unstuck myself ;-)
> 
> If you want, I'll apply this as a separate change on top of your patch set.
> As I don't see anything wrong with your current code.
> 
> And when you are satisfied with this, just give me a "tested-by" and I'll
> push it too.

sounds great, thanks
jirka

Re: [PATCH 7/8] ftrace: Add multi direct modify interface

[Steven Rostedt]

On Fri, 15 Oct 2021 14:05:25 +0200
Jiri Olsa <jolsa@redhat.com> wrote:

> ATM I'm bit stuck on the bpf side of this whole change, I'll test
> it with my other changes when I unstuck myself ;-)

If you want, I'll apply this as a separate change on top of your patch set.
As I don't see anything wrong with your current code.

And when you are satisfied with this, just give me a "tested-by" and I'll
push it too.

-- Steve

Re: [PATCH 7/8] ftrace: Add multi direct modify interface

[Jiri Olsa]

On Thu, Oct 14, 2021 at 04:28:19PM -0400, Steven Rostedt wrote:
> On Fri,  8 Oct 2021 11:13:35 +0200
> Jiri Olsa <jolsa@redhat.com> wrote:
> 
> > +	/*
> > +	 * Shutdown the ops, change 'direct' pointer for each
> > +	 * ops entry in direct_functions hash and startup the
> > +	 * ops back again.
> > +	 *
> > +	 * Note there is no callback called for @ops object after
> > +	 * this ftrace_shutdown call until ftrace_startup is called
> > +	 * later on.
> > +	 */
> > +	err = ftrace_shutdown(ops, 0);
> > +	if (err)
> > +		goto out_unlock;
> 
> I believe I said before that we can do this by adding a stub ops that match
> all the functions with the direct ops being modified. This will cause the
> loop function to be called, which will call the direct function helper,
> which will then call the direct function that is found. That is, there is
> no "pause" in calling the direct callers. Either the old direct is called,
> or the new one. When the function returns, all are calling the new one.
> 
> That is, instead of:
> 
> [ Changing direct call from my_direct_1 to my_direct_2 ]
> 
>   <traced_func>:
>      call my_direct_1
> 
>  ||||||||||||||||||||
>  vvvvvvvvvvvvvvvvvvvv
> 
>   <traced_func>:
>      nop
> 
>  ||||||||||||||||||||
>  vvvvvvvvvvvvvvvvvvvv
> 
>   <traced_func>:
>      call my_direct_2
> 
> 
> We have it do:
> 
>   <traced_func>:
>      call my_direct_1
> 
>  ||||||||||||||||||||
>  vvvvvvvvvvvvvvvvvvvv
> 
>   <traced_func>:
>      call ftrace_caller
> 
> 
>   <ftrace_caller>:
>     [..]
>     call ftrace_ops_list_func
> 
> 
> ftrace_ops_list_func()
> {
> 	ops->func() -> direct_helper -> set rax to my_direct_1 or my_direct_2
> }
> 
>    call rax (to either my_direct_1 or my_direct_2

nice! :) I did not see that as a problem and something that can be
done later, thanks for doing this

> 
>  ||||||||||||||||||||
>  vvvvvvvvvvvvvvvvvvvv
> 
>   <traced_func>:
>      call my_direct_2
> 
> 
> I did this on top of this patch:

ATM I'm bit stuck on the bpf side of this whole change, I'll test
it with my other changes when I unstuck myself ;-)

thanks,
jirka

> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  kernel/trace/ftrace.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 30120342176e..7ad1e8ae5855 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -5561,8 +5561,12 @@ EXPORT_SYMBOL_GPL(unregister_ftrace_direct_multi);
>   */
>  int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
>  {
> -	struct ftrace_hash *hash = ops->func_hash->filter_hash;
> +	struct ftrace_hash *hash;
>  	struct ftrace_func_entry *entry, *iter;
> +	static struct ftrace_ops tmp_ops = {
> +		.func		= ftrace_stub,
> +		.flags		= FTRACE_OPS_FL_STUB,
> +	};
>  	int i, size;
>  	int err;
>  
> @@ -5572,21 +5576,22 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
>  		return -EINVAL;
>  
>  	mutex_lock(&direct_mutex);
> -	mutex_lock(&ftrace_lock);
> +
> +	/* Enable the tmp_ops to have the same functions as the direct ops */
> +	ftrace_ops_init(&tmp_ops);
> +	tmp_ops.func_hash = ops->func_hash;
> +
> +	err = register_ftrace_function(&tmp_ops);
> +	if (err)
> +		goto out_direct;
>  
>  	/*
> -	 * Shutdown the ops, change 'direct' pointer for each
> -	 * ops entry in direct_functions hash and startup the
> -	 * ops back again.
> -	 *
> -	 * Note there is no callback called for @ops object after
> -	 * this ftrace_shutdown call until ftrace_startup is called
> -	 * later on.
> +	 * Now the ftrace_ops_list_func() is called to do the direct callers.
> +	 * We can safely change the direct functions attached to each entry.
>  	 */
> -	err = ftrace_shutdown(ops, 0);
> -	if (err)
> -		goto out_unlock;
> +	mutex_lock(&ftrace_lock);
>  
> +	hash = ops->func_hash->filter_hash;
>  	size = 1 << hash->size_bits;
>  	for (i = 0; i < size; i++) {
>  		hlist_for_each_entry(iter, &hash->buckets[i], hlist) {
> @@ -5597,10 +5602,12 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
>  		}
>  	}
>  
> -	err = ftrace_startup(ops, 0);
> +	/* Removing the tmp_ops will add the updated direct callers to the functions */
> +	unregister_ftrace_function(&tmp_ops);
>  
>   out_unlock:
>  	mutex_unlock(&ftrace_lock);
> + out_direct:
>  	mutex_unlock(&direct_mutex);
>  	return err;
>  }
> -- 
> 2.31.1
> 

Re: [PATCH 7/8] ftrace: Add multi direct modify interface

[Steven Rostedt]

On Fri,  8 Oct 2021 11:13:35 +0200
Jiri Olsa <jolsa@redhat.com> wrote:

> +	/*
> +	 * Shutdown the ops, change 'direct' pointer for each
> +	 * ops entry in direct_functions hash and startup the
> +	 * ops back again.
> +	 *
> +	 * Note there is no callback called for @ops object after
> +	 * this ftrace_shutdown call until ftrace_startup is called
> +	 * later on.
> +	 */
> +	err = ftrace_shutdown(ops, 0);
> +	if (err)
> +		goto out_unlock;

I believe I said before that we can do this by adding a stub ops that match
all the functions with the direct ops being modified. This will cause the
loop function to be called, which will call the direct function helper,
which will then call the direct function that is found. That is, there is
no "pause" in calling the direct callers. Either the old direct is called,
or the new one. When the function returns, all are calling the new one.

That is, instead of:

[ Changing direct call from my_direct_1 to my_direct_2 ]

  <traced_func>:
     call my_direct_1

 ||||||||||||||||||||
 vvvvvvvvvvvvvvvvvvvv

  <traced_func>:
     nop

 ||||||||||||||||||||
 vvvvvvvvvvvvvvvvvvvv

  <traced_func>:
     call my_direct_2


We have it do:

  <traced_func>:
     call my_direct_1

 ||||||||||||||||||||
 vvvvvvvvvvvvvvvvvvvv

  <traced_func>:
     call ftrace_caller


  <ftrace_caller>:
    [..]
    call ftrace_ops_list_func


ftrace_ops_list_func()
{
	ops->func() -> direct_helper -> set rax to my_direct_1 or my_direct_2
}

   call rax (to either my_direct_1 or my_direct_2

 ||||||||||||||||||||
 vvvvvvvvvvvvvvvvvvvv

  <traced_func>:
     call my_direct_2


I did this on top of this patch:

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 30120342176e..7ad1e8ae5855 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5561,8 +5561,12 @@ EXPORT_SYMBOL_GPL(unregister_ftrace_direct_multi);
  */
 int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
 {
-	struct ftrace_hash *hash = ops->func_hash->filter_hash;
+	struct ftrace_hash *hash;
 	struct ftrace_func_entry *entry, *iter;
+	static struct ftrace_ops tmp_ops = {
+		.func		= ftrace_stub,
+		.flags		= FTRACE_OPS_FL_STUB,
+	};
 	int i, size;
 	int err;
 
@@ -5572,21 +5576,22 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
 		return -EINVAL;
 
 	mutex_lock(&direct_mutex);
-	mutex_lock(&ftrace_lock);
+
+	/* Enable the tmp_ops to have the same functions as the direct ops */
+	ftrace_ops_init(&tmp_ops);
+	tmp_ops.func_hash = ops->func_hash;
+
+	err = register_ftrace_function(&tmp_ops);
+	if (err)
+		goto out_direct;
 
 	/*
-	 * Shutdown the ops, change 'direct' pointer for each
-	 * ops entry in direct_functions hash and startup the
-	 * ops back again.
-	 *
-	 * Note there is no callback called for @ops object after
-	 * this ftrace_shutdown call until ftrace_startup is called
-	 * later on.
+	 * Now the ftrace_ops_list_func() is called to do the direct callers.
+	 * We can safely change the direct functions attached to each entry.
 	 */
-	err = ftrace_shutdown(ops, 0);
-	if (err)
-		goto out_unlock;
+	mutex_lock(&ftrace_lock);
 
+	hash = ops->func_hash->filter_hash;
 	size = 1 << hash->size_bits;
 	for (i = 0; i < size; i++) {
 		hlist_for_each_entry(iter, &hash->buckets[i], hlist) {
@@ -5597,10 +5602,12 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
 		}
 	}
 
-	err = ftrace_startup(ops, 0);
+	/* Removing the tmp_ops will add the updated direct callers to the functions */
+	unregister_ftrace_function(&tmp_ops);
 
  out_unlock:
 	mutex_unlock(&ftrace_lock);
+ out_direct:
 	mutex_unlock(&direct_mutex);
 	return err;
 }
-- 
2.31.1

[PATCH 7/8] ftrace: Add multi direct modify interface

[Jiri Olsa]

Adding interface to modify registered direct function
for ftrace_ops. Adding following function:

   modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)

The function changes the currently registered direct
function for all attached functions.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/ftrace.h |  6 ++++
 kernel/trace/ftrace.c  | 62 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index ba5d02ba8166..c15b767f39cf 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -318,6 +318,8 @@ int ftrace_modify_direct_caller(struct ftrace_func_entry *entry,
 unsigned long ftrace_find_rec_direct(unsigned long ip);
 int register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr);
 int unregister_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr);
+int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr);
+
 #else
 struct ftrace_ops;
 # define ftrace_direct_func_count 0
@@ -357,6 +359,10 @@ static inline int unregister_ftrace_direct_multi(struct ftrace_ops *ops, unsigne
 {
 	return -ENODEV;
 }
+static inline int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
+{
+	return -ENODEV;
+}
 #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
 
 #ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index f9df7bffb770..d92f2591c3fc 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5547,6 +5547,68 @@ int unregister_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
 	return err;
 }
 EXPORT_SYMBOL_GPL(unregister_ftrace_direct_multi);
+
+/**
+ * modify_ftrace_direct_multi - Modify an existing direct 'multi' call
+ * to call something else
+ * @ops: The address of the struct ftrace_ops object
+ * @addr: The address of the new trampoline to call at @ops functions
+ *
+ * This is used to unregister currently registered direct caller and
+ * register new one @addr on functions registered in @ops object.
+ *
+ * Note there's window between ftrace_shutdown and ftrace_startup calls
+ * where there will be no callbacks called.
+ *
+ * Returns: zero on success. Non zero on error, which includes:
+ *  -EINVAL - The @ops object was not properly registered.
+ */
+int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
+{
+	struct ftrace_hash *hash = ops->func_hash->filter_hash;
+	struct ftrace_func_entry *entry, *iter;
+	int i, size;
+	int err;
+
+	if (check_direct_multi(ops))
+		return -EINVAL;
+	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
+		return -EINVAL;
+
+	mutex_lock(&direct_mutex);
+	mutex_lock(&ftrace_lock);
+
+	/*
+	 * Shutdown the ops, change 'direct' pointer for each
+	 * ops entry in direct_functions hash and startup the
+	 * ops back again.
+	 *
+	 * Note there is no callback called for @ops object after
+	 * this ftrace_shutdown call until ftrace_startup is called
+	 * later on.
+	 */
+	err = ftrace_shutdown(ops, 0);
+	if (err)
+		goto out_unlock;
+
+	size = 1 << hash->size_bits;
+	for (i = 0; i < size; i++) {
+		hlist_for_each_entry(iter, &hash->buckets[i], hlist) {
+			entry = __ftrace_lookup_ip(direct_functions, iter->ip);
+			if (!entry)
+				continue;
+			entry->direct = addr;
+		}
+	}
+
+	err = ftrace_startup(ops, 0);
+
+ out_unlock:
+	mutex_unlock(&ftrace_lock);
+	mutex_unlock(&direct_mutex);
+	return err;
+}
+EXPORT_SYMBOL_GPL(modify_ftrace_direct_multi);
 #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
 
 /**
-- 
2.31.1