[for-next][PATCH 00/18] function_graph: Add separate depth counter to prevent trace corruption

[for-next][PATCH 00/18] function_graph: Add separate depth counter to prevent trace corruption

[Steven Rostedt]

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(-)

[for-next][PATCH 01/18] function_graph: Create function_graph_enter() to consolidate architecture code

[Steven Rostedt]

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

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.

Cc: stable@kernel.org
Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/linux/ftrace.h               |  3 +++
 kernel/trace/trace_functions_graph.c | 16 ++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index a397907e8d72..5717e8f81c59 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -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);
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index 169b3c44ee97..28f2602435d0 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -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,
-- 
2.19.1

[for-next][PATCH 02/18] x86/function_graph: Simplify with function_graph_entry()

[Steven Rostedt]

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

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 <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: stable@kernel.org
Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/x86/kernel/ftrace.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 01ebcb6f263e..7ee8067cbf45 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -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 */
-- 
2.19.1

[for-next][PATCH 03/18] ARM: function_graph: Simplify with function_graph_entry()

[Steven Rostedt]

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

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 <linux@armlinux.org.uk>
Cc: linux-arm-kernel@lists.infradead.org
Cc: stable@kernel.org
Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/arm/kernel/ftrace.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
index 0142fcfcc3d3..bda949fd84e8 100644
--- a/arch/arm/kernel/ftrace.c
+++ b/arch/arm/kernel/ftrace.c
@@ -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
-- 
2.19.1

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

[Steven Rostedt]

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

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 <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: stable@kernel.org
Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/arm64/kernel/ftrace.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index 50986e388d2b..57e962290df3 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -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;
 }
 
-- 
2.19.1

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

[Will Deacon]

On Wed, Nov 21, 2018 at 07:28:05PM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> 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 <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: stable@kernel.org
> Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  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:

Acked-by: Will Deacon <will.deacon@arm.com>

Will

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

[Steven Rostedt]

On Tue, 27 Nov 2018 18:07:23 +0000
Will Deacon <will.deacon@arm.com> wrote:

> On Wed, Nov 21, 2018 at 07:28:05PM -0500, Steven Rostedt wrote:
> > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > 
> > 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 <catalin.marinas@arm.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: stable@kernel.org
> > Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > ---
> >  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.

Ah you're right!

> 
> Anyway, looks like no change for us, so:
> 
> Acked-by: Will Deacon <will.deacon@arm.com>

Thanks!

-- Steve

[for-next][PATCH 05/18] microblaze: function_graph: Simplify with function_graph_entry()

[Steven Rostedt]

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

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 <monstr@monstr.eu>
Cc: stable@kernel.org
Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/microblaze/kernel/ftrace.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/arch/microblaze/kernel/ftrace.c b/arch/microblaze/kernel/ftrace.c
index d57563c58a26..224eea40e1ee 100644
--- a/arch/microblaze/kernel/ftrace.c
+++ b/arch/microblaze/kernel/ftrace.c
@@ -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 */
 
-- 
2.19.1

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

[Steven Rostedt]

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

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 <ralf@linux-mips.org>
Cc: Paul Burton <paul.burton@mips.com>
Cc: James Hogan <jhogan@kernel.org>
Cc: linux-mips@linux-mips.org
Cc: stable@kernel.org
Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/mips/kernel/ftrace.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
index 7f3dfdbc3657..b122cbb4aad1 100644
--- a/arch/mips/kernel/ftrace.c
+++ b/arch/mips/kernel/ftrace.c
@@ -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();
-- 
2.19.1

[for-next][PATCH 07/18] nds32: function_graph: Simplify with function_graph_entry()

[Steven Rostedt]

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

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 <greentime@andestech.com>
Cc: stable@kernel.org
Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/nds32/kernel/ftrace.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/arch/nds32/kernel/ftrace.c b/arch/nds32/kernel/ftrace.c
index a0a9679ad5de..8a41372551ff 100644
--- a/arch/nds32/kernel/ftrace.c
+++ b/arch/nds32/kernel/ftrace.c
@@ -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)
-- 
2.19.1

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

[Steven Rostedt]

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

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" <jejb@parisc-linux.org>
Cc: Helge Deller <deller@gmx.de>
Cc: linux-parisc@vger.kernel.org
Cc: stable@kernel.org
Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/parisc/kernel/ftrace.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/arch/parisc/kernel/ftrace.c b/arch/parisc/kernel/ftrace.c
index 6fa8535d3cce..e46a4157a894 100644
--- a/arch/parisc/kernel/ftrace.c
+++ b/arch/parisc/kernel/ftrace.c
@@ -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 */
 
-- 
2.19.1

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

[Helge Deller]

HI Sasha,

On 23.11.18 08:30, Sasha Levin wrote:
> 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?

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

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

[Steven Rostedt]

On Fri, 23 Nov 2018 10:06:05 +0100
Helge Deller <deller@gmx.de> wrote:


> > 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

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

[Sasha Levin]

On Fri, Nov 23, 2018 at 12:12:53PM -0500, Steven Rostedt wrote:
>On Fri, 23 Nov 2018 10:06:05 +0100
>Helge Deller <deller@gmx.de> wrote:
>
>
>> > 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.

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

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

[Steven Rostedt]

On Fri, 23 Nov 2018 13:34:15 -0500
Sasha Levin <sashal@kernel.org> 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

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

[Sasha Levin]

On Fri, Nov 23, 2018 at 02:26:17PM -0500, Steven Rostedt wrote:
>On Fri, 23 Nov 2018 13:34:15 -0500
>Sasha Levin <sashal@kernel.org> 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

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

[Steven Rostedt]

On Fri, 23 Nov 2018 15:00:11 -0500
Sasha Levin <sashal@kernel.org> wrote:

> On Fri, Nov 23, 2018 at 02:26:17PM -0500, Steven Rostedt wrote:
> >On Fri, 23 Nov 2018 13:34:15 -0500
> >Sasha Levin <sashal@kernel.org> 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.

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.

> 
> 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.

> 
> 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

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

[Sasha Levin]

On Sat, Nov 24, 2018 at 01:46:34PM -0500, Steven Rostedt wrote:
>On Fri, 23 Nov 2018 15:00:11 -0500
>Sasha Levin <sashal@kernel.org> 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.
>
>>
>> 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

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

[Steven Rostedt]

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

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 <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: stable@kernel.org
Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/powerpc/kernel/trace/ftrace.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 4bf051d3e21e..b65c8a34ad6e 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -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;
 }
-- 
2.19.1

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

[Steven Rostedt]

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

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 <greentime@andestech.com>
Cc: Alan Kao <alankao@andestech.com>
Cc: Palmer Dabbelt <palmer@sifive.com>
Cc: stable@kernel.org
Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/riscv/kernel/ftrace.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
index 1157b6b52d25..c433f6d3dd64 100644
--- a/arch/riscv/kernel/ftrace.c
+++ b/arch/riscv/kernel/ftrace.c
@@ -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
-- 
2.19.1

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

[Palmer Dabbelt]

On Wed, 21 Nov 2018 16:28:11 PST (-0800), rostedt@goodmis.org wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> 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 <greentime@andestech.com>
> Cc: Alan Kao <alankao@andestech.com>
> Cc: Palmer Dabbelt <palmer@sifive.com>
> Cc: stable@kernel.org
> Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  arch/riscv/kernel/ftrace.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
> index 1157b6b52d25..c433f6d3dd64 100644
> --- a/arch/riscv/kernel/ftrace.c
> +++ b/arch/riscv/kernel/ftrace.c
> @@ -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

Reviewed-by: Palmer Dabbelt <palmer@sifive.com>

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

[Steven Rostedt]

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

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 <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Julian Wiedmann <jwi@linux.ibm.com>
Cc: linux-s390@vger.kernel.org
Cc: stable@kernel.org
Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/s390/kernel/ftrace.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/arch/s390/kernel/ftrace.c b/arch/s390/kernel/ftrace.c
index 84be7f02d0c2..39b13d71a8fe 100644
--- a/arch/s390/kernel/ftrace.c
+++ b/arch/s390/kernel/ftrace.c
@@ -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;
 }
-- 
2.19.1

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

[Martin Schwidefsky]

On Wed, 21 Nov 2018 19:28:12 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> 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 <schwidefsky@de.ibm.com>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: Julian Wiedmann <jwi@linux.ibm.com>
> Cc: linux-s390@vger.kernel.org
> Cc: stable@kernel.org
> Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

A quick test showed that the patch series works fine on s390.
Acked-by: Martin Schwidefsky <schwidefsky@de.ibm.com>

> ---
>  arch/s390/kernel/ftrace.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/s390/kernel/ftrace.c b/arch/s390/kernel/ftrace.c
> index 84be7f02d0c2..39b13d71a8fe 100644
> --- a/arch/s390/kernel/ftrace.c
> +++ b/arch/s390/kernel/ftrace.c
> @@ -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;
>  }


-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

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

[Steven Rostedt]

On Thu, 22 Nov 2018 07:43:19 +0100
Martin Schwidefsky <schwidefsky@de.ibm.com> wrote:

> A quick test showed that the patch series works fine on s390.
> Acked-by: Martin Schwidefsky <schwidefsky@de.ibm.com>

Thanks for testing, and the ack!

-- Steve

[for-next][PATCH 12/18] sh/function_graph: Simplify with function_graph_entry()

[Steven Rostedt]

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

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 <ysato@users.sourceforge.jp>
Cc: Rich Felker <dalias@libc.org>
Cc: linux-sh@vger.kernel.org
Cc: stable@kernel.org
Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/sh/kernel/ftrace.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/arch/sh/kernel/ftrace.c b/arch/sh/kernel/ftrace.c
index 96dd9f7da250..1b04270e5460 100644
--- a/arch/sh/kernel/ftrace.c
+++ b/arch/sh/kernel/ftrace.c
@@ -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 */
-- 
2.19.1

[for-next][PATCH 13/18] sparc/function_graph: Simplify with function_graph_entry()

[Steven Rostedt]

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

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" <davem@davemloft.net>
Cc: sparclinux@vger.kernel.org
Cc: stable@kernel.org
Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/sparc/kernel/ftrace.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/arch/sparc/kernel/ftrace.c b/arch/sparc/kernel/ftrace.c
index 915dda4ae412..684b84ce397f 100644
--- a/arch/sparc/kernel/ftrace.c
+++ b/arch/sparc/kernel/ftrace.c
@@ -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;
-- 
2.19.1

[for-next][PATCH 14/18] function_graph: Make ftrace_push_return_trace() static

[Steven Rostedt]

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

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.

Cc: stable@kernel.org
Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/linux/ftrace.h               | 3 ---
 kernel/trace/trace_functions_graph.c | 2 +-
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 5717e8f81c59..dd16e8218db3 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -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);
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index 28f2602435d0..88ca787a1cdc 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -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)
 {
-- 
2.19.1

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

[Steven Rostedt]

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

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.

Cc: stable@kernel.org
Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/linux/sched.h                |  1 +
 kernel/trace/ftrace.c                |  3 +++
 kernel/trace/trace_functions_graph.c | 21 +++++++++++++--------
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index a51c13c2b1a0..d6183a55e8eb 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -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;
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index f536f601bd46..48513954713c 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -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;
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index 88ca787a1cdc..02d4081a7f5a 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -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.
-- 
2.19.1

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

[Peter Zijlstra]

On Wed, Nov 21, 2018 at 07:28:16PM -0500, Steven Rostedt wrote:
> @@ -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))

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.

> +		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.

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

[Steven Rostedt]

On Thu, 22 Nov 2018 11:03:22 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Nov 21, 2018 at 07:28:16PM -0500, Steven Rostedt wrote:
> > @@ -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))  
> 
> 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.

> 
> > +		goto out;
> > +
> > +	return 0;
> > + out:
> > +	current->curr_ret_depth--;
> > +	return -EBUSY;
> >  }  
> 
> [diff "default"]
> 	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

> 
> 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

[for-next][PATCH 16/18] function_graph: Move return callback before update of curr_ret_stack

[Steven Rostedt]

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

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.

Cc: stable@kernel.org
Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_functions_graph.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index 02d4081a7f5a..4f0d72ae6362 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -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);
-- 
2.19.1

[for-next][PATCH 17/18] function_graph: Reverse the order of pushing the ret_stack and the callback

[Steven Rostedt]

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

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.

Cc: stable@kernel.org
Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_functions_graph.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index 4f0d72ae6362..2561460d7baf 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -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;
-- 
2.19.1

[for-next][PATCH 18/18] function_graph: Have profiler use curr_ret_stack and not depth

[Steven Rostedt]

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

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.

Cc: stable@kernel.org
Fixes: 03274a3ffb449 ("tracing/fgraph: Adjust fgraph depth before calling trace return callback")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 48513954713c..77734451cb05 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -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)
-- 
2.19.1

Re: [for-next][PATCH 00/18] function_graph: Add separate depth counter to prevent trace corruption

[Masami Hiramatsu]

Hi Steve,

This series looks good to me.

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

for this series.

Thank you!

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


On Wed, 21 Nov 2018 19:28:01 -0500
Steven Rostedt <rostedt@goodmis.org> 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(-)


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [for-next][PATCH 00/18] function_graph: Add separate depth counter to prevent trace corruption

[Joe Lawrence]

On Wed, Nov 21, 2018 at 07:28:01PM -0500, Steven Rostedt wrote:
>
> [ ... 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

Re: [for-next][PATCH 00/18] function_graph: Add separate depth counter to prevent trace corruption

[Steven Rostedt]

On Wed, 28 Nov 2018 15:39:31 -0500
Joe Lawrence <joe.lawrence@redhat.com> 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    |  }
> 
> Operator error, or did the patchset break something?

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

-- Steve

Re: [for-next][PATCH 00/18] function_graph: Add separate depth counter to prevent trace corruption

[Steven Rostedt]

On Wed, 28 Nov 2018 16:00:21 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 28 Nov 2018 15:39:31 -0500
> Joe Lawrence <joe.lawrence@redhat.com> 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    |  }
> > 
> > Operator error, or did the patchset break something?  
> 
> Nope, that does seem to be a bug :-/
>

Does this patch fix it for you?

-- Steve

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 3b8c0e24ab30..1c8f4aa7020e 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -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);
 }
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index 2561460d7baf..69fbb6225637 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -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;
diff --git a/kernel/trace/trace_irqsoff.c b/kernel/trace/trace_irqsoff.c
index b7357f9f82a3..b20bf076ce7f 100644
--- a/kernel/trace/trace_irqsoff.c
+++ b/kernel/trace/trace_irqsoff.c
@@ -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;
 
diff --git a/kernel/trace/trace_sched_wakeup.c b/kernel/trace/trace_sched_wakeup.c
index a86b303e6c67..069867f4eae6 100644
--- a/kernel/trace/trace_sched_wakeup.c
+++ b/kernel/trace/trace_sched_wakeup.c
@@ -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;
 

Re: [for-next][PATCH 00/18] function_graph: Add separate depth counter to prevent trace corruption

[Steven Rostedt]

On Wed, 28 Nov 2018 22:29:36 -0500
Steven Rostedt <rostedt@goodmis.org> 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

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 3b8c0e24ab30..447bd96ee658 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -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);
 }
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index 2561460d7baf..086af4f5c3e8 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -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;
diff --git a/kernel/trace/trace_irqsoff.c b/kernel/trace/trace_irqsoff.c
index b7357f9f82a3..98ea6d28df15 100644
--- a/kernel/trace/trace_irqsoff.c
+++ b/kernel/trace/trace_irqsoff.c
@@ -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;
 
diff --git a/kernel/trace/trace_sched_wakeup.c b/kernel/trace/trace_sched_wakeup.c
index a86b303e6c67..7d04b9890755 100644
--- a/kernel/trace/trace_sched_wakeup.c
+++ b/kernel/trace/trace_sched_wakeup.c
@@ -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;
 

Re: [for-next][PATCH 00/18] function_graph: Add separate depth counter to prevent trace corruption

[Joe Lawrence]

On 11/28/2018 11:24 PM, Steven Rostedt wrote:
> On Wed, 28 Nov 2018 22:29:36 -0500
> Steven Rostedt <rostedt@goodmis.org> 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
> 
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 3b8c0e24ab30..447bd96ee658 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -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);
>  }
> diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> index 2561460d7baf..086af4f5c3e8 100644
> --- a/kernel/trace/trace_functions_graph.c
> +++ b/kernel/trace/trace_functions_graph.c
> @@ -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;
> diff --git a/kernel/trace/trace_irqsoff.c b/kernel/trace/trace_irqsoff.c
> index b7357f9f82a3..98ea6d28df15 100644
> --- a/kernel/trace/trace_irqsoff.c
> +++ b/kernel/trace/trace_irqsoff.c
> @@ -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;
>  
> diff --git a/kernel/trace/trace_sched_wakeup.c b/kernel/trace/trace_sched_wakeup.c
> index a86b303e6c67..7d04b9890755 100644
> --- a/kernel/trace/trace_sched_wakeup.c
> +++ b/kernel/trace/trace_sched_wakeup.c
> @@ -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

Re: [for-next][PATCH 00/18] function_graph: Add separate depth counter to prevent trace corruption

[Steven Rostedt]

On Thu, 29 Nov 2018 10:08:55 -0500
Joe Lawrence <joe.lawrence@redhat.com> 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

Re: [for-next][PATCH 00/18] function_graph: Add separate depth counter to prevent trace corruption

[Joe Lawrence]

On 11/29/2018 11:17 AM, Steven Rostedt wrote:
> On Thu, 29 Nov 2018 10:08:55 -0500
> Joe Lawrence <joe.lawrence@redhat.com> 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!

Tested-by: Joe Lawrence <joe.lawrence@redhat.com>

-- Joe