Skip to contentSkip to conversation

function_graph: Add separate depth counter to prevent trace corruption

PATCH In review Steven Rostedt 9 participants 44 messages Nov 22, 2018 – Dec 3, 2018 1 message not in the archive

Subsystems (17)
lore
Share & tools
lore

Steven Rostedt

author maintainer
#0 lore raw

cover function_graph: Add separate depth counter to prevent trace corruption

While working on rewriting function graph tracer, I found a design flaw in the code. Commit 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback") tried to fix a bug that caused interrupts not to be traced if max_depth was set to 1 (for debugging NO_HZ_FULL), because if it came in just as another function that was being traced that had a depth of 1 was exiting. This caused the timing of being in the kernel to be off. The fix was simply to move the calling of the return function after the updating of curr_ret_stack index as that was what was being used.

The change log even says that it was safe to do this, but unfortunately it was not, and the barrier() there was specifically *for* that function (which shows why one should document barriers).

The problem is that the return callback may still be using what's on the shadow stack and by changing the shadow stack pointer, it may allow for another function being traced to overwrite that data. Note, if this happens, it will only cause garbage data to be traced and will not affect the actual operations of the kernel (ie. it wont crash).

Unfortunately just reverting that will bring back the old bug. The real way to fix that old bug is to create another counter to handle the depth, but this means that we need to change all the architectures that implement function graph tracing (that's 12 of them). Luckily, I need to do this anyway in my re-design so this is a good thing.

Since all the archictectures do basicall the same thing to prepare the function graph trace to be traced, I made a generic function that they all can use and simplified the logic of all the architectures. Then I'm able to fix the design problem in one place.

I pushed this code up to let zero-day have a whack at it, and I also downloaded the latest 8.1.0 cross compilers for all the archs that are affected and compiled tested them all (and got rid of all the warnings I introduced as well).

I marked this all for stable, but in reality it may not need to be ported as it will probably not be trivial to do so, becaues you'll need to also fix the architectures that are no longer used (although do we care about them?). But if someone really cares about correct timings of the function graph profiler when the options/graph-time is set to zero, then be my guest.

Feel free to test this! I'll be pushing this to linux-next and let it sit there a week or so before pushing it to Linus.

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

Head SHA1: 1ffd62e282a649483afb0bb4d76f7244b3c10075

Steven Rostedt (VMware) (18):
function_graph: Create function_graph_enter() to consolidate architecture code x86/function_graph: Simplify with function_graph_entry()
ARM: function_graph: Simplify with function_graph_entry()
arm64: function_graph: Simplify with function_graph_entry()
microblaze: function_graph: Simplify with function_graph_entry()
MIPS: function_graph: Simplify with function_graph_entry()
nds32: function_graph: Simplify with function_graph_entry()
parisc: function_graph: Simplify with function_graph_entry() powerpc/function_graph: Simplify with function_graph_entry() riscv/function_graph: Simplify with function_graph_entry() s390/function_graph: Simplify with function_graph_entry() sh/function_graph: Simplify with function_graph_entry() sparc/function_graph: Simplify with function_graph_entry()
function_graph: Make ftrace_push_return_trace() static
function_graph: Use new curr_ret_depth to manage depth instead of curr_ret_stack
function_graph: Move return callback before update of curr_ret_stack
function_graph: Reverse the order of pushing the ret_stack and the callback
function_graph: Have profiler use curr_ret_stack and not depth

----

 arch/arm/kernel/ftrace.c             | 17 +------------
 arch/arm64/kernel/ftrace.c           | 15 +----------
 arch/microblaze/kernel/ftrace.c      | 15 ++---------
 arch/mips/kernel/ftrace.c            | 14 ++---------
 arch/nds32/kernel/ftrace.c           | 18 ++-----------
 arch/parisc/kernel/ftrace.c          | 17 +++----------
 arch/powerpc/kernel/trace/ftrace.c   | 15 ++---------
 arch/riscv/kernel/ftrace.c           | 14 ++---------
 arch/s390/kernel/ftrace.c            | 13 ++--------
 arch/sh/kernel/ftrace.c              | 16 ++----------
 arch/sparc/kernel/ftrace.c           | 11 +-------
 arch/x86/kernel/ftrace.c             | 15 +----------
 include/linux/ftrace.h               |  4 +--
 include/linux/sched.h                |  1 +
 kernel/trace/ftrace.c                |  7 ++++--
 kernel/trace/trace_functions_graph.c | 49 ++++++++++++++++++++++++++++--------
 16 files changed, 67 insertions(+), 174 deletions(-)
author maintainer
#1 lore raw

PATCH 1/18 function_graph: Create function_graph_enter() to consolidate architecture code

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

Currently all the architectures do basically the same thing in preparing the function graph tracer on entry to a function. This code can be pulled into a generic location and then this will allow the function graph tracer to be fixed, as well as extended.

Create a new function graph helper function_graph_enter() that will call the hook function (ftrace_graph_entry) and the shadow stack operation (ftrace_push_return_trace), and remove the need of the architecture code to manage the shadow stack.

This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.

Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
---
 include/linux/ftrace.h               |  3 +++
 kernel/trace/trace_functions_graph.c | 16 ++++++++++++++++
 2 files changed, 19 insertions(+)
include/linux/ftrace.h +3 -0
index a397907e8d72..5717e8f81c59 100644
@@ -779,6 +779,9 @@ extern void return_to_handler(void);
 extern int
 ftrace_push_return_trace(unsigned long ret, unsigned long func, int *depth,
 			 unsigned long frame_pointer, unsigned long *retp);
+extern int
+function_graph_enter(unsigned long ret, unsigned long func,
+		     unsigned long frame_pointer, unsigned long *retp);
 
 unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx,
 				    unsigned long ret, unsigned long *retp);
kernel/trace/trace_functions_graph.c +16 -0
index 169b3c44ee97..28f2602435d0 100644
@@ -182,6 +182,22 @@ ftrace_push_return_trace(unsigned long ret, unsigned long func, int *depth,
 	return 0;
 }
 
+int function_graph_enter(unsigned long ret, unsigned long func,
+			 unsigned long frame_pointer, unsigned long *retp)
+{
+	struct ftrace_graph_ent trace;
+
+	trace.func = func;
+	trace.depth = current->curr_ret_stack + 1;
+
+	/* Only trace if the calling function expects to */
+	if (!ftrace_graph_entry(&trace))
+		return -EBUSY;
+
+	return ftrace_push_return_trace(ret, func, &trace.depth,
+					frame_pointer, retp);
+}
+
 /* Retrieve a function return address to the trace stack on thread info.*/
 static void
 ftrace_pop_return_trace(struct ftrace_graph_ret *trace, unsigned long *ret,
Signature · 1 line
2.19.1
author maintainer
#2 lore raw

PATCH 2/18 x86/function_graph: Simplify with function_graph_entry()

In reply to Steven Rostedt

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

The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().

Have x86 use the new code, and remove the shadow stack management as well as having to set up the trace structure.

This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.

Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
---
 arch/x86/kernel/ftrace.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)
arch/x86/kernel/ftrace.c +1 -14
index 01ebcb6f263e..7ee8067cbf45 100644
@@ -994,7 +994,6 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
 {
 	unsigned long old;
 	int faulted;
-	struct ftrace_graph_ent trace;
 	unsigned long return_hooker = (unsigned long)
 				&return_to_handler;
 
@@ -1046,19 +1045,7 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
 		return;
 	}
 
-	trace.func = self_addr;
-	trace.depth = current->curr_ret_stack + 1;
-
-	/* Only trace if the calling function expects to */
-	if (!ftrace_graph_entry(&trace)) {
+	if (function_graph_enter(old, self_addr, frame_pointer, parent))
 		*parent = old;
-		return;
-	}
-
-	if (ftrace_push_return_trace(old, self_addr, &trace.depth,
-				     frame_pointer, parent) == -EBUSY) {
-		*parent = old;
-		return;
-	}
 }
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
Signature · 1 line
2.19.1
author maintainer
#3 lore raw

PATCH 3/18 ARM: function_graph: Simplify with function_graph_entry()

In reply to Steven Rostedt

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

The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().

Have ARM use the new code, and remove the shadow stack management as well as having to set up the trace structure.

This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.

Cc Russell King <[email protected]>
Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
---
 arch/arm/kernel/ftrace.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)
arch/arm/kernel/ftrace.c +1 -16
index 0142fcfcc3d3..bda949fd84e8 100644
@@ -183,9 +183,7 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 			   unsigned long frame_pointer)
 {
 	unsigned long return_hooker = (unsigned long) &return_to_handler;
-	struct ftrace_graph_ent trace;
 	unsigned long old;
-	int err;
 
 	if (unlikely(atomic_read(&current->tracing_graph_pause)))
 		return;
@@ -193,21 +191,8 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 	old = *parent;
 	*parent = return_hooker;
 
-	trace.func = self_addr;
-	trace.depth = current->curr_ret_stack + 1;
-
-	/* Only trace if the calling function expects to */
-	if (!ftrace_graph_entry(&trace)) {
+	if (function_graph_enter(old, self_addr, frame_pointer, NULL))
 		*parent = old;
-		return;
-	}
-
-	err = ftrace_push_return_trace(old, self_addr, &trace.depth,
-				       frame_pointer, NULL);
-	if (err == -EBUSY) {
-		*parent = old;
-		return;
-	}
 }
 
 #ifdef CONFIG_DYNAMIC_FTRACE
Signature · 1 line
2.19.1
maintainer
#4 lore raw

PATCH 3/18 ARM: function_graph: Simplify with function_graph_entry()

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 03274a3ffb44 tracing/fgraph: Adjust fgraph depth before calling trace return callback.

The bot has tested the following trees: v4.19.3, v4.14.82, v4.9.138, v4.4.164, v3.18.126.

v4.19.3: Build OK!
v4.14.82: Build OK!
v4.9.138: Build OK!
v4.4.164: Failed to apply! Possible dependencies:
Unable to calculate

v3.18.126: Failed to apply! Possible dependencies:
Unable to calculate

How should we proceed with this patch?

--
Thanks,
Sasha

author maintainer
#5 lore raw

PATCH 4/18 arm64: function_graph: Simplify with function_graph_entry()

In reply to Steven Rostedt

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

The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().

Have arm64 use the new code, and remove the shadow stack management as well as having to set up the trace structure.

This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.

Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
---
 arch/arm64/kernel/ftrace.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)
arch/arm64/kernel/ftrace.c +1 -14
index 50986e388d2b..57e962290df3 100644
@@ -216,8 +216,6 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 {
 	unsigned long return_hooker = (unsigned long)&return_to_handler;
 	unsigned long old;
-	struct ftrace_graph_ent trace;
-	int err;
 
 	if (unlikely(atomic_read(&current->tracing_graph_pause)))
 		return;
@@ -229,18 +227,7 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 	 */
 	old = *parent;
 
-	trace.func = self_addr;
-	trace.depth = current->curr_ret_stack + 1;
-
-	/* Only trace if the calling function expects to */
-	if (!ftrace_graph_entry(&trace))
-		return;
-
-	err = ftrace_push_return_trace(old, self_addr, &trace.depth,
-				       frame_pointer, NULL);
-	if (err == -EBUSY)
-		return;
-	else
+	if (!function_graph_enter(old, self_addr, frame_pointer, NULL))
 		*parent = return_hooker;
 }
 
Signature · 1 line
2.19.1
Acked-by on v1 4/18
#6 lore raw

Re: [for-next][PATCH 04/18] arm64: function_graph: Simplify with function_graph_entry()

Quoted 18 lines · Steven Rostedt

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

The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().

Have arm64 use the new code, and remove the shadow stack management as well as having to set up the trace structure.

This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.

Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
Quoted 3 lines
---
 arch/arm64/kernel/ftrace.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

Minor nit: the subject refers to function_graph_entry(), but looks like you settled on function_graph_enter() in the end.

Anyway, looks like no change for us, so:

Will

author maintainer
#7 lore raw

Re: [for-next][PATCH 04/18] arm64: function_graph: Simplify with function_graph_entry()

Quoted 23 lines · On Tue, 27 Nov 2018 18:07:23 +0000 Will Deacon <[email protected]> wrote:
Quoted 18 lines · Steven Rostedt

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

The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().

Have arm64 use the new code, and remove the shadow stack management as well as having to set up the trace structure.

This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.

Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
Quoted 3 lines
---
 arch/arm64/kernel/ftrace.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)  
Quoted 2 lines

Minor nit: the subject refers to function_graph_entry(), but looks like you settled on function_graph_enter() in the end.

Ah you're right!

Quoted 4 lines

Anyway, looks like no change for us, so:

Thanks!

-- Steve

author maintainer
#8 lore raw

PATCH 5/18 microblaze: function_graph: Simplify with function_graph_entry()

In reply to Steven Rostedt

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

The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().

Have microblaze use the new code, and remove the shadow stack management as well as having to set up the trace structure.

This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.

Cc Michal Simek <[email protected]>
Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
---
 arch/microblaze/kernel/ftrace.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)
arch/microblaze/kernel/ftrace.c +2 -13
index d57563c58a26..224eea40e1ee 100644
@@ -22,8 +22,7 @@
 void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
 {
 	unsigned long old;
-	int faulted, err;
-	struct ftrace_graph_ent trace;
+	int faulted;
 	unsigned long return_hooker = (unsigned long)
 				&return_to_handler;
 
@@ -63,18 +62,8 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
 		return;
 	}
 
-	err = ftrace_push_return_trace(old, self_addr, &trace.depth, 0, NULL);
-	if (err == -EBUSY) {
+	if (function_graph_enter(old, self_addr, 0, NULL))
 		*parent = old;
-		return;
-	}
-
-	trace.func = self_addr;
-	/* Only trace if the calling function expects to */
-	if (!ftrace_graph_entry(&trace)) {
-		current->curr_ret_stack--;
-		*parent = old;
-	}
 }
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
 
Signature · 1 line
2.19.1
author maintainer
#9 lore raw

PATCH 6/18 MIPS: function_graph: Simplify with function_graph_entry()

In reply to Steven Rostedt

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

The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().

Have MIPS use the new code, and remove the shadow stack management as well as having to set up the trace structure.

This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.

Cc Ralf Baechle <[email protected]>
Cc Paul Burton <[email protected]>
Cc James Hogan <[email protected]>
Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
---
 arch/mips/kernel/ftrace.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)
arch/mips/kernel/ftrace.c +2 -12
index 7f3dfdbc3657..b122cbb4aad1 100644
@@ -322,7 +322,6 @@ void prepare_ftrace_return(unsigned long *parent_ra_addr, unsigned long self_ra,
 			   unsigned long fp)
 {
 	unsigned long old_parent_ra;
-	struct ftrace_graph_ent trace;
 	unsigned long return_hooker = (unsigned long)
 	    &return_to_handler;
 	int faulted, insns;
@@ -369,12 +368,6 @@ void prepare_ftrace_return(unsigned long *parent_ra_addr, unsigned long self_ra,
 	if (unlikely(faulted))
 		goto out;
 
-	if (ftrace_push_return_trace(old_parent_ra, self_ra, &trace.depth, fp,
-				     NULL) == -EBUSY) {
-		*parent_ra_addr = old_parent_ra;
-		return;
-	}
-
 	/*
 	 * Get the recorded ip of the current mcount calling site in the
 	 * __mcount_loc section, which will be used to filter the function
@@ -382,13 +375,10 @@ void prepare_ftrace_return(unsigned long *parent_ra_addr, unsigned long self_ra,
 	 */
 
 	insns = core_kernel_text(self_ra) ? 2 : MCOUNT_OFFSET_INSNS + 1;
-	trace.func = self_ra - (MCOUNT_INSN_SIZE * insns);
+	self_ra -= (MCOUNT_INSN_SIZE * insns);
 
-	/* Only trace if the calling function expects to */
-	if (!ftrace_graph_entry(&trace)) {
-		current->curr_ret_stack--;
+	if (function_graph_enter(old_parent_ra, self_ra, fp, NULL))
 		*parent_ra_addr = old_parent_ra;
-	}
 	return;
 out:
 	ftrace_graph_stop();
Signature · 1 line
2.19.1
maintainer
#10 lore raw

Re: [for-next][PATCH 06/18] MIPS: function_graph: Simplify with function_graph_entry()

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 03274a3ffb44 tracing/fgraph: Adjust fgraph depth before calling trace return callback.

The bot has tested the following trees: v4.19.3, v4.14.82, v4.9.138, v4.4.164, v3.18.126.

v4.19.3: Build OK!
v4.14.82: Build OK!
v4.9.138: Failed to apply! Possible dependencies:
Unable to calculate

v4.4.164: Failed to apply! Possible dependencies:
Unable to calculate

v3.18.126: Failed to apply! Possible dependencies:
Unable to calculate

How should we proceed with this patch?

--
Thanks,
Sasha

author maintainer
#11 lore raw

PATCH 7/18 nds32: function_graph: Simplify with function_graph_entry()

In reply to Steven Rostedt

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

The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().

Have nds32 use the new code, and remove the shadow stack management as well as having to set up the trace structure.

This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.

Cc Greentime Hu <[email protected]>
Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
---
 arch/nds32/kernel/ftrace.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)
arch/nds32/kernel/ftrace.c +2 -16
index a0a9679ad5de..8a41372551ff 100644
@@ -211,29 +211,15 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 			   unsigned long frame_pointer)
 {
 	unsigned long return_hooker = (unsigned long)&return_to_handler;
-	struct ftrace_graph_ent trace;
 	unsigned long old;
-	int err;
 
 	if (unlikely(atomic_read(&current->tracing_graph_pause)))
 		return;
 
 	old = *parent;
 
-	trace.func = self_addr;
-	trace.depth = current->curr_ret_stack + 1;
-
-	/* Only trace if the calling function expects to */
-	if (!ftrace_graph_entry(&trace))
-		return;
-
-	err = ftrace_push_return_trace(old, self_addr, &trace.depth,
-				       frame_pointer, NULL);
-
-	if (err == -EBUSY)
-		return;
-
-	*parent = return_hooker;
+	if (!function_graph_enter(old, self_addr, frame_pointer, NULL))
+		*parent = return_hooker;
 }
 
 noinline void ftrace_graph_caller(void)
Signature · 1 line
2.19.1
author maintainer
#12 lore raw

PATCH 8/18 parisc: function_graph: Simplify with function_graph_entry()

In reply to Steven Rostedt

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

The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().

Have parisc use the new code, and remove the shadow stack management as well as having to set up the trace structure.

This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.

Cc "James E.J. Bottomley" <[email protected]>
Cc Helge Deller <[email protected]>
Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
---
 arch/parisc/kernel/ftrace.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)
arch/parisc/kernel/ftrace.c +3 -14
index 6fa8535d3cce..e46a4157a894 100644
@@ -30,7 +30,6 @@ static void __hot prepare_ftrace_return(unsigned long *parent,
 					unsigned long self_addr)
 {
 	unsigned long old;
-	struct ftrace_graph_ent trace;
 	extern int parisc_return_to_handler;
 
 	if (unlikely(ftrace_graph_is_dead()))
@@ -41,19 +40,9 @@ static void __hot prepare_ftrace_return(unsigned long *parent,
 
 	old = *parent;
 
-	trace.func = self_addr;
-	trace.depth = current->curr_ret_stack + 1;
-
-	/* Only trace if the calling function expects to */
-	if (!ftrace_graph_entry(&trace))
-		return;
-
-        if (ftrace_push_return_trace(old, self_addr, &trace.depth,
-				     0, NULL) == -EBUSY)
-                return;
-
-	/* activate parisc_return_to_handler() as return point */
-	*parent = (unsigned long) &parisc_return_to_handler;
+	if (!function_graph_enter(old, self_addr, 0, NULL))
+		/* activate parisc_return_to_handler() as return point */
+		*parent = (unsigned long) &parisc_return_to_handler;
 }
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
 
Signature · 1 line
2.19.1

A message that is not in the archive (sent off-list or not yet received): [email protected]

maintainer
#14 lore raw

Re: [for-next][PATCH 08/18] parisc: function_graph: Simplify with function_graph_entry()

HI Sasha,

Quoted 11 lines · Sasha Levin

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 03274a3ffb44 tracing/fgraph: Adjust fgraph depth before calling trace return callback.

The bot has tested the following trees: v4.19.3, v4.14.82, v4.9.138, v4.4.164, v3.18.126.

v4.19.3: Build OK!
v4.14.82: Build OK!
v4.9.138: Build OK!
v4.4.164: Failed to apply! Possible dependencies:
Unable to calculate

Quoted 4 lines

v3.18.126: Failed to apply! Possible dependencies:
Unable to calculate

How should we proceed with this patch?

My suggestion, although I didn't looked too much on it:
Apply it to v4.9 and higher only.
I think I started fixing trace functionality on parisc around 4.6, which is probably why applying it fails on v4.4 and v3.x

Helge

author maintainer
#15 lore raw

Re: [for-next][PATCH 08/18] parisc: function_graph: Simplify with function_graph_entry()

Quoted 6 lines · On Fri, 23 Nov 2018 10:06:05 +0100 Helge Deller <[email protected]> wrote:
Quoted 1 line

How should we proceed with this patch?

My suggestion, although I didn't looked too much on it:
Apply it to v4.9 and higher only.
I think I started fixing trace functionality on parisc around 4.6, which is probably why applying it fails on v4.4 and v3.x

The problem is, if you backport the generic patches, it will completely break any arch that isn't updated. This also includes the archs that are no longer supported upstream, as they were not changed to handle the generic updates either.

-- Steve

maintainer
#16 lore raw

Re: [for-next][PATCH 08/18] parisc: function_graph: Simplify with function_graph_entry()

Quoted 11 lines · Steven Rostedt
Quoted 6 lines · On Fri, 23 Nov 2018 10:06:05 +0100 Helge Deller <[email protected]> wrote:
Quoted 1 line

How should we proceed with this patch?

My suggestion, although I didn't looked too much on it:
Apply it to v4.9 and higher only.
I think I started fixing trace functionality on parisc around 4.6, which is probably why applying it fails on v4.4 and v3.x

Quoted 4 lines

The problem is, if you backport the generic patches, it will completely break any arch that isn't updated. This also includes the archs that are no longer supported upstream, as they were not changed to handle the generic updates either.

Does this mean that someone (Steve) will send a backport of this to all relevant stable trees? Right now it looks like the series will randomly apply on a mix of trees, which can't be good.

--
Thanks,
Sasha

author maintainer
#17 lore raw

Re: [for-next][PATCH 08/18] parisc: function_graph: Simplify with function_graph_entry()

Quoted 3 lines · On Fri, 23 Nov 2018 13:34:15 -0500 Sasha Levin <[email protected]> wrote:

Does this mean that someone (Steve) will send a backport of this to all relevant stable trees? Right now it looks like the series will randomly apply on a mix of trees, which can't be good.

Nope. I stated that in my 0 patch.

-- Steve

maintainer
#18 lore raw

Re: [for-next][PATCH 08/18] parisc: function_graph: Simplify with function_graph_entry()

Quoted 8 lines · Steven Rostedt
Quoted 3 lines · On Fri, 23 Nov 2018 13:34:15 -0500 Sasha Levin <[email protected]> wrote:

Does this mean that someone (Steve) will send a backport of this to all relevant stable trees? Right now it looks like the series will randomly apply on a mix of trees, which can't be good.

Nope. I stated that in my 0 patch.

That's not good though, if you don't intend for them to be automagically backported to stable trees by Greg, then they shouldn't be tagged at all and if someone is interested then he can provide a backport.

What will happen with these is that once Greg's scripts process Linus's tree he'll end up with this patch series inconsistently backported to stable trees, which is not what you want here.

Sure, we can wait for the "added to the xyz stable tree" mails and object then, but why risk breaking the trees?

--
Thanks.
Sasha

author maintainer
#19 lore raw

Re: [for-next][PATCH 08/18] parisc: function_graph: Simplify with function_graph_entry()

Quoted 8 lines · On Fri, 23 Nov 2018 15:00:11 -0500 Sasha Levin <[email protected]> wrote:
Quoted 7 lines · Steven Rostedt
Quoted 3 lines · On Fri, 23 Nov 2018 13:34:15 -0500 Sasha Levin <[email protected]> wrote:

Does this mean that someone (Steve) will send a backport of this to all relevant stable trees? Right now it looks like the series will randomly apply on a mix of trees, which can't be good.

Quoted 5 lines
Quoted 1 line

Nope. I stated that in my 0 patch.

That's not good though, if you don't intend for them to be automagically backported to stable trees by Greg, then they shouldn't be tagged at all and if someone is interested then he can provide a backport.

For the most part they will be fine going back a few releases. But how far back is questionable before they start getting into issues. I talked a bit about this to Greg on IRC and he seemed fine with me adding the stable tag.

If they don't port back properly, it wont be a silent failure. They will either build or they wont. I'm suspect that you build all supported archs for your stable trees, right? If the patch fails to build, then either have someone that cares for that arch back port it, or don't back port the series. Simple as that.

Quoted 4 lines

What will happen with these is that once Greg's scripts process Linus's tree he'll end up with this patch series inconsistently backported to stable trees, which is not what you want here.

It's not like it won't work and then start to work again. Once they start failing in older versions, they will probably fail in all versions before that.

Quoted 3 lines

Sure, we can wait for the "added to the xyz stable tree" mails and object then, but why risk breaking the trees?

Again, it's not much different than other stable patches that need to be fixed for older trees. If they build, they are fine, if they don't then they need to be fixed. You'll know right at build time.

-- Steve

maintainer
#20 lore raw

Re: [for-next][PATCH 08/18] parisc: function_graph: Simplify with function_graph_entry()

Quoted 11 lines · Steven Rostedt
Quoted 3 lines · On Fri, 23 Nov 2018 15:00:11 -0500 Sasha Levin <[email protected]> wrote:

What will happen with these is that once Greg's scripts process Linus's tree he'll end up with this patch series inconsistently backported to stable trees, which is not what you want here.

It's not like it won't work and then start to work again. Once they start failing in older versions, they will probably fail in all versions before that.

Quoted 1 line
Quoted 6 lines
Quoted 2 lines

Sure, we can wait for the "added to the xyz stable tree" mails and object then, but why risk breaking the trees?

Again, it's not much different than other stable patches that need to be fixed for older trees. If they build, they are fine, if they don't then they need to be fixed. You'll know right at build time.

So the only tree it applied and built was 4.19, all older trees failed on at least one arch.

I'm going to remove any parts of this series from all other trees.

--
Thanks,
Sasha

author maintainer
#21 lore raw

PATCH 9/18 powerpc/function_graph: Simplify with function_graph_entry()

In reply to Steven Rostedt

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

The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().

Have powerpc use the new code, and remove the shadow stack management as well as having to set up the trace structure.

This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.

Cc Benjamin Herrenschmidt <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Michael Ellerman <[email protected]>
Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
---
 arch/powerpc/kernel/trace/ftrace.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)
arch/powerpc/kernel/trace/ftrace.c +2 -13
index 4bf051d3e21e..b65c8a34ad6e 100644
@@ -950,7 +950,6 @@ int ftrace_disable_ftrace_graph_caller(void)
  */
 unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip)
 {
-	struct ftrace_graph_ent trace;
 	unsigned long return_hooker;
 
 	if (unlikely(ftrace_graph_is_dead()))
@@ -961,18 +960,8 @@ unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip)
 
 	return_hooker = ppc_function_entry(return_to_handler);
 
-	trace.func = ip;
-	trace.depth = current->curr_ret_stack + 1;
-
-	/* Only trace if the calling function expects to */
-	if (!ftrace_graph_entry(&trace))
-		goto out;
-
-	if (ftrace_push_return_trace(parent, ip, &trace.depth, 0,
-				     NULL) == -EBUSY)
-		goto out;
-
-	parent = return_hooker;
+	if (!function_graph_enter(parent, ip, 0, NULL))
+		parent = return_hooker;
 out:
 	return parent;
 }
Signature · 1 line
2.19.1
maintainer
#22 lore raw

Re: [for-next][PATCH 09/18] powerpc/function_graph: Simplify with function_graph_entry()

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 03274a3ffb44 tracing/fgraph: Adjust fgraph depth before calling trace return callback.

The bot has tested the following trees: v4.19.3, v4.14.82, v4.9.138, v4.4.164, v3.18.126.

v4.19.3: Build OK!
v4.14.82: Build OK!
v4.9.138: Failed to apply! Possible dependencies:
Unable to calculate

v4.4.164: Failed to apply! Possible dependencies:
Unable to calculate

v3.18.126: Failed to apply! Possible dependencies:
Unable to calculate

How should we proceed with this patch?

--
Thanks,
Sasha

author maintainer
#23 lore raw

PATCH 10/18 riscv/function_graph: Simplify with function_graph_entry()

In reply to Steven Rostedt

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

The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().

Have riscv use the new code, and remove the shadow stack management as well as having to set up the trace structure.

This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.

Cc Greentime Hu <[email protected]>
Cc Alan Kao <[email protected]>
Cc Palmer Dabbelt <[email protected]>
Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
---
 arch/riscv/kernel/ftrace.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)
arch/riscv/kernel/ftrace.c +2 -12
index 1157b6b52d25..c433f6d3dd64 100644
@@ -132,7 +132,6 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 {
 	unsigned long return_hooker = (unsigned long)&return_to_handler;
 	unsigned long old;
-	struct ftrace_graph_ent trace;
 	int err;
 
 	if (unlikely(atomic_read(&current->tracing_graph_pause)))
@@ -144,17 +143,8 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 	 */
 	old = *parent;
 
-	trace.func = self_addr;
-	trace.depth = current->curr_ret_stack + 1;
-
-	if (!ftrace_graph_entry(&trace))
-		return;
-
-	err = ftrace_push_return_trace(old, self_addr, &trace.depth,
-				       frame_pointer, parent);
-	if (err == -EBUSY)
-		return;
-	*parent = return_hooker;
+	if (function_graph_enter(old, self_addr, frame_pointer, parent))
+		*parent = return_hooker;
 }
 
 #ifdef CONFIG_DYNAMIC_FTRACE
Signature · 1 line
2.19.1
Reviewed-by on v1 10/18
#24 lore raw

Re: [for-next][PATCH 10/18] riscv/function_graph: Simplify with function_graph_entry()

Quoted 44 lines · On Wed, 21 Nov 2018 16:28:11 PST (-0800), [email protected] wrote:

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

The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().

Have riscv use the new code, and remove the shadow stack management as well as having to set up the trace structure.

This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.

Cc Greentime Hu <[email protected]>
Cc Alan Kao <[email protected]>
Cc Palmer Dabbelt <[email protected]>
Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
---
 arch/riscv/kernel/ftrace.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)
arch/riscv/kernel/ftrace.c +0 -7
index 1157b6b52d25..c433f6d3dd64 100644
@@ -132,7 +132,6 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 {
 	unsigned long return_hooker = (unsigned long)&return_to_handler;
 	unsigned long old;
-	struct ftrace_graph_ent trace;
 	int err;
 
 	if (unlikely(atomic_read(&current->tracing_graph_pause)))
@@ -144,17 +143,8 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 	 */
 	old = *parent;
 
-	trace.func = self_addr;
-	trace.depth = current->curr_ret_stack + 1;
-
-	if (!ftrace_graph_entry(&trace))
-		return;
-

arch/riscv/kernel/ftrace.c void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr, · line 150

Quoted 10 lines
arch/riscv/kernel/ftrace.c +2 -5
-	err = ftrace_push_return_trace(old, self_addr, &trace.depth,
-				       frame_pointer, parent);
-	if (err == -EBUSY)
-		return;
-	*parent = return_hooker;
+	if (function_graph_enter(old, self_addr, frame_pointer, parent))
+		*parent = return_hooker;
 }
 
 #ifdef CONFIG_DYNAMIC_FTRACE
author maintainer
#25 lore raw

PATCH 11/18 s390/function_graph: Simplify with function_graph_entry()

In reply to Steven Rostedt

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

The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().

Have s390 use the new code, and remove the shadow stack management as well as having to set up the trace structure.

This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.

Cc Martin Schwidefsky <[email protected]>
Cc Heiko Carstens <[email protected]>
Cc Julian Wiedmann <[email protected]>
Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
---
 arch/s390/kernel/ftrace.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)
arch/s390/kernel/ftrace.c +2 -11
index 84be7f02d0c2..39b13d71a8fe 100644
@@ -203,22 +203,13 @@ device_initcall(ftrace_plt_init);
  */
 unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip)
 {
-	struct ftrace_graph_ent trace;
-
 	if (unlikely(ftrace_graph_is_dead()))
 		goto out;
 	if (unlikely(atomic_read(&current->tracing_graph_pause)))
 		goto out;
 	ip -= MCOUNT_INSN_SIZE;
-	trace.func = ip;
-	trace.depth = current->curr_ret_stack + 1;
-	/* Only trace if the calling function expects to. */
-	if (!ftrace_graph_entry(&trace))
-		goto out;
-	if (ftrace_push_return_trace(parent, ip, &trace.depth, 0,
-				     NULL) == -EBUSY)
-		goto out;
-	parent = (unsigned long) return_to_handler;
+	if (!function_graph_enter(parent, ip, 0, NULL))
+		parent = (unsigned long) return_to_handler;
 out:
 	return parent;
 }
Signature · 1 line
2.19.1
Acked-by on v1 11/18
#26 lore raw

Re: [for-next][PATCH 11/18] s390/function_graph: Simplify with function_graph_entry()

Quoted 12 lines · On Wed, 21 Nov 2018 19:28:12 -0500 Steven Rostedt <[email protected]> wrote:

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

The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().

Have s390 use the new code, and remove the shadow stack management as well as having to set up the trace structure.

This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.

Quoted 7 lines
Cc Martin Schwidefsky <[email protected]>
Cc Heiko Carstens <[email protected]>
Cc Julian Wiedmann <[email protected]>
Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")

A quick test showed that the patch series works fine on s390.

Quoted 33 lines
---
 arch/s390/kernel/ftrace.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)
arch/s390/kernel/ftrace.c +2 -11
index 84be7f02d0c2..39b13d71a8fe 100644
@@ -203,22 +203,13 @@ device_initcall(ftrace_plt_init);
  */
 unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip)
 {
-	struct ftrace_graph_ent trace;
-
 	if (unlikely(ftrace_graph_is_dead()))
 		goto out;
 	if (unlikely(atomic_read(&current->tracing_graph_pause)))
 		goto out;
 	ip -= MCOUNT_INSN_SIZE;
-	trace.func = ip;
-	trace.depth = current->curr_ret_stack + 1;
-	/* Only trace if the calling function expects to. */
-	if (!ftrace_graph_entry(&trace))
-		goto out;
-	if (ftrace_push_return_trace(parent, ip, &trace.depth, 0,
-				     NULL) == -EBUSY)
-		goto out;
-	parent = (unsigned long) return_to_handler;
+	if (!function_graph_enter(parent, ip, 0, NULL))
+		parent = (unsigned long) return_to_handler;
 out:
 	return parent;
 }
Signature · 4 lines
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.
author maintainer
#28 lore raw

PATCH 12/18 sh/function_graph: Simplify with function_graph_entry()

In reply to Steven Rostedt

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

The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().

Have superh use the new code, and remove the shadow stack management as well as having to set up the trace structure.

This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.

Cc Yoshinori Sato <[email protected]>
Cc Rich Felker <[email protected]>
Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
---
 arch/sh/kernel/ftrace.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)
arch/sh/kernel/ftrace.c +2 -14
index 96dd9f7da250..1b04270e5460 100644
@@ -321,8 +321,7 @@ int ftrace_disable_ftrace_graph_caller(void)
 void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
 {
 	unsigned long old;
-	int faulted, err;
-	struct ftrace_graph_ent trace;
+	int faulted;
 	unsigned long return_hooker = (unsigned long)&return_to_handler;
 
 	if (unlikely(ftrace_graph_is_dead()))
@@ -365,18 +364,7 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
 		return;
 	}
 
-	err = ftrace_push_return_trace(old, self_addr, &trace.depth, 0, NULL);
-	if (err == -EBUSY) {
+	if (function_graph_enter(old, self_addr, 0, NULL))
 		__raw_writel(old, parent);
-		return;
-	}
-
-	trace.func = self_addr;
-
-	/* Only trace if the calling function expects to */
-	if (!ftrace_graph_entry(&trace)) {
-		current->curr_ret_stack--;
-		__raw_writel(old, parent);
-	}
 }
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
Signature · 1 line
2.19.1
author maintainer
#29 lore raw

PATCH 13/18 sparc/function_graph: Simplify with function_graph_entry()

In reply to Steven Rostedt

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

The function_graph_entry() function does the work of calling the function graph hook function and the management of the shadow stack, simplifying the work done in the architecture dependent prepare_ftrace_return().

Have sparc use the new code, and remove the shadow stack management as well as having to set up the trace structure.

This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.

Cc "David S. Miller" <[email protected]>
Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
---
 arch/sparc/kernel/ftrace.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)
arch/sparc/kernel/ftrace.c +1 -10
index 915dda4ae412..684b84ce397f 100644
@@ -126,20 +126,11 @@ unsigned long prepare_ftrace_return(unsigned long parent,
 				    unsigned long frame_pointer)
 {
 	unsigned long return_hooker = (unsigned long) &return_to_handler;
-	struct ftrace_graph_ent trace;
 
 	if (unlikely(atomic_read(&current->tracing_graph_pause)))
 		return parent + 8UL;
 
-	trace.func = self_addr;
-	trace.depth = current->curr_ret_stack + 1;
-
-	/* Only trace if the calling function expects to */
-	if (!ftrace_graph_entry(&trace))
-		return parent + 8UL;
-
-	if (ftrace_push_return_trace(parent, self_addr, &trace.depth,
-				     frame_pointer, NULL) == -EBUSY)
+	if (function_graph_enter(parent, self_addr, frame_pointer, NULL))
 		return parent + 8UL;
 
 	return return_hooker;
Signature · 1 line
2.19.1
author maintainer
#30 lore raw

PATCH 14/18 function_graph: Make ftrace_push_return_trace() static

In reply to Steven Rostedt

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

As all architectures now call function_graph_enter() to do the entry work, no architecture should ever call ftrace_push_return_trace(). Make it static.

This is needed to prepare for a fix of a design bug on how the curr_ret_stack is used.

Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
---
 include/linux/ftrace.h               | 3 ---
 kernel/trace/trace_functions_graph.c | 2 +-
 2 files changed, 1 insertion(+), 4 deletions(-)
include/linux/ftrace.h +0 -3
index 5717e8f81c59..dd16e8218db3 100644
@@ -776,9 +776,6 @@ struct ftrace_ret_stack {
  */
 extern void return_to_handler(void);
 
-extern int
-ftrace_push_return_trace(unsigned long ret, unsigned long func, int *depth,
-			 unsigned long frame_pointer, unsigned long *retp);
 extern int
 function_graph_enter(unsigned long ret, unsigned long func,
 		     unsigned long frame_pointer, unsigned long *retp);
kernel/trace/trace_functions_graph.c +1 -1
index 28f2602435d0..88ca787a1cdc 100644
@@ -118,7 +118,7 @@ print_graph_duration(struct trace_array *tr, unsigned long long duration,
 		     struct trace_seq *s, u32 flags);
 
 /* Add a function return address to the trace stack on thread info.*/
-int
+static int
 ftrace_push_return_trace(unsigned long ret, unsigned long func, int *depth,
 			 unsigned long frame_pointer, unsigned long *retp)
 {
Signature · 1 line
2.19.1
author maintainer
#31 lore raw

PATCH 15/18 function_graph: Use new curr_ret_depth to manage depth instead of curr_ret_stack

In reply to Steven Rostedt

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

Currently, the depth of the ret_stack is determined by curr_ret_stack index. The issue is that there's a race between setting of the curr_ret_stack and calling of the callback attached to the return of the function.

Commit 03274a3ffb44 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback") moved the calling of the callback to after the setting of the curr_ret_stack, even stating that it was safe to do so, when in fact, it was the reason there was a barrier() there (yes, I should have commented that barrier()).

Not only does the curr_ret_stack keep track of the current call graph depth, it also keeps the ret_stack content from being overwritten by new data.

The function profiler, uses the "subtime" variable of ret_stack structure and by moving the curr_ret_stack, it allows for interrupts to use the same structure it was using, corrupting the data, and breaking the profiler.

To fix this, there needs to be two variables to handle the call stack depth and the pointer to where the ret_stack is being used, as they need to change at two different locations.

Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
---
 include/linux/sched.h                |  1 +
 kernel/trace/ftrace.c                |  3 +++
 kernel/trace/trace_functions_graph.c | 21 +++++++++++++--------
 3 files changed, 17 insertions(+), 8 deletions(-)
include/linux/sched.h +1 -0
index a51c13c2b1a0..d6183a55e8eb 100644
@@ -1116,6 +1116,7 @@ struct task_struct {
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	/* Index of current stored address in ret_stack: */
 	int				curr_ret_stack;
+	int				curr_ret_depth;
 
 	/* Stack of return addresses for return function tracing: */
 	struct ftrace_ret_stack		*ret_stack;
kernel/trace/ftrace.c +3 -0
index f536f601bd46..48513954713c 100644
@@ -6814,6 +6814,7 @@ static int alloc_retstack_tasklist(struct ftrace_ret_stack **ret_stack_list)
 			atomic_set(&t->tracing_graph_pause, 0);
 			atomic_set(&t->trace_overrun, 0);
 			t->curr_ret_stack = -1;
+			t->curr_ret_depth = -1;
 			/* Make sure the tasks see the -1 first: */
 			smp_wmb();
 			t->ret_stack = ret_stack_list[start++];
@@ -7038,6 +7039,7 @@ graph_init_task(struct task_struct *t, struct ftrace_ret_stack *ret_stack)
 void ftrace_graph_init_idle_task(struct task_struct *t, int cpu)
 {
 	t->curr_ret_stack = -1;
+	t->curr_ret_depth = -1;
 	/*
 	 * The idle task has no parent, it either has its own
 	 * stack or no stack at all.
@@ -7068,6 +7070,7 @@ void ftrace_graph_init_task(struct task_struct *t)
 	/* Make sure we do not use the parent ret_stack */
 	t->ret_stack = NULL;
 	t->curr_ret_stack = -1;
+	t->curr_ret_depth = -1;
 
 	if (ftrace_graph_active) {
 		struct ftrace_ret_stack *ret_stack;
kernel/trace/trace_functions_graph.c +13 -8
index 88ca787a1cdc..02d4081a7f5a 100644
@@ -119,7 +119,7 @@ print_graph_duration(struct trace_array *tr, unsigned long long duration,
 
 /* Add a function return address to the trace stack on thread info.*/
 static int
-ftrace_push_return_trace(unsigned long ret, unsigned long func, int *depth,
+ftrace_push_return_trace(unsigned long ret, unsigned long func,
 			 unsigned long frame_pointer, unsigned long *retp)
 {
 	unsigned long long calltime;
@@ -177,8 +177,6 @@ ftrace_push_return_trace(unsigned long ret, unsigned long func, int *depth,
 #ifdef HAVE_FUNCTION_GRAPH_RET_ADDR_PTR
 	current->ret_stack[index].retp = retp;
 #endif
-	*depth = current->curr_ret_stack;
-
 	return 0;
 }
 
@@ -188,14 +186,20 @@ int function_graph_enter(unsigned long ret, unsigned long func,
 	struct ftrace_graph_ent trace;
 
 	trace.func = func;
-	trace.depth = current->curr_ret_stack + 1;
+	trace.depth = ++current->curr_ret_depth;
 
 	/* Only trace if the calling function expects to */
 	if (!ftrace_graph_entry(&trace))
-		return -EBUSY;
+		goto out;
 
-	return ftrace_push_return_trace(ret, func, &trace.depth,
-					frame_pointer, retp);
+	if (ftrace_push_return_trace(ret, func,
+				     frame_pointer, retp))
+		goto out;
+
+	return 0;
+ out:
+	current->curr_ret_depth--;
+	return -EBUSY;
 }
 
 /* Retrieve a function return address to the trace stack on thread info.*/
@@ -257,7 +261,7 @@ ftrace_pop_return_trace(struct ftrace_graph_ret *trace, unsigned long *ret,
 	trace->func = current->ret_stack[index].func;
 	trace->calltime = current->ret_stack[index].calltime;
 	trace->overrun = atomic_read(&current->trace_overrun);
-	trace->depth = index;
+	trace->depth = current->curr_ret_depth;
 }
 
 /*
@@ -273,6 +277,7 @@ unsigned long ftrace_return_to_handler(unsigned long frame_pointer)
 	trace.rettime = trace_clock_local();
 	barrier();
 	current->curr_ret_stack--;
+	current->curr_ret_depth--;
 	/*
 	 * The curr_ret_stack can be less than -1 only if it was
 	 * filtered out and it's about to return from the function.
Signature · 1 line
2.19.1
maintainer
#32 lore raw

Re: [for-next][PATCH 15/18] function_graph: Use new curr_ret_depth to manage depth instead of curr_ret_stack

Quoted 6 lines · Steven Rostedt
kernel/trace/trace_functions_graph.c +1 -1
@@ -188,14 +186,20 @@ int function_graph_enter(unsigned long ret, unsigned long func,
 	struct ftrace_graph_ent trace;
 
 	trace.func = func;
-	trace.depth = current->curr_ret_stack + 1;
+	trace.depth = ++current->curr_ret_depth;

kernel/trace/trace_functions_graph.c int function_graph_enter(unsigned long ret, unsigned long func, · line 196

Quoted 10 lines
kernel/trace/trace_functions_graph.c +3 -3
 
 	/* Only trace if the calling function expects to */
 	if (!ftrace_graph_entry(&trace))
-		return -EBUSY;
+		goto out;
 
-	return ftrace_push_return_trace(ret, func, &trace.depth,
-					frame_pointer, retp);
+	if (ftrace_push_return_trace(ret, func,
+				     frame_pointer, retp))

You can unwrap that line, by my counting that's at 69 chars, so unless you're editing on a C64 it should fit your screen.

kernel/trace/trace_functions_graph.c int function_graph_enter(unsigned long ret, unsigned long func, · line 203

Quoted 7 lines
kernel/trace/trace_functions_graph.c +6 -0
+		goto out;
+
+	return 0;
+ out:
+	current->curr_ret_depth--;
+	return -EBUSY;
 }

[diff "default"]

	xfuncname = "^[[:alpha:]$_].*[^:]$"

avoids the need for that ludicrous label indenting.

Also, "error" might be a better label name.

author maintainer
#33 lore raw

Re: [for-next][PATCH 15/18] function_graph: Use new curr_ret_depth to manage depth instead of curr_ret_stack

Quoted 13 lines · On Thu, 22 Nov 2018 11:03:22 +0100 Peter Zijlstra <[email protected]> wrote:
Quoted 12 lines · Steven Rostedt
kernel/trace/trace_functions_graph.c +2 -2
@@ -188,14 +186,20 @@ int function_graph_enter(unsigned long ret, unsigned long func,
 	struct ftrace_graph_ent trace;
 
 	trace.func = func;
-	trace.depth = current->curr_ret_stack + 1;
+	trace.depth = ++current->curr_ret_depth;
 
 	/* Only trace if the calling function expects to */
 	if (!ftrace_graph_entry(&trace))
-		return -EBUSY;
+		goto out;
 
Quoted 7 lines

kernel/trace/trace_functions_graph.c int function_graph_enter(unsigned long ret, unsigned long func, · line 196

Quoted 4 lines
kernel/trace/trace_functions_graph.c +2 -2
-	return ftrace_push_return_trace(ret, func, &trace.depth,
-					frame_pointer, retp);
+	if (ftrace_push_return_trace(ret, func,
+				     frame_pointer, retp))  

You can unwrap that line, by my counting that's at 69 chars, so unless you're editing on a C64 it should fit your screen.

Thanks I may add a clean up patch. I already ran this code under several hours of testing, and only plan on touching it if there's more than cosmetic issues.

The reason that looks like that is because I just massaged the line it replaced, and didn't take into account that I could now make it into a single line.

Quoted 10 lines

kernel/trace/trace_functions_graph.c int function_graph_enter(unsigned long ret, unsigned long func, · line 203

Quoted 8 lines
kernel/trace/trace_functions_graph.c +6 -0
+		goto out;
+
+	return 0;
+ out:
+	current->curr_ret_depth--;
+	return -EBUSY;
 }  

[diff "default"]

Quoted 3 lines
	xfuncname = "^[[:alpha:]$_].*[^:]$"

avoids the need for that ludicrous label indenting.

I still prefer the 'space', and it fits with the rest of the file ;-)

TomAYto / TomAHto

Quoted 2 lines

Also, "error" might be a better label name.

True. I'll add that on top too, but for the stable/rc release, that isn't needed.

Thanks for taking a look at this code.

-- Steve

author maintainer
#34 lore raw

PATCH 16/18 function_graph: Move return callback before update of curr_ret_stack

In reply to Steven Rostedt

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

In the past, curr_ret_stack had two functions. One was to denote the depth of the call graph, the other is to keep track of where on the ret_stack the data is used. Although they may be slightly related, there are two cases where they need to be used differently.

The one case is that it keeps the ret_stack data from being corrupted by an interrupt coming in and overwriting the data still in use. The other is just to know where the depth of the stack currently is.

The function profiler uses the ret_stack to save a "subtime" variable that is part of the data on the ret_stack. If curr_ret_stack is modified too early, then this variable can be corrupted.

The "max_depth" option, when set to 1, will record the first functions going into the kernel. To see all top functions (when dealing with timings), the depth variable needs to be lowered before calling the return hook. But by lowering the curr_ret_stack, it makes the data on the ret_stack still being used by the return hook susceptible to being overwritten.

Now that there's two variables to handle both cases (curr_ret_depth), we can move them to the locations where they can handle both cases.

Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
---
 kernel/trace/trace_functions_graph.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)
kernel/trace/trace_functions_graph.c +13 -9
index 02d4081a7f5a..4f0d72ae6362 100644
@@ -261,7 +261,13 @@ ftrace_pop_return_trace(struct ftrace_graph_ret *trace, unsigned long *ret,
 	trace->func = current->ret_stack[index].func;
 	trace->calltime = current->ret_stack[index].calltime;
 	trace->overrun = atomic_read(&current->trace_overrun);
-	trace->depth = current->curr_ret_depth;
+	trace->depth = current->curr_ret_depth--;
+	/*
+	 * We still want to trace interrupts coming in if
+	 * max_depth is set to 1. Make sure the decrement is
+	 * seen before ftrace_graph_return.
+	 */
+	barrier();
 }
 
 /*
@@ -275,9 +281,14 @@ unsigned long ftrace_return_to_handler(unsigned long frame_pointer)
 
 	ftrace_pop_return_trace(&trace, &ret, frame_pointer);
 	trace.rettime = trace_clock_local();
+	ftrace_graph_return(&trace);
+	/*
+	 * The ftrace_graph_return() may still access the current
+	 * ret_stack structure, we need to make sure the update of
+	 * curr_ret_stack is after that.
+	 */
 	barrier();
 	current->curr_ret_stack--;
-	current->curr_ret_depth--;
 	/*
 	 * The curr_ret_stack can be less than -1 only if it was
 	 * filtered out and it's about to return from the function.
@@ -288,13 +299,6 @@ unsigned long ftrace_return_to_handler(unsigned long frame_pointer)
 		return ret;
 	}
 
-	/*
-	 * The trace should run after decrementing the ret counter
-	 * in case an interrupt were to come in. We don't want to
-	 * lose the interrupt if max_depth is set.
-	 */
-	ftrace_graph_return(&trace);
-
 	if (unlikely(!ret)) {
 		ftrace_graph_stop();
 		WARN_ON(1);
Signature · 1 line
2.19.1
author maintainer
#35 lore raw

PATCH 17/18 function_graph: Reverse the order of pushing the ret_stack and the callback

In reply to Steven Rostedt

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

The function graph profiler uses the ret_stack to store the "subtime" and reuse it by nested functions and also on the return. But the current logic has the profiler callback called before the ret_stack is updated, and it is just modifying the ret_stack that will later be allocated (it's just lucky that the "subtime" is not touched when it is allocated).

This could also cause a crash if we are at the end of the ret_stack when this happens.

By reversing the order of the allocating the ret_stack and then calling the callbacks attached to a function being traced, the ret_stack entry is no longer used before it is allocated.

Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
---
 kernel/trace/trace_functions_graph.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
kernel/trace/trace_functions_graph.c +6 -4
index 4f0d72ae6362..2561460d7baf 100644
@@ -188,15 +188,17 @@ int function_graph_enter(unsigned long ret, unsigned long func,
 	trace.func = func;
 	trace.depth = ++current->curr_ret_depth;
 
-	/* Only trace if the calling function expects to */
-	if (!ftrace_graph_entry(&trace))
-		goto out;
-
 	if (ftrace_push_return_trace(ret, func,
 				     frame_pointer, retp))
 		goto out;
 
+	/* Only trace if the calling function expects to */
+	if (!ftrace_graph_entry(&trace))
+		goto out_ret;
+
 	return 0;
+ out_ret:
+	current->curr_ret_stack--;
  out:
 	current->curr_ret_depth--;
 	return -EBUSY;
Signature · 1 line
2.19.1
author maintainer
#36 lore raw

PATCH 18/18 function_graph: Have profiler use curr_ret_stack and not depth

In reply to Steven Rostedt

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

The profiler uses trace->depth to find its entry on the ret_stack, but the depth may not match the actual location of where its entry is (if an interrupt were to preempt the processing of the profiler for another function, the depth and the curr_ret_stack will be different).

Have it use the curr_ret_stack as the index to find its ret_stack entry instead of using the depth variable, as that is no longer guaranteed to be the same.

Fixes 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
---
 kernel/trace/ftrace.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
kernel/trace/ftrace.c +2 -2
index 48513954713c..77734451cb05 100644
@@ -817,7 +817,7 @@ function_profile_call(unsigned long ip, unsigned long parent_ip,
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 static int profile_graph_entry(struct ftrace_graph_ent *trace)
 {
-	int index = trace->depth;
+	int index = current->curr_ret_stack;
 
 	function_profile_call(trace->func, 0, NULL, NULL);
 
@@ -852,7 +852,7 @@ static void profile_graph_return(struct ftrace_graph_ret *trace)
 	if (!fgraph_graph_time) {
 		int index;
 
-		index = trace->depth;
+		index = current->curr_ret_stack;
 
 		/* Append this call time to the parent time to subtract */
 		if (index)
Signature · 1 line
2.19.1
maintainer Reviewed-by on the v1 series
#37 lore raw
In reply to Steven Rostedt

Hi Steve,

This series looks good to me.

for this series.

Thank you!

# Now I know why sometimes func-graph depth looks broken...

Quoted 90 lines · On Wed, 21 Nov 2018 19:28:01 -0500 Steven Rostedt <[email protected]> wrote:

While working on rewriting function graph tracer, I found a design flaw in the code. Commit 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback") tried to fix a bug that caused interrupts not to be traced if max_depth was set to 1 (for debugging NO_HZ_FULL), because if it came in just as another function that was being traced that had a depth of 1 was exiting. This caused the timing of being in the kernel to be off. The fix was simply to move the calling of the return function after the updating of curr_ret_stack index as that was what was being used.

The change log even says that it was safe to do this, but unfortunately it was not, and the barrier() there was specifically *for* that function (which shows why one should document barriers).

The problem is that the return callback may still be using what's on the shadow stack and by changing the shadow stack pointer, it may allow for another function being traced to overwrite that data. Note, if this happens, it will only cause garbage data to be traced and will not affect the actual operations of the kernel (ie. it wont crash).

Unfortunately just reverting that will bring back the old bug. The real way to fix that old bug is to create another counter to handle the depth, but this means that we need to change all the architectures that implement function graph tracing (that's 12 of them). Luckily, I need to do this anyway in my re-design so this is a good thing.

Since all the archictectures do basicall the same thing to prepare the function graph trace to be traced, I made a generic function that they all can use and simplified the logic of all the architectures. Then I'm able to fix the design problem in one place.

I pushed this code up to let zero-day have a whack at it, and I also downloaded the latest 8.1.0 cross compilers for all the archs that are affected and compiled tested them all (and got rid of all the warnings I introduced as well).

I marked this all for stable, but in reality it may not need to be ported as it will probably not be trivial to do so, becaues you'll need to also fix the architectures that are no longer used (although do we care about them?). But if someone really cares about correct timings of the function graph profiler when the options/graph-time is set to zero, then be my guest.

Feel free to test this! I'll be pushing this to linux-next and let it sit there a week or so before pushing it to Linus.

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

Head SHA1: 1ffd62e282a649483afb0bb4d76f7244b3c10075

Steven Rostedt (VMware) (18):
function_graph: Create function_graph_enter() to consolidate architecture code x86/function_graph: Simplify with function_graph_entry()
ARM: function_graph: Simplify with function_graph_entry()
arm64: function_graph: Simplify with function_graph_entry()
microblaze: function_graph: Simplify with function_graph_entry()
MIPS: function_graph: Simplify with function_graph_entry()
nds32: function_graph: Simplify with function_graph_entry()
parisc: function_graph: Simplify with function_graph_entry() powerpc/function_graph: Simplify with function_graph_entry() riscv/function_graph: Simplify with function_graph_entry() s390/function_graph: Simplify with function_graph_entry() sh/function_graph: Simplify with function_graph_entry() sparc/function_graph: Simplify with function_graph_entry()
function_graph: Make ftrace_push_return_trace() static
function_graph: Use new curr_ret_depth to manage depth instead of curr_ret_stack
function_graph: Move return callback before update of curr_ret_stack
function_graph: Reverse the order of pushing the ret_stack and the callback
function_graph: Have profiler use curr_ret_stack and not depth

----

 arch/arm/kernel/ftrace.c             | 17 +------------
 arch/arm64/kernel/ftrace.c           | 15 +----------
 arch/microblaze/kernel/ftrace.c      | 15 ++---------
 arch/mips/kernel/ftrace.c            | 14 ++---------
 arch/nds32/kernel/ftrace.c           | 18 ++-----------
 arch/parisc/kernel/ftrace.c          | 17 +++----------
 arch/powerpc/kernel/trace/ftrace.c   | 15 ++---------
 arch/riscv/kernel/ftrace.c           | 14 ++---------
 arch/s390/kernel/ftrace.c            | 13 ++--------
 arch/sh/kernel/ftrace.c              | 16 ++----------
 arch/sparc/kernel/ftrace.c           | 11 +-------
 arch/x86/kernel/ftrace.c             | 15 +----------
 include/linux/ftrace.h               |  4 +--
 include/linux/sched.h                |  1 +
 kernel/trace/ftrace.c                |  7 ++++--
 kernel/trace/trace_functions_graph.c | 49 ++++++++++++++++++++++++++++--------
 16 files changed, 67 insertions(+), 174 deletions(-)
Signature · 1 line
Masami Hiramatsu <[email protected]>
#38 lore raw
In reply to Steven Rostedt
Quoted 8 lines · Steven Rostedt

[ ... snip ... ]

Feel free to test this! I'll be pushing this to linux-next and let it sit there a week or so before pushing it to Linus.

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

Hi Steve,

With your ftrace/urgent branch linked above, if I try a quick function_graph test like the following:

SYSFS=/sys/kernel/debug/tracing

echo 0 > "$SYSFS/tracing_on"
echo cmdline_proc_show > "$SYSFS/set_graph_function" echo function_graph > "$SYSFS/current_tracer" echo 1 > "$SYSFS/tracing_on"

I see a bunch of scheduler interrupt functions in the trace/trace_pipe without even invoking cmdline_proc_show().

This tests works as expected with Linux 4.20-rc3 though:

% cat /sys/kernel/debug/tracing/trace_pipe

   2)               |  cmdline_proc_show() {
   2)   0.320 us    |    seq_puts();
   2)   0.030 us    |    seq_putc();
   2)   1.352 us    |  }

Operator error, or did the patchset break something?

Regards,

-- Joe

author maintainer
#39 lore raw
Quoted 17 lines · On Wed, 28 Nov 2018 15:39:31 -0500 Joe Lawrence <[email protected]> wrote:

Hi Steve,

With your ftrace/urgent branch linked above, if I try a quick function_graph test like the following:

SYSFS=/sys/kernel/debug/tracing

echo 0 > "$SYSFS/tracing_on"
echo cmdline_proc_show > "$SYSFS/set_graph_function" echo function_graph > "$SYSFS/current_tracer" echo 1 > "$SYSFS/tracing_on"

I see a bunch of scheduler interrupt functions in the trace/trace_pipe without even invoking cmdline_proc_show().

This tests works as expected with Linux 4.20-rc3 though:

Quoted 7 lines

% cat /sys/kernel/debug/tracing/trace_pipe

   2)               |  cmdline_proc_show() {
   2)   0.320 us    |    seq_puts();
   2)   0.030 us    |    seq_putc();
   2)   1.352 us    |  }

Operator error, or did the patchset break something?

Nope, that does seem to be a bug :-/

-- Steve

author maintainer
#40 lore raw
Quoted 26 lines · On Wed, 28 Nov 2018 16:00:21 -0500 Steven Rostedt <[email protected]> wrote:
Quoted 23 lines · On Wed, 28 Nov 2018 15:39:31 -0500 Joe Lawrence <[email protected]> wrote:

Hi Steve,

With your ftrace/urgent branch linked above, if I try a quick function_graph test like the following:

SYSFS=/sys/kernel/debug/tracing

echo 0 > "$SYSFS/tracing_on"
echo cmdline_proc_show > "$SYSFS/set_graph_function" echo function_graph > "$SYSFS/current_tracer" echo 1 > "$SYSFS/tracing_on"

I see a bunch of scheduler interrupt functions in the trace/trace_pipe without even invoking cmdline_proc_show().

This tests works as expected with Linux 4.20-rc3 though:

% cat /sys/kernel/debug/tracing/trace_pipe

   2)               |  cmdline_proc_show() {
   2)   0.320 us    |    seq_puts();
   2)   0.030 us    |    seq_putc();
   2)   1.352 us    |  }
Quoted 4 lines
Quoted 1 line

Operator error, or did the patchset break something?

Nope, that does seem to be a bug :-/

Does this patch fix it for you?

-- Steve

kernel/trace/trace.h +7 -1
index 3b8c0e24ab30..1c8f4aa7020e 100644
@@ -512,6 +512,9 @@ enum {
  * can only be modified by current, we can reuse trace_recursion.
  */
 	TRACE_IRQ_BIT,
+
+	/* Set if the function is in the set_graph_function file */
+	TRACE_GRAPH_BIT,
 };
 
 #define trace_recursion_set(bit)	do { (current)->trace_recursion |= (1<<(bit)); } while (0)
@@ -855,6 +858,8 @@ static inline int ftrace_graph_addr(unsigned long addr)
 	}
 
 	if (ftrace_lookup_ip(ftrace_graph_hash, addr)) {
+
+		trace_recursion_set(TRACE_GRAPH_BIT);
 		/*
 		 * If no irqs are to be traced, but a set_graph_function
 		 * is set, and called by an interrupt handler, we still
@@ -901,7 +906,8 @@ extern unsigned int fgraph_max_depth;
 static inline bool ftrace_graph_ignore_func(struct ftrace_graph_ent *trace)
 {
 	/* trace it when it is-nested-in or is a function enabled. */
-	return !(trace->depth || ftrace_graph_addr(trace->func)) ||
+	return !(trace_recursion_test(TRACE_GRAPH_BIT) ||
+		 ftrace_graph_addr(trace->func)) ||
 		(trace->depth < 0) ||
 		(fgraph_max_depth && trace->depth >= fgraph_max_depth);
 }
kernel/trace/trace_functions_graph.c +5 -0
index 2561460d7baf..69fbb6225637 100644
@@ -509,6 +509,9 @@ void trace_graph_return(struct ftrace_graph_ret *trace)
 	int cpu;
 	int pc;
 
+	if (!trace->depth)
+		trace_recursion_clear(TRACE_GRAPH_BIT);
+
 	local_irq_save(flags);
 	cpu = raw_smp_processor_id();
 	data = per_cpu_ptr(tr->trace_buffer.data, cpu);
@@ -532,6 +535,8 @@ void set_graph_array(struct trace_array *tr)
 
 static void trace_graph_thresh_return(struct ftrace_graph_ret *trace)
 {
+	if (!trace->depth)
+		trace_recursion_clear(TRACE_GRAPH_BIT);
 	if (tracing_thresh &&
 	    (trace->rettime - trace->calltime < tracing_thresh))
 		return;
kernel/trace/trace_irqsoff.c +3 -0
index b7357f9f82a3..b20bf076ce7f 100644
@@ -208,6 +208,9 @@ static void irqsoff_graph_return(struct ftrace_graph_ret *trace)
 	unsigned long flags;
 	int pc;
 
+	if (!trace->depth)
+		trace_recursion_clear(TRACE_GRAPH_BIT);
+
 	if (!func_prolog_dec(tr, &data, &flags))
 		return;
 
kernel/trace/trace_sched_wakeup.c +3 -0
index a86b303e6c67..069867f4eae6 100644
@@ -270,6 +270,9 @@ static void wakeup_graph_return(struct ftrace_graph_ret *trace)
 	unsigned long flags;
 	int pc;
 
+	if (!trace->depth)
+		trace_recursion_clear(TRACE_GRAPH_BIT);
+
 	if (!func_prolog_preempt_disable(tr, &data, &pc))
 		return;
 
author maintainer
#41 lore raw
Quoted 1 line · On Wed, 28 Nov 2018 22:29:36 -0500 Steven Rostedt <[email protected]> wrote:

Does this patch fix it for you?

Take 2. I realized that the reason for the interrupts being traced is because it is more likely to interrupt when the depth is already 0, setting it to 1 causing the interrupt to think it's already in a function that wants to be traced (when that wasn't decided yet).

The fix uses a bit that gets set when we want to trace (and will trace till that bit is cleared). That bit gets cleared on the return function when depth is zero. But if we are tracing something in a interrupt that happened to interrupt the beginning of a function that already set the depth to 0, then we need to clear the bit at depth of 1 not zero (and 2 if we want to trace a function that interrupted a softirq, that interrupted a start of normal function. and 3 if we want to trace an NMI function that had the very unlikely case of interrupting a start of a interrupt function, that interrupted the start of a softirq function, that interrupted a start of a normal context function!).

If that happens then we will continue to trace functions when we don't want to. To solve that, I need to record the depth (0-3) when the bit is set, and clear it when the return function is at that same depth (0-3).

-- Steve

kernel/trace/trace.h +54 -3
index 3b8c0e24ab30..447bd96ee658 100644
@@ -512,12 +512,44 @@ enum {
  * can only be modified by current, we can reuse trace_recursion.
  */
 	TRACE_IRQ_BIT,
+
+	/* Set if the function is in the set_graph_function file */
+	TRACE_GRAPH_BIT,
+
+	/*
+	 * In the very unlikely case that an interrupt came in
+	 * at a start of graph tracing, and we want to trace
+	 * the function in that interrupt, the depth can be greater
+	 * than zero, because of the preempted start of a previous
+	 * trace. In an even more unlikely case, depth could be 2
+	 * if a softirq interrupted the start of graph tracing,
+	 * followed by an interrupt preempting a start of graph
+	 * tracing in the softirq, and depth can even be 3
+	 * if an NMI came in at the start of an interrupt function
+	 * that preempted a softirq start of a function that
+	 * preempted normal context!!!! Luckily, it can't be
+	 * greater than 3, so the next two bits are a mask
+	 * of what the depth is when we set TRACE_GRAPH_BIT
+	 */
+
+	TRACE_GRAPH_DEPTH_START_BIT,
+	TRACE_GRAPH_DEPTH_END_BIT,
 };
 
 #define trace_recursion_set(bit)	do { (current)->trace_recursion |= (1<<(bit)); } while (0)
 #define trace_recursion_clear(bit)	do { (current)->trace_recursion &= ~(1<<(bit)); } while (0)
 #define trace_recursion_test(bit)	((current)->trace_recursion & (1<<(bit)))
 
+#define trace_recursion_depth() \
+	(((current)->trace_recursion >> TRACE_GRAPH_DEPTH_START_BIT) & 3)
+#define trace_recursion_set_depth(depth) \
+	do {								\
+		current->trace_recursion &=				\
+			~(3 << TRACE_GRAPH_DEPTH_START_BIT);		\
+		current->trace_recursion |=				\
+			((depth) & 3) << TRACE_GRAPH_DEPTH_START_BIT;	\
+	} while (0)
+
 #define TRACE_CONTEXT_BITS	4
 
 #define TRACE_FTRACE_START	TRACE_FTRACE_BIT
@@ -843,8 +875,9 @@ extern void __trace_graph_return(struct trace_array *tr,
 extern struct ftrace_hash *ftrace_graph_hash;
 extern struct ftrace_hash *ftrace_graph_notrace_hash;
 
-static inline int ftrace_graph_addr(unsigned long addr)
+static inline int ftrace_graph_addr(struct ftrace_graph_ent *trace)
 {
+	unsigned long addr = trace->func;
 	int ret = 0;
 
 	preempt_disable_notrace();
@@ -855,6 +888,14 @@ static inline int ftrace_graph_addr(unsigned long addr)
 	}
 
 	if (ftrace_lookup_ip(ftrace_graph_hash, addr)) {
+
+		/*
+		 * This needs to be cleared on the return functions
+		 * when the depth is zero.
+		 */
+		trace_recursion_set(TRACE_GRAPH_BIT);
+		trace_recursion_set_depth(trace->depth);
+
 		/*
 		 * If no irqs are to be traced, but a set_graph_function
 		 * is set, and called by an interrupt handler, we still
@@ -872,6 +913,13 @@ static inline int ftrace_graph_addr(unsigned long addr)
 	return ret;
 }
 
+static inline void ftrace_graph_addr_finish(struct ftrace_graph_ret *trace)
+{
+	if (trace_recursion_test(TRACE_GRAPH_BIT) &&
+	    trace->depth == trace_recursion_depth())
+		trace_recursion_clear(TRACE_GRAPH_BIT);
+}
+
 static inline int ftrace_graph_notrace_addr(unsigned long addr)
 {
 	int ret = 0;
@@ -885,7 +933,7 @@ static inline int ftrace_graph_notrace_addr(unsigned long addr)
 	return ret;
 }
 #else
-static inline int ftrace_graph_addr(unsigned long addr)
+static inline int ftrace_graph_addr(struct ftrace_graph_ent *trace)
 {
 	return 1;
 }
@@ -894,6 +942,8 @@ static inline int ftrace_graph_notrace_addr(unsigned long addr)
 {
 	return 0;
 }
+static inline void ftrace_graph_addr_finish(struct ftrace_graph_ret *trace)
+{ }
 #endif /* CONFIG_DYNAMIC_FTRACE */
 
 extern unsigned int fgraph_max_depth;
@@ -901,7 +951,8 @@ extern unsigned int fgraph_max_depth;
 static inline bool ftrace_graph_ignore_func(struct ftrace_graph_ent *trace)
 {
 	/* trace it when it is-nested-in or is a function enabled. */
-	return !(trace->depth || ftrace_graph_addr(trace->func)) ||
+	return !(trace_recursion_test(TRACE_GRAPH_BIT) ||
+		 ftrace_graph_addr(trace)) ||
 		(trace->depth < 0) ||
 		(fgraph_max_depth && trace->depth >= fgraph_max_depth);
 }
kernel/trace/trace_functions_graph.c +4 -0
index 2561460d7baf..086af4f5c3e8 100644
@@ -509,6 +509,8 @@ void trace_graph_return(struct ftrace_graph_ret *trace)
 	int cpu;
 	int pc;
 
+	ftrace_graph_addr_finish(trace);
+
 	local_irq_save(flags);
 	cpu = raw_smp_processor_id();
 	data = per_cpu_ptr(tr->trace_buffer.data, cpu);
@@ -532,6 +534,8 @@ void set_graph_array(struct trace_array *tr)
 
 static void trace_graph_thresh_return(struct ftrace_graph_ret *trace)
 {
+	ftrace_graph_addr_finish(trace);
+
 	if (tracing_thresh &&
 	    (trace->rettime - trace->calltime < tracing_thresh))
 		return;
kernel/trace/trace_irqsoff.c +2 -0
index b7357f9f82a3..98ea6d28df15 100644
@@ -208,6 +208,8 @@ static void irqsoff_graph_return(struct ftrace_graph_ret *trace)
 	unsigned long flags;
 	int pc;
 
+	ftrace_graph_addr_finish(trace);
+
 	if (!func_prolog_dec(tr, &data, &flags))
 		return;
 
kernel/trace/trace_sched_wakeup.c +2 -0
index a86b303e6c67..7d04b9890755 100644
@@ -270,6 +270,8 @@ static void wakeup_graph_return(struct ftrace_graph_ret *trace)
 	unsigned long flags;
 	int pc;
 
+	ftrace_graph_addr_finish(trace);
+
 	if (!func_prolog_preempt_disable(tr, &data, &pc))
 		return;
 
#42 lore raw
Quoted 187 lines · Steven Rostedt
Quoted 1 line · On Wed, 28 Nov 2018 22:29:36 -0500 Steven Rostedt <[email protected]> wrote:

Does this patch fix it for you?

Take 2. I realized that the reason for the interrupts being traced is because it is more likely to interrupt when the depth is already 0, setting it to 1 causing the interrupt to think it's already in a function that wants to be traced (when that wasn't decided yet).

The fix uses a bit that gets set when we want to trace (and will trace till that bit is cleared). That bit gets cleared on the return function when depth is zero. But if we are tracing something in a interrupt that happened to interrupt the beginning of a function that already set the depth to 0, then we need to clear the bit at depth of 1 not zero (and 2 if we want to trace a function that interrupted a softirq, that interrupted a start of normal function. and 3 if we want to trace an NMI function that had the very unlikely case of interrupting a start of a interrupt function, that interrupted the start of a softirq function, that interrupted a start of a normal context function!).

If that happens then we will continue to trace functions when we don't want to. To solve that, I need to record the depth (0-3) when the bit is set, and clear it when the return function is at that same depth (0-3).

-- Steve

kernel/trace/trace.h +54 -3
index 3b8c0e24ab30..447bd96ee658 100644
@@ -512,12 +512,44 @@ enum {
  * can only be modified by current, we can reuse trace_recursion.
  */
 	TRACE_IRQ_BIT,
+
+	/* Set if the function is in the set_graph_function file */
+	TRACE_GRAPH_BIT,
+
+	/*
+	 * In the very unlikely case that an interrupt came in
+	 * at a start of graph tracing, and we want to trace
+	 * the function in that interrupt, the depth can be greater
+	 * than zero, because of the preempted start of a previous
+	 * trace. In an even more unlikely case, depth could be 2
+	 * if a softirq interrupted the start of graph tracing,
+	 * followed by an interrupt preempting a start of graph
+	 * tracing in the softirq, and depth can even be 3
+	 * if an NMI came in at the start of an interrupt function
+	 * that preempted a softirq start of a function that
+	 * preempted normal context!!!! Luckily, it can't be
+	 * greater than 3, so the next two bits are a mask
+	 * of what the depth is when we set TRACE_GRAPH_BIT
+	 */
+
+	TRACE_GRAPH_DEPTH_START_BIT,
+	TRACE_GRAPH_DEPTH_END_BIT,
 };
 
 #define trace_recursion_set(bit)	do { (current)->trace_recursion |= (1<<(bit)); } while (0)
 #define trace_recursion_clear(bit)	do { (current)->trace_recursion &= ~(1<<(bit)); } while (0)
 #define trace_recursion_test(bit)	((current)->trace_recursion & (1<<(bit)))
 
+#define trace_recursion_depth() \
+	(((current)->trace_recursion >> TRACE_GRAPH_DEPTH_START_BIT) & 3)
+#define trace_recursion_set_depth(depth) \
+	do {								\
+		current->trace_recursion &=				\
+			~(3 << TRACE_GRAPH_DEPTH_START_BIT);		\
+		current->trace_recursion |=				\
+			((depth) & 3) << TRACE_GRAPH_DEPTH_START_BIT;	\
+	} while (0)
+
 #define TRACE_CONTEXT_BITS	4
 
 #define TRACE_FTRACE_START	TRACE_FTRACE_BIT
@@ -843,8 +875,9 @@ extern void __trace_graph_return(struct trace_array *tr,
 extern struct ftrace_hash *ftrace_graph_hash;
 extern struct ftrace_hash *ftrace_graph_notrace_hash;
 
-static inline int ftrace_graph_addr(unsigned long addr)
+static inline int ftrace_graph_addr(struct ftrace_graph_ent *trace)
 {
+	unsigned long addr = trace->func;
 	int ret = 0;
 
 	preempt_disable_notrace();
@@ -855,6 +888,14 @@ static inline int ftrace_graph_addr(unsigned long addr)
 	}
 
 	if (ftrace_lookup_ip(ftrace_graph_hash, addr)) {
+
+		/*
+		 * This needs to be cleared on the return functions
+		 * when the depth is zero.
+		 */
+		trace_recursion_set(TRACE_GRAPH_BIT);
+		trace_recursion_set_depth(trace->depth);
+
 		/*
 		 * If no irqs are to be traced, but a set_graph_function
 		 * is set, and called by an interrupt handler, we still
@@ -872,6 +913,13 @@ static inline int ftrace_graph_addr(unsigned long addr)
 	return ret;
 }
 
+static inline void ftrace_graph_addr_finish(struct ftrace_graph_ret *trace)
+{
+	if (trace_recursion_test(TRACE_GRAPH_BIT) &&
+	    trace->depth == trace_recursion_depth())
+		trace_recursion_clear(TRACE_GRAPH_BIT);
+}
+
 static inline int ftrace_graph_notrace_addr(unsigned long addr)
 {
 	int ret = 0;
@@ -885,7 +933,7 @@ static inline int ftrace_graph_notrace_addr(unsigned long addr)
 	return ret;
 }
 #else
-static inline int ftrace_graph_addr(unsigned long addr)
+static inline int ftrace_graph_addr(struct ftrace_graph_ent *trace)
 {
 	return 1;
 }
@@ -894,6 +942,8 @@ static inline int ftrace_graph_notrace_addr(unsigned long addr)
 {
 	return 0;
 }
+static inline void ftrace_graph_addr_finish(struct ftrace_graph_ret *trace)
+{ }
 #endif /* CONFIG_DYNAMIC_FTRACE */
 
 extern unsigned int fgraph_max_depth;
@@ -901,7 +951,8 @@ extern unsigned int fgraph_max_depth;
 static inline bool ftrace_graph_ignore_func(struct ftrace_graph_ent *trace)
 {
 	/* trace it when it is-nested-in or is a function enabled. */
-	return !(trace->depth || ftrace_graph_addr(trace->func)) ||
+	return !(trace_recursion_test(TRACE_GRAPH_BIT) ||
+		 ftrace_graph_addr(trace)) ||
 		(trace->depth < 0) ||
 		(fgraph_max_depth && trace->depth >= fgraph_max_depth);
 }
kernel/trace/trace_functions_graph.c +4 -0
index 2561460d7baf..086af4f5c3e8 100644
@@ -509,6 +509,8 @@ void trace_graph_return(struct ftrace_graph_ret *trace)
 	int cpu;
 	int pc;
 
+	ftrace_graph_addr_finish(trace);
+
 	local_irq_save(flags);
 	cpu = raw_smp_processor_id();
 	data = per_cpu_ptr(tr->trace_buffer.data, cpu);
@@ -532,6 +534,8 @@ void set_graph_array(struct trace_array *tr)
 
 static void trace_graph_thresh_return(struct ftrace_graph_ret *trace)
 {
+	ftrace_graph_addr_finish(trace);
+
 	if (tracing_thresh &&
 	    (trace->rettime - trace->calltime < tracing_thresh))
 		return;
kernel/trace/trace_irqsoff.c +2 -0
index b7357f9f82a3..98ea6d28df15 100644
@@ -208,6 +208,8 @@ static void irqsoff_graph_return(struct ftrace_graph_ret *trace)
 	unsigned long flags;
 	int pc;
 
+	ftrace_graph_addr_finish(trace);
+
 	if (!func_prolog_dec(tr, &data, &flags))
 		return;
 
kernel/trace/trace_sched_wakeup.c +0 -0
index a86b303e6c67..7d04b9890755 100644

kernel/trace/trace_sched_wakeup.c static void wakeup_graph_return(struct ftrace_graph_ret *trace) · line 277

Quoted 8 lines
kernel/trace/trace_sched_wakeup.c +2 -0
@@ -270,6 +270,8 @@ static void wakeup_graph_return(struct ftrace_graph_ret *trace)
 	unsigned long flags;
 	int pc;
 
+	ftrace_graph_addr_finish(trace);
+
 	if (!func_prolog_preempt_disable(tr, &data, &pc))
 		return;
 

With the "take 2" patch, I only see smp_irq_work_interrupt() graph when the graph_function is in progress.. for example:

 3)               |  cmdline_proc_show() {
 3)   ==========> |
 3)               |    smp_irq_work_interrupt() {
 3)               |      irq_enter() {
 3)               |        rcu_irq_enter() {
 3)   0.141 us    |          rcu_dynticks_curr_cpu_in_eqs();
 3)   0.589 us    |        }
 3)   0.877 us    |      }
 3)               |      __wake_up() {
 3)               |        __wake_up_common_lock() {
 3)   0.122 us    |          _raw_spin_lock_irqsave();
 3)               |          __wake_up_common() {
 3)               |            autoremove_wake_function() {
 3)               |              default_wake_function() {
 3)               |                try_to_wake_up() {
 3)   0.209 us    |                  _raw_spin_lock_irqsave();
 3)               |                  select_task_rq_fair() {
 3)               |                    select_idle_sibling() {
 3)   0.140 us    |                      available_idle_cpu();
 3)   0.408 us    |                    }
 3)   0.711 us    |                  }
 3)               |                  native_smp_send_reschedule() {
 3)   4.298 us    |                    x2apic_send_IPI();
 3)   4.574 us    |                  }
 3)   0.144 us    |                  _raw_spin_unlock_irqrestore();
 3)   6.677 us    |                }
 3)   6.915 us    |              }
 3)   7.182 us    |            }
 3)   7.620 us    |          }
 3)   0.132 us    |          _raw_spin_unlock_irqrestore();
 3)   8.401 us    |        }
 3)   9.066 us    |      }
 3)               |      irq_exit() {
 3)   0.128 us    |        idle_cpu();
 3)               |        rcu_irq_exit() {
 3)   0.122 us    |          rcu_dynticks_curr_cpu_in_eqs();
 3) + 10.598 us   |        }
 3) + 11.128 us   |      }
 3) + 22.075 us   |    }
 3)   <========== |
 3)   0.414 us    |    seq_puts();
 3)   0.128 us    |    seq_putc();
 3) + 29.040 us   |  }

-- Joe

author maintainer
#43 lore raw
Quoted 2 lines · On Thu, 29 Nov 2018 10:08:55 -0500 Joe Lawrence <[email protected]> wrote:

With the "take 2" patch, I only see smp_irq_work_interrupt() graph when the graph_function is in progress.. for example:

In other words it works :-)

Can I get a Tested-by from you?

-- Steve

Tested-by
#44 lore raw
Quoted 10 lines · Steven Rostedt
Quoted 2 lines · On Thu, 29 Nov 2018 10:08:55 -0500 Joe Lawrence <[email protected]> wrote:

With the "take 2" patch, I only see smp_irq_work_interrupt() graph when the graph_function is in progress.. for example:

In other words it works :-)

Can I get a Tested-by from you?

Yup, thanks for fixing!

-- Joe