Re: [PATCH v11 1/5] arm64: Call stack_backtrace() only from within walk_stackframe()
From: Mark Rutland <mark.rutland@arm.com>
Date: 2021-11-30 15:14:08
Also in:
linux-arm-kernel, lkml
On Tue, Nov 23, 2021 at 01:37:19PM -0600, madvenka@linux.microsoft.com wrote:
From: "Madhavan T. Venkataraman" <redacted> 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.
quoted hunk ↗ jump to hunk
Use struct stackframe only within the unwind functions. Signed-off-by: Madhavan T. Venkataraman <redacted> --- 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.