On 11/30/21 9:05 AM, Mark Rutland wrote:
> 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.
>
Will do this in the next version.
>> 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().
>
This is just to simplify things when we eventually add arch_stack_walk_reliable().
That is all. All of the unwinding is done by a single unwinding function and
there are two consumers of that unwinding function - arch_stack_walk() and
arch_stack_walk_reliable().
> 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.
>
Will do this in the next version.
Thanks,
Madhavan
