function_graph: Rewrite to allow multiple users

From: "Steven Rostedt (VMware)" <[email protected]>

In order to make the function graph infrastructure more generic, there can not be code specific for the function_graph tracer in the generic code. This includes the set_graph_notrace logic, that stops all graph calls when a function in the set_graph_notrace is hit.

By using the trace_recursion mask, we can use a bit in the current task_struct to implement the notrace code, and move the logic out of fgraph.c and into trace_functions_graph.c and keeps it affecting only the tracer and not all call graph callbacks.

Signed-off-by Steven Rostedt (VMware) <[email protected]>

---
 kernel/trace/fgraph.c                | 21 ---------------------
 kernel/trace/trace.h                 |  6 ++++++
 kernel/trace/trace_functions_graph.c | 21 +++++++++++++++++++++
 3 files changed, 27 insertions(+), 21 deletions(-)

kernel/trace/fgraph.c +0 -21

index e8fcf1b2b38c..c684968b87e7 100644
@@ -64,30 +64,9 @@ ftrace_push_return_trace(unsigned long ret, unsigned long func,
 		return -EBUSY;
 	}
 
-	/*
-	 * The curr_ret_stack is an index to ftrace return stack of
-	 * current task.  Its value should be in [0, FTRACE_RETFUNC_
-	 * DEPTH) when the function graph tracer is used.  To support
-	 * filtering out specific functions, it makes the index
-	 * negative by subtracting huge value (FTRACE_NOTRACE_DEPTH)
-	 * so when it sees a negative index the ftrace will ignore
-	 * the record.  And the index gets recovered when returning
-	 * from the filtered function by adding the FTRACE_NOTRACE_
-	 * DEPTH and then it'll continue to record functions normally.
-	 *
-	 * The curr_ret_stack is initialized to -1 and get increased
-	 * in this function.  So it can be less than -1 only if it was
-	 * filtered out via ftrace_graph_notrace_addr() which can be
-	 * set from set_graph_notrace file in tracefs by user.
-	 */
-	if (current->curr_ret_stack < -1)
-		return -EBUSY;
-
 	calltime = trace_clock_local();
 
 	index = ++current->curr_ret_stack;
-	if (ftrace_graph_notrace_addr(func))
-		current->curr_ret_stack -= FTRACE_NOTRACE_DEPTH;
 	barrier();
 	current->ret_stack[index].ret = ret;
 	current->ret_stack[index].func = func;

kernel/trace/trace.h +6 -0

index 3b8c0e24ab30..f3ad85830961 100644
@@ -512,6 +512,12 @@ enum {
  * can only be modified by current, we can reuse trace_recursion.
  */
 	TRACE_IRQ_BIT,
+/*
+ * To implement set_graph_notrace, if this bit is set, we ignore
+ * function graph tracing of called functions, until the return
+ * function is called to clear it.
+ */
+	TRACE_GRAPH_NOTRACE_BIT,
 };
 
 #define trace_recursion_set(bit)	do { (current)->trace_recursion |= (1<<(bit)); } while (0)

kernel/trace/trace_functions_graph.c +21 -0

index af1759cd6eab..4748dc1bf5e1 100644
@@ -188,6 +188,18 @@ int trace_graph_entry(struct ftrace_graph_ent *trace)
 	int cpu;
 	int pc;
 
+	if (trace_recursion_test(TRACE_GRAPH_NOTRACE_BIT))
+		return 0;
+
+	if (ftrace_graph_notrace_addr(trace->func)) {
+		trace_recursion_set(TRACE_GRAPH_NOTRACE_BIT);
+		/*
+		 * Need to return 1 to have the return called
+		 * that will clear the NOTRACE bit.
+		 */
+		return 1;
+	}
+
 	if (!ftrace_trace_task(tr))
 		return 0;
 

@@ -288,6 +300,11 @@ void trace_graph_return(struct ftrace_graph_ret *trace)
 	int cpu;
 	int pc;
 
+	if (trace_recursion_test(TRACE_GRAPH_NOTRACE_BIT)) {
+		trace_recursion_clear(TRACE_GRAPH_NOTRACE_BIT);
+		return;
+	}
+
 	local_irq_save(flags);
 	cpu = raw_smp_processor_id();
 	data = per_cpu_ptr(tr->trace_buffer.data, cpu);

@@ -311,6 +328,10 @@ void set_graph_array(struct trace_array *tr)
 
 static void trace_graph_thresh_return(struct ftrace_graph_ret *trace)
 {
+	if (trace_recursion_test(TRACE_GRAPH_NOTRACE_BIT)) {
+		trace_recursion_clear(TRACE_GRAPH_NOTRACE_BIT);
+		return;
+	}
 	if (tracing_thresh &&
 	    (trace->rettime - trace->calltime < tracing_thresh))
 		return;

Signature · 1 line

2.19.1

Acked-by Namhyung Kim <[email protected]>

Quoted 1 line · On Fri, 23 Nov 2018 09:01:18 +0900 Namhyung Kim <[email protected]> wrote:

Acked-by Namhyung Kim <[email protected]>

Thanks Namhyung!

Quoted 4 lines · Steven Rostedt

On Fri, 23 Nov 2018 09:01:18 +0900
Namhyung Kim <[email protected]> wrote:

Quoted 3 lines

Quoted 1 line

Acked-by Namhyung Kim <[email protected]>

Thanks Namhyung!

It'd be nice if you cc me for the whole patchset (and other tracing patches) next time.

Quoted 5 lines · On Sat, 24 Nov 2018 14:49:34 +0900 Namhyung Kim <[email protected]> wrote:

Quoted 4 lines · Steven Rostedt

On Fri, 23 Nov 2018 09:01:18 +0900
Namhyung Kim <[email protected]> wrote:

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

Acked-by Namhyung Kim <[email protected]>

Thanks Namhyung!

It'd be nice if you cc me for the whole patchset (and other tracing patches) next time.

Bah, Sorry, that was an oversight. You were one of the people I planned on Cc'ing the entire patch set to, but in the process of adding names I was more focused on who was at Plumbers that I talked to, and trying to remember everyone that asked to be Cc'd. I didn't mean to leave you off. That was a mistake.

From: "Steven Rostedt (VMware)" <[email protected]>

The curr_ret_stack is no longer set to -1 when not tracing a function. It is now done differently, and the FTRACE_NOTRACE_DEPTH value is no longer used. Remove the reference to it.

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc [email protected]

Signed-off-by Steven Rostedt (VMware) <[email protected]>

---
 arch/arm64/kernel/stacktrace.c | 3 ---
 1 file changed, 3 deletions(-)

arch/arm64/kernel/stacktrace.c +0 -0

index 4989f7ea1e59..7723dadf25be 100644

Hi Steve,

Quoted 5 lines · Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>

The curr_ret_stack is no longer set to -1 when not tracing a function. It is now done differently, and the FTRACE_NOTRACE_DEPTH value is no longer used. Remove the reference to it.

Do you have a pointer to the commit that changed that behaviour? I just want to make sure we're not missing something in our unwind_frame() code.

Quoted 9 lines

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc linux-arm-kernel at lists.infradead.org

Signed-off-by Steven Rostedt (VMware) <[email protected]>

---
 arch/arm64/kernel/stacktrace.c | 3 ---
 1 file changed, 3 deletions(-)

arch/arm64/kernel/stacktrace.c int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame) · line 63

Quoted 8 lines

arch/arm64/kernel/stacktrace.c +0 -0

index 4989f7ea1e59..7723dadf25be 100644
@@ -61,9 +61,6 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 			(frame->pc == (unsigned long)return_to_handler)) {
 		if (WARN_ON_ONCE(frame->graph == -1))
 			return -EINVAL;  

Hmm, so is this code redundant too ^^ ?

arch/arm64/kernel/stacktrace.c int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame) · line 66

Quoted 4 lines

arch/arm64/kernel/stacktrace.c +0 -3

-		if (frame->graph < -1)
-			frame->graph += FTRACE_NOTRACE_DEPTH;
-  

Do we still need to initialise frame->graph in __save_stack_trace()?

From: "Steven Rostedt (VMware)" <[email protected]>

To make the function graph infrastructure more managable, the code needs to be in its own file (fgraph.c). Move the code that is specific for managing the function graph infrastructure out of ftrace.c and into fgraph.c

Signed-off-by Steven Rostedt (VMware) <[email protected]>

Quoted 7 lines · Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>

To make the function graph infrastructure more managable, the code needs to be in its own file (fgraph.c). Move the code that is specific for managing the function graph infrastructure out of ftrace.c and into fgraph.c

Signed-off-by Steven Rostedt (VMware) <[email protected]>

I think this patch causes a build error if CONFIG_FUNCTION_PROFILER is disabled but function graph is enabled. The following diff fixes it for me.

thanks,

- Joel

----8<------

kernel/trace/ftrace.c +0 -0

index 3b8c307c7ff0..ce38bb962f91 100644
@@ -382,6 +382,15 @@ static void ftrace_update_pid_func(void)
 	update_ftrace_function();
 }

kernel/trace/ftrace.c +0 -6

 #ifdef CONFIG_FUNCTION_PROFILER
 struct ftrace_profile {
 	struct hlist_node		node;

@@ -783,12 +792,6 @@ function_profile_call(unsigned long ip, unsigned long parent_ip,
 }
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
-static bool fgraph_graph_time = true;
-
-void ftrace_graph_graph_time_control(bool enable)
-{
-	fgraph_graph_time = enable;
-}
 
 static int profile_graph_entry(struct ftrace_graph_ent *trace)
 {

I think the better answer is to move it into trace_functions_graph.c.

Quoted 1 line · On Fri, 23 Nov 2018 12:58:34 -0500 Steven Rostedt <[email protected]> wrote:

I think the better answer is to move it into trace_functions_graph.c.

I take that back. I think the better answer is to not call that function if the profiler is not set, nor have that option even available. Because it has no meaning without the profiler.

Quoted 7 lines · Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>

To make the function graph infrastructure more managable, the code needs to be in its own file (fgraph.c). Move the code that is specific for managing the function graph infrastructure out of ftrace.c and into fgraph.c

Signed-off-by Steven Rostedt (VMware) <[email protected]>

I think this patch causes a build error if CONFIG_FUNCTION_PROFILER is disabled but function graph is enabled. The following diff fixes it for me.

thanks,

- Joel

----8<------

kernel/trace/ftrace.c +9 -6

index 3b8c307c7ff0..ce38bb962f91 100644
@@ -382,6 +382,15 @@ static void ftrace_update_pid_func(void)
 	update_ftrace_function();
 }
 
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+static bool fgraph_graph_time = true;
+
+void ftrace_graph_graph_time_control(bool enable)
+{
+	fgraph_graph_time = enable;
+}
+#endif
+
 #ifdef CONFIG_FUNCTION_PROFILER
 struct ftrace_profile {
 	struct hlist_node		node;

@@ -783,12 +792,6 @@ function_profile_call(unsigned long ip, unsigned long parent_ip,
 }
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
-static bool fgraph_graph_time = true;
-
-void ftrace_graph_graph_time_control(bool enable)
-{
-	fgraph_graph_time = enable;
-}
 
 static int profile_graph_entry(struct ftrace_graph_ent *trace)
 {

From: "Steven Rostedt (VMware)" <[email protected]>

Currently the registering of function graph is to pass in a entry and return function. We need to have a way to associate those functions together where the entry can determine to run the return hook. Having a structure that contains both functions will facilitate the process of converting the code to be able to do such.

This is similar to the way function hooks are enabled (it passes in ftrace_ops). Instead of passing in the functions to use, a single structure is passed in to the registering function.

The unregister function is now passed in the fgraph_ops handle. When we allow more than one callback to the function graph hooks, this will let the system know which one to remove.

Signed-off-by Steven Rostedt (VMware) <[email protected]>

---
 include/linux/ftrace.h               | 24 +++++++++++++++++-------
 kernel/trace/fgraph.c                |  9 ++++-----
 kernel/trace/ftrace.c                | 10 +++++++---
 kernel/trace/trace_functions_graph.c | 21 ++++++++++++++++-----
 kernel/trace/trace_irqsoff.c         | 10 +++++++---
 kernel/trace/trace_sched_wakeup.c    | 10 +++++++---
 kernel/trace/trace_selftest.c        |  8 ++++++--
 7 files changed, 64 insertions(+), 28 deletions(-)

include/linux/ftrace.h +17 -7

index f98063e273e5..477ff9412d26 100644
@@ -749,6 +749,18 @@ typedef int (*trace_func_graph_ent_t)(struct ftrace_graph_ent *); /* entry */
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 
+struct fgraph_ops {
+	trace_func_graph_ent_t		entryfunc;
+	trace_func_graph_ret_t		retfunc;
+	struct fgraph_ops __rcu		*next;
+	unsigned long			flags;
+	void				*private;
+#ifdef CONFIG_DYNAMIC_FTRACE
+	struct ftrace_ops_hash		local_hash;
+	struct ftrace_ops_hash		*func_hash;
+#endif
+};
+
 /*
  * Stack of return addresses for functions
  * of a thread.

@@ -792,8 +804,9 @@ unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx,
 
 #define FTRACE_RETFUNC_DEPTH 50
 #define FTRACE_RETSTACK_ALLOC_SIZE 32
-extern int register_ftrace_graph(trace_func_graph_ret_t retfunc,
-				trace_func_graph_ent_t entryfunc);
+
+extern int register_ftrace_graph(struct fgraph_ops *ops);
+extern void unregister_ftrace_graph(struct fgraph_ops *ops);
 
 extern bool ftrace_graph_is_dead(void);
 extern void ftrace_graph_stop(void);

@@ -802,8 +815,6 @@ extern void ftrace_graph_stop(void);
 extern trace_func_graph_ret_t ftrace_graph_return;
 extern trace_func_graph_ent_t ftrace_graph_entry;
 
-extern void unregister_ftrace_graph(void);
-
 extern void ftrace_graph_init_task(struct task_struct *t);
 extern void ftrace_graph_exit_task(struct task_struct *t);
 extern void ftrace_graph_init_idle_task(struct task_struct *t, int cpu);

@@ -830,12 +841,11 @@ static inline void ftrace_graph_init_task(struct task_struct *t) { }
 static inline void ftrace_graph_exit_task(struct task_struct *t) { }
 static inline void ftrace_graph_init_idle_task(struct task_struct *t, int cpu) { }
 
-static inline int register_ftrace_graph(trace_func_graph_ret_t retfunc,
-			  trace_func_graph_ent_t entryfunc)
+static inline int register_ftrace_graph(struct fgraph_ops *ops);
 {
 	return -1;
 }
-static inline void unregister_ftrace_graph(void) { }
+static inline void unregister_ftrace_graph(struct fgraph_ops *ops) { }
 
 static inline int task_curr_ret_stack(struct task_struct *tsk)
 {

kernel/trace/fgraph.c +4 -5

index b9c7dbbbdd96..f3a89ecac671 100644
@@ -491,8 +491,7 @@ static int start_graph_tracing(void)
 	return ret;
 }
 
-int register_ftrace_graph(trace_func_graph_ret_t retfunc,
-			trace_func_graph_ent_t entryfunc)
+int register_ftrace_graph(struct fgraph_ops *gops)
 {
 	int ret = 0;
 

@@ -513,7 +512,7 @@ int register_ftrace_graph(trace_func_graph_ret_t retfunc,
 		goto out;
 	}
 
-	ftrace_graph_return = retfunc;
+	ftrace_graph_return = gops->retfunc;
 
 	/*
 	 * Update the indirect function to the entryfunc, and the

@@ -521,7 +520,7 @@ int register_ftrace_graph(trace_func_graph_ret_t retfunc,
 	 * call the update fgraph entry function to determine if
 	 * the entryfunc should be called directly or not.
 	 */
-	__ftrace_graph_entry = entryfunc;
+	__ftrace_graph_entry = gops->entryfunc;
 	ftrace_graph_entry = ftrace_graph_entry_test;
 	update_function_graph_func();
 

@@ -531,7 +530,7 @@ int register_ftrace_graph(trace_func_graph_ret_t retfunc,
 	return ret;
 }
 
-void unregister_ftrace_graph(void)
+void unregister_ftrace_graph(struct fgraph_ops *gops)
 {
 	mutex_lock(&ftrace_lock);
 

kernel/trace/ftrace.c +7 -3

index 64e635994648..d057dde081e7 100644
@@ -849,15 +849,19 @@ static void profile_graph_return(struct ftrace_graph_ret *trace)
 	local_irq_restore(flags);
 }
 
+static struct fgraph_ops fprofiler_ops = {
+	.entryfunc = &profile_graph_entry,
+	.retfunc = &profile_graph_return,
+};
+
 static int register_ftrace_profiler(void)
 {
-	return register_ftrace_graph(&profile_graph_return,
-				     &profile_graph_entry);
+	return register_ftrace_graph(&fprofiler_ops);
 }
 
 static void unregister_ftrace_profiler(void)
 {
-	unregister_ftrace_graph();
+	unregister_ftrace_graph(&fprofiler_ops);
 }
 #else
 static struct ftrace_ops ftrace_profile_ops __read_mostly = {

kernel/trace/trace_functions_graph.c +0 -0

index 0e0ff08357cf..7c7fd13d2373 100644

Quoted 9 lines

static struct ftrace_ops ftrace_profile_ops __read_mostly = {

kernel/trace/trace_functions_graph.c +0 -0

index 0e0ff08357cf..7c7fd13d2373 100644
@@ -336,17 +336,25 @@ static void trace_graph_thresh_return(struct ftrace_graph_ret *trace)
 		trace_graph_return(trace);
 }
 

kernel/trace/trace_functions_graph.c static void trace_graph_thresh_return(struct ftrace_graph_ret *trace) · line 339

Quoted 1 line

kernel/trace/trace_functions_graph.c +1 -0

+static struct fgraph_ops funcgraph_threash_ops = {  

minor nit: should be funcgraph_thresh_ops ?

From: "Steven Rostedt (VMware)" <[email protected]>

Currently the registering of function graph is to pass in a entry and return function. We need to have a way to associate those functions together where the entry can determine to run the return hook. Having a structure that contains both functions will facilitate the process of converting the code to be able to do such.

This is similar to the way function hooks are enabled (it passes in ftrace_ops). Instead of passing in the functions to use, a single structure is passed in to the registering function.

The unregister function is now passed in the fgraph_ops handle. When we allow more than one callback to the function graph hooks, this will let the system know which one to remove.

Signed-off-by Steven Rostedt (VMware) <[email protected]>

---
 include/linux/ftrace.h               | 24 +++++++++++++++++-------
 kernel/trace/fgraph.c                |  9 ++++-----
 kernel/trace/ftrace.c                | 10 +++++++---
 kernel/trace/trace_functions_graph.c | 21 ++++++++++++++++-----
 kernel/trace/trace_irqsoff.c         | 10 +++++++---
 kernel/trace/trace_sched_wakeup.c    | 10 +++++++---
 kernel/trace/trace_selftest.c        |  8 ++++++--
 7 files changed, 64 insertions(+), 28 deletions(-)

include/linux/ftrace.h +1 -0

index f98063e273e5..477ff9412d26 100644
@@ -749,6 +749,18 @@ typedef int (*trace_func_graph_ent_t)(struct ftrace_graph_ent *); /* entry */
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 
+struct fgraph_ops {

Quoted 11 lines

include/linux/ftrace.h +3 -0

index f98063e273e5..477ff9412d26 100644
@@ -749,6 +749,18 @@ typedef int (*trace_func_graph_ent_t)(struct ftrace_graph_ent *); /* entry */
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 
+struct fgraph_ops {
+	trace_func_graph_ent_t		entryfunc;
+	trace_func_graph_ret_t		retfunc;  

include/linux/ftrace.h typedef int (*trace_func_graph_ent_t)(struct ftrace_graph_ent *); /* entry */ · line 761

Quoted 7 lines

include/linux/ftrace.h +7 -0

+	struct fgraph_ops __rcu		*next;
+	unsigned long			flags;
+	void				*private;
+#ifdef CONFIG_DYNAMIC_FTRACE
+	struct ftrace_ops_hash		local_hash;
+	struct ftrace_ops_hash		*func_hash;
+#endif  

Hmm, can we introduce these fields when we actually use it?

BTW, would you have any idea for using private field?

From: "Steven Rostedt (VMware)" <[email protected]>

The static inline function task_curr_ret_stack() is unused, remove it.

Signed-off-by Steven Rostedt (VMware) <[email protected]>

---
 include/linux/ftrace.h | 10 ----------
 1 file changed, 10 deletions(-)

include/linux/ftrace.h +0 -10

index 477ff9412d26..5544df21a886 100644
@@ -819,11 +819,6 @@ extern void ftrace_graph_init_task(struct task_struct *t);
 extern void ftrace_graph_exit_task(struct task_struct *t);
 extern void ftrace_graph_init_idle_task(struct task_struct *t, int cpu);
 
-static inline int task_curr_ret_stack(struct task_struct *t)
-{
-	return t->curr_ret_stack;
-}
-
 static inline void pause_graph_tracing(void)
 {
 	atomic_inc(&current->tracing_graph_pause);

@@ -847,11 +842,6 @@ static inline int register_ftrace_graph(struct fgraph_ops *ops);
 }
 static inline void unregister_ftrace_graph(struct fgraph_ops *ops) { }
 
-static inline int task_curr_ret_stack(struct task_struct *tsk)
-{
-	return -1;
-}
-
 static inline unsigned long
 ftrace_graph_ret_addr(struct task_struct *task, int *idx, unsigned long ret,
 		      unsigned long *retp)

Signature · 1 line

2.19.1

Quoted 3 lines · On Wed, 21 Nov 2018 20:27:16 -0500 Steven Rostedt <[email protected]> wrote:

From: "Steven Rostedt (VMware)" <[email protected]>

The static inline function task_curr_ret_stack() is unused, remove it.

This looks able to be applied without this series. I think we should apply this to for-next branch?

Reviewed-by Masami Hiramatsu <[email protected]>

The static inline function task_curr_ret_stack() is unused, remove it.

Quoted 1 line · Steven Rostedt

The static inline function task_curr_ret_stack() is unused, remove it.

Just want ot make sure I understand this correctly, instead of using this function, the convention now is to just directly assign t->curr_ret_stack = -1?

Quoted 3 lines · On Mon, 26 Nov 2018 00:02:07 -1000 Joey Pabalinas <[email protected]>…

Quoted 1 line · Steven Rostedt

The static inline function task_curr_ret_stack() is unused, remove it.

Quoted 3 lines

Just want ot make sure I understand this correctly, instead of using this function, the convention now is to just directly assign t->curr_ret_stack = -1?

Not sure what you mean. This function would just return the value of curr_ret_stack, it didn't modify it.

-- Steve

From: "Steven Rostedt (VMware)" <[email protected]>

Move the function function_graph_get_addr() to fgraph.c, as the management of the curr_ret_stack is going to change, and all the accesses to ret_stack needs to be done in fgraph.c.

Signed-off-by Steven Rostedt (VMware) <[email protected]>

---
 kernel/trace/fgraph.c                | 55 ++++++++++++++++++++++++++++
 kernel/trace/trace_functions_graph.c | 55 ----------------------------
 2 files changed, 55 insertions(+), 55 deletions(-)

kernel/trace/fgraph.c +55 -0

index f3a89ecac671..c7d612897e33 100644
@@ -233,6 +233,61 @@ unsigned long ftrace_return_to_handler(unsigned long frame_pointer)
 	return ret;
 }
 
+/**
+ * ftrace_graph_ret_addr - convert a potentially modified stack return address
+ *			   to its original value
+ *
+ * This function can be called by stack unwinding code to convert a found stack
+ * return address ('ret') to its original value, in case the function graph
+ * tracer has modified it to be 'return_to_handler'.  If the address hasn't
+ * been modified, the unchanged value of 'ret' is returned.
+ *
+ * 'idx' is a state variable which should be initialized by the caller to zero
+ * before the first call.
+ *
+ * 'retp' is a pointer to the return address on the stack.  It's ignored if
+ * the arch doesn't have HAVE_FUNCTION_GRAPH_RET_ADDR_PTR defined.
+ */
+#ifdef HAVE_FUNCTION_GRAPH_RET_ADDR_PTR
+unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx,
+				    unsigned long ret, unsigned long *retp)
+{
+	int index = task->curr_ret_stack;
+	int i;
+
+	if (ret != (unsigned long)return_to_handler)
+		return ret;
+
+	if (index < 0)
+		return ret;
+
+	for (i = 0; i <= index; i++)
+		if (task->ret_stack[i].retp == retp)
+			return task->ret_stack[i].ret;
+
+	return ret;
+}
+#else /* !HAVE_FUNCTION_GRAPH_RET_ADDR_PTR */
+unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx,
+				    unsigned long ret, unsigned long *retp)
+{
+	int task_idx;
+
+	if (ret != (unsigned long)return_to_handler)
+		return ret;
+
+	task_idx = task->curr_ret_stack;
+
+	if (!task->ret_stack || task_idx < *idx)
+		return ret;
+
+	task_idx -= *idx;
+	(*idx)++;
+
+	return task->ret_stack[task_idx].ret;
+}
+#endif /* HAVE_FUNCTION_GRAPH_RET_ADDR_PTR */
+
 static struct ftrace_ops graph_ops = {
 	.func			= ftrace_stub,
 	.flags			= FTRACE_OPS_FL_RECURSION_SAFE |

kernel/trace/trace_functions_graph.c +0 -55

index 7c7fd13d2373..0f9cbc30645d 100644
@@ -90,61 +90,6 @@ static void
 print_graph_duration(struct trace_array *tr, unsigned long long duration,
 		     struct trace_seq *s, u32 flags);
 
-/**
- * ftrace_graph_ret_addr - convert a potentially modified stack return address
- *			   to its original value
- *
- * This function can be called by stack unwinding code to convert a found stack
- * return address ('ret') to its original value, in case the function graph
- * tracer has modified it to be 'return_to_handler'.  If the address hasn't
- * been modified, the unchanged value of 'ret' is returned.
- *
- * 'idx' is a state variable which should be initialized by the caller to zero
- * before the first call.
- *
- * 'retp' is a pointer to the return address on the stack.  It's ignored if
- * the arch doesn't have HAVE_FUNCTION_GRAPH_RET_ADDR_PTR defined.
- */
-#ifdef HAVE_FUNCTION_GRAPH_RET_ADDR_PTR
-unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx,
-				    unsigned long ret, unsigned long *retp)
-{
-	int index = task->curr_ret_stack;
-	int i;
-
-	if (ret != (unsigned long)return_to_handler)
-		return ret;
-
-	if (index < 0)
-		return ret;
-
-	for (i = 0; i <= index; i++)
-		if (task->ret_stack[i].retp == retp)
-			return task->ret_stack[i].ret;
-
-	return ret;
-}
-#else /* !HAVE_FUNCTION_GRAPH_RET_ADDR_PTR */
-unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx,
-				    unsigned long ret, unsigned long *retp)
-{
-	int task_idx;
-
-	if (ret != (unsigned long)return_to_handler)
-		return ret;
-
-	task_idx = task->curr_ret_stack;
-
-	if (!task->ret_stack || task_idx < *idx)
-		return ret;
-
-	task_idx -= *idx;
-	(*idx)++;
-
-	return task->ret_stack[task_idx].ret;
-}
-#endif /* HAVE_FUNCTION_GRAPH_RET_ADDR_PTR */
-
 int __trace_graph_entry(struct trace_array *tr,
 				struct ftrace_graph_ent *trace,
 				unsigned long flags,

Signature · 1 line

2.19.1

Quoted 5 lines · Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>

Move the function function_graph_get_addr() to fgraph.c, as the management of the curr_ret_stack is going to change, and all the accesses to ret_stack needs to be done in fgraph.c.

s/ftrace_graph_get_addr/ftrace_graph_ret_addr/

From: "Steven Rostedt (VMware)" <[email protected]>

In order to make it possible to have multiple callbacks registered with the function_graph tracer, the retstack needs to be converted from an array of ftrace_ret_stack structures to an array of longs. This will allow to store the list of callbacks on the stack for the return side of the functions.

[ Note, this currently breaks architectures that access the ret_stack of a
task to handle unwinding when 'return_to_handler' is on the stack ]

Signed-off-by Steven Rostedt (VMware) <[email protected]>

---
 include/linux/sched.h |   2 +-
 kernel/trace/fgraph.c | 123 +++++++++++++++++++++++-------------------
 2 files changed, 70 insertions(+), 55 deletions(-)

include/linux/sched.h +1 -1

index d6183a55e8eb..71a084a300da 100644
@@ -1119,7 +1119,7 @@ struct task_struct {
 	int				curr_ret_depth;
 
 	/* Stack of return addresses for return function tracing: */
-	struct ftrace_ret_stack		*ret_stack;
+	unsigned long			*ret_stack;
 
 	/* Timestamp for last schedule: */
 	unsigned long long		ftrace_timestamp;

kernel/trace/fgraph.c +11 -0

index 9b85638ecded..1389fe39f64c 100644
@@ -23,6 +23,17 @@
 #define ASSIGN_OPS_HASH(opsname, val)
 #endif
 
+#define FGRAPH_RET_SIZE (sizeof(struct ftrace_ret_stack))
+#define FGRAPH_RET_INDEX (ALIGN(FGRAPH_RET_SIZE, sizeof(long)) / sizeof(long))
+#define SHADOW_STACK_SIZE (FTRACE_RETFUNC_DEPTH * FGRAPH_RET_SIZE)
+#define SHADOW_STACK_INDEX			\
+	(ALIGN(SHADOW_STACK_SIZE, sizeof(long)) / sizeof(long))
+#define SHADOW_STACK_MAX_INDEX (SHADOW_STACK_INDEX - FGRAPH_RET_INDEX)
+
+#define RET_STACK(t, index) ((struct ftrace_ret_stack *)(&(t)->ret_stack[index]))
+#define RET_STACK_INC(c) ({ c += FGRAPH_RET_INDEX; })
+#define RET_STACK_DEC(c) ({ c -= FGRAPH_RET_INDEX; })
+

kernel/trace/fgraph.c +1 -1

@@ -514,7 +531,7 @@ void ftrace_graph_init_task(struct task_struct *t)
 
 void ftrace_graph_exit_task(struct task_struct *t)
 {
-	struct ftrace_ret_stack	*ret_stack = t->ret_stack;
+	unsigned long *ret_stack = t->ret_stack;
 
 	t->ret_stack = NULL;
 	/* NULL must become visible to IRQs before we free it: */

@@ -526,12 +543,10 @@ void ftrace_graph_exit_task(struct task_struct *t)
 /* Allocate a return stack for each task */
 static int start_graph_tracing(void)

Quoted 48 lines · Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>

In order to make it possible to have multiple callbacks registered with the function_graph tracer, the retstack needs to be converted from an array of ftrace_ret_stack structures to an array of longs. This will allow to store the list of callbacks on the stack for the return side of the functions.

[ Note, this currently breaks architectures that access the ret_stack of a
task to handle unwinding when 'return_to_handler' is on the stack ]

Signed-off-by Steven Rostedt (VMware) <[email protected]>

---
 include/linux/sched.h |   2 +-
 kernel/trace/fgraph.c | 123 +++++++++++++++++++++++-------------------
 2 files changed, 70 insertions(+), 55 deletions(-)

include/linux/sched.h +1 -1

index d6183a55e8eb..71a084a300da 100644
@@ -1119,7 +1119,7 @@ struct task_struct {
 	int				curr_ret_depth;
 
 	/* Stack of return addresses for return function tracing: */
-	struct ftrace_ret_stack		*ret_stack;
+	unsigned long			*ret_stack;
 
 	/* Timestamp for last schedule: */
 	unsigned long long		ftrace_timestamp;

kernel/trace/fgraph.c +11 -0

index 9b85638ecded..1389fe39f64c 100644
@@ -23,6 +23,17 @@
 #define ASSIGN_OPS_HASH(opsname, val)
 #endif
 
+#define FGRAPH_RET_SIZE (sizeof(struct ftrace_ret_stack))
+#define FGRAPH_RET_INDEX (ALIGN(FGRAPH_RET_SIZE, sizeof(long)) / sizeof(long))
+#define SHADOW_STACK_SIZE (FTRACE_RETFUNC_DEPTH * FGRAPH_RET_SIZE)
+#define SHADOW_STACK_INDEX			\
+	(ALIGN(SHADOW_STACK_SIZE, sizeof(long)) / sizeof(long))
+#define SHADOW_STACK_MAX_INDEX (SHADOW_STACK_INDEX - FGRAPH_RET_INDEX)
+
+#define RET_STACK(t, index) ((struct ftrace_ret_stack *)(&(t)->ret_stack[index]))
+#define RET_STACK_INC(c) ({ c += FGRAPH_RET_INDEX; })
+#define RET_STACK_DEC(c) ({ c -= FGRAPH_RET_INDEX; })
+

[...]

Quoted 12 lines

kernel/trace/fgraph.c +1 -1

@@ -514,7 +531,7 @@ void ftrace_graph_init_task(struct task_struct *t)
 
 void ftrace_graph_exit_task(struct task_struct *t)
 {
-	struct ftrace_ret_stack	*ret_stack = t->ret_stack;
+	unsigned long *ret_stack = t->ret_stack;
 
 	t->ret_stack = NULL;
 	/* NULL must become visible to IRQs before we free it: */

@@ -526,12 +543,10 @@ void ftrace_graph_exit_task(struct task_struct *t)
 /* Allocate a return stack for each task */
 static int start_graph_tracing(void)

kernel/trace/fgraph.c void ftrace_graph_exit_task(struct task_struct *t) · line 550

Quoted 10 lines

kernel/trace/fgraph.c +2 -4

 {
-	struct ftrace_ret_stack **ret_stack_list;
+	unsigned long **ret_stack_list;
 	int ret, cpu;
 
-	ret_stack_list = kmalloc_array(FTRACE_RETSTACK_ALLOC_SIZE,
-				       sizeof(struct ftrace_ret_stack *),
-				       GFP_KERNEL);
+	ret_stack_list = kmalloc(SHADOW_STACK_SIZE, GFP_KERNEL);
 

I had dumped the fgraph size related macros to understand the patch better, I got:

[    0.909528] val of FGRAPH_RET_SIZE is 40
[    0.910250] val of FGRAPH_RET_INDEX is 5
[    0.910866] val of FGRAPH_ARRAY_SIZE is 16
[    0.911488] val of FGRAPH_ARRAY_MASK is 255
[    0.912134] val of FGRAPH_MAX_INDEX is 16
[    0.912751] val of FGRAPH_INDEX_SHIFT is 8
[    0.913382] val of FGRAPH_FRAME_SIZE is 168
[    0.914033] val of FGRAPH_FRAME_INDEX is 21
                      FTRACE_RETFUNC_DEPTH is 50
[    0.914686] val of SHADOW_STACK_SIZE is 8400

I had a concern about memory overhead per-task. It seems the total memory needed per task for the stack is 8400 bytes (with my configuration with FUNCTION_PROFILE
turned off).

Where as before it would be 32 * 40 = 1280 bytes. That looks like ~7 times more than before.

On my system with ~4000 threads, that becomes ~32MB which seems a bit wasteful especially if there was only one or 2 function graph callbacks registered and most of the callback array in the stack isn't used.

Could we make the array size configurable at compile time and start it with a small number like 4 or 6?

Also for patches 1 through 10:

Reviewed-by Joel Fernandes (Google) <[email protected]>

thanks,

- Joel

Quoted 72 lines

Quoted 30 lines

include/linux/sched.h +1 -1

@@ -1119,7 +1119,7 @@ struct task_struct {
 	int				curr_ret_depth;
 
 	/* Stack of return addresses for return function tracing: */
-	struct ftrace_ret_stack		*ret_stack;
+	unsigned long			*ret_stack;
 
 	/* Timestamp for last schedule: */
 	unsigned long long		ftrace_timestamp;

kernel/trace/fgraph.c +11 -0

index 9b85638ecded..1389fe39f64c 100644
@@ -23,6 +23,17 @@
 #define ASSIGN_OPS_HASH(opsname, val)
 #endif
 
+#define FGRAPH_RET_SIZE (sizeof(struct ftrace_ret_stack))
+#define FGRAPH_RET_INDEX (ALIGN(FGRAPH_RET_SIZE, sizeof(long)) / sizeof(long))
+#define SHADOW_STACK_SIZE (FTRACE_RETFUNC_DEPTH * FGRAPH_RET_SIZE)
+#define SHADOW_STACK_INDEX			\
+	(ALIGN(SHADOW_STACK_SIZE, sizeof(long)) / sizeof(long))
+#define SHADOW_STACK_MAX_INDEX (SHADOW_STACK_INDEX - FGRAPH_RET_INDEX)
+
+#define RET_STACK(t, index) ((struct ftrace_ret_stack *)(&(t)->ret_stack[index]))
+#define RET_STACK_INC(c) ({ c += FGRAPH_RET_INDEX; })
+#define RET_STACK_DEC(c) ({ c -= FGRAPH_RET_INDEX; })
+  

[...]

Quoted 12 lines

kernel/trace/fgraph.c +1 -1

@@ -514,7 +531,7 @@ void ftrace_graph_init_task(struct task_struct *t)
 
 void ftrace_graph_exit_task(struct task_struct *t)
 {
-	struct ftrace_ret_stack	*ret_stack = t->ret_stack;
+	unsigned long *ret_stack = t->ret_stack;
 
 	t->ret_stack = NULL;
 	/* NULL must become visible to IRQs before we free it: */

@@ -526,12 +543,10 @@ void ftrace_graph_exit_task(struct task_struct *t)
 /* Allocate a return stack for each task */
 static int start_graph_tracing(void)

kernel/trace/fgraph.c void ftrace_graph_exit_task(struct task_struct *t) · line 550

Quoted 10 lines

kernel/trace/fgraph.c +2 -4

 {
-	struct ftrace_ret_stack **ret_stack_list;
+	unsigned long **ret_stack_list;
 	int ret, cpu;
 
-	ret_stack_list = kmalloc_array(FTRACE_RETSTACK_ALLOC_SIZE,
-				       sizeof(struct ftrace_ret_stack *),
-				       GFP_KERNEL);
+	ret_stack_list = kmalloc(SHADOW_STACK_SIZE, GFP_KERNEL);
 

I had dumped the fgraph size related macros to understand the patch better, I got:

[    0.909528] val of FGRAPH_RET_SIZE is 40
[    0.910250] val of FGRAPH_RET_INDEX is 5
[    0.910866] val of FGRAPH_ARRAY_SIZE is 16
[    0.911488] val of FGRAPH_ARRAY_MASK is 255
[    0.912134] val of FGRAPH_MAX_INDEX is 16
[    0.912751] val of FGRAPH_INDEX_SHIFT is 8
[    0.913382] val of FGRAPH_FRAME_SIZE is 168
[    0.914033] val of FGRAPH_FRAME_INDEX is 21
                      FTRACE_RETFUNC_DEPTH is 50
[    0.914686] val of SHADOW_STACK_SIZE is 8400

I had a concern about memory overhead per-task. It seems the total memory needed per task for the stack is 8400 bytes (with my configuration with FUNCTION_PROFILE
turned off).

Quoted 2 lines

Where as before it would be 32 * 40 = 1280 bytes. That looks like ~7 times more than before.

Hmm, this seems too big... I thought the shadow-stack size should be smaller than 1 page (4kB). Steve, can we give a 4k page for shadow stack and define FTRACE_RETFUNC_DEPTH = 4096 / FGRAPH_RET_SIZE ?

Quoted 4 lines

On my system with ~4000 threads, that becomes ~32MB which seems a bit wasteful especially if there was only one or 2 function graph callbacks registered and most of the callback array in the stack isn't used.

Quoted 3 lines

Could we make the array size configurable at compile time and start it with a small number like 4 or 6?

Or, we can introduce online setting :)

Quoted 72 lines · On Tue, 27 Nov 2018 01:07:55 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 72 lines

Quoted 30 lines

include/linux/sched.h +1 -1

@@ -1119,7 +1119,7 @@ struct task_struct {
 	int				curr_ret_depth;
 
 	/* Stack of return addresses for return function tracing: */
-	struct ftrace_ret_stack		*ret_stack;
+	unsigned long			*ret_stack;
 
 	/* Timestamp for last schedule: */
 	unsigned long long		ftrace_timestamp;

kernel/trace/fgraph.c +11 -0

index 9b85638ecded..1389fe39f64c 100644
@@ -23,6 +23,17 @@
 #define ASSIGN_OPS_HASH(opsname, val)
 #endif
 
+#define FGRAPH_RET_SIZE (sizeof(struct ftrace_ret_stack))
+#define FGRAPH_RET_INDEX (ALIGN(FGRAPH_RET_SIZE, sizeof(long)) / sizeof(long))
+#define SHADOW_STACK_SIZE (FTRACE_RETFUNC_DEPTH * FGRAPH_RET_SIZE)
+#define SHADOW_STACK_INDEX			\
+	(ALIGN(SHADOW_STACK_SIZE, sizeof(long)) / sizeof(long))
+#define SHADOW_STACK_MAX_INDEX (SHADOW_STACK_INDEX - FGRAPH_RET_INDEX)
+
+#define RET_STACK(t, index) ((struct ftrace_ret_stack *)(&(t)->ret_stack[index]))
+#define RET_STACK_INC(c) ({ c += FGRAPH_RET_INDEX; })
+#define RET_STACK_DEC(c) ({ c -= FGRAPH_RET_INDEX; })
+  

[...]

Quoted 12 lines

kernel/trace/fgraph.c +1 -1

@@ -514,7 +531,7 @@ void ftrace_graph_init_task(struct task_struct *t)
 
 void ftrace_graph_exit_task(struct task_struct *t)
 {
-	struct ftrace_ret_stack	*ret_stack = t->ret_stack;
+	unsigned long *ret_stack = t->ret_stack;
 
 	t->ret_stack = NULL;
 	/* NULL must become visible to IRQs before we free it: */

@@ -526,12 +543,10 @@ void ftrace_graph_exit_task(struct task_struct *t)
 /* Allocate a return stack for each task */
 static int start_graph_tracing(void)

kernel/trace/fgraph.c void ftrace_graph_exit_task(struct task_struct *t) · line 550

Quoted 10 lines

kernel/trace/fgraph.c +2 -4

 {
-	struct ftrace_ret_stack **ret_stack_list;
+	unsigned long **ret_stack_list;
 	int ret, cpu;
 
-	ret_stack_list = kmalloc_array(FTRACE_RETSTACK_ALLOC_SIZE,
-				       sizeof(struct ftrace_ret_stack *),
-				       GFP_KERNEL);
+	ret_stack_list = kmalloc(SHADOW_STACK_SIZE, GFP_KERNEL);
 

I had dumped the fgraph size related macros to understand the patch better, I got:

[    0.909528] val of FGRAPH_RET_SIZE is 40
[    0.910250] val of FGRAPH_RET_INDEX is 5
[    0.910866] val of FGRAPH_ARRAY_SIZE is 16
[    0.911488] val of FGRAPH_ARRAY_MASK is 255
[    0.912134] val of FGRAPH_MAX_INDEX is 16
[    0.912751] val of FGRAPH_INDEX_SHIFT is 8
[    0.913382] val of FGRAPH_FRAME_SIZE is 168
[    0.914033] val of FGRAPH_FRAME_INDEX is 21
                      FTRACE_RETFUNC_DEPTH is 50
[    0.914686] val of SHADOW_STACK_SIZE is 8400

I had a concern about memory overhead per-task. It seems the total memory needed per task for the stack is 8400 bytes (with my configuration with FUNCTION_PROFILE
turned off).

Quoted 6 lines

Quoted 2 lines

Where as before it would be 32 * 40 = 1280 bytes. That looks like ~7 times more than before.

Hmm, this seems too big... I thought the shadow-stack size should be smaller than 1 page (4kB). Steve, can we give a 4k page for shadow stack and define FTRACE_RETFUNC_DEPTH = 4096 / FGRAPH_RET_SIZE ?

For the first pass, I decided not to worry about the size. It made the code less complex :-)

Yes, I plan on working on making the size of the stack smaller, but that will probably be added on patches to do so.

Quoted 3 lines

Quoted 3 lines

On my system with ~4000 threads, that becomes ~32MB which seems a bit wasteful especially if there was only one or 2 function graph callbacks registered and most of the callback array in the stack isn't used.

Note, all 4000 threads could be doing those trace backs, and if you are doing full function graph tracing, it will use a lot.

Quoted 4 lines

Quoted 2 lines

Could we make the array size configurable at compile time and start it with a small number like 4 or 6?

Or, we can introduce online setting :)

Yes, that can easily be added. I didn't try to make this into the perfect solution, I wanted a solid one first, and then massage it into something that is more efficient, both with memory consumption and performance.

Joel and Masami, thanks for the feedback.

Also for patches 1 through 10:

Reviewed-by Joel Fernandes (Google) <[email protected]>

Well the fuction graph tracer is arguably the strongest of the tracers. It shows both the entrance and exit of a function, can give the timings of a function, and shows the execution of the code quite nicely.

But it has one major flaw.

It can't let more than one user access it at a time.

Quoted 7 lines · Steven Rostedt

Well the fuction graph tracer is arguably the strongest of the tracers. It shows both the entrance and exit of a function, can give the timings of a function, and shows the execution of the code quite nicely.

But it has one major flaw.

It can't let more than one user access it at a time.

The reason I 'never' use it is performance, it _sucks_.. I've never ran into the multi-user issue.

So while I don't think the rewrite is bad, this argument here is.

Quoted 7 lines · On Thu, 22 Nov 2018 11:08:12 +0100 Peter Zijlstra <[email protected]> wrote:

Quoted 6 lines · Steven Rostedt

Well the fuction graph tracer is arguably the strongest of the tracers. It shows both the entrance and exit of a function, can give the timings of a function, and shows the execution of the code quite nicely.

But it has one major flaw.

Quoted 4 lines

Quoted 1 line

It can't let more than one user access it at a time.

The reason I 'never' use it is performance, it _sucks_.. I've never ran into the multi-user issue.

And performance is also something to fix (it has improved lately, you probably haven't noticed).

Quoted 1 line

So while I don't think the rewrite is bad, this argument here is.

Except that we plan on merging kretprobe with function graph tracing. This also solves the issue that Mark Rutland has with ret protection. He has a solution for function graph tracing, but not with kretprobes.

And yes, there's also the case of being able to trace to different buffers where you can have a full function graph tracing enabled and also trace a subset that you want to have.

Just because you don't need it, doesn't mean it's not needed by others.

I talked with many of you at Plumbers about rewriting the function graph tracer. Well, this is it. I was originally going to produce just a proof of concept, but when I found that I had to fix a design flaw and that covered all the arch code anyway, I decided to do more of a RFC patch set.

I probably should add more comments to the code, and update the function graph design documentation, but I wanted to get this out before the US Turkey day for your enjoyment while you try to let your pants buckle again.

Why the rewrite?

Well the fuction graph tracer is arguably the strongest of the tracers. It shows both the entrance and exit of a function, can give the timings of a function, and shows the execution of the code quite nicely.

But it has one major flaw.

It can't let more than one user access it at a time. The function tracer has had that feature for years now, but due to the design of the function graph tracer it was difficult to implement. Why?

Because you must maintain the state of a three-tuple.

Task, Function, Callback

The state is determined at by the entryfunc and must be passed to the retfunc when the function being traced returns. But this is not an easy task, as that state can be different for each task, each function and each callback.

What's the solution? I use the shadow stack that is already being used to store the function return addresses.

A big thanks to Masami Hiramatsu for suggesting this idea!

For now, I only allow an 16 users of the function graph tracer at a time. That should be more than enough. I create an array of 16 fgraph_ops pointers. When a user registers their fgraph_ops to the function graph tracer, it is assigned an index into that array, which will hold a pointer to the fgraph_ops being registered.

On entry of the function, the array is iterated and each entryfunc of the fgraph_ops in the array is called. If the entryfunc returns non-zero, then the index of that fgraph_ops is pushed on the shadow stack (along with the index to the "ret_stack entry" structure, for fast access to it). If the entryfunc returns zero, then it is ignored. If at least one function returned non-zero then the return of the traced function will also be traced.

On the return of the function, the shadow stack is examined and all the indexes that were pushed on the stack is read, and each fgraph_ops retfunc is called in the reverse order.

When a fgraph_ops is unregistered, its index in the array is set to point to a "stub" fgraph_ops that holds stub functions that just return "0" for the entryfunc and does nothing for the retfunc. This is because the retfunc may be called literally days after the entryfunc is called and we want to be able to free the fgraph_ops that is unregistered.

Note, if another fgraph_ops is registered in the same location, its retfunc may be called that was set by a previous fgraph_ops. This is not a regression because that's what can happen today if you unregister a callback from the current function_graph tracer and register another one. If this is an issue, there are ways to solve it.

Quoted 5 lines

Note, if another fgraph_ops is registered in the same location, its retfunc may be called that was set by a previous fgraph_ops. This is not a regression because that's what can happen today if you unregister a callback from the current function_graph tracer and register another one. If this is an issue, there are ways to solve it.

Yeah, I need the solution, maybe an API to get correct return address? :)

By the way, are there any way to hold a private data on each ret_stack entry? Since kretprobe supports "entry data" passed from entry_handler to return handler, we have to store the data or data-instance on the ret_stack.

This feature is used by systemtap to save the function entry data, like function parameters etc., so that return handler analyzes the parameters with return value.

Quoted 6 lines · On Mon, 26 Nov 2018 18:21:12 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 5 lines

Note, if another fgraph_ops is registered in the same location, its retfunc may be called that was set by a previous fgraph_ops. This is not a regression because that's what can happen today if you unregister a callback from the current function_graph tracer and register another one. If this is an issue, there are ways to solve it.

Quoted 1 line

Yeah, I need the solution, maybe an API to get correct return address? :)

One way to solve this is to also have a counter array that gets updated every time the index array gets updated. And save the counter to the shadow stack index as well. This way, we only call the return if the counter on the stack matches what's in the counter on the counter array for the index.

Quoted 7 lines

By the way, are there any way to hold a private data on each ret_stack entry? Since kretprobe supports "entry data" passed from entry_handler to return handler, we have to store the data or data-instance on the ret_stack.

This feature is used by systemtap to save the function entry data, like function parameters etc., so that return handler analyzes the parameters with return value.

Yes, I remember you telling me about this at plumbers, and while I was writing this code I had that in mind. It wouldn't be too hard to implement, I just left it off for now. I also left it off because I have some questions about what exactly is needed. What size do you require to be stored. Especially if we want to keep the shadow stack smaller. I was going to find a way to implement some of the data that is already stored via the ret_stack with this instead, and make the ret_stack entry smaller. Should we allow just sizeof(long)*3? or just let user choose any size and if they run out of stack, then too bad. We just wont let it crash.

Quoted 5 lines

One way to solve this is to also have a counter array that gets updated every time the index array gets updated. And save the counter to the shadow stack index as well. This way, we only call the return if the counter on the stack matches what's in the counter on the counter array for the index.

Hmm, but we already know the current stack "header" entry when calling handlers, don't we? I thought we just calcurate out from curr_ret_stack.

Quoted 9 lines

Quoted 8 lines

By the way, are there any way to hold a private data on each ret_stack entry? Since kretprobe supports "entry data" passed from entry_handler to return handler, we have to store the data or data-instance on the ret_stack.

This feature is used by systemtap to save the function entry data, like function parameters etc., so that return handler analyzes the parameters with return value.

Quoted 10 lines

Yes, I remember you telling me about this at plumbers, and while I was writing this code I had that in mind. It wouldn't be too hard to implement, I just left it off for now. I also left it off because I have some questions about what exactly is needed. What size do you require to be stored. Especially if we want to keep the shadow stack smaller. I was going to find a way to implement some of the data that is already stored via the ret_stack with this instead, and make the ret_stack entry smaller. Should we allow just sizeof(long)*3? or just let user choose any size and if they run out of stack, then too bad. We just wont let it crash.

I need only sizeof(unsigned long). If the kretprobe user requires more, it will be fall back to current method -- get an "instance" and store its address to the entry :-)

Quoted 6 lines · On Thu, 29 Nov 2018 23:29:27 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 5 lines

One way to solve this is to also have a counter array that gets updated every time the index array gets updated. And save the counter to the shadow stack index as well. This way, we only call the return if the counter on the stack matches what's in the counter on the counter array for the index.

Quoted 2 lines

Hmm, but we already know the current stack "header" entry when calling handlers, don't we? I thought we just calcurate out from curr_ret_stack.

Basically we have this:

array: | &fgraph_ops_1 | &fgraph_ops_2 | &fgraph_ops_stub | ...

On entry of function we do:

	for (i = 0; i < array_entries; i++) {
		if (array[i]->entryfunc(...)) {
			push i onto ret_stack;
		}
	}

On the return side, we do:

	idx = pop ret_stack;

	array[idx]->retfunc(...);

We only call the retfunc of a fgraph_ops if it returned non-zero from its entryfunc(). The return can happen a long time from now (which is why I don't save the &fgraph_ops on the ret_stack, because then we would never be able to free it).

In the mean time, lets say we unregistered (and freed) fgraph_ops_2 and then added fgraph_ops_3, so the array looks like:

array: | &fgraph_ops_1 | &fgraph_ops_3 | &fgraph_ops_stub | ...

Then a function that was called when fgraph_ops_2 was on the stack returns, it will call array[1]->retfunc() which now belongs to fgraph_ops_3 and not fgraph_ops_2.

But if we add a counter array that gets updated when new ops are added to the array, we have this:

 cnt_array: |       4       |     2         |       0          |
     array: | &fgraph_ops_1 | &fgraph_ops_2 | &fgraph_ops_stub | ...

And do:

	for (i = 0; i < array_entries; i++) {
		if (array[i]->entryfunc(...)) {
			idx = cnt_array[i] << 8 | i;
			push idx onto ret_stack;
		}
	}

Then on return we have:

	idx = pop ret_stack;

	if (idx >> 8 == cnt_array[idx & 0xff])
		array[idx & 0xff]->retfunc(...);

It wouldn't call fgraph_ops_3 because we would change the cnt_array when we remove fgraph_ops_2 and the return would not match, as cnt_array[1] would then be "3".

Quoted 20 lines

Quoted 19 lines

Quoted 8 lines

By the way, are there any way to hold a private data on each ret_stack entry? Since kretprobe supports "entry data" passed from entry_handler to return handler, we have to store the data or data-instance on the ret_stack.

This feature is used by systemtap to save the function entry data, like function parameters etc., so that return handler analyzes the parameters with return value.

Yes, I remember you telling me about this at plumbers, and while I was writing this code I had that in mind. It wouldn't be too hard to implement, I just left it off for now. I also left it off because I have some questions about what exactly is needed. What size do you require to be stored. Especially if we want to keep the shadow stack smaller. I was going to find a way to implement some of the data that is already stored via the ret_stack with this instead, and make the ret_stack entry smaller. Should we allow just sizeof(long)*3? or just let user choose any size and if they run out of stack, then too bad. We just wont let it crash.

Quoted 3 lines

I need only sizeof(unsigned long). If the kretprobe user requires more, it will be fall back to current method -- get an "instance" and store its address to the entry :-)

Awesome, then this shouldn't be too hard to implement.

Quoted 12 lines · On Thu, 29 Nov 2018 11:46:52 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 8 lines · On Thu, 29 Nov 2018 23:29:27 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 5 lines

One way to solve this is to also have a counter array that gets updated every time the index array gets updated. And save the counter to the shadow stack index as well. This way, we only call the return if the counter on the stack matches what's in the counter on the counter array for the index.

Hmm, but we already know the current stack "header" entry when calling handlers, don't we? I thought we just calcurate out from curr_ret_stack.

Quoted 6 lines

Basically we have this:

array: | &fgraph_ops_1 | &fgraph_ops_2 | &fgraph_ops_stub | ...

On entry of function we do:

push header(including original ret_addr) onto ret_stack

Quoted 9 lines

	for (i = 0; i < array_entries; i++) {
		if (array[i]->entryfunc(...)) {
			push i onto ret_stack;
		}
	}

On the return side, we do:

Quoted 3 lines

	idx = pop ret_stack;

	array[idx]->retfunc(...);  

So at this point we have the header on ret_stack, don't we? :)

Anyway, I think we may provide an API for unwinder to find correct original return address form ret_stack. That is OK for me.

Quoted 5 lines

Quoted 3 lines

I need only sizeof(unsigned long). If the kretprobe user requires more, it will be fall back to current method -- get an "instance" and store its address to the entry :-)

Awesome, then this shouldn't be too hard to implement.

Oops, anyway I noticed that I must store a value on each area so that we can identify which kretprobe is using that if there are several kretprobes on same function. So, kretprobe implementation will be something like below.

kretprobe_retfunc(trace, regs)

{
	kp = get_kprobe(trace->func);

	if (private == to_kretprobe(kp)) // this is directly mapped to current kprobe
		goto found_kretprobe;

	if (!list_empty(&kp->list)) {	// we need to find from multiple kretprobes
		list_for_each_entry(kp, &kp->list, list)
			if (private == kp)
				goto found_kretprobe;
	}

	// Or this must be an instance
	struct kretprobe_instance *ri = trace->private;
	rp = ri->rp;
	if (valid_kretprobe(rp))
		rp->handler(ri, regs);
	kretprobe_recycle_instance(ri);
	goto out;

found_kretprobe:

	struct kretprobe_instance rii = {.rp = to_kretprobe(kp),
		.ret_addr=trace->ret, .task = current}
	rp->handler(&rii, regs);

out:

	return 0;
}

I think we talked about pt_regs, which is redundant for return probe, so it should be just a return value. (but how we pass it? trace->retval?)

That is OK for ftrace (but the transition needs more code).
And I would like to ask ebpf and systemtap people that is OK since it will change the kernel ABI.

Quoted 17 lines · On Fri, 30 Nov 2018 11:26:58 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 14 lines · On Thu, 29 Nov 2018 11:46:52 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 8 lines · On Thu, 29 Nov 2018 23:29:27 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 5 lines

One way to solve this is to also have a counter array that gets updated every time the index array gets updated. And save the counter to the shadow stack index as well. This way, we only call the return if the counter on the stack matches what's in the counter on the counter array for the index.

Hmm, but we already know the current stack "header" entry when calling handlers, don't we? I thought we just calcurate out from curr_ret_stack.

Basically we have this:

Quoted 5 lines

Quoted 4 lines

array: | &fgraph_ops_1 | &fgraph_ops_2 | &fgraph_ops_stub | ...

On entry of function we do:

push header(including original ret_addr) onto ret_stack

We can't put the ret_addr of the callback on the stack. What if that ret_addr is a module, and it gets unloaded? We must not call it.

Quoted 13 lines

Quoted 12 lines

	for (i = 0; i < array_entries; i++) {
		if (array[i]->entryfunc(...)) {
			push i onto ret_stack;
		}
	}

On the return side, we do:

	idx = pop ret_stack;

	array[idx]->retfunc(...);  

Quoted 4 lines

So at this point we have the header on ret_stack, don't we? :)

Anyway, I think we may provide an API for unwinder to find correct original return address form ret_stack. That is OK for me.

Yes. In fact, I have something that worked for that. I'll have to test it some more.

Quoted 36 lines

Quoted 5 lines

Quoted 3 lines

I need only sizeof(unsigned long). If the kretprobe user requires more, it will be fall back to current method -- get an "instance" and store its address to the entry :-)

Awesome, then this shouldn't be too hard to implement.

Oops, anyway I noticed that I must store a value on each area so that we can identify which kretprobe is using that if there are several kretprobes on same function. So, kretprobe implementation will be something like below.

kretprobe_retfunc(trace, regs)

{
	kp = get_kprobe(trace->func);

	if (private == to_kretprobe(kp)) // this is directly mapped to current kprobe
		goto found_kretprobe;

	if (!list_empty(&kp->list)) {	// we need to find from multiple kretprobes
		list_for_each_entry(kp, &kp->list, list)
			if (private == kp)
				goto found_kretprobe;
	}

	// Or this must be an instance
	struct kretprobe_instance *ri = trace->private;
	rp = ri->rp;
	if (valid_kretprobe(rp))
		rp->handler(ri, regs);
	kretprobe_recycle_instance(ri);
	goto out;

found_kretprobe:

	struct kretprobe_instance rii = {.rp = to_kretprobe(kp),
		.ret_addr=trace->ret, .task = current}
	rp->handler(&rii, regs);

Quoted 6 lines

out:

	return 0;
}

I think we talked about pt_regs, which is redundant for return probe, so it should be just a return value. (but how we pass it? trace->retval?)

Yeah, we can add that.

Quoted 3 lines

That is OK for ftrace (but the transition needs more code).
And I would like to ask ebpf and systemtap people that is OK since it will change the kernel ABI.

I agree.

-- Steve