Re: [PATCH v11 1/5] arm64: Call stack_backtrace() only from within walk_stackframe()

[Mark Rutland]

On Tue, Nov 23, 2021 at 01:37:19PM -0600, madvenka@xxxxxxxxxxxxxxxxxxx wrote:
> From: "Madhavan T. Venkataraman" <madvenka@xxxxxxxxxxxxxxxxxxx>
> 
> Currently, arch_stack_walk() calls start_backtrace() and walk_stackframe()
> separately. There is no need to do that. Instead, call start_backtrace()
> from within walk_stackframe(). In other words, walk_stackframe() is the only
> unwind function a consumer needs to call.
> 
> Currently, the only consumer is arch_stack_walk(). In the future,
> arch_stack_walk_reliable() will be another consumer.
> 
> Currently, there is a check for a NULL task in unwind_frame(). It is not
> needed since all current consumers pass a non-NULL task.

Can you split the NULL check change into a preparatory patch? That change is
fine in isolation (and easier to review/ack), and it's nicer for future
bisection to not group that with unrelated changes.

> Use struct stackframe only within the unwind functions.
> 
> Signed-off-by: Madhavan T. Venkataraman <madvenka@xxxxxxxxxxxxxxxxxxx>
> ---
>  arch/arm64/kernel/stacktrace.c | 41 ++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 0fb58fed54cb..7217c4f63ef7 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -69,9 +69,6 @@ static int notrace unwind_frame(struct task_struct *tsk,
>  	unsigned long fp = frame->fp;
>  	struct stack_info info;
>  
> -	if (!tsk)
> -		tsk = current;
> -
>  	/* Final frame; nothing to unwind */
>  	if (fp == (unsigned long)task_pt_regs(tsk)->stackframe)
>  		return -ENOENT;
> @@ -143,15 +140,19 @@ static int notrace unwind_frame(struct task_struct *tsk,
>  NOKPROBE_SYMBOL(unwind_frame);
>  
>  static void notrace walk_stackframe(struct task_struct *tsk,
> -				    struct stackframe *frame,
> +				    unsigned long fp, unsigned long pc,
>  				    bool (*fn)(void *, unsigned long), void *data)
>  {
> +	struct stackframe frame;
> +
> +	start_backtrace(&frame, fp, pc);
> +
>  	while (1) {
>  		int ret;
>  
> -		if (!fn(data, frame->pc))
> +		if (!fn(data, frame.pc))
>  			break;
> -		ret = unwind_frame(tsk, frame);
> +		ret = unwind_frame(tsk, &frame);
>  		if (ret < 0)
>  			break;
>  	}
> @@ -195,17 +196,19 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
>  			      void *cookie, struct task_struct *task,
>  			      struct pt_regs *regs)
>  {
> -	struct stackframe frame;
> -
> -	if (regs)
> -		start_backtrace(&frame, regs->regs[29], regs->pc);
> -	else if (task == current)
> -		start_backtrace(&frame,
> -				(unsigned long)__builtin_frame_address(1),
> -				(unsigned long)__builtin_return_address(0));
> -	else
> -		start_backtrace(&frame, thread_saved_fp(task),
> -				thread_saved_pc(task));
> -
> -	walk_stackframe(task, &frame, consume_entry, cookie);
> +	unsigned long fp, pc;
> +
> +	if (regs) {
> +		fp = regs->regs[29];
> +		pc = regs->pc;
> +	} else if (task == current) {
> +		/* Skip arch_stack_walk() in the stack trace. */
> +		fp = (unsigned long)__builtin_frame_address(1);
> +		pc = (unsigned long)__builtin_return_address(0);
> +	} else {
> +		/* Caller guarantees that the task is not running. */
> +		fp = thread_saved_fp(task);
> +		pc = thread_saved_pc(task);
> +	}
> +	walk_stackframe(task, fp, pc, consume_entry, cookie);

I'd prefer to leave this as-is. The new and old structure are largely
equivalent, so we haven't made this any simpler, but we have added more
arguments to walk_stackframe().

One thing I *would* like to do is move tsk into strcut stackframe, so we only
need to pass that around, which'll make it easier to refactor the core unwind
logic.

Thanks,
Mark.