Skip to contentSkip to conversation

Ftrace fixes

PATCH Superseded Julien Thierry 4 participants 10 messages Nov 1, 2017 – Nov 6, 2017

Subsystems (4)
Arm (32-bit) perf events arm64 ftrace

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Julien Thierry

author
#0 lore raw

cover Ftrace fixes

Hi,

Quoted 1 line

From what Steve Rostedt said, we shouldn't rely on ftrace_trace_function

not being ftrace_stub when using the function graph tracer. As x86 does, always check whether function graph tracing is requested in static tracing.

While testing that I didn't break things, I discovered that the perf ftrace builtin doesn't handle kernels built without function graph tracer (regardless of whether tracing is dynamic or not).

First patch fixes the ftrace issue.
Second patch fixes the perf front-end.

Cheers,

Julien Thierry (2):
arm64: Fix static use of function graph
perf: Fix ftrace builtin when kernel doesn't have function_graph

 arch/arm64/kernel/entry-ftrace.S | 12 +++---------
 tools/perf/builtin-ftrace.c      |  3 ++-
 2 files changed, 5 insertions(+), 10 deletions(-)

--
1.9.1

author
#1 lore raw

PATCH 1/2 arm64: Fix static use of function graph

Function graph does not work currently when CONFIG_DYNAMIC_TRACE is not set. This is because ftrace_function_trace is not always set to ftrace_stub when function_graph is in use.

Do not skip checking of graph tracer functions when ftrace_function_trace is set.

Signed-off-by Julien Thierry <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc AKASHI Takahiro <[email protected]>
---
 arch/arm64/kernel/entry-ftrace.S | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)
arch/arm64/kernel/entry-ftrace.S +3 -9
index e1be42e..1175f58 100644
@@ -108,13 +108,8 @@ ENTRY(_mcount)
 	mcount_get_lr	x1		//       function's lr (= parent's pc)
 	blr	x2			//   (*ftrace_trace_function)(pc, lr);
 
-#ifndef CONFIG_FUNCTION_GRAPH_TRACER
-skip_ftrace_call:			//   return;
-	mcount_exit			// }
-#else
-	mcount_exit			//   return;
-					// }
-skip_ftrace_call:
+skip_ftrace_call:			// }
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	ldr_l	x2, ftrace_graph_return
 	cmp	x0, x2			//   if ((ftrace_graph_return
 	b.ne	ftrace_graph_caller	//        != ftrace_stub)

@@ -123,9 +118,8 @@ skip_ftrace_call:
 	adr_l	x0, ftrace_graph_entry_stub //     != ftrace_graph_entry_stub))
 	cmp	x0, x2
 	b.ne	ftrace_graph_caller	//     ftrace_graph_caller();
-
-	mcount_exit
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
+	mcount_exit
 ENDPROC(_mcount)
 
 #else /* CONFIG_DYNAMIC_FTRACE */

--
1.9.1

maintainer
#2 lore raw

PATCH 1/2 arm64: Fix static use of function graph

Quoted 8 lines · On Wed, 1 Nov 2017 14:33:43 +0000 Julien Thierry <[email protected]>…

Function graph does not work currently when CONFIG_DYNAMIC_TRACE is not set. This is because ftrace_function_trace is not always set to ftrace_stub when function_graph is in use.

Do not skip checking of graph tracer functions when ftrace_function_trace is set.

Signed-off-by Julien Thierry <[email protected]>

My arm code is very rusty and I have no idea if this works or not, but for what it's worth.

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

-- Steve

Quoted 41 lines
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc AKASHI Takahiro <[email protected]>
---
 arch/arm64/kernel/entry-ftrace.S | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)
arch/arm64/kernel/entry-ftrace.S +3 -10
index e1be42e..1175f58 100644
@@ -108,13 +108,8 @@ ENTRY(_mcount)
 	mcount_get_lr	x1		//       function's lr (= parent's pc)
 	blr	x2			//   (*ftrace_trace_function)(pc, lr);
 
-#ifndef CONFIG_FUNCTION_GRAPH_TRACER
-skip_ftrace_call:			//   return;
-	mcount_exit			// }
-#else
-	mcount_exit			//   return;
-					// }
-skip_ftrace_call:
+skip_ftrace_call:			// }
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	ldr_l	x2, ftrace_graph_return
 	cmp	x0, x2			//   if ((ftrace_graph_return
 	b.ne	ftrace_graph_caller	//        != ftrace_stub)

@@ -123,9 +118,8 @@ skip_ftrace_call:
 	adr_l	x0, ftrace_graph_entry_stub //     != ftrace_graph_entry_stub))
 	cmp	x0, x2
 	b.ne	ftrace_graph_caller	//     ftrace_graph_caller();
-
-	mcount_exit
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
+	mcount_exit
 ENDPROC(_mcount)
 
 #else /* CONFIG_DYNAMIC_FTRACE */
--

1.9.1

#3 lore raw

PATCH 1/2 arm64: Fix static use of function graph

In reply to Julien Thierry
Quoted 4 lines · Julien Thierry

Function graph does not work currently when CONFIG_DYNAMIC_TRACE is not set. This is because ftrace_function_trace is not always set to ftrace_stub when function_graph is in use.

Quoted 8 lines

Do not skip checking of graph tracer functions when ftrace_function_trace is set.

Signed-off-by Julien Thierry <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc AKASHI Takahiro <[email protected]>
Reviewed-by AKASHI Takahiro <[email protected]>

I think that arm has the same issue, hoping the following patch will fix it:
===

Quoted 1 line

From e25bcf50d1acde698285a0c64f72d97f8b17e3fb Mon Sep 17 00:00:00 2001

From: AKASHI Takahiro <[email protected]>
Date: Thu, 2 Nov 2017 11:35:04 +0900
Subject: [PATCH] arm: ftrace: function_graph with DYNAMIC_FTRACE

---
 arch/arm/kernel/entry-ftrace.S | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
arch/arm/kernel/entry-ftrace.S +5 -7
index efcd9f25a14b..ef94c73ad996 100644
@@ -68,8 +68,12 @@
 	ldr	r2, [r0]
 	adr	r0, .Lftrace_stub
 	cmp	r0, r2
-	bne	1f
+	beq	1f
 
+ 	mcount_get_lr	r1			@ lr of instrumented func
+	mcount_adjust_addr	r0, lr		@ instrumented function
+	blx	r2
+1:
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	ldr     r1, =ftrace_graph_return
 	ldr     r2, [r1]

@@ -84,12 +88,6 @@
 #endif
 
 	mcount_exit
-
-1: 	mcount_get_lr	r1			@ lr of instrumented func
-	mcount_adjust_addr	r0, lr		@ instrumented function
-	badr	lr, 2f
-	mov	pc, r2
-2:	mcount_exit
 .endm
 
 #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
Signature · 1 line
2.14.1
maintainer
#4 lore raw

PATCH 1/2 arm64: Fix static use of function graph

Quoted 33 lines · AKASHI Takahiro
Quoted 4 lines · Julien Thierry

Function graph does not work currently when CONFIG_DYNAMIC_TRACE is not set. This is because ftrace_function_trace is not always set to ftrace_stub when function_graph is in use.

Quoted 8 lines

Do not skip checking of graph tracer functions when ftrace_function_trace is set.

Signed-off-by Julien Thierry <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc AKASHI Takahiro <[email protected]>
Reviewed-by AKASHI Takahiro <[email protected]>

I think that arm has the same issue, hoping the following patch will fix it:
===

Quoted 1 line

From e25bcf50d1acde698285a0c64f72d97f8b17e3fb Mon Sep 17 00:00:00 2001

From: AKASHI Takahiro <[email protected]>
Date: Thu, 2 Nov 2017 11:35:04 +0900
Subject: [PATCH] arm: ftrace: function_graph with DYNAMIC_FTRACE

---
 arch/arm/kernel/entry-ftrace.S | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
arch/arm/kernel/entry-ftrace.S +0 -0
index efcd9f25a14b..ef94c73ad996 100644

arch/arm/kernel/entry-ftrace.S line 75

Quoted 9 lines
arch/arm/kernel/entry-ftrace.S +4 -1
@@ -68,8 +68,12 @@
 	ldr	r2, [r0]
 	adr	r0, .Lftrace_stub
 	cmp	r0, r2
-	bne	1f
+	beq	1f
 
+ 	mcount_get_lr	r1			@ lr of instrumented func
+	mcount_adjust_addr	r0, lr		@ instrumented function
+	blx	r2

NAK. Not all CPUs support "blx". I don't see why you'd make this gratuitous change when just moving code around. Please separate your changes in future.

Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
author
#5 lore raw

PATCH 1/2 arm64: Fix static use of function graph

Hi,

Quoted 15 lines · Russell King - ARM Linux
Quoted 14 lines · AKASHI Takahiro
Quoted 12 lines · Julien Thierry

Function graph does not work currently when CONFIG_DYNAMIC_TRACE is not set. This is because ftrace_function_trace is not always set to ftrace_stub when function_graph is in use.

Do not skip checking of graph tracer functions when ftrace_function_trace is set.

Signed-off-by Julien Thierry <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc AKASHI Takahiro <[email protected]>
Quoted 4 lines
Quoted 4 lines
Reviewed-by AKASHI Takahiro <[email protected]>

I think that arm has the same issue, hoping the following patch will fix it:

Thanks for noticing this and for the patch. I'll re-spin a version for this series and include your patch.

Quoted 21 lines
Quoted 21 lines

===

Quoted 1 line

From e25bcf50d1acde698285a0c64f72d97f8b17e3fb Mon Sep 17 00:00:00 2001

From: AKASHI Takahiro <[email protected]>
Date: Thu, 2 Nov 2017 11:35:04 +0900
Subject: [PATCH] arm: ftrace: function_graph with DYNAMIC_FTRACE

---
  arch/arm/kernel/entry-ftrace.S | 12 +++++-------
  1 file changed, 5 insertions(+), 7 deletions(-)
arch/arm/kernel/entry-ftrace.S +1 -1
index efcd9f25a14b..ef94c73ad996 100644
@@ -68,8 +68,12 @@
  	ldr	r2, [r0]
  	adr	r0, .Lftrace_stub
  	cmp	r0, r2
-	bne	1f
+	beq	1f
Quoted 8 lines

arch/arm/kernel/entry-ftrace.S line 75

Quoted 3 lines
arch/arm/kernel/entry-ftrace.S +3 -0
+ 	mcount_get_lr	r1			@ lr of instrumented func
+	mcount_adjust_addr	r0, lr		@ instrumented function
+	blx	r2

NAK. Not all CPUs support "blx". I don't see why you'd make this gratuitous change when just moving code around. Please separate your changes in future.

I'll change the blx back to the badr + mov instructions in the new version.

Thanks,

Signature · 1 line
Julien Thierry
#6 lore raw

PATCH 1/2 arm64: Fix static use of function graph

Quoted 18 lines · Julien Thierry

Hi,

Quoted 15 lines · Russell King - ARM Linux
Quoted 14 lines · AKASHI Takahiro
Quoted 12 lines · Julien Thierry

Function graph does not work currently when CONFIG_DYNAMIC_TRACE is not set. This is because ftrace_function_trace is not always set to ftrace_stub when function_graph is in use.

Do not skip checking of graph tracer functions when ftrace_function_trace is set.

Signed-off-by Julien Thierry <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc AKASHI Takahiro <[email protected]>
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
Reviewed-by AKASHI Takahiro <[email protected]>

I think that arm has the same issue, hoping the following patch will fix it:

Thanks for noticing this and for the patch. I'll re-spin a version for this series and include your patch.

Thanks. You'd better fix a bug in the subject, which should be "!DYNAMIC_FTRACE" or use the same title as yours.

-Takahiro AKASHI

Quoted 35 lines
Quoted 19 lines
Quoted 19 lines

===

Quoted 1 line

From e25bcf50d1acde698285a0c64f72d97f8b17e3fb Mon Sep 17 00:00:00 2001

From: AKASHI Takahiro <[email protected]>
Date: Thu, 2 Nov 2017 11:35:04 +0900
Subject: [PATCH] arm: ftrace: function_graph with DYNAMIC_FTRACE

---
 arch/arm/kernel/entry-ftrace.S | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
arch/arm/kernel/entry-ftrace.S +0 -1
index efcd9f25a14b..ef94c73ad996 100644
@@ -68,8 +68,12 @@
 	ldr	r2, [r0]
 	adr	r0, .Lftrace_stub
 	cmp	r0, r2
-	bne	1f
Quoted 9 lines

arch/arm/kernel/entry-ftrace.S line 75

Quoted 4 lines
arch/arm/kernel/entry-ftrace.S +4 -0
+	beq	1f
+ 	mcount_get_lr	r1			@ lr of instrumented func
+	mcount_adjust_addr	r0, lr		@ instrumented function
+	blx	r2

NAK. Not all CPUs support "blx". I don't see why you'd make this gratuitous change when just moving code around. Please separate your changes in future.

I'll change the blx back to the badr + mov instructions in the new version.

Thanks,

Signature · 1 line
Julien Thierry
author
#7 lore raw

PATCH 2/2 perf: Fix ftrace builtin when kernel doesn't have function_graph

In reply to Julien Thierry

When linux is built without support for function graph tracer, the ftrace builtin of perf will fail when trying to reset max_graph_depth because the file does not exist. This prevents the use of function tracer from perf.

Do not attempt to write this file when the tracer in use is not function_graph.

Signed-off-by Julien Thierry <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc linux-kernel at vger.kernel.org
---
 tools/perf/builtin-ftrace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
tools/perf/builtin-ftrace.c +2 -1
index 25a42ac..48120f2 100644
@@ -130,7 +130,8 @@ static int reset_tracing_files(struct perf_ftrace *ftrace __maybe_unused)
 	if (reset_tracing_cpu() < 0)
 		return -1;
 
-	if (write_tracing_file("max_graph_depth", "0") < 0)
+	if (!strcmp(ftrace->tracer, "function_graph") &&
+	    write_tracing_file("max_graph_depth", "0") < 0)
 		return -1;
 
 	reset_tracing_filters();

--
1.9.1

maintainer
#8 lore raw

Re: [PATCH 2/2] perf: Fix ftrace builtin when kernel doesn't have function_graph

Quoted 22 lines · On Wed, 1 Nov 2017 14:33:44 +0000 Julien Thierry <[email protected]>…

When linux is built without support for function graph tracer, the ftrace builtin of perf will fail when trying to reset max_graph_depth because the file does not exist. This prevents the use of function tracer from perf.

Do not attempt to write this file when the tracer in use is not function_graph.

Signed-off-by Julien Thierry <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc linux-kernel at vger.kernel.org
---
 tools/perf/builtin-ftrace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
tools/perf/builtin-ftrace.c +0 -0
index 25a42ac..48120f2 100644

tools/perf/builtin-ftrace.c static int reset_tracing_files(struct perf_ftrace *ftrace __maybe_unused) · line 134

Quoted 6 lines
tools/perf/builtin-ftrace.c +2 -1
@@ -130,7 +130,8 @@ static int reset_tracing_files(struct perf_ftrace *ftrace __maybe_unused)
 	if (reset_tracing_cpu() < 0)
 		return -1;
 
-	if (write_tracing_file("max_graph_depth", "0") < 0)
+	if (!strcmp(ftrace->tracer, "function_graph") &&
+	    write_tracing_file("max_graph_depth", "0") < 0)

Hmm, instead of doing this, could we just do a stat on the file first. As with trace-cmd, I like to reset files like this even when not enabling function_graph tracer.

-- Steve

Quoted 5 lines
tools/perf/builtin-ftrace.c +0 -1
 		return -1;
 
 	reset_tracing_filters();
--

1.9.1

author
#9 lore raw

Re: [PATCH 2/2] perf: Fix ftrace builtin when kernel doesn't have function_graph

Quoted 28 lines · Steven Rostedt
Quoted 25 lines · On Wed, 1 Nov 2017 14:33:44 +0000 Julien Thierry <[email protected]>…

When linux is built without support for function graph tracer, the ftrace builtin of perf will fail when trying to reset max_graph_depth because the file does not exist. This prevents the use of function tracer from perf.

Do not attempt to write this file when the tracer in use is not function_graph.

Signed-off-by Julien Thierry <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc linux-kernel at vger.kernel.org
---
  tools/perf/builtin-ftrace.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
tools/perf/builtin-ftrace.c +0 -0
index 25a42ac..48120f2 100644
@@ -130,7 +130,8 @@ static int reset_tracing_files(struct perf_ftrace *ftrace __maybe_unused)
  	if (reset_tracing_cpu() < 0)
  		return -1;
Quoted 8 lines

tools/perf/builtin-ftrace.c static int reset_tracing_files(struct perf_ftrace *ftrace __maybe_unused) · line 134

Quoted 3 lines
tools/perf/builtin-ftrace.c +2 -1
-	if (write_tracing_file("max_graph_depth", "0") < 0)
+	if (!strcmp(ftrace->tracer, "function_graph") &&
+	    write_tracing_file("max_graph_depth", "0") < 0)

Hmm, instead of doing this, could we just do a stat on the file first. As with trace-cmd, I like to reset files like this even when not enabling function_graph tracer.

Sounds reasonable.

I'll include that change in the next version of the patches.

Thanks,

Signature · 1 line
Julien Thierry