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.
