[PATCH v3 0/7] arm64: ftrace: fix incorrect output from stack tracer

[PATCH v3 0/7] arm64: ftrace: fix incorrect output from stack tracer

[AKASHI Takahiro]

This is the third patch series for fixing stack tracer on arm64.
The original issue was reported by Jungseok[1], and then I found more
issues[2].
(Steven, Jungseok, sorry for not replying to your comments directly.)

I address here all the issues and implement fixes described in [2] except
for interrupt-triggered problems, ie. II-3). Recent discussions[3] about
introducing a dedicated interrupt stack suggests that we may avoid walking
through from an interrupt stack to a process stack.
(So interrupt-stack patch is a prerequisite.)

Basically,
patch1 corresponds to the original issue.
patch2 is a proactive improvement of function_graph tracer. 
patch3 corresponds to II-4(functions under function_graph tracer).
patch4 corresponds to II-5(leaf function).
patch5, 6 and 7 correspond to II-1(slurping stack) and II-2(differences
between x86 and arm64).

Each fix can be applied independently, but if patch5, 6 and 7 are
acceptable, patch1 is not necessary because patch7 replaces a default
stack tracer.

I tested the code with v4.3-rc3 + Jungseok's patch v3[4].

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/354126.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/355920.html 
[3] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/368003.html
[4] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/371451.html

AKASHI Takahiro (7):
  arm64: ftrace: adjust callsite addresses examined by stack tracer
  arm64: ftrace: modify a stack frame in a safe way
  arm64: ftrace: fix a stack tracer's output under function graph
    tracer
  arm64: ftrace: allow for tracing leaf functions
  ftrace: allow arch-specific stack tracer
  arm64: insn: add instruction decoders for ldp/stp and add/sub
  arm64: ftrace: add arch-specific stack tracer

 arch/arm64/Makefile                 |    5 +
 arch/arm64/include/asm/ftrace.h     |    7 +-
 arch/arm64/include/asm/insn.h       |   18 +++
 arch/arm64/include/asm/stacktrace.h |    1 +
 arch/arm64/kernel/ftrace.c          |   77 +++++++++++--
 arch/arm64/kernel/insn.c            |  102 +++++++++++++++++
 arch/arm64/kernel/stacktrace.c      |  213 ++++++++++++++++++++++++++++++++++-
 include/linux/ftrace.h              |   17 +++
 kernel/trace/trace_stack.c          |   27 +++--
 9 files changed, 442 insertions(+), 25 deletions(-)

-- 
1.7.9.5

[PATCH v3 0/7] arm64: ftrace: fix incorrect output from stack tracer

[AKASHI Takahiro]

This is the third patch series for fixing stack tracer on arm64.
The original issue was reported by Jungseok[1], and then I found more
issues[2].
(Steven, Jungseok, sorry for not replying to your comments directly.)

I address here all the issues and implement fixes described in [2] except
for interrupt-triggered problems, ie. II-3). Recent discussions[3] about
introducing a dedicated interrupt stack suggests that we may avoid walking
through from an interrupt stack to a process stack.
(So interrupt-stack patch is a prerequisite.)

Basically,
patch1 corresponds to the original issue.
patch2 is a proactive improvement of function_graph tracer. 
patch3 corresponds to II-4(functions under function_graph tracer).
patch4 corresponds to II-5(leaf function).
patch5, 6 and 7 correspond to II-1(slurping stack) and II-2(differences
between x86 and arm64).

Each fix can be applied independently, but if patch5, 6 and 7 are
acceptable, patch1 is not necessary because patch7 replaces a default
stack tracer.

I tested the code with v4.3-rc3 + Jungseok's patch v3[4].

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/354126.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/355920.html 
[3] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/368003.html
[4] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/371451.html

AKASHI Takahiro (7):
  arm64: ftrace: adjust callsite addresses examined by stack tracer
  arm64: ftrace: modify a stack frame in a safe way
  arm64: ftrace: fix a stack tracer's output under function graph
    tracer
  arm64: ftrace: allow for tracing leaf functions
  ftrace: allow arch-specific stack tracer
  arm64: insn: add instruction decoders for ldp/stp and add/sub
  arm64: ftrace: add arch-specific stack tracer

 arch/arm64/Makefile                 |    5 +
 arch/arm64/include/asm/ftrace.h     |    7 +-
 arch/arm64/include/asm/insn.h       |   18 +++
 arch/arm64/include/asm/stacktrace.h |    1 +
 arch/arm64/kernel/ftrace.c          |   77 +++++++++++--
 arch/arm64/kernel/insn.c            |  102 +++++++++++++++++
 arch/arm64/kernel/stacktrace.c      |  213 ++++++++++++++++++++++++++++++++++-
 include/linux/ftrace.h              |   17 +++
 kernel/trace/trace_stack.c          |   27 +++--
 9 files changed, 442 insertions(+), 25 deletions(-)

-- 
1.7.9.5

[PATCH v3 1/7] arm64: ftrace: adjust callsite addresses examined by stack tracer

[AKASHI Takahiro]

On arm64, no PC values returned by save_stack_trace() will match to LR
values saved in stack frames on a stack after the following commit:
    commit e306dfd06fcb ("ARM64: unwind: Fix PC calculation")
As a result, the output from stack tracer will be messed up.

This patch introduces an arch-defined macro, FTRACE_STACK_FRAME_OFFSET,
so that check_stack() can handle this case correctly.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 arch/arm64/include/asm/ftrace.h |    5 +++--
 arch/arm64/kernel/stacktrace.c  |    7 ++++---
 include/linux/ftrace.h          |    7 +++++++
 kernel/trace/trace_stack.c      |    5 +++--
 4 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index c5534fa..2b43e20 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -13,8 +13,9 @@
 
 #include <asm/insn.h>
 
-#define MCOUNT_ADDR		((unsigned long)_mcount)
-#define MCOUNT_INSN_SIZE	AARCH64_INSN_SIZE
+#define MCOUNT_ADDR			((unsigned long)_mcount)
+#define MCOUNT_INSN_SIZE		AARCH64_INSN_SIZE
+#define FTRACE_STACK_FRAME_OFFSET	AARCH64_INSN_SIZE
 
 #ifndef __ASSEMBLY__
 #include <linux/compat.h>
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 407991b..bc0689a 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -20,6 +20,7 @@
 #include <linux/sched.h>
 #include <linux/stacktrace.h>
 
+#include <asm/insn.h>
 #include <asm/stacktrace.h>
 
 /*
@@ -49,10 +50,10 @@ int notrace unwind_frame(struct stackframe *frame)
 	frame->sp = fp + 0x10;
 	frame->fp = *(unsigned long *)(fp);
 	/*
-	 * -4 here because we care about the PC at time of bl,
-	 * not where the return will go.
+	 * decrement PC by AARCH64_INSN_SIZE here because we care about
+	 * the PC at time of bl, not where the return will go.
 	 */
-	frame->pc = *(unsigned long *)(fp + 8) - 4;
+	frame->pc = *(unsigned long *)(fp + 8) - AARCH64_INSN_SIZE;
 
 	return 0;
 }
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 6cd8c0e..d77b195 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -263,6 +263,13 @@ static inline void ftrace_kill(void) { }
 #endif /* CONFIG_FUNCTION_TRACER */
 
 #ifdef CONFIG_STACK_TRACER
+/*
+ * the offset value to add to return address from save_stack_trace()
+ */
+#ifndef FTRACE_STACK_FRAME_OFFSET
+#define FTRACE_STACK_FRAME_OFFSET 0
+#endif
+
 extern int stack_tracer_enabled;
 int
 stack_trace_sysctl(struct ctl_table *table, int write,
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index b746399..30521ea 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -105,7 +105,7 @@ check_stack(unsigned long ip, unsigned long *stack)
 
 	/* Skip over the overhead of the stack tracer itself */
 	for (i = 0; i < max_stack_trace.nr_entries; i++) {
-		if (stack_dump_trace[i] == ip)
+		if ((stack_dump_trace[i] + FTRACE_STACK_FRAME_OFFSET) == ip)
 			break;
 	}
 
@@ -133,7 +133,8 @@ check_stack(unsigned long ip, unsigned long *stack)
 		for (; p < top && i < max_stack_trace.nr_entries; p++) {
 			if (stack_dump_trace[i] == ULONG_MAX)
 				break;
-			if (*p == stack_dump_trace[i]) {
+			if (*p == (stack_dump_trace[i]
+					+ FTRACE_STACK_FRAME_OFFSET)) {
 				stack_dump_trace[x] = stack_dump_trace[i++];
 				this_size = stack_dump_index[x++] =
 					(top - p) * sizeof(unsigned long);
-- 
1.7.9.5

[PATCH v3 1/7] arm64: ftrace: adjust callsite addresses examined by stack tracer

[AKASHI Takahiro]

On arm64, no PC values returned by save_stack_trace() will match to LR
values saved in stack frames on a stack after the following commit:
    commit e306dfd06fcb ("ARM64: unwind: Fix PC calculation")
As a result, the output from stack tracer will be messed up.

This patch introduces an arch-defined macro, FTRACE_STACK_FRAME_OFFSET,
so that check_stack() can handle this case correctly.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 arch/arm64/include/asm/ftrace.h |    5 +++--
 arch/arm64/kernel/stacktrace.c  |    7 ++++---
 include/linux/ftrace.h          |    7 +++++++
 kernel/trace/trace_stack.c      |    5 +++--
 4 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index c5534fa..2b43e20 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -13,8 +13,9 @@
 
 #include <asm/insn.h>
 
-#define MCOUNT_ADDR		((unsigned long)_mcount)
-#define MCOUNT_INSN_SIZE	AARCH64_INSN_SIZE
+#define MCOUNT_ADDR			((unsigned long)_mcount)
+#define MCOUNT_INSN_SIZE		AARCH64_INSN_SIZE
+#define FTRACE_STACK_FRAME_OFFSET	AARCH64_INSN_SIZE
 
 #ifndef __ASSEMBLY__
 #include <linux/compat.h>
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 407991b..bc0689a 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -20,6 +20,7 @@
 #include <linux/sched.h>
 #include <linux/stacktrace.h>
 
+#include <asm/insn.h>
 #include <asm/stacktrace.h>
 
 /*
@@ -49,10 +50,10 @@ int notrace unwind_frame(struct stackframe *frame)
 	frame->sp = fp + 0x10;
 	frame->fp = *(unsigned long *)(fp);
 	/*
-	 * -4 here because we care about the PC at time of bl,
-	 * not where the return will go.
+	 * decrement PC by AARCH64_INSN_SIZE here because we care about
+	 * the PC at time of bl, not where the return will go.
 	 */
-	frame->pc = *(unsigned long *)(fp + 8) - 4;
+	frame->pc = *(unsigned long *)(fp + 8) - AARCH64_INSN_SIZE;
 
 	return 0;
 }
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 6cd8c0e..d77b195 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -263,6 +263,13 @@ static inline void ftrace_kill(void) { }
 #endif /* CONFIG_FUNCTION_TRACER */
 
 #ifdef CONFIG_STACK_TRACER
+/*
+ * the offset value to add to return address from save_stack_trace()
+ */
+#ifndef FTRACE_STACK_FRAME_OFFSET
+#define FTRACE_STACK_FRAME_OFFSET 0
+#endif
+
 extern int stack_tracer_enabled;
 int
 stack_trace_sysctl(struct ctl_table *table, int write,
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index b746399..30521ea 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -105,7 +105,7 @@ check_stack(unsigned long ip, unsigned long *stack)
 
 	/* Skip over the overhead of the stack tracer itself */
 	for (i = 0; i < max_stack_trace.nr_entries; i++) {
-		if (stack_dump_trace[i] == ip)
+		if ((stack_dump_trace[i] + FTRACE_STACK_FRAME_OFFSET) == ip)
 			break;
 	}
 
@@ -133,7 +133,8 @@ check_stack(unsigned long ip, unsigned long *stack)
 		for (; p < top && i < max_stack_trace.nr_entries; p++) {
 			if (stack_dump_trace[i] == ULONG_MAX)
 				break;
-			if (*p == stack_dump_trace[i]) {
+			if (*p == (stack_dump_trace[i]
+					+ FTRACE_STACK_FRAME_OFFSET)) {
 				stack_dump_trace[x] = stack_dump_trace[i++];
 				this_size = stack_dump_index[x++] =
 					(top - p) * sizeof(unsigned long);
-- 
1.7.9.5

Re: [PATCH v3 1/7] arm64: ftrace: adjust callsite addresses examined by stack tracer

[Jungseok Lee]

On Oct 8, 2015, at 7:01 PM, AKASHI Takahiro wrote:

Hi Akashi,

> On arm64, no PC values returned by save_stack_trace() will match to LR
> values saved in stack frames on a stack after the following commit:
>    commit e306dfd06fcb ("ARM64: unwind: Fix PC calculation")
> As a result, the output from stack tracer will be messed up.
> 
> This patch introduces an arch-defined macro, FTRACE_STACK_FRAME_OFFSET,
> so that check_stack() can handle this case correctly.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> arch/arm64/include/asm/ftrace.h |    5 +++--
> arch/arm64/kernel/stacktrace.c  |    7 ++++---
> include/linux/ftrace.h          |    7 +++++++
> kernel/trace/trace_stack.c      |    5 +++--
> 4 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index c5534fa..2b43e20 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -13,8 +13,9 @@
> 
> #include <asm/insn.h>
> 
> -#define MCOUNT_ADDR		((unsigned long)_mcount)
> -#define MCOUNT_INSN_SIZE	AARCH64_INSN_SIZE
> +#define MCOUNT_ADDR			((unsigned long)_mcount)
> +#define MCOUNT_INSN_SIZE		AARCH64_INSN_SIZE
> +#define FTRACE_STACK_FRAME_OFFSET	AARCH64_INSN_SIZE
> 
> #ifndef __ASSEMBLY__
> #include <linux/compat.h>
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 407991b..bc0689a 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -20,6 +20,7 @@
> #include <linux/sched.h>
> #include <linux/stacktrace.h>
> 
> +#include <asm/insn.h>
> #include <asm/stacktrace.h>
> 
> /*
> @@ -49,10 +50,10 @@ int notrace unwind_frame(struct stackframe *frame)
> 	frame->sp = fp + 0x10;
> 	frame->fp = *(unsigned long *)(fp);
> 	/*
> -	 * -4 here because we care about the PC at time of bl,
> -	 * not where the return will go.
> +	 * decrement PC by AARCH64_INSN_SIZE here because we care about
> +	 * the PC at time of bl, not where the return will go.
> 	 */
> -	frame->pc = *(unsigned long *)(fp + 8) - 4;
> +	frame->pc = *(unsigned long *)(fp + 8) - AARCH64_INSN_SIZE;
> 
> 	return 0;
> }
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 6cd8c0e..d77b195 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -263,6 +263,13 @@ static inline void ftrace_kill(void) { }
> #endif /* CONFIG_FUNCTION_TRACER */
> 
> #ifdef CONFIG_STACK_TRACER
> +/*
> + * the offset value to add to return address from save_stack_trace()
> + */
> +#ifndef FTRACE_STACK_FRAME_OFFSET
> +#define FTRACE_STACK_FRAME_OFFSET 0
> +#endif
> +
> extern int stack_tracer_enabled;
> int
> stack_trace_sysctl(struct ctl_table *table, int write,
> diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
> index b746399..30521ea 100644
> --- a/kernel/trace/trace_stack.c
> +++ b/kernel/trace/trace_stack.c
> @@ -105,7 +105,7 @@ check_stack(unsigned long ip, unsigned long *stack)
> 
> 	/* Skip over the overhead of the stack tracer itself */
> 	for (i = 0; i < max_stack_trace.nr_entries; i++) {
> -		if (stack_dump_trace[i] == ip)
> +		if ((stack_dump_trace[i] + FTRACE_STACK_FRAME_OFFSET) == ip)
> 			break;
> 	}
> 
> @@ -133,7 +133,8 @@ check_stack(unsigned long ip, unsigned long *stack)
> 		for (; p < top && i < max_stack_trace.nr_entries; p++) {
> 			if (stack_dump_trace[i] == ULONG_MAX)
> 				break;
> -			if (*p == stack_dump_trace[i]) {
> +			if (*p == (stack_dump_trace[i]
> +					+ FTRACE_STACK_FRAME_OFFSET)) {
> 				stack_dump_trace[x] = stack_dump_trace[i++];
> 				this_size = stack_dump_index[x++] =
> 					(top - p) * sizeof(unsigned long);
> -- 

This change is always on my tree for IRQ stack feature development. It makes
stack_trace prints out useful info although it can't give an accurate data.
I believe this hunk helps to figure out max stack depth context.

Acked-by: Jungseok Lee <jungseoklee85@gmail.com>

Thanks!

Best Regards
Jungseok Lee

[PATCH v3 1/7] arm64: ftrace: adjust callsite addresses examined by stack tracer

[Jungseok Lee]

On Oct 8, 2015, at 7:01 PM, AKASHI Takahiro wrote:

Hi Akashi,

> On arm64, no PC values returned by save_stack_trace() will match to LR
> values saved in stack frames on a stack after the following commit:
>    commit e306dfd06fcb ("ARM64: unwind: Fix PC calculation")
> As a result, the output from stack tracer will be messed up.
> 
> This patch introduces an arch-defined macro, FTRACE_STACK_FRAME_OFFSET,
> so that check_stack() can handle this case correctly.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> arch/arm64/include/asm/ftrace.h |    5 +++--
> arch/arm64/kernel/stacktrace.c  |    7 ++++---
> include/linux/ftrace.h          |    7 +++++++
> kernel/trace/trace_stack.c      |    5 +++--
> 4 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index c5534fa..2b43e20 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -13,8 +13,9 @@
> 
> #include <asm/insn.h>
> 
> -#define MCOUNT_ADDR		((unsigned long)_mcount)
> -#define MCOUNT_INSN_SIZE	AARCH64_INSN_SIZE
> +#define MCOUNT_ADDR			((unsigned long)_mcount)
> +#define MCOUNT_INSN_SIZE		AARCH64_INSN_SIZE
> +#define FTRACE_STACK_FRAME_OFFSET	AARCH64_INSN_SIZE
> 
> #ifndef __ASSEMBLY__
> #include <linux/compat.h>
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 407991b..bc0689a 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -20,6 +20,7 @@
> #include <linux/sched.h>
> #include <linux/stacktrace.h>
> 
> +#include <asm/insn.h>
> #include <asm/stacktrace.h>
> 
> /*
> @@ -49,10 +50,10 @@ int notrace unwind_frame(struct stackframe *frame)
> 	frame->sp = fp + 0x10;
> 	frame->fp = *(unsigned long *)(fp);
> 	/*
> -	 * -4 here because we care about the PC at time of bl,
> -	 * not where the return will go.
> +	 * decrement PC by AARCH64_INSN_SIZE here because we care about
> +	 * the PC at time of bl, not where the return will go.
> 	 */
> -	frame->pc = *(unsigned long *)(fp + 8) - 4;
> +	frame->pc = *(unsigned long *)(fp + 8) - AARCH64_INSN_SIZE;
> 
> 	return 0;
> }
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 6cd8c0e..d77b195 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -263,6 +263,13 @@ static inline void ftrace_kill(void) { }
> #endif /* CONFIG_FUNCTION_TRACER */
> 
> #ifdef CONFIG_STACK_TRACER
> +/*
> + * the offset value to add to return address from save_stack_trace()
> + */
> +#ifndef FTRACE_STACK_FRAME_OFFSET
> +#define FTRACE_STACK_FRAME_OFFSET 0
> +#endif
> +
> extern int stack_tracer_enabled;
> int
> stack_trace_sysctl(struct ctl_table *table, int write,
> diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
> index b746399..30521ea 100644
> --- a/kernel/trace/trace_stack.c
> +++ b/kernel/trace/trace_stack.c
> @@ -105,7 +105,7 @@ check_stack(unsigned long ip, unsigned long *stack)
> 
> 	/* Skip over the overhead of the stack tracer itself */
> 	for (i = 0; i < max_stack_trace.nr_entries; i++) {
> -		if (stack_dump_trace[i] == ip)
> +		if ((stack_dump_trace[i] + FTRACE_STACK_FRAME_OFFSET) == ip)
> 			break;
> 	}
> 
> @@ -133,7 +133,8 @@ check_stack(unsigned long ip, unsigned long *stack)
> 		for (; p < top && i < max_stack_trace.nr_entries; p++) {
> 			if (stack_dump_trace[i] == ULONG_MAX)
> 				break;
> -			if (*p == stack_dump_trace[i]) {
> +			if (*p == (stack_dump_trace[i]
> +					+ FTRACE_STACK_FRAME_OFFSET)) {
> 				stack_dump_trace[x] = stack_dump_trace[i++];
> 				this_size = stack_dump_index[x++] =
> 					(top - p) * sizeof(unsigned long);
> -- 

This change is always on my tree for IRQ stack feature development. It makes
stack_trace prints out useful info although it can't give an accurate data.
I believe this hunk helps to figure out max stack depth context.

Acked-by: Jungseok Lee <jungseoklee85@gmail.com>

Thanks!

Best Regards
Jungseok Lee

Re: [PATCH v3 1/7] arm64: ftrace: adjust callsite addresses examined by stack tracer

[Steven Rostedt]

On Thu,  8 Oct 2015 19:01:38 +0900
AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:

>  extern int stack_tracer_enabled;
>  int
>  stack_trace_sysctl(struct ctl_table *table, int write,
> diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
> index b746399..30521ea 100644
> --- a/kernel/trace/trace_stack.c
> +++ b/kernel/trace/trace_stack.c
> @@ -105,7 +105,7 @@ check_stack(unsigned long ip, unsigned long *stack)
>  
>  	/* Skip over the overhead of the stack tracer itself */
>  	for (i = 0; i < max_stack_trace.nr_entries; i++) {
> -		if (stack_dump_trace[i] == ip)
> +		if ((stack_dump_trace[i] + FTRACE_STACK_FRAME_OFFSET) == ip)
>  			break;
>  	}
>  
> @@ -133,7 +133,8 @@ check_stack(unsigned long ip, unsigned long *stack)
>  		for (; p < top && i < max_stack_trace.nr_entries; p++) {
>  			if (stack_dump_trace[i] == ULONG_MAX)
>  				break;
> -			if (*p == stack_dump_trace[i]) {
> +			if (*p == (stack_dump_trace[i]
> +					+ FTRACE_STACK_FRAME_OFFSET)) {

I'm fine with the patch, but this is one of those cases that I think
the 80 column max limit produces uglier code than just going a little
over.

Or, we can add a helper variable in both locations:

	addr = stack_dump_trace[i] + FTRACE_STACK_FRAME_OFFSET;
	if (*p == addr) {

gcc should be smart enough to optimize out the addr variable.

-- Steve

>  				stack_dump_trace[x] = stack_dump_trace[i++];
>  				this_size = stack_dump_index[x++] =
>  					(top - p) * sizeof(unsigned long);

[PATCH v3 1/7] arm64: ftrace: adjust callsite addresses examined by stack tracer

[Steven Rostedt]

On Thu,  8 Oct 2015 19:01:38 +0900
AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:

>  extern int stack_tracer_enabled;
>  int
>  stack_trace_sysctl(struct ctl_table *table, int write,
> diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
> index b746399..30521ea 100644
> --- a/kernel/trace/trace_stack.c
> +++ b/kernel/trace/trace_stack.c
> @@ -105,7 +105,7 @@ check_stack(unsigned long ip, unsigned long *stack)
>  
>  	/* Skip over the overhead of the stack tracer itself */
>  	for (i = 0; i < max_stack_trace.nr_entries; i++) {
> -		if (stack_dump_trace[i] == ip)
> +		if ((stack_dump_trace[i] + FTRACE_STACK_FRAME_OFFSET) == ip)
>  			break;
>  	}
>  
> @@ -133,7 +133,8 @@ check_stack(unsigned long ip, unsigned long *stack)
>  		for (; p < top && i < max_stack_trace.nr_entries; p++) {
>  			if (stack_dump_trace[i] == ULONG_MAX)
>  				break;
> -			if (*p == stack_dump_trace[i]) {
> +			if (*p == (stack_dump_trace[i]
> +					+ FTRACE_STACK_FRAME_OFFSET)) {

I'm fine with the patch, but this is one of those cases that I think
the 80 column max limit produces uglier code than just going a little
over.

Or, we can add a helper variable in both locations:

	addr = stack_dump_trace[i] + FTRACE_STACK_FRAME_OFFSET;
	if (*p == addr) {

gcc should be smart enough to optimize out the addr variable.

-- Steve

>  				stack_dump_trace[x] = stack_dump_trace[i++];
>  				this_size = stack_dump_index[x++] =
>  					(top - p) * sizeof(unsigned long);

Re: [PATCH v3 1/7] arm64: ftrace: adjust callsite addresses examined by stack tracer

[AKASHI Takahiro]

On 10/14/2015 12:37 AM, Steven Rostedt wrote:
> On Thu,  8 Oct 2015 19:01:38 +0900
> AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
>>   extern int stack_tracer_enabled;
>>   int
>>   stack_trace_sysctl(struct ctl_table *table, int write,
>> diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
>> index b746399..30521ea 100644
>> --- a/kernel/trace/trace_stack.c
>> +++ b/kernel/trace/trace_stack.c
>> @@ -105,7 +105,7 @@ check_stack(unsigned long ip, unsigned long *stack)
>>
>>   	/* Skip over the overhead of the stack tracer itself */
>>   	for (i = 0; i < max_stack_trace.nr_entries; i++) {
>> -		if (stack_dump_trace[i] == ip)
>> +		if ((stack_dump_trace[i] + FTRACE_STACK_FRAME_OFFSET) == ip)
>>   			break;
>>   	}
>>
>> @@ -133,7 +133,8 @@ check_stack(unsigned long ip, unsigned long *stack)
>>   		for (; p < top && i < max_stack_trace.nr_entries; p++) {
>>   			if (stack_dump_trace[i] == ULONG_MAX)
>>   				break;
>> -			if (*p == stack_dump_trace[i]) {
>> +			if (*p == (stack_dump_trace[i]
>> +					+ FTRACE_STACK_FRAME_OFFSET)) {
>
> I'm fine with the patch, but this is one of those cases that I think
> the 80 column max limit produces uglier code than just going a little
> over.
>
> Or, we can add a helper variable in both locations:
>
> 	addr = stack_dump_trace[i] + FTRACE_STACK_FRAME_OFFSET;
> 	if (*p == addr) {
>
> gcc should be smart enough to optimize out the addr variable.

Thanks, I will take this.
(please note that this patch won't be necessary if my patch5,6 and 7 are
accepted.)

-Takahiro AKASHI

> -- Steve
>
>>   				stack_dump_trace[x] = stack_dump_trace[i++];
>>   				this_size = stack_dump_index[x++] =
>>   					(top - p) * sizeof(unsigned long);
>

[PATCH v3 1/7] arm64: ftrace: adjust callsite addresses examined by stack tracer

[AKASHI Takahiro]

On 10/14/2015 12:37 AM, Steven Rostedt wrote:
> On Thu,  8 Oct 2015 19:01:38 +0900
> AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
>>   extern int stack_tracer_enabled;
>>   int
>>   stack_trace_sysctl(struct ctl_table *table, int write,
>> diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
>> index b746399..30521ea 100644
>> --- a/kernel/trace/trace_stack.c
>> +++ b/kernel/trace/trace_stack.c
>> @@ -105,7 +105,7 @@ check_stack(unsigned long ip, unsigned long *stack)
>>
>>   	/* Skip over the overhead of the stack tracer itself */
>>   	for (i = 0; i < max_stack_trace.nr_entries; i++) {
>> -		if (stack_dump_trace[i] == ip)
>> +		if ((stack_dump_trace[i] + FTRACE_STACK_FRAME_OFFSET) == ip)
>>   			break;
>>   	}
>>
>> @@ -133,7 +133,8 @@ check_stack(unsigned long ip, unsigned long *stack)
>>   		for (; p < top && i < max_stack_trace.nr_entries; p++) {
>>   			if (stack_dump_trace[i] == ULONG_MAX)
>>   				break;
>> -			if (*p == stack_dump_trace[i]) {
>> +			if (*p == (stack_dump_trace[i]
>> +					+ FTRACE_STACK_FRAME_OFFSET)) {
>
> I'm fine with the patch, but this is one of those cases that I think
> the 80 column max limit produces uglier code than just going a little
> over.
>
> Or, we can add a helper variable in both locations:
>
> 	addr = stack_dump_trace[i] + FTRACE_STACK_FRAME_OFFSET;
> 	if (*p == addr) {
>
> gcc should be smart enough to optimize out the addr variable.

Thanks, I will take this.
(please note that this patch won't be necessary if my patch5,6 and 7 are
accepted.)

-Takahiro AKASHI

> -- Steve
>
>>   				stack_dump_trace[x] = stack_dump_trace[i++];
>>   				this_size = stack_dump_index[x++] =
>>   					(top - p) * sizeof(unsigned long);
>

[PATCH v3 2/7] arm64: ftrace: modify a stack frame in a safe way

[AKASHI Takahiro]

Function graph tracer modifies a return address (LR) in a stack frame by
calling ftrace_prepare_return() in a traced function's function prologue.
The current code does this modification before preserving an original
address at ftrace_push_return_trace() and there is always a small window
of inconsistency when an interrupt occurs.

This doesn't matter, as far as an interrupt stack is introduced, because
stack tracer won't be invoked in an interrupt context. But it would be
better to proactively minimize such a window by moving the LR modification
after ftrace_push_return_trace().

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 arch/arm64/kernel/ftrace.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index c851be7..314f82d 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -125,23 +125,20 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 	 * on other archs. It's unlikely on AArch64.
 	 */
 	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)) {
-		*parent = old;
+	if (!ftrace_graph_entry(&trace))
 		return;
-	}
 
 	err = ftrace_push_return_trace(old, self_addr, &trace.depth,
 				       frame_pointer);
-	if (err == -EBUSY) {
-		*parent = old;
+	if (err == -EBUSY)
 		return;
-	}
+	else
+		*parent = return_hooker;
 }
 
 #ifdef CONFIG_DYNAMIC_FTRACE
-- 
1.7.9.5

[PATCH v3 2/7] arm64: ftrace: modify a stack frame in a safe way

[AKASHI Takahiro]

Function graph tracer modifies a return address (LR) in a stack frame by
calling ftrace_prepare_return() in a traced function's function prologue.
The current code does this modification before preserving an original
address at ftrace_push_return_trace() and there is always a small window
of inconsistency when an interrupt occurs.

This doesn't matter, as far as an interrupt stack is introduced, because
stack tracer won't be invoked in an interrupt context. But it would be
better to proactively minimize such a window by moving the LR modification
after ftrace_push_return_trace().

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 arch/arm64/kernel/ftrace.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index c851be7..314f82d 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -125,23 +125,20 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 	 * on other archs. It's unlikely on AArch64.
 	 */
 	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)) {
-		*parent = old;
+	if (!ftrace_graph_entry(&trace))
 		return;
-	}
 
 	err = ftrace_push_return_trace(old, self_addr, &trace.depth,
 				       frame_pointer);
-	if (err == -EBUSY) {
-		*parent = old;
+	if (err == -EBUSY)
 		return;
-	}
+	else
+		*parent = return_hooker;
 }
 
 #ifdef CONFIG_DYNAMIC_FTRACE
-- 
1.7.9.5

[PATCH v3 3/7] arm64: ftrace: fix a stack tracer's output under function graph tracer

[AKASHI Takahiro]

Function graph tracer modifies a return address (LR) in a stack frame
to hook a function return. This will result in many useless entries
(return_to_handler) showing up in a stack tracer's output.

This patch replaces such entries with originals values preserved in
current->ret_stack[].

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 arch/arm64/include/asm/ftrace.h |    2 ++
 arch/arm64/kernel/stacktrace.c  |   20 ++++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index 2b43e20..b7d597c 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -29,6 +29,8 @@ struct dyn_arch_ftrace {
 
 extern unsigned long ftrace_graph_call;
 
+extern void return_to_handler(void);
+
 static inline unsigned long ftrace_call_adjust(unsigned long addr)
 {
 	/*
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index bc0689a..1555a8b 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -78,6 +78,9 @@ struct stack_trace_data {
 	struct stack_trace *trace;
 	unsigned int no_sched_functions;
 	unsigned int skip;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	unsigned int ret_stack_index;
+#endif
 };
 
 static int save_trace(struct stackframe *frame, void *d)
@@ -86,6 +89,20 @@ static int save_trace(struct stackframe *frame, void *d)
 	struct stack_trace *trace = data->trace;
 	unsigned long addr = frame->pc;
 
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	if (addr == (unsigned long)return_to_handler - AARCH64_INSN_SIZE) {
+		/*
+		 * This is a case where function graph tracer has
+		 * modified a return address (LR) in a stack frame
+		 * to hook a function return.
+		 * So replace it to an original value.
+		 */
+		frame->pc = addr =
+			current->ret_stack[data->ret_stack_index--].ret
+							- AARCH64_INSN_SIZE;
+	}
+#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
+
 	if (data->no_sched_functions && in_sched_functions(addr))
 		return 0;
 	if (data->skip) {
@@ -105,6 +122,9 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 
 	data.trace = trace;
 	data.skip = trace->skip;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	data.ret_stack_index = current->curr_ret_stack;
+#endif
 
 	if (tsk != current) {
 		data.no_sched_functions = 1;
-- 
1.7.9.5

[PATCH v3 3/7] arm64: ftrace: fix a stack tracer's output under function graph tracer

[AKASHI Takahiro]

Function graph tracer modifies a return address (LR) in a stack frame
to hook a function return. This will result in many useless entries
(return_to_handler) showing up in a stack tracer's output.

This patch replaces such entries with originals values preserved in
current->ret_stack[].

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 arch/arm64/include/asm/ftrace.h |    2 ++
 arch/arm64/kernel/stacktrace.c  |   20 ++++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index 2b43e20..b7d597c 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -29,6 +29,8 @@ struct dyn_arch_ftrace {
 
 extern unsigned long ftrace_graph_call;
 
+extern void return_to_handler(void);
+
 static inline unsigned long ftrace_call_adjust(unsigned long addr)
 {
 	/*
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index bc0689a..1555a8b 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -78,6 +78,9 @@ struct stack_trace_data {
 	struct stack_trace *trace;
 	unsigned int no_sched_functions;
 	unsigned int skip;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	unsigned int ret_stack_index;
+#endif
 };
 
 static int save_trace(struct stackframe *frame, void *d)
@@ -86,6 +89,20 @@ static int save_trace(struct stackframe *frame, void *d)
 	struct stack_trace *trace = data->trace;
 	unsigned long addr = frame->pc;
 
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	if (addr == (unsigned long)return_to_handler - AARCH64_INSN_SIZE) {
+		/*
+		 * This is a case where function graph tracer has
+		 * modified a return address (LR) in a stack frame
+		 * to hook a function return.
+		 * So replace it to an original value.
+		 */
+		frame->pc = addr =
+			current->ret_stack[data->ret_stack_index--].ret
+							- AARCH64_INSN_SIZE;
+	}
+#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
+
 	if (data->no_sched_functions && in_sched_functions(addr))
 		return 0;
 	if (data->skip) {
@@ -105,6 +122,9 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 
 	data.trace = trace;
 	data.skip = trace->skip;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	data.ret_stack_index = current->curr_ret_stack;
+#endif
 
 	if (tsk != current) {
 		data.no_sched_functions = 1;
-- 
1.7.9.5

Re: [PATCH v3 3/7] arm64: ftrace: fix a stack tracer's output under function graph tracer

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2264 bytes --]

Hi AKASHI,

[auto build test ERROR on v4.3-rc4 -- if it's inappropriate base, please ignore]

config: arm64-allmodconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

Note: the linux-review/AKASHI-Takahiro/arm64-ftrace-fix-incorrect-output-from-stack-tracer HEAD f6c03913c640aa1d196348c49f15cfdc264393e0 builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

   arch/arm64/kernel/stacktrace.c: In function 'save_trace':
>> arch/arm64/kernel/stacktrace.c:93:29: error: 'return_to_handler' undeclared (first use in this function)
     if (addr == (unsigned long)return_to_handler - AARCH64_INSN_SIZE) {
                                ^
   arch/arm64/kernel/stacktrace.c:93:29: note: each undeclared identifier is reported only once for each function it appears in
>> arch/arm64/kernel/stacktrace.c:101:4: error: invalid use of undefined type 'struct ftrace_ret_stack'
       current->ret_stack[data->ret_stack_index--].ret
       ^
>> arch/arm64/kernel/stacktrace.c:101:22: error: dereferencing pointer to incomplete type
       current->ret_stack[data->ret_stack_index--].ret
                         ^

vim +/return_to_handler +93 arch/arm64/kernel/stacktrace.c

    87	{
    88		struct stack_trace_data *data = d;
    89		struct stack_trace *trace = data->trace;
    90		unsigned long addr = frame->pc;
    91	
    92	#ifdef CONFIG_FUNCTION_GRAPH_TRACER
  > 93		if (addr == (unsigned long)return_to_handler - AARCH64_INSN_SIZE) {
    94			/*
    95			 * This is a case where function graph tracer has
    96			 * modified a return address (LR) in a stack frame
    97			 * to hook a function return.
    98			 * So replace it to an original value.
    99			 */
   100			frame->pc = addr =
 > 101				current->ret_stack[data->ret_stack_index--].ret
   102								- AARCH64_INSN_SIZE;
   103		}
   104	#endif /* CONFIG_FUNCTION_GRAPH_TRACER */

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 45525 bytes --]

[PATCH v3 3/7] arm64: ftrace: fix a stack tracer's output under function graph tracer

[kbuild test robot]

Hi AKASHI,

[auto build test ERROR on v4.3-rc4 -- if it's inappropriate base, please ignore]

config: arm64-allmodconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

Note: the linux-review/AKASHI-Takahiro/arm64-ftrace-fix-incorrect-output-from-stack-tracer HEAD f6c03913c640aa1d196348c49f15cfdc264393e0 builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

   arch/arm64/kernel/stacktrace.c: In function 'save_trace':
>> arch/arm64/kernel/stacktrace.c:93:29: error: 'return_to_handler' undeclared (first use in this function)
     if (addr == (unsigned long)return_to_handler - AARCH64_INSN_SIZE) {
                                ^
   arch/arm64/kernel/stacktrace.c:93:29: note: each undeclared identifier is reported only once for each function it appears in
>> arch/arm64/kernel/stacktrace.c:101:4: error: invalid use of undefined type 'struct ftrace_ret_stack'
       current->ret_stack[data->ret_stack_index--].ret
       ^
>> arch/arm64/kernel/stacktrace.c:101:22: error: dereferencing pointer to incomplete type
       current->ret_stack[data->ret_stack_index--].ret
                         ^

vim +/return_to_handler +93 arch/arm64/kernel/stacktrace.c

    87	{
    88		struct stack_trace_data *data = d;
    89		struct stack_trace *trace = data->trace;
    90		unsigned long addr = frame->pc;
    91	
    92	#ifdef CONFIG_FUNCTION_GRAPH_TRACER
  > 93		if (addr == (unsigned long)return_to_handler - AARCH64_INSN_SIZE) {
    94			/*
    95			 * This is a case where function graph tracer has
    96			 * modified a return address (LR) in a stack frame
    97			 * to hook a function return.
    98			 * So replace it to an original value.
    99			 */
   100			frame->pc = addr =
 > 101				current->ret_stack[data->ret_stack_index--].ret
   102								- AARCH64_INSN_SIZE;
   103		}
   104	#endif /* CONFIG_FUNCTION_GRAPH_TRACER */

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/octet-stream
Size: 45525 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151009/8c359503/attachment-0001.obj>

Re: [PATCH v3 3/7] arm64: ftrace: fix a stack tracer's output under function graph tracer

[Jungseok Lee]

On Oct 8, 2015, at 7:01 PM, AKASHI Takahiro wrote:

Hi Akashi,

> Function graph tracer modifies a return address (LR) in a stack frame
> to hook a function return. This will result in many useless entries
> (return_to_handler) showing up in a stack tracer's output.
> 
> This patch replaces such entries with originals values preserved in
> current->ret_stack[].
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

The following diff should be folded into this patch, not [7/7] one, to
address build breakage.

----8<----
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 407991b..7126d4d 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -17,6 +17,7 @@
  */
 #include <linux/kernel.h>
 #include <linux/export.h>
+#include <linux/ftrace.h>
 #include <linux/sched.h>
 #include <linux/stacktrace.h>
----8<----

Best Regards
Jungseok Lee

[PATCH v3 3/7] arm64: ftrace: fix a stack tracer's output under function graph tracer

[Jungseok Lee]

On Oct 8, 2015, at 7:01 PM, AKASHI Takahiro wrote:

Hi Akashi,

> Function graph tracer modifies a return address (LR) in a stack frame
> to hook a function return. This will result in many useless entries
> (return_to_handler) showing up in a stack tracer's output.
> 
> This patch replaces such entries with originals values preserved in
> current->ret_stack[].
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

The following diff should be folded into this patch, not [7/7] one, to
address build breakage.

----8<----
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 407991b..7126d4d 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -17,6 +17,7 @@
  */
 #include <linux/kernel.h>
 #include <linux/export.h>
+#include <linux/ftrace.h>
 #include <linux/sched.h>
 #include <linux/stacktrace.h>
----8<----

Best Regards
Jungseok Lee

Re: [PATCH v3 3/7] arm64: ftrace: fix a stack tracer's output under function graph tracer

[AKASHI Takahiro]

On 10/14/2015 12:24 AM, Jungseok Lee wrote:
> On Oct 8, 2015, at 7:01 PM, AKASHI Takahiro wrote:
>
> Hi Akashi,
>
>> Function graph tracer modifies a return address (LR) in a stack frame
>> to hook a function return. This will result in many useless entries
>> (return_to_handler) showing up in a stack tracer's output.
>>
>> This patch replaces such entries with originals values preserved in
>> current->ret_stack[].
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>
> The following diff should be folded into this patch, not [7/7] one, to
> address build breakage.

Thanks.
I fixed all the build errors reported by kbuild test robot.

-Takahiro AKASHI

> ----8<----
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 407991b..7126d4d 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -17,6 +17,7 @@
>    */
>   #include <linux/kernel.h>
>   #include <linux/export.h>
> +#include <linux/ftrace.h>
>   #include <linux/sched.h>
>   #include <linux/stacktrace.h>
> ----8<----
>
> Best Regards
> Jungseok Lee
>

[PATCH v3 3/7] arm64: ftrace: fix a stack tracer's output under function graph tracer

[AKASHI Takahiro]

On 10/14/2015 12:24 AM, Jungseok Lee wrote:
> On Oct 8, 2015, at 7:01 PM, AKASHI Takahiro wrote:
>
> Hi Akashi,
>
>> Function graph tracer modifies a return address (LR) in a stack frame
>> to hook a function return. This will result in many useless entries
>> (return_to_handler) showing up in a stack tracer's output.
>>
>> This patch replaces such entries with originals values preserved in
>> current->ret_stack[].
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>
> The following diff should be folded into this patch, not [7/7] one, to
> address build breakage.

Thanks.
I fixed all the build errors reported by kbuild test robot.

-Takahiro AKASHI

> ----8<----
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 407991b..7126d4d 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -17,6 +17,7 @@
>    */
>   #include <linux/kernel.h>
>   #include <linux/export.h>
> +#include <linux/ftrace.h>
>   #include <linux/sched.h>
>   #include <linux/stacktrace.h>
> ----8<----
>
> Best Regards
> Jungseok Lee
>

[PATCH v3 4/7] arm64: ftrace: allow for tracing leaf functions

[AKASHI Takahiro]

CONFIG_FRAME_POINTER is always enabled on arm64, adding
-fno-omit-frame-pointer, while ftrace, CONFIG_FUNCTION_TRACER, adds -pg
to KBUILD_CFLAGS.
The combination of these options, however, does not implicitly enables
-fno-omit-leaf-frame-pointer option, and no leaf functions are actually
traced under ftrace.

This patch allows for tracing leaf functions by adding
-fno-omit-leaf-frame-pointer option, but does it only when
CONFIG_STACK_TRACER is configured because it will increase the kernel
size and add extra instructions on runtime.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 arch/arm64/Makefile |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index f9914d7..71cad91 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -28,6 +28,11 @@ endif
 
 KBUILD_CFLAGS	+= -mgeneral-regs-only $(lseinstr)
 KBUILD_AFLAGS	+= $(lseinstr)
+ifdef CONFIG_STACK_TRACER
+ifdef CONFIG_FRAME_POINTER
+KBUILD_CFLAGS	+= -mno-omit-leaf-frame-pointer
+endif
+endif
 
 ifeq ($(CONFIG_CPU_BIG_ENDIAN), y)
 KBUILD_CPPFLAGS	+= -mbig-endian
-- 
1.7.9.5

[PATCH v3 4/7] arm64: ftrace: allow for tracing leaf functions

[AKASHI Takahiro]

CONFIG_FRAME_POINTER is always enabled on arm64, adding
-fno-omit-frame-pointer, while ftrace, CONFIG_FUNCTION_TRACER, adds -pg
to KBUILD_CFLAGS.
The combination of these options, however, does not implicitly enables
-fno-omit-leaf-frame-pointer option, and no leaf functions are actually
traced under ftrace.

This patch allows for tracing leaf functions by adding
-fno-omit-leaf-frame-pointer option, but does it only when
CONFIG_STACK_TRACER is configured because it will increase the kernel
size and add extra instructions on runtime.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 arch/arm64/Makefile |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index f9914d7..71cad91 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -28,6 +28,11 @@ endif
 
 KBUILD_CFLAGS	+= -mgeneral-regs-only $(lseinstr)
 KBUILD_AFLAGS	+= $(lseinstr)
+ifdef CONFIG_STACK_TRACER
+ifdef CONFIG_FRAME_POINTER
+KBUILD_CFLAGS	+= -mno-omit-leaf-frame-pointer
+endif
+endif
 
 ifeq ($(CONFIG_CPU_BIG_ENDIAN), y)
 KBUILD_CPPFLAGS	+= -mbig-endian
-- 
1.7.9.5

[PATCH v3 5/7] ftrace: allow arch-specific stack tracer

[AKASHI Takahiro]

A stack frame may be used in a different way depending on cpu architecture.
Thus it is not always appropriate to slurp the stack contents, as current
check_stack() does, in order to calcurate a stack index (height) at a given
function call. At least not on arm64.
In addition, there is a possibility that we will mistakenly detect a stale
stack frame which has not been overwritten.

This patch makes check_stack() a weak function so as to later implement
arch-specific version.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/linux/ftrace.h     |   10 ++++++++++
 kernel/trace/trace_stack.c |   22 ++++++++++++++--------
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index d77b195..e538400 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -270,7 +270,17 @@ static inline void ftrace_kill(void) { }
 #define FTRACE_STACK_FRAME_OFFSET 0
 #endif
 
+#define STACK_TRACE_ENTRIES 500
+
+struct stack_trace;
+
+extern unsigned stack_dump_index[];
+extern struct stack_trace max_stack_trace;
+extern unsigned long max_stack_size;
+extern arch_spinlock_t max_stack_lock;
+
 extern int stack_tracer_enabled;
+void print_max_stack(void);
 int
 stack_trace_sysctl(struct ctl_table *table, int write,
 		   void __user *buffer, size_t *lenp,
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 30521ea..ff1a191 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -16,24 +16,22 @@
 
 #include "trace.h"
 
-#define STACK_TRACE_ENTRIES 500
-
 static unsigned long stack_dump_trace[STACK_TRACE_ENTRIES+1] =
 	 { [0 ... (STACK_TRACE_ENTRIES)] = ULONG_MAX };
-static unsigned stack_dump_index[STACK_TRACE_ENTRIES];
+unsigned stack_dump_index[STACK_TRACE_ENTRIES];
 
 /*
  * Reserve one entry for the passed in ip. This will allow
  * us to remove most or all of the stack size overhead
  * added by the stack tracer itself.
  */
-static struct stack_trace max_stack_trace = {
+struct stack_trace max_stack_trace = {
 	.max_entries		= STACK_TRACE_ENTRIES - 1,
 	.entries		= &stack_dump_trace[0],
 };
 
-static unsigned long max_stack_size;
-static arch_spinlock_t max_stack_lock =
+unsigned long max_stack_size;
+arch_spinlock_t max_stack_lock =
 	(arch_spinlock_t)__ARCH_SPIN_LOCK_UNLOCKED;
 
 static DEFINE_PER_CPU(int, trace_active);
@@ -42,7 +40,7 @@ static DEFINE_MUTEX(stack_sysctl_mutex);
 int stack_tracer_enabled;
 static int last_stack_tracer_enabled;
 
-static inline void print_max_stack(void)
+void print_max_stack(void)
 {
 	long i;
 	int size;
@@ -65,7 +63,15 @@ static inline void print_max_stack(void)
 	}
 }
 
-static inline void
+/*
+ * When arch-specific code overides this function, the following
+ * data should be filled up, assuming max_stack_lock is held to
+ * prevent concurrent updates.
+ *     stack_dump_index[]
+ *     max_stack_trace
+ *     max_stack_size
+ */
+void __weak
 check_stack(unsigned long ip, unsigned long *stack)
 {
 	unsigned long this_size, flags; unsigned long *p, *top, *start;
-- 
1.7.9.5

[PATCH v3 5/7] ftrace: allow arch-specific stack tracer

[AKASHI Takahiro]

A stack frame may be used in a different way depending on cpu architecture.
Thus it is not always appropriate to slurp the stack contents, as current
check_stack() does, in order to calcurate a stack index (height) at a given
function call. At least not on arm64.
In addition, there is a possibility that we will mistakenly detect a stale
stack frame which has not been overwritten.

This patch makes check_stack() a weak function so as to later implement
arch-specific version.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/linux/ftrace.h     |   10 ++++++++++
 kernel/trace/trace_stack.c |   22 ++++++++++++++--------
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index d77b195..e538400 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -270,7 +270,17 @@ static inline void ftrace_kill(void) { }
 #define FTRACE_STACK_FRAME_OFFSET 0
 #endif
 
+#define STACK_TRACE_ENTRIES 500
+
+struct stack_trace;
+
+extern unsigned stack_dump_index[];
+extern struct stack_trace max_stack_trace;
+extern unsigned long max_stack_size;
+extern arch_spinlock_t max_stack_lock;
+
 extern int stack_tracer_enabled;
+void print_max_stack(void);
 int
 stack_trace_sysctl(struct ctl_table *table, int write,
 		   void __user *buffer, size_t *lenp,
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 30521ea..ff1a191 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -16,24 +16,22 @@
 
 #include "trace.h"
 
-#define STACK_TRACE_ENTRIES 500
-
 static unsigned long stack_dump_trace[STACK_TRACE_ENTRIES+1] =
 	 { [0 ... (STACK_TRACE_ENTRIES)] = ULONG_MAX };
-static unsigned stack_dump_index[STACK_TRACE_ENTRIES];
+unsigned stack_dump_index[STACK_TRACE_ENTRIES];
 
 /*
  * Reserve one entry for the passed in ip. This will allow
  * us to remove most or all of the stack size overhead
  * added by the stack tracer itself.
  */
-static struct stack_trace max_stack_trace = {
+struct stack_trace max_stack_trace = {
 	.max_entries		= STACK_TRACE_ENTRIES - 1,
 	.entries		= &stack_dump_trace[0],
 };
 
-static unsigned long max_stack_size;
-static arch_spinlock_t max_stack_lock =
+unsigned long max_stack_size;
+arch_spinlock_t max_stack_lock =
 	(arch_spinlock_t)__ARCH_SPIN_LOCK_UNLOCKED;
 
 static DEFINE_PER_CPU(int, trace_active);
@@ -42,7 +40,7 @@ static DEFINE_MUTEX(stack_sysctl_mutex);
 int stack_tracer_enabled;
 static int last_stack_tracer_enabled;
 
-static inline void print_max_stack(void)
+void print_max_stack(void)
 {
 	long i;
 	int size;
@@ -65,7 +63,15 @@ static inline void print_max_stack(void)
 	}
 }
 
-static inline void
+/*
+ * When arch-specific code overides this function, the following
+ * data should be filled up, assuming max_stack_lock is held to
+ * prevent concurrent updates.
+ *     stack_dump_index[]
+ *     max_stack_trace
+ *     max_stack_size
+ */
+void __weak
 check_stack(unsigned long ip, unsigned long *stack)
 {
 	unsigned long this_size, flags; unsigned long *p, *top, *start;
-- 
1.7.9.5

Re: [PATCH v3 5/7] ftrace: allow arch-specific stack tracer

[Steven Rostedt]

On Thu,  8 Oct 2015 19:01:42 +0900
AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:

>  include/linux/ftrace.h     |   10 ++++++++++
>  kernel/trace/trace_stack.c |   22 ++++++++++++++--------
>  2 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index d77b195..e538400 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -270,7 +270,17 @@ static inline void ftrace_kill(void) { }
>  #define FTRACE_STACK_FRAME_OFFSET 0
>  #endif
>  
> +#define STACK_TRACE_ENTRIES 500
> +
> +struct stack_trace;
> +
> +extern unsigned stack_dump_index[];
> +extern struct stack_trace max_stack_trace;
> +extern unsigned long max_stack_size;
> +extern arch_spinlock_t max_stack_lock;
> +
>  extern int stack_tracer_enabled;
> +void print_max_stack(void);

OK, if we are making these external, they need to have better name
space naming.

stack_dump_index	=>	stack_trace_index
max_stack_trace		=>	stack_trace_max
max_stack_size		=>	stack_trace_max_size
print_max_stack()	=>	stack_trace_print()


-- Steve

>  int
>  stack_trace_sysctl(struct ctl_table *table, int write,
>  		   void __user *buffer, size_t *lenp,
> diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
> index 30521ea..ff1a191 100644
> --- a/kernel/trace/trace_stack.c
> +++ b/kernel/trace/trace_stack.c
> @@ -16,24 +16,22 @@
>  
>  #include "trace.h"
>  

[PATCH v3 5/7] ftrace: allow arch-specific stack tracer

[Steven Rostedt]

On Thu,  8 Oct 2015 19:01:42 +0900
AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:

>  include/linux/ftrace.h     |   10 ++++++++++
>  kernel/trace/trace_stack.c |   22 ++++++++++++++--------
>  2 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index d77b195..e538400 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -270,7 +270,17 @@ static inline void ftrace_kill(void) { }
>  #define FTRACE_STACK_FRAME_OFFSET 0
>  #endif
>  
> +#define STACK_TRACE_ENTRIES 500
> +
> +struct stack_trace;
> +
> +extern unsigned stack_dump_index[];
> +extern struct stack_trace max_stack_trace;
> +extern unsigned long max_stack_size;
> +extern arch_spinlock_t max_stack_lock;
> +
>  extern int stack_tracer_enabled;
> +void print_max_stack(void);

OK, if we are making these external, they need to have better name
space naming.

stack_dump_index	=>	stack_trace_index
max_stack_trace		=>	stack_trace_max
max_stack_size		=>	stack_trace_max_size
print_max_stack()	=>	stack_trace_print()


-- Steve

>  int
>  stack_trace_sysctl(struct ctl_table *table, int write,
>  		   void __user *buffer, size_t *lenp,
> diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
> index 30521ea..ff1a191 100644
> --- a/kernel/trace/trace_stack.c
> +++ b/kernel/trace/trace_stack.c
> @@ -16,24 +16,22 @@
>  
>  #include "trace.h"
>  

[PATCH v3 6/7] arm64: insn: add instruction decoders for ldp/stp and add/sub

[AKASHI Takahiro]

A function prologue analyzer is a requisite for implementing stack tracer
and getting better views of stack usages on arm64.
To implement a function prologue analyzer, we have to be able to decode,
at least, stp, add, sub and mov instructions.

This patch adds decoders for those instructions, that are used solely
by stack tracer for now, but generic enough for other uses.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 arch/arm64/include/asm/insn.h |   18 ++++++++
 arch/arm64/kernel/insn.c      |  102 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 120 insertions(+)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 30e50eb..8d5c538 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -165,6 +165,8 @@ enum aarch64_insn_ldst_type {
 	AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX,
 	AARCH64_INSN_LDST_LOAD_PAIR_POST_INDEX,
 	AARCH64_INSN_LDST_STORE_PAIR_POST_INDEX,
+	AARCH64_INSN_LDST_LOAD_PAIR,
+	AARCH64_INSN_LDST_STORE_PAIR,
 };
 
 enum aarch64_insn_adsb_type {
@@ -225,6 +227,8 @@ static __always_inline u32 aarch64_insn_get_##abbr##_value(void) \
 
 __AARCH64_INSN_FUNCS(str_reg,	0x3FE0EC00, 0x38206800)
 __AARCH64_INSN_FUNCS(ldr_reg,	0x3FE0EC00, 0x38606800)
+__AARCH64_INSN_FUNCS(stp,	0x7FC00000, 0x29000000)
+__AARCH64_INSN_FUNCS(ldp,	0x7FC00000, 0x29400000)
 __AARCH64_INSN_FUNCS(stp_post,	0x7FC00000, 0x28800000)
 __AARCH64_INSN_FUNCS(ldp_post,	0x7FC00000, 0x28C00000)
 __AARCH64_INSN_FUNCS(stp_pre,	0x7FC00000, 0x29800000)
@@ -277,6 +281,7 @@ __AARCH64_INSN_FUNCS(hint,	0xFFFFF01F, 0xD503201F)
 __AARCH64_INSN_FUNCS(br,	0xFFFFFC1F, 0xD61F0000)
 __AARCH64_INSN_FUNCS(blr,	0xFFFFFC1F, 0xD63F0000)
 __AARCH64_INSN_FUNCS(ret,	0xFFFFFC1F, 0xD65F0000)
+__AARCH64_INSN_FUNCS(eret,	0xFFFFFFFF, 0xD69F00E0)
 
 #undef	__AARCH64_INSN_FUNCS
 
@@ -370,6 +375,19 @@ bool aarch32_insn_is_wide(u32 insn);
 u32 aarch32_insn_extract_reg_num(u32 insn, int offset);
 u32 aarch32_insn_mcr_extract_opc2(u32 insn);
 u32 aarch32_insn_mcr_extract_crm(u32 insn);
+int aarch64_insn_decode_add_sub_imm(u32 insn,
+				    enum aarch64_insn_register *dst,
+				    enum aarch64_insn_register *src,
+				    int *imm,
+				    enum aarch64_insn_variant *variant,
+				    enum aarch64_insn_adsb_type *type);
+int aarch64_insn_decode_load_store_pair(u32 insn,
+					enum aarch64_insn_register *reg1,
+					enum aarch64_insn_register *reg2,
+					enum aarch64_insn_register *base,
+					int *offset,
+					enum aarch64_insn_variant *variant,
+					enum aarch64_insn_ldst_type *type);
 #endif /* __ASSEMBLY__ */
 
 #endif	/* __ASM_INSN_H */
diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
index f341866..baf2d52 100644
--- a/arch/arm64/kernel/insn.c
+++ b/arch/arm64/kernel/insn.c
@@ -33,6 +33,7 @@
 #include <asm/insn.h>
 
 #define AARCH64_INSN_SF_BIT	BIT(31)
+#define AARCH64_INSN_S_BIT	BIT(29)
 #define AARCH64_INSN_N_BIT	BIT(22)
 
 static int aarch64_insn_encoding_class[] = {
@@ -1141,3 +1142,104 @@ u32 aarch32_insn_mcr_extract_crm(u32 insn)
 {
 	return insn & CRM_MASK;
 }
+
+enum aarch64_insn_register aarch64_insn_extract_reg_num(u32 insn,
+				enum aarch64_insn_register_type type)
+{
+	int shift;
+
+	switch (type) {
+	case AARCH64_INSN_REGTYPE_RT:
+	case AARCH64_INSN_REGTYPE_RD:
+		shift = 0;
+		break;
+	case AARCH64_INSN_REGTYPE_RN:
+		shift = 5;
+		break;
+	case AARCH64_INSN_REGTYPE_RT2:
+	case AARCH64_INSN_REGTYPE_RA:
+		shift = 10;
+		break;
+	case AARCH64_INSN_REGTYPE_RM:
+		shift = 16;
+		break;
+	default:
+		pr_err("%s: unknown register type decoding %d\n", __func__,
+		       type);
+		return ~0L;
+	}
+
+	return (insn & (GENMASK(4, 0) << shift)) >> shift;
+}
+
+int aarch64_insn_decode_add_sub_imm(u32 insn,
+				    enum aarch64_insn_register *dst,
+				    enum aarch64_insn_register *src,
+				    int *imm,
+				    enum aarch64_insn_variant *variant,
+				    enum aarch64_insn_adsb_type *type)
+{
+	if (aarch64_insn_is_add_imm(insn))
+		*type =	((insn) & AARCH64_INSN_S_BIT) ?
+				AARCH64_INSN_ADSB_ADD_SETFLAGS :
+				AARCH64_INSN_ADSB_ADD;
+	else if (aarch64_insn_is_sub_imm(insn))
+		*type =	((insn) & AARCH64_INSN_S_BIT) ?
+				AARCH64_INSN_ADSB_SUB_SETFLAGS :
+				AARCH64_INSN_ADSB_SUB;
+	else
+		return 0;
+
+	*variant = (insn & AARCH64_INSN_SF_BIT) ? AARCH64_INSN_VARIANT_64BIT :
+					AARCH64_INSN_VARIANT_32BIT;
+
+	*dst = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RD);
+
+	*src = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RN);
+
+	/* TODO: ignore shilft field[23:22] */
+	*imm = (int)aarch64_insn_decode_immediate(AARCH64_INSN_IMM_12, insn);
+
+	return 1;
+}
+
+int aarch64_insn_decode_load_store_pair(u32 insn,
+					enum aarch64_insn_register *reg1,
+					enum aarch64_insn_register *reg2,
+					enum aarch64_insn_register *base,
+					int *offset,
+					enum aarch64_insn_variant *variant,
+					enum aarch64_insn_ldst_type *type)
+{
+	int imm;
+
+	if (aarch64_insn_is_stp(insn))
+		*type = AARCH64_INSN_LDST_STORE_PAIR;
+	else if (aarch64_insn_is_stp_post(insn))
+		*type = AARCH64_INSN_LDST_STORE_PAIR_POST_INDEX;
+	else if (aarch64_insn_is_stp_pre(insn))
+		*type = AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX;
+	else if (aarch64_insn_is_ldp(insn))
+		*type = AARCH64_INSN_LDST_LOAD_PAIR;
+	else if (aarch64_insn_is_ldp_post(insn))
+		*type = AARCH64_INSN_LDST_LOAD_PAIR_POST_INDEX;
+	else if (aarch64_insn_is_ldp_pre(insn))
+		*type = AARCH64_INSN_LDST_LOAD_PAIR_PRE_INDEX;
+	else
+		return 0;
+
+	*variant = (insn & AARCH64_INSN_S_BIT) ? AARCH64_INSN_VARIANT_64BIT :
+				AARCH64_INSN_VARIANT_32BIT;
+
+	*reg1 = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RT);
+
+	*reg2 = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RT2);
+
+	*base = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RN);
+
+	imm = (int)aarch64_insn_decode_immediate(AARCH64_INSN_IMM_7, insn);
+	asm("sbfm %0, %0, 0, 6" : "+r" (imm));
+	*offset = imm * 8;
+
+	return 1;
+}
-- 
1.7.9.5

[PATCH v3 6/7] arm64: insn: add instruction decoders for ldp/stp and add/sub

[AKASHI Takahiro]

A function prologue analyzer is a requisite for implementing stack tracer
and getting better views of stack usages on arm64.
To implement a function prologue analyzer, we have to be able to decode,
at least, stp, add, sub and mov instructions.

This patch adds decoders for those instructions, that are used solely
by stack tracer for now, but generic enough for other uses.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 arch/arm64/include/asm/insn.h |   18 ++++++++
 arch/arm64/kernel/insn.c      |  102 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 120 insertions(+)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 30e50eb..8d5c538 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -165,6 +165,8 @@ enum aarch64_insn_ldst_type {
 	AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX,
 	AARCH64_INSN_LDST_LOAD_PAIR_POST_INDEX,
 	AARCH64_INSN_LDST_STORE_PAIR_POST_INDEX,
+	AARCH64_INSN_LDST_LOAD_PAIR,
+	AARCH64_INSN_LDST_STORE_PAIR,
 };
 
 enum aarch64_insn_adsb_type {
@@ -225,6 +227,8 @@ static __always_inline u32 aarch64_insn_get_##abbr##_value(void) \
 
 __AARCH64_INSN_FUNCS(str_reg,	0x3FE0EC00, 0x38206800)
 __AARCH64_INSN_FUNCS(ldr_reg,	0x3FE0EC00, 0x38606800)
+__AARCH64_INSN_FUNCS(stp,	0x7FC00000, 0x29000000)
+__AARCH64_INSN_FUNCS(ldp,	0x7FC00000, 0x29400000)
 __AARCH64_INSN_FUNCS(stp_post,	0x7FC00000, 0x28800000)
 __AARCH64_INSN_FUNCS(ldp_post,	0x7FC00000, 0x28C00000)
 __AARCH64_INSN_FUNCS(stp_pre,	0x7FC00000, 0x29800000)
@@ -277,6 +281,7 @@ __AARCH64_INSN_FUNCS(hint,	0xFFFFF01F, 0xD503201F)
 __AARCH64_INSN_FUNCS(br,	0xFFFFFC1F, 0xD61F0000)
 __AARCH64_INSN_FUNCS(blr,	0xFFFFFC1F, 0xD63F0000)
 __AARCH64_INSN_FUNCS(ret,	0xFFFFFC1F, 0xD65F0000)
+__AARCH64_INSN_FUNCS(eret,	0xFFFFFFFF, 0xD69F00E0)
 
 #undef	__AARCH64_INSN_FUNCS
 
@@ -370,6 +375,19 @@ bool aarch32_insn_is_wide(u32 insn);
 u32 aarch32_insn_extract_reg_num(u32 insn, int offset);
 u32 aarch32_insn_mcr_extract_opc2(u32 insn);
 u32 aarch32_insn_mcr_extract_crm(u32 insn);
+int aarch64_insn_decode_add_sub_imm(u32 insn,
+				    enum aarch64_insn_register *dst,
+				    enum aarch64_insn_register *src,
+				    int *imm,
+				    enum aarch64_insn_variant *variant,
+				    enum aarch64_insn_adsb_type *type);
+int aarch64_insn_decode_load_store_pair(u32 insn,
+					enum aarch64_insn_register *reg1,
+					enum aarch64_insn_register *reg2,
+					enum aarch64_insn_register *base,
+					int *offset,
+					enum aarch64_insn_variant *variant,
+					enum aarch64_insn_ldst_type *type);
 #endif /* __ASSEMBLY__ */
 
 #endif	/* __ASM_INSN_H */
diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
index f341866..baf2d52 100644
--- a/arch/arm64/kernel/insn.c
+++ b/arch/arm64/kernel/insn.c
@@ -33,6 +33,7 @@
 #include <asm/insn.h>
 
 #define AARCH64_INSN_SF_BIT	BIT(31)
+#define AARCH64_INSN_S_BIT	BIT(29)
 #define AARCH64_INSN_N_BIT	BIT(22)
 
 static int aarch64_insn_encoding_class[] = {
@@ -1141,3 +1142,104 @@ u32 aarch32_insn_mcr_extract_crm(u32 insn)
 {
 	return insn & CRM_MASK;
 }
+
+enum aarch64_insn_register aarch64_insn_extract_reg_num(u32 insn,
+				enum aarch64_insn_register_type type)
+{
+	int shift;
+
+	switch (type) {
+	case AARCH64_INSN_REGTYPE_RT:
+	case AARCH64_INSN_REGTYPE_RD:
+		shift = 0;
+		break;
+	case AARCH64_INSN_REGTYPE_RN:
+		shift = 5;
+		break;
+	case AARCH64_INSN_REGTYPE_RT2:
+	case AARCH64_INSN_REGTYPE_RA:
+		shift = 10;
+		break;
+	case AARCH64_INSN_REGTYPE_RM:
+		shift = 16;
+		break;
+	default:
+		pr_err("%s: unknown register type decoding %d\n", __func__,
+		       type);
+		return ~0L;
+	}
+
+	return (insn & (GENMASK(4, 0) << shift)) >> shift;
+}
+
+int aarch64_insn_decode_add_sub_imm(u32 insn,
+				    enum aarch64_insn_register *dst,
+				    enum aarch64_insn_register *src,
+				    int *imm,
+				    enum aarch64_insn_variant *variant,
+				    enum aarch64_insn_adsb_type *type)
+{
+	if (aarch64_insn_is_add_imm(insn))
+		*type =	((insn) & AARCH64_INSN_S_BIT) ?
+				AARCH64_INSN_ADSB_ADD_SETFLAGS :
+				AARCH64_INSN_ADSB_ADD;
+	else if (aarch64_insn_is_sub_imm(insn))
+		*type =	((insn) & AARCH64_INSN_S_BIT) ?
+				AARCH64_INSN_ADSB_SUB_SETFLAGS :
+				AARCH64_INSN_ADSB_SUB;
+	else
+		return 0;
+
+	*variant = (insn & AARCH64_INSN_SF_BIT) ? AARCH64_INSN_VARIANT_64BIT :
+					AARCH64_INSN_VARIANT_32BIT;
+
+	*dst = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RD);
+
+	*src = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RN);
+
+	/* TODO: ignore shilft field[23:22] */
+	*imm = (int)aarch64_insn_decode_immediate(AARCH64_INSN_IMM_12, insn);
+
+	return 1;
+}
+
+int aarch64_insn_decode_load_store_pair(u32 insn,
+					enum aarch64_insn_register *reg1,
+					enum aarch64_insn_register *reg2,
+					enum aarch64_insn_register *base,
+					int *offset,
+					enum aarch64_insn_variant *variant,
+					enum aarch64_insn_ldst_type *type)
+{
+	int imm;
+
+	if (aarch64_insn_is_stp(insn))
+		*type = AARCH64_INSN_LDST_STORE_PAIR;
+	else if (aarch64_insn_is_stp_post(insn))
+		*type = AARCH64_INSN_LDST_STORE_PAIR_POST_INDEX;
+	else if (aarch64_insn_is_stp_pre(insn))
+		*type = AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX;
+	else if (aarch64_insn_is_ldp(insn))
+		*type = AARCH64_INSN_LDST_LOAD_PAIR;
+	else if (aarch64_insn_is_ldp_post(insn))
+		*type = AARCH64_INSN_LDST_LOAD_PAIR_POST_INDEX;
+	else if (aarch64_insn_is_ldp_pre(insn))
+		*type = AARCH64_INSN_LDST_LOAD_PAIR_PRE_INDEX;
+	else
+		return 0;
+
+	*variant = (insn & AARCH64_INSN_S_BIT) ? AARCH64_INSN_VARIANT_64BIT :
+				AARCH64_INSN_VARIANT_32BIT;
+
+	*reg1 = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RT);
+
+	*reg2 = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RT2);
+
+	*base = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RN);
+
+	imm = (int)aarch64_insn_decode_immediate(AARCH64_INSN_IMM_7, insn);
+	asm("sbfm %0, %0, 0, 6" : "+r" (imm));
+	*offset = imm * 8;
+
+	return 1;
+}
-- 
1.7.9.5

[PATCH v3 7/7] arm64: ftrace: add arch-specific stack tracer

[AKASHI Takahiro]

Stack tracer on arm64, check_stack(), is uniqeue in the following
points:
* analyze a function prologue of a traced function to estimate a more
  accurate stack pointer value, replacing naive '<child's fp> + 0x10.'
* use walk_stackframe(), instead of slurping stack contents as orignal
  check_stack() does, to identify a stack frame and a stack index (height)
  for every callsite.

Regarding a function prologue analyzer, there is no guarantee that we can
handle all the possible patterns of function prologue as gcc does not use
any fixed templates to generate them. 'Instruction scheduling' is another
issue here.
Nevertheless, the current version will surely cover almost all the cases
in the kernel image and give us useful information on stack pointers.

So this patch utilizes a function prologue only for stack tracer, and
does not affect the behaviors of existing stacktrace functions.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 arch/arm64/include/asm/stacktrace.h |    1 +
 arch/arm64/kernel/ftrace.c          |   66 +++++++++++++
 arch/arm64/kernel/stacktrace.c      |  186 ++++++++++++++++++++++++++++++++++-
 3 files changed, 250 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 7318f6d..bfaa22a 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -25,5 +25,6 @@ struct stackframe {
 extern int unwind_frame(struct stackframe *frame);
 extern void walk_stackframe(struct stackframe *frame,
 			    int (*fn)(struct stackframe *, void *), void *data);
+extern void save_stack_trace_sp(struct stack_trace *trace, unsigned long *sp);
 
 #endif	/* __ASM_STACKTRACE_H */
diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index 314f82d..fa37875 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -9,6 +9,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/bug.h>
 #include <linux/ftrace.h>
 #include <linux/swab.h>
 #include <linux/uaccess.h>
@@ -16,6 +17,7 @@
 #include <asm/cacheflush.h>
 #include <asm/ftrace.h>
 #include <asm/insn.h>
+#include <asm/stacktrace.h>
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 /*
@@ -173,3 +175,67 @@ int ftrace_disable_ftrace_graph_caller(void)
 }
 #endif /* CONFIG_DYNAMIC_FTRACE */
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
+
+#ifdef CONFIG_STACK_TRACER
+static unsigned long stack_dump_sp[STACK_TRACE_ENTRIES];
+static unsigned long raw_max_stack_size;
+
+void check_stack(unsigned long ip, unsigned long *stack)
+{
+	unsigned long this_size, flags;
+	unsigned long top;
+	int i, j;
+
+	this_size = ((unsigned long)stack) & (THREAD_SIZE-1);
+	this_size = THREAD_SIZE - this_size;
+
+	if (this_size <= raw_max_stack_size)
+		return;
+
+	/* we do not handle an interrupt stack yet */
+	if (!object_is_on_stack(stack))
+		return;
+
+	local_irq_save(flags);
+	arch_spin_lock(&max_stack_lock);
+
+	/* check again */
+	if (this_size <= raw_max_stack_size)
+		goto out;
+
+	/* find out stack frames */
+	max_stack_trace.nr_entries = 0;
+	max_stack_trace.skip = 0;
+	save_stack_trace_sp(&max_stack_trace, stack_dump_sp);
+	max_stack_trace.nr_entries--; /* for the last entry ('-1') */
+
+	/* calculate a stack index for each function */
+	top = ((unsigned long)stack & ~(THREAD_SIZE-1)) + THREAD_SIZE;
+	for (i = 0; i < max_stack_trace.nr_entries; i++)
+		stack_dump_index[i] = top - stack_dump_sp[i];
+	raw_max_stack_size = this_size;
+
+	/* Skip over the overhead of the stack tracer itself */
+	for (i = 0; i < max_stack_trace.nr_entries; i++) {
+		if ((max_stack_trace.entries[i] + FTRACE_STACK_FRAME_OFFSET)
+				== ip)
+			break;
+	}
+
+	max_stack_trace.nr_entries -= i;
+	for (j = 0; j < max_stack_trace.nr_entries; j++) {
+		stack_dump_index[j] = stack_dump_index[j + i];
+		max_stack_trace.entries[j] = max_stack_trace.entries[j + i];
+	}
+	max_stack_size = stack_dump_index[0];
+
+	if (task_stack_end_corrupted(current)) {
+		WARN(1, "task stack is corrupted.\n");
+		print_max_stack();
+	}
+
+ out:
+	arch_spin_unlock(&max_stack_lock);
+	local_irq_restore(flags);
+}
+#endif /* CONFIG_STACK_TRACER */
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 1555a8b..1b293fe 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -17,12 +17,151 @@
  */
 #include <linux/kernel.h>
 #include <linux/export.h>
+#include <linux/ftrace.h>
 #include <linux/sched.h>
 #include <linux/stacktrace.h>
 
 #include <asm/insn.h>
 #include <asm/stacktrace.h>
 
+#ifdef CONFIG_STACK_TRACER
+/*
+ * This function parses a function prologue of a traced function and
+ * determines its stack size.
+ * A return value indicates a location of @pc in a function prologue.
+ * @return value:
+ * <case 1>                       <case 1'>
+ * 1:
+ *     sub sp, sp, #XX            sub sp, sp, #XX
+ * 2:
+ *     stp x29, x30, [sp, #YY]    stp x29, x30, [sp, #--ZZ]!
+ * 3:
+ *     add x29, sp, #YY           mov x29, sp
+ * 0:
+ *
+ * <case 2>
+ * 1:
+ *     stp x29, x30, [sp, #-XX]!
+ * 3:
+ *     mov x29, sp
+ * 0:
+ *
+ * @size: sp offset from calller's sp (XX or XX + ZZ)
+ * @size2: fp offset from new sp (YY or 0)
+ */
+static int analyze_function_prologue(unsigned long pc,
+		unsigned long *size, unsigned long *size2)
+{
+	unsigned long offset;
+	u32 *addr, insn;
+	int pos = -1;
+	enum aarch64_insn_register src, dst, reg1, reg2, base;
+	int imm;
+	enum aarch64_insn_variant variant;
+	enum aarch64_insn_adsb_type adsb_type;
+	enum aarch64_insn_ldst_type ldst_type;
+
+	*size = *size2 = 0;
+
+	if (!pc)
+		goto out;
+
+	if (unlikely(!kallsyms_lookup_size_offset(pc, NULL, &offset)))
+		goto out;
+
+	addr = (u32 *)(pc - offset);
+#ifdef CONFIG_DYNAMIC_FTRACE
+	if (addr == (u32 *)ftrace_call)
+		addr = (u32 *)ftrace_caller;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	else if (addr == (u32 *)ftrace_graph_caller)
+#ifdef CONFIG_DYNAMIC_FTRACE
+		addr = (u32 *)ftrace_caller;
+#else
+		addr = (u32 *)_mcount;
+#endif
+#endif
+#endif
+
+	insn = *addr;
+	pos = 1;
+
+	/* analyze a function prologue */
+	while ((unsigned long)addr < pc) {
+		if (aarch64_insn_is_branch_imm(insn) ||
+		    aarch64_insn_is_br(insn) ||
+		    aarch64_insn_is_blr(insn) ||
+		    aarch64_insn_is_ret(insn) ||
+		    aarch64_insn_is_eret(insn))
+			/* exiting a basic block */
+			goto out;
+
+		if (aarch64_insn_decode_add_sub_imm(insn, &dst, &src,
+					&imm, &variant, &adsb_type)) {
+			if ((adsb_type == AARCH64_INSN_ADSB_SUB) &&
+				(dst == AARCH64_INSN_REG_SP) &&
+				(src == AARCH64_INSN_REG_SP)) {
+				/*
+				 * Starting the following sequence:
+				 *   sub sp, sp, #xx
+				 *   stp x29, x30, [sp, #yy]
+				 *   add x29, sp, #yy
+				 */
+				WARN_ON(pos != 1);
+				pos = 2;
+				*size += imm;
+			} else if ((adsb_type == AARCH64_INSN_ADSB_ADD) &&
+				(dst == AARCH64_INSN_REG_29) &&
+				(src == AARCH64_INSN_REG_SP)) {
+				/*
+				 *   add x29, sp, #yy
+				 * or
+				 *   mov x29, sp
+				 */
+				WARN_ON(pos != 3);
+				pos = 0;
+				*size2 = imm;
+
+				break;
+			}
+		} else if (aarch64_insn_decode_load_store_pair(insn,
+					&reg1, &reg2, &base, &imm,
+					&variant, &ldst_type)) {
+			if ((ldst_type ==
+				AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX) &&
+			    (reg1 == AARCH64_INSN_REG_29) &&
+			    (reg2 == AARCH64_INSN_REG_30) &&
+			    (base == AARCH64_INSN_REG_SP)) {
+				/*
+				 * Starting the following sequence:
+				 *   stp x29, x30, [sp, #-xx]!
+				 *   mov x29, sp
+				 */
+				WARN_ON(!((pos == 1) || (pos == 2)));
+				pos = 3;
+				*size += -imm;
+			} else if ((ldst_type ==
+				AARCH64_INSN_LDST_STORE_PAIR) &&
+			    (reg1 == AARCH64_INSN_REG_29) &&
+			    (reg2 == AARCH64_INSN_REG_30) &&
+			    (base == AARCH64_INSN_REG_SP)) {
+				/*
+				 *   stp x29, x30, [sp, #yy]
+				 */
+				WARN_ON(pos != 2);
+				pos = 3;
+			}
+		}
+
+		addr++;
+		insn = *addr;
+	}
+
+out:
+	return pos;
+}
+#endif
+
 /*
  * AArch64 PCS assigns the frame pointer to x29.
  *
@@ -81,6 +220,9 @@ struct stack_trace_data {
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	unsigned int ret_stack_index;
 #endif
+#ifdef CONFIG_STACK_TRACER
+	unsigned long *sp;
+#endif
 };
 
 static int save_trace(struct stackframe *frame, void *d)
@@ -110,12 +252,33 @@ static int save_trace(struct stackframe *frame, void *d)
 		return 0;
 	}
 
+#ifdef CONFIG_STACK_TRACER
+	if (data->sp) {
+		if (trace->nr_entries) {
+			unsigned long child_pc, sp_off, fp_off;
+			int pos;
+
+			child_pc = trace->entries[trace->nr_entries - 1];
+			pos = analyze_function_prologue(child_pc,
+					&sp_off, &fp_off);
+			/*
+			 * frame->sp - 0x10 is actually a child's fp.
+			 * See above.
+			 */
+			data->sp[trace->nr_entries] = (pos < 0 ? frame->sp :
+					(frame->sp - 0x10) + sp_off - fp_off);
+		} else {
+			data->sp[0] = frame->sp;
+		}
+	}
+#endif
 	trace->entries[trace->nr_entries++] = addr;
 
 	return trace->nr_entries >= trace->max_entries;
 }
 
-void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
+static void __save_stack_trace_tsk(struct task_struct *tsk,
+		struct stack_trace *trace, unsigned long *stack_dump_sp)
 {
 	struct stack_trace_data data;
 	struct stackframe frame;
@@ -125,6 +288,9 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	data.ret_stack_index = current->curr_ret_stack;
 #endif
+#ifdef CONFIG_STACK_TRACER
+	data.sp = stack_dump_sp;
+#endif
 
 	if (tsk != current) {
 		data.no_sched_functions = 1;
@@ -135,7 +301,8 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 		data.no_sched_functions = 0;
 		frame.fp = (unsigned long)__builtin_frame_address(0);
 		frame.sp = current_stack_pointer;
-		frame.pc = (unsigned long)save_stack_trace_tsk;
+		asm("1:");
+		asm("ldr %0, =1b" : "=r" (frame.pc));
 	}
 
 	walk_stackframe(&frame, save_trace, &data);
@@ -143,9 +310,22 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 		trace->entries[trace->nr_entries++] = ULONG_MAX;
 }
 
+void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
+{
+	__save_stack_trace_tsk(tsk, trace, NULL);
+}
+
 void save_stack_trace(struct stack_trace *trace)
 {
-	save_stack_trace_tsk(current, trace);
+	__save_stack_trace_tsk(current, trace, NULL);
 }
 EXPORT_SYMBOL_GPL(save_stack_trace);
+
+#ifdef CONFIG_STACK_TRACER
+void save_stack_trace_sp(struct stack_trace *trace,
+					unsigned long *stack_dump_sp)
+{
+	__save_stack_trace_tsk(current, trace, stack_dump_sp);
+}
+#endif /* CONFIG_STACK_TRACER */
 #endif
-- 
1.7.9.5

[PATCH v3 7/7] arm64: ftrace: add arch-specific stack tracer

[AKASHI Takahiro]

Stack tracer on arm64, check_stack(), is uniqeue in the following
points:
* analyze a function prologue of a traced function to estimate a more
  accurate stack pointer value, replacing naive '<child's fp> + 0x10.'
* use walk_stackframe(), instead of slurping stack contents as orignal
  check_stack() does, to identify a stack frame and a stack index (height)
  for every callsite.

Regarding a function prologue analyzer, there is no guarantee that we can
handle all the possible patterns of function prologue as gcc does not use
any fixed templates to generate them. 'Instruction scheduling' is another
issue here.
Nevertheless, the current version will surely cover almost all the cases
in the kernel image and give us useful information on stack pointers.

So this patch utilizes a function prologue only for stack tracer, and
does not affect the behaviors of existing stacktrace functions.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 arch/arm64/include/asm/stacktrace.h |    1 +
 arch/arm64/kernel/ftrace.c          |   66 +++++++++++++
 arch/arm64/kernel/stacktrace.c      |  186 ++++++++++++++++++++++++++++++++++-
 3 files changed, 250 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 7318f6d..bfaa22a 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -25,5 +25,6 @@ struct stackframe {
 extern int unwind_frame(struct stackframe *frame);
 extern void walk_stackframe(struct stackframe *frame,
 			    int (*fn)(struct stackframe *, void *), void *data);
+extern void save_stack_trace_sp(struct stack_trace *trace, unsigned long *sp);
 
 #endif	/* __ASM_STACKTRACE_H */
diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index 314f82d..fa37875 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -9,6 +9,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/bug.h>
 #include <linux/ftrace.h>
 #include <linux/swab.h>
 #include <linux/uaccess.h>
@@ -16,6 +17,7 @@
 #include <asm/cacheflush.h>
 #include <asm/ftrace.h>
 #include <asm/insn.h>
+#include <asm/stacktrace.h>
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 /*
@@ -173,3 +175,67 @@ int ftrace_disable_ftrace_graph_caller(void)
 }
 #endif /* CONFIG_DYNAMIC_FTRACE */
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
+
+#ifdef CONFIG_STACK_TRACER
+static unsigned long stack_dump_sp[STACK_TRACE_ENTRIES];
+static unsigned long raw_max_stack_size;
+
+void check_stack(unsigned long ip, unsigned long *stack)
+{
+	unsigned long this_size, flags;
+	unsigned long top;
+	int i, j;
+
+	this_size = ((unsigned long)stack) & (THREAD_SIZE-1);
+	this_size = THREAD_SIZE - this_size;
+
+	if (this_size <= raw_max_stack_size)
+		return;
+
+	/* we do not handle an interrupt stack yet */
+	if (!object_is_on_stack(stack))
+		return;
+
+	local_irq_save(flags);
+	arch_spin_lock(&max_stack_lock);
+
+	/* check again */
+	if (this_size <= raw_max_stack_size)
+		goto out;
+
+	/* find out stack frames */
+	max_stack_trace.nr_entries = 0;
+	max_stack_trace.skip = 0;
+	save_stack_trace_sp(&max_stack_trace, stack_dump_sp);
+	max_stack_trace.nr_entries--; /* for the last entry ('-1') */
+
+	/* calculate a stack index for each function */
+	top = ((unsigned long)stack & ~(THREAD_SIZE-1)) + THREAD_SIZE;
+	for (i = 0; i < max_stack_trace.nr_entries; i++)
+		stack_dump_index[i] = top - stack_dump_sp[i];
+	raw_max_stack_size = this_size;
+
+	/* Skip over the overhead of the stack tracer itself */
+	for (i = 0; i < max_stack_trace.nr_entries; i++) {
+		if ((max_stack_trace.entries[i] + FTRACE_STACK_FRAME_OFFSET)
+				== ip)
+			break;
+	}
+
+	max_stack_trace.nr_entries -= i;
+	for (j = 0; j < max_stack_trace.nr_entries; j++) {
+		stack_dump_index[j] = stack_dump_index[j + i];
+		max_stack_trace.entries[j] = max_stack_trace.entries[j + i];
+	}
+	max_stack_size = stack_dump_index[0];
+
+	if (task_stack_end_corrupted(current)) {
+		WARN(1, "task stack is corrupted.\n");
+		print_max_stack();
+	}
+
+ out:
+	arch_spin_unlock(&max_stack_lock);
+	local_irq_restore(flags);
+}
+#endif /* CONFIG_STACK_TRACER */
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 1555a8b..1b293fe 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -17,12 +17,151 @@
  */
 #include <linux/kernel.h>
 #include <linux/export.h>
+#include <linux/ftrace.h>
 #include <linux/sched.h>
 #include <linux/stacktrace.h>
 
 #include <asm/insn.h>
 #include <asm/stacktrace.h>
 
+#ifdef CONFIG_STACK_TRACER
+/*
+ * This function parses a function prologue of a traced function and
+ * determines its stack size.
+ * A return value indicates a location of @pc in a function prologue.
+ * @return value:
+ * <case 1>                       <case 1'>
+ * 1:
+ *     sub sp, sp, #XX            sub sp, sp, #XX
+ * 2:
+ *     stp x29, x30, [sp, #YY]    stp x29, x30, [sp, #--ZZ]!
+ * 3:
+ *     add x29, sp, #YY           mov x29, sp
+ * 0:
+ *
+ * <case 2>
+ * 1:
+ *     stp x29, x30, [sp, #-XX]!
+ * 3:
+ *     mov x29, sp
+ * 0:
+ *
+ * @size: sp offset from calller's sp (XX or XX + ZZ)
+ * @size2: fp offset from new sp (YY or 0)
+ */
+static int analyze_function_prologue(unsigned long pc,
+		unsigned long *size, unsigned long *size2)
+{
+	unsigned long offset;
+	u32 *addr, insn;
+	int pos = -1;
+	enum aarch64_insn_register src, dst, reg1, reg2, base;
+	int imm;
+	enum aarch64_insn_variant variant;
+	enum aarch64_insn_adsb_type adsb_type;
+	enum aarch64_insn_ldst_type ldst_type;
+
+	*size = *size2 = 0;
+
+	if (!pc)
+		goto out;
+
+	if (unlikely(!kallsyms_lookup_size_offset(pc, NULL, &offset)))
+		goto out;
+
+	addr = (u32 *)(pc - offset);
+#ifdef CONFIG_DYNAMIC_FTRACE
+	if (addr == (u32 *)ftrace_call)
+		addr = (u32 *)ftrace_caller;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	else if (addr == (u32 *)ftrace_graph_caller)
+#ifdef CONFIG_DYNAMIC_FTRACE
+		addr = (u32 *)ftrace_caller;
+#else
+		addr = (u32 *)_mcount;
+#endif
+#endif
+#endif
+
+	insn = *addr;
+	pos = 1;
+
+	/* analyze a function prologue */
+	while ((unsigned long)addr < pc) {
+		if (aarch64_insn_is_branch_imm(insn) ||
+		    aarch64_insn_is_br(insn) ||
+		    aarch64_insn_is_blr(insn) ||
+		    aarch64_insn_is_ret(insn) ||
+		    aarch64_insn_is_eret(insn))
+			/* exiting a basic block */
+			goto out;
+
+		if (aarch64_insn_decode_add_sub_imm(insn, &dst, &src,
+					&imm, &variant, &adsb_type)) {
+			if ((adsb_type == AARCH64_INSN_ADSB_SUB) &&
+				(dst == AARCH64_INSN_REG_SP) &&
+				(src == AARCH64_INSN_REG_SP)) {
+				/*
+				 * Starting the following sequence:
+				 *   sub sp, sp, #xx
+				 *   stp x29, x30, [sp, #yy]
+				 *   add x29, sp, #yy
+				 */
+				WARN_ON(pos != 1);
+				pos = 2;
+				*size += imm;
+			} else if ((adsb_type == AARCH64_INSN_ADSB_ADD) &&
+				(dst == AARCH64_INSN_REG_29) &&
+				(src == AARCH64_INSN_REG_SP)) {
+				/*
+				 *   add x29, sp, #yy
+				 * or
+				 *   mov x29, sp
+				 */
+				WARN_ON(pos != 3);
+				pos = 0;
+				*size2 = imm;
+
+				break;
+			}
+		} else if (aarch64_insn_decode_load_store_pair(insn,
+					&reg1, &reg2, &base, &imm,
+					&variant, &ldst_type)) {
+			if ((ldst_type ==
+				AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX) &&
+			    (reg1 == AARCH64_INSN_REG_29) &&
+			    (reg2 == AARCH64_INSN_REG_30) &&
+			    (base == AARCH64_INSN_REG_SP)) {
+				/*
+				 * Starting the following sequence:
+				 *   stp x29, x30, [sp, #-xx]!
+				 *   mov x29, sp
+				 */
+				WARN_ON(!((pos == 1) || (pos == 2)));
+				pos = 3;
+				*size += -imm;
+			} else if ((ldst_type ==
+				AARCH64_INSN_LDST_STORE_PAIR) &&
+			    (reg1 == AARCH64_INSN_REG_29) &&
+			    (reg2 == AARCH64_INSN_REG_30) &&
+			    (base == AARCH64_INSN_REG_SP)) {
+				/*
+				 *   stp x29, x30, [sp, #yy]
+				 */
+				WARN_ON(pos != 2);
+				pos = 3;
+			}
+		}
+
+		addr++;
+		insn = *addr;
+	}
+
+out:
+	return pos;
+}
+#endif
+
 /*
  * AArch64 PCS assigns the frame pointer to x29.
  *
@@ -81,6 +220,9 @@ struct stack_trace_data {
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	unsigned int ret_stack_index;
 #endif
+#ifdef CONFIG_STACK_TRACER
+	unsigned long *sp;
+#endif
 };
 
 static int save_trace(struct stackframe *frame, void *d)
@@ -110,12 +252,33 @@ static int save_trace(struct stackframe *frame, void *d)
 		return 0;
 	}
 
+#ifdef CONFIG_STACK_TRACER
+	if (data->sp) {
+		if (trace->nr_entries) {
+			unsigned long child_pc, sp_off, fp_off;
+			int pos;
+
+			child_pc = trace->entries[trace->nr_entries - 1];
+			pos = analyze_function_prologue(child_pc,
+					&sp_off, &fp_off);
+			/*
+			 * frame->sp - 0x10 is actually a child's fp.
+			 * See above.
+			 */
+			data->sp[trace->nr_entries] = (pos < 0 ? frame->sp :
+					(frame->sp - 0x10) + sp_off - fp_off);
+		} else {
+			data->sp[0] = frame->sp;
+		}
+	}
+#endif
 	trace->entries[trace->nr_entries++] = addr;
 
 	return trace->nr_entries >= trace->max_entries;
 }
 
-void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
+static void __save_stack_trace_tsk(struct task_struct *tsk,
+		struct stack_trace *trace, unsigned long *stack_dump_sp)
 {
 	struct stack_trace_data data;
 	struct stackframe frame;
@@ -125,6 +288,9 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	data.ret_stack_index = current->curr_ret_stack;
 #endif
+#ifdef CONFIG_STACK_TRACER
+	data.sp = stack_dump_sp;
+#endif
 
 	if (tsk != current) {
 		data.no_sched_functions = 1;
@@ -135,7 +301,8 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 		data.no_sched_functions = 0;
 		frame.fp = (unsigned long)__builtin_frame_address(0);
 		frame.sp = current_stack_pointer;
-		frame.pc = (unsigned long)save_stack_trace_tsk;
+		asm("1:");
+		asm("ldr %0, =1b" : "=r" (frame.pc));
 	}
 
 	walk_stackframe(&frame, save_trace, &data);
@@ -143,9 +310,22 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 		trace->entries[trace->nr_entries++] = ULONG_MAX;
 }
 
+void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
+{
+	__save_stack_trace_tsk(tsk, trace, NULL);
+}
+
 void save_stack_trace(struct stack_trace *trace)
 {
-	save_stack_trace_tsk(current, trace);
+	__save_stack_trace_tsk(current, trace, NULL);
 }
 EXPORT_SYMBOL_GPL(save_stack_trace);
+
+#ifdef CONFIG_STACK_TRACER
+void save_stack_trace_sp(struct stack_trace *trace,
+					unsigned long *stack_dump_sp)
+{
+	__save_stack_trace_tsk(current, trace, stack_dump_sp);
+}
+#endif /* CONFIG_STACK_TRACER */
 #endif
-- 
1.7.9.5

Re: [PATCH v3 7/7] arm64: ftrace: add arch-specific stack tracer

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1771 bytes --]

Hi AKASHI,

[auto build test WARNING on v4.3-rc4 -- if it's inappropriate base, please ignore]

config: arm64-defconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All warnings (new ones prefixed by >>):

   In file included from arch/arm64/kernel/process.c:53:0:
>> arch/arm64/include/asm/stacktrace.h:28:40: warning: 'struct stack_trace' declared inside parameter list
    extern void save_stack_trace_sp(struct stack_trace *trace, unsigned long *sp);
                                           ^
>> arch/arm64/include/asm/stacktrace.h:28:40: warning: its scope is only this definition or declaration, which is probably not what you want

vim +28 arch/arm64/include/asm/stacktrace.h

    12	 *
    13	 * You should have received a copy of the GNU General Public License
    14	 * along with this program.  If not, see <http://www.gnu.org/licenses/>.
    15	 */
    16	#ifndef __ASM_STACKTRACE_H
    17	#define __ASM_STACKTRACE_H
    18	
    19	struct stackframe {
    20		unsigned long fp;
    21		unsigned long sp;
    22		unsigned long pc;
    23	};
    24	
    25	extern int unwind_frame(struct stackframe *frame);
    26	extern void walk_stackframe(struct stackframe *frame,
    27				    int (*fn)(struct stackframe *, void *), void *data);
  > 28	extern void save_stack_trace_sp(struct stack_trace *trace, unsigned long *sp);
    29	
    30	#endif	/* __ASM_STACKTRACE_H */

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 16486 bytes --]

[PATCH v3 7/7] arm64: ftrace: add arch-specific stack tracer

[kbuild test robot]

Hi AKASHI,

[auto build test WARNING on v4.3-rc4 -- if it's inappropriate base, please ignore]

config: arm64-defconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All warnings (new ones prefixed by >>):

   In file included from arch/arm64/kernel/process.c:53:0:
>> arch/arm64/include/asm/stacktrace.h:28:40: warning: 'struct stack_trace' declared inside parameter list
    extern void save_stack_trace_sp(struct stack_trace *trace, unsigned long *sp);
                                           ^
>> arch/arm64/include/asm/stacktrace.h:28:40: warning: its scope is only this definition or declaration, which is probably not what you want

vim +28 arch/arm64/include/asm/stacktrace.h

    12	 *
    13	 * You should have received a copy of the GNU General Public License
    14	 * along with this program.  If not, see <http://www.gnu.org/licenses/>.
    15	 */
    16	#ifndef __ASM_STACKTRACE_H
    17	#define __ASM_STACKTRACE_H
    18	
    19	struct stackframe {
    20		unsigned long fp;
    21		unsigned long sp;
    22		unsigned long pc;
    23	};
    24	
    25	extern int unwind_frame(struct stackframe *frame);
    26	extern void walk_stackframe(struct stackframe *frame,
    27				    int (*fn)(struct stackframe *, void *), void *data);
  > 28	extern void save_stack_trace_sp(struct stack_trace *trace, unsigned long *sp);
    29	
    30	#endif	/* __ASM_STACKTRACE_H */

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/octet-stream
Size: 16486 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151009/9176171a/attachment.obj>

Re: [PATCH v3 0/7] arm64: ftrace: fix incorrect output from stack tracer

[Jungseok Lee]

On Oct 8, 2015, at 7:01 PM, AKASHI Takahiro wrote:

Hi Akashi,

> This is the third patch series for fixing stack tracer on arm64.
> The original issue was reported by Jungseok[1], and then I found more
> issues[2].
> (Steven, Jungseok, sorry for not replying to your comments directly.)
> 
> I address here all the issues and implement fixes described in [2] except
> for interrupt-triggered problems, ie. II-3). Recent discussions[3] about
> introducing a dedicated interrupt stack suggests that we may avoid walking
> through from an interrupt stack to a process stack.
> (So interrupt-stack patch is a prerequisite.)
> 
> Basically,
> patch1 corresponds to the original issue.
> patch2 is a proactive improvement of function_graph tracer. 
> patch3 corresponds to II-4(functions under function_graph tracer).
> patch4 corresponds to II-5(leaf function).
> patch5, 6 and 7 correspond to II-1(slurping stack) and II-2(differences
> between x86 and arm64).
> 
> Each fix can be applied independently, but if patch5, 6 and 7 are
> acceptable, patch1 is not necessary because patch7 replaces a default
> stack tracer.
> 
> I tested the code with v4.3-rc3 + Jungseok's patch v3[4].
> 
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/354126.html
> [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/355920.html 
> [3] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/368003.html
> [4] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/371451.html

The [4] is not a valid patch. I hope the test has been going with the following one.

	http://www.kernelhub.org/?msg=841034&p=2

I will leave comments after playing with this series on top of my IRQ stack tree.

Best Regards
Jungseok Lee

[PATCH v3 0/7] arm64: ftrace: fix incorrect output from stack tracer

[Jungseok Lee]

On Oct 8, 2015, at 7:01 PM, AKASHI Takahiro wrote:

Hi Akashi,

> This is the third patch series for fixing stack tracer on arm64.
> The original issue was reported by Jungseok[1], and then I found more
> issues[2].
> (Steven, Jungseok, sorry for not replying to your comments directly.)
> 
> I address here all the issues and implement fixes described in [2] except
> for interrupt-triggered problems, ie. II-3). Recent discussions[3] about
> introducing a dedicated interrupt stack suggests that we may avoid walking
> through from an interrupt stack to a process stack.
> (So interrupt-stack patch is a prerequisite.)
> 
> Basically,
> patch1 corresponds to the original issue.
> patch2 is a proactive improvement of function_graph tracer. 
> patch3 corresponds to II-4(functions under function_graph tracer).
> patch4 corresponds to II-5(leaf function).
> patch5, 6 and 7 correspond to II-1(slurping stack) and II-2(differences
> between x86 and arm64).
> 
> Each fix can be applied independently, but if patch5, 6 and 7 are
> acceptable, patch1 is not necessary because patch7 replaces a default
> stack tracer.
> 
> I tested the code with v4.3-rc3 + Jungseok's patch v3[4].
> 
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/354126.html
> [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/355920.html 
> [3] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/368003.html
> [4] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/371451.html

The [4] is not a valid patch. I hope the test has been going with the following one.

	http://www.kernelhub.org/?msg=841034&p=2

I will leave comments after playing with this series on top of my IRQ stack tree.

Best Regards
Jungseok Lee

Re: [PATCH v3 0/7] arm64: ftrace: fix incorrect output from stack tracer

[Jungseok Lee]

On Oct 8, 2015, at 11:45 PM, Jungseok Lee wrote:
> On Oct 8, 2015, at 7:01 PM, AKASHI Takahiro wrote:
> 
> Hi Akashi,
> 
>> This is the third patch series for fixing stack tracer on arm64.
>> The original issue was reported by Jungseok[1], and then I found more
>> issues[2].
>> (Steven, Jungseok, sorry for not replying to your comments directly.)
>> 
>> I address here all the issues and implement fixes described in [2] except
>> for interrupt-triggered problems, ie. II-3). Recent discussions[3] about
>> introducing a dedicated interrupt stack suggests that we may avoid walking
>> through from an interrupt stack to a process stack.
>> (So interrupt-stack patch is a prerequisite.)
>> 
>> Basically,
>> patch1 corresponds to the original issue.
>> patch2 is a proactive improvement of function_graph tracer. 
>> patch3 corresponds to II-4(functions under function_graph tracer).
>> patch4 corresponds to II-5(leaf function).
>> patch5, 6 and 7 correspond to II-1(slurping stack) and II-2(differences
>> between x86 and arm64).
>> 
>> Each fix can be applied independently, but if patch5, 6 and 7 are
>> acceptable, patch1 is not necessary because patch7 replaces a default
>> stack tracer.

I've used this series *without Patch1* for over a week.

It works well till now. I have not observed any issues, such as function graph
thing, of which I'm aware. Please note that I have not reviewed internals of
the analyzer and related logics carefully.

Best Regards
Jungseok Lee

>> I tested the code with v4.3-rc3 + Jungseok's patch v3[4].
>> 
>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/354126.html
>> [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/355920.html 
>> [3] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/368003.html
>> [4] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/371451.html
> 
> The [4] is not a valid patch. I hope the test has been going with the following one.
> 
> 	http://www.kernelhub.org/?msg=841034&p=2
> 
> I will leave comments after playing with this series on top of my IRQ stack tree.
> 
> Best Regards
> Jungseok Lee

[PATCH v3 0/7] arm64: ftrace: fix incorrect output from stack tracer

[Jungseok Lee]

On Oct 8, 2015, at 11:45 PM, Jungseok Lee wrote:
> On Oct 8, 2015, at 7:01 PM, AKASHI Takahiro wrote:
> 
> Hi Akashi,
> 
>> This is the third patch series for fixing stack tracer on arm64.
>> The original issue was reported by Jungseok[1], and then I found more
>> issues[2].
>> (Steven, Jungseok, sorry for not replying to your comments directly.)
>> 
>> I address here all the issues and implement fixes described in [2] except
>> for interrupt-triggered problems, ie. II-3). Recent discussions[3] about
>> introducing a dedicated interrupt stack suggests that we may avoid walking
>> through from an interrupt stack to a process stack.
>> (So interrupt-stack patch is a prerequisite.)
>> 
>> Basically,
>> patch1 corresponds to the original issue.
>> patch2 is a proactive improvement of function_graph tracer. 
>> patch3 corresponds to II-4(functions under function_graph tracer).
>> patch4 corresponds to II-5(leaf function).
>> patch5, 6 and 7 correspond to II-1(slurping stack) and II-2(differences
>> between x86 and arm64).
>> 
>> Each fix can be applied independently, but if patch5, 6 and 7 are
>> acceptable, patch1 is not necessary because patch7 replaces a default
>> stack tracer.

I've used this series *without Patch1* for over a week.

It works well till now. I have not observed any issues, such as function graph
thing, of which I'm aware. Please note that I have not reviewed internals of
the analyzer and related logics carefully.

Best Regards
Jungseok Lee

>> I tested the code with v4.3-rc3 + Jungseok's patch v3[4].
>> 
>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/354126.html
>> [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/355920.html 
>> [3] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/368003.html
>> [4] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/371451.html
> 
> The [4] is not a valid patch. I hope the test has been going with the following one.
> 
> 	http://www.kernelhub.org/?msg=841034&p=2
> 
> I will leave comments after playing with this series on top of my IRQ stack tree.
> 
> Best Regards
> Jungseok Lee

Re: [PATCH v3 0/7] arm64: ftrace: fix incorrect output from stack tracer

[Will Deacon]

On Thu, Oct 08, 2015 at 07:01:37PM +0900, AKASHI Takahiro wrote:
> This is the third patch series for fixing stack tracer on arm64.
> The original issue was reported by Jungseok[1], and then I found more
> issues[2].
> (Steven, Jungseok, sorry for not replying to your comments directly.)
> 
> I address here all the issues and implement fixes described in [2] except
> for interrupt-triggered problems, ie. II-3). Recent discussions[3] about
> introducing a dedicated interrupt stack suggests that we may avoid walking
> through from an interrupt stack to a process stack.
> (So interrupt-stack patch is a prerequisite.)
> 
> Basically,
> patch1 corresponds to the original issue.
> patch2 is a proactive improvement of function_graph tracer. 
> patch3 corresponds to II-4(functions under function_graph tracer).
> patch4 corresponds to II-5(leaf function).
> patch5, 6 and 7 correspond to II-1(slurping stack) and II-2(differences
> between x86 and arm64).
> 
> Each fix can be applied independently, but if patch5, 6 and 7 are
> acceptable, patch1 is not necessary because patch7 replaces a default
> stack tracer.

Given the comments and kbuild robot build errors, do you plan to post a
new version of this series?

Will

[PATCH v3 0/7] arm64: ftrace: fix incorrect output from stack tracer

[Will Deacon]

On Thu, Oct 08, 2015 at 07:01:37PM +0900, AKASHI Takahiro wrote:
> This is the third patch series for fixing stack tracer on arm64.
> The original issue was reported by Jungseok[1], and then I found more
> issues[2].
> (Steven, Jungseok, sorry for not replying to your comments directly.)
> 
> I address here all the issues and implement fixes described in [2] except
> for interrupt-triggered problems, ie. II-3). Recent discussions[3] about
> introducing a dedicated interrupt stack suggests that we may avoid walking
> through from an interrupt stack to a process stack.
> (So interrupt-stack patch is a prerequisite.)
> 
> Basically,
> patch1 corresponds to the original issue.
> patch2 is a proactive improvement of function_graph tracer. 
> patch3 corresponds to II-4(functions under function_graph tracer).
> patch4 corresponds to II-5(leaf function).
> patch5, 6 and 7 correspond to II-1(slurping stack) and II-2(differences
> between x86 and arm64).
> 
> Each fix can be applied independently, but if patch5, 6 and 7 are
> acceptable, patch1 is not necessary because patch7 replaces a default
> stack tracer.

Given the comments and kbuild robot build errors, do you plan to post a
new version of this series?

Will

Re: [PATCH v3 0/7] arm64: ftrace: fix incorrect output from stack tracer

[AKASHI Takahiro]

On 10/29/2015 12:23 AM, Will Deacon wrote:
> On Thu, Oct 08, 2015 at 07:01:37PM +0900, AKASHI Takahiro wrote:
>> This is the third patch series for fixing stack tracer on arm64.
>> The original issue was reported by Jungseok[1], and then I found more
>> issues[2].
>> (Steven, Jungseok, sorry for not replying to your comments directly.)
>>
>> I address here all the issues and implement fixes described in [2] except
>> for interrupt-triggered problems, ie. II-3). Recent discussions[3] about
>> introducing a dedicated interrupt stack suggests that we may avoid walking
>> through from an interrupt stack to a process stack.
>> (So interrupt-stack patch is a prerequisite.)
>>
>> Basically,
>> patch1 corresponds to the original issue.
>> patch2 is a proactive improvement of function_graph tracer.
>> patch3 corresponds to II-4(functions under function_graph tracer).
>> patch4 corresponds to II-5(leaf function).
>> patch5, 6 and 7 correspond to II-1(slurping stack) and II-2(differences
>> between x86 and arm64).
>>
>> Each fix can be applied independently, but if patch5, 6 and 7 are
>> acceptable, patch1 is not necessary because patch7 replaces a default
>> stack tracer.
>
> Given the comments and kbuild robot build errors, do you plan to post a
> new version of this series?

Yes, I do.
Do you have any comments that I should address before submitting a new version?
Apart from build errors, I admit that I should drop patch #4 ("arm64: ftrace:
allow for tracing leaf functions") just because I was somewhat confused.
I confirmed that "-pg" option actually disables omit-leaf-stack-frame.

Thanks,
-Takahiro AKASHI

> Will
>

[PATCH v3 0/7] arm64: ftrace: fix incorrect output from stack tracer

[AKASHI Takahiro]

On 10/29/2015 12:23 AM, Will Deacon wrote:
> On Thu, Oct 08, 2015 at 07:01:37PM +0900, AKASHI Takahiro wrote:
>> This is the third patch series for fixing stack tracer on arm64.
>> The original issue was reported by Jungseok[1], and then I found more
>> issues[2].
>> (Steven, Jungseok, sorry for not replying to your comments directly.)
>>
>> I address here all the issues and implement fixes described in [2] except
>> for interrupt-triggered problems, ie. II-3). Recent discussions[3] about
>> introducing a dedicated interrupt stack suggests that we may avoid walking
>> through from an interrupt stack to a process stack.
>> (So interrupt-stack patch is a prerequisite.)
>>
>> Basically,
>> patch1 corresponds to the original issue.
>> patch2 is a proactive improvement of function_graph tracer.
>> patch3 corresponds to II-4(functions under function_graph tracer).
>> patch4 corresponds to II-5(leaf function).
>> patch5, 6 and 7 correspond to II-1(slurping stack) and II-2(differences
>> between x86 and arm64).
>>
>> Each fix can be applied independently, but if patch5, 6 and 7 are
>> acceptable, patch1 is not necessary because patch7 replaces a default
>> stack tracer.
>
> Given the comments and kbuild robot build errors, do you plan to post a
> new version of this series?

Yes, I do.
Do you have any comments that I should address before submitting a new version?
Apart from build errors, I admit that I should drop patch #4 ("arm64: ftrace:
allow for tracing leaf functions") just because I was somewhat confused.
I confirmed that "-pg" option actually disables omit-leaf-stack-frame.

Thanks,
-Takahiro AKASHI

> Will
>