[PATCH] arm64: stacktrace: Move start_backtrace() out of the header

[PATCH] arm64: stacktrace: Move start_backtrace() out of the header

[Mark Brown]

Currently start_backtrace() is a static inline function in the header.
Since it really shouldn't be sufficiently performance critical that we
actually need to have it inlined move it into a C file, this will save
anyone else scratching their head about why it is defined in the header.
As far as I can see it's only there because it was factored out of the
various callers.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/stacktrace.h | 24 ++----------------------
 arch/arm64/kernel/stacktrace.c      | 24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index eb29b1fe8255..4b33ca620679 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -148,27 +148,7 @@ static inline bool on_accessible_stack(const struct task_struct *tsk,
 	return false;
 }
 
-static inline void start_backtrace(struct stackframe *frame,
-				   unsigned long fp, unsigned long pc)
-{
-	frame->fp = fp;
-	frame->pc = pc;
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-	frame->graph = 0;
-#endif
-
-	/*
-	 * Prime the first unwind.
-	 *
-	 * In unwind_frame() we'll check that the FP points to a valid stack,
-	 * which can't be STACK_TYPE_UNKNOWN, and the first unwind will be
-	 * treated as a transition to whichever stack that happens to be. The
-	 * prev_fp value won't be used, but we set it to 0 such that it is
-	 * definitely not an accessible stack address.
-	 */
-	bitmap_zero(frame->stacks_done, __NR_STACK_TYPES);
-	frame->prev_fp = 0;
-	frame->prev_type = STACK_TYPE_UNKNOWN;
-}
+void start_backtrace(struct stackframe *frame, unsigned long fp,
+		     unsigned long pc);
 
 #endif	/* __ASM_STACKTRACE_H */
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 7337506d6a7f..4c889bd098af 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -35,6 +35,30 @@
  *	add	sp, sp, #0x10
  */
 
+
+void start_backtrace(struct stackframe *frame, unsigned long fp,
+		     unsigned long pc)
+{
+	frame->fp = fp;
+	frame->pc = pc;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	frame->graph = 0;
+#endif
+
+	/*
+	 * Prime the first unwind.
+	 *
+	 * In unwind_frame() we'll check that the FP points to a valid stack,
+	 * which can't be STACK_TYPE_UNKNOWN, and the first unwind will be
+	 * treated as a transition to whichever stack that happens to be. The
+	 * prev_fp value won't be used, but we set it to 0 such that it is
+	 * definitely not an accessible stack address.
+	 */
+	bitmap_zero(frame->stacks_done, __NR_STACK_TYPES);
+	frame->prev_fp = 0;
+	frame->prev_type = STACK_TYPE_UNKNOWN;
+}
+
 /*
  * Unwind from one frame record (A) to the next frame record (B).
  *
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: stacktrace: Move start_backtrace() out of the header

[Mark Rutland]

On Fri, Mar 19, 2021 at 05:40:22PM +0000, Mark Brown wrote:
> Currently start_backtrace() is a static inline function in the header.
> Since it really shouldn't be sufficiently performance critical that we
> actually need to have it inlined move it into a C file, this will save
> anyone else scratching their head about why it is defined in the header.
> As far as I can see it's only there because it was factored out of the
> various callers.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

Given keeping this close to unwind_frame() makes it easier to see the
whole story at once:

Acked-by: Mark Rutland <mark.rutland@arm.com>

It'd be nice to see users of this all moved over to arch_stack_walk(),
so that we can make this static along with walk_stackframe(), and have
all stack walks using the same interface.

Thanks,
Mark.

> ---
>  arch/arm64/include/asm/stacktrace.h | 24 ++----------------------
>  arch/arm64/kernel/stacktrace.c      | 24 ++++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index eb29b1fe8255..4b33ca620679 100644
> --- a/arch/arm64/include/asm/stacktrace.h
> +++ b/arch/arm64/include/asm/stacktrace.h
> @@ -148,27 +148,7 @@ static inline bool on_accessible_stack(const struct task_struct *tsk,
>  	return false;
>  }
>  
> -static inline void start_backtrace(struct stackframe *frame,
> -				   unsigned long fp, unsigned long pc)
> -{
> -	frame->fp = fp;
> -	frame->pc = pc;
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> -	frame->graph = 0;
> -#endif
> -
> -	/*
> -	 * Prime the first unwind.
> -	 *
> -	 * In unwind_frame() we'll check that the FP points to a valid stack,
> -	 * which can't be STACK_TYPE_UNKNOWN, and the first unwind will be
> -	 * treated as a transition to whichever stack that happens to be. The
> -	 * prev_fp value won't be used, but we set it to 0 such that it is
> -	 * definitely not an accessible stack address.
> -	 */
> -	bitmap_zero(frame->stacks_done, __NR_STACK_TYPES);
> -	frame->prev_fp = 0;
> -	frame->prev_type = STACK_TYPE_UNKNOWN;
> -}
> +void start_backtrace(struct stackframe *frame, unsigned long fp,
> +		     unsigned long pc);
>  
>  #endif	/* __ASM_STACKTRACE_H */
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 7337506d6a7f..4c889bd098af 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -35,6 +35,30 @@
>   *	add	sp, sp, #0x10
>   */
>  
> +
> +void start_backtrace(struct stackframe *frame, unsigned long fp,
> +		     unsigned long pc)
> +{
> +	frame->fp = fp;
> +	frame->pc = pc;
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +	frame->graph = 0;
> +#endif
> +
> +	/*
> +	 * Prime the first unwind.
> +	 *
> +	 * In unwind_frame() we'll check that the FP points to a valid stack,
> +	 * which can't be STACK_TYPE_UNKNOWN, and the first unwind will be
> +	 * treated as a transition to whichever stack that happens to be. The
> +	 * prev_fp value won't be used, but we set it to 0 such that it is
> +	 * definitely not an accessible stack address.
> +	 */
> +	bitmap_zero(frame->stacks_done, __NR_STACK_TYPES);
> +	frame->prev_fp = 0;
> +	frame->prev_type = STACK_TYPE_UNKNOWN;
> +}
> +
>  /*
>   * Unwind from one frame record (A) to the next frame record (B).
>   *
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: stacktrace: Move start_backtrace() out of the header

[Catalin Marinas]

On Fri, 19 Mar 2021 17:40:22 +0000, Mark Brown wrote:
> Currently start_backtrace() is a static inline function in the header.
> Since it really shouldn't be sufficiently performance critical that we
> actually need to have it inlined move it into a C file, this will save
> anyone else scratching their head about why it is defined in the header.
> As far as I can see it's only there because it was factored out of the
> various callers.

Applied to arm64 (for-next/misc), thanks!

[1/1] arm64: stacktrace: Move start_backtrace() out of the header
      https://git.kernel.org/arm64/c/b07f3499661c

-- 
Catalin


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel