Re: [PATCH 3/3] arm64: stacktrace: Convert to ARCH_STACKWALK

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1967 bytes --]

On Thu, Jul 16, 2020 at 01:56:13PM +0200, Miroslav Benes wrote:
> On Wed, 15 Jul 2020, Mark Brown wrote:

> > -void save_stack_trace(struct stack_trace *trace)
> > -{
> > -	__save_stack_trace(current, trace, 0);
> > +	walk_stackframe(task, &frame, consume_entry, cookie);
> >  }

> just an idea for further improvement (and it might be a matter of taste). 

Yeah, there's some more stuff that can be done - the reason I'm looking
at this code is to do reliable stack trace which is going to require at
least some changes to the actual unwinder, this just seemed like a
useful block moving things forwards in itself and I particularly wanted
feedback on patch 1.

> Wouldn't it be slightly better to do one more step and define "struct 
> unwind_state" instead of "struct stackframe" and also some iterator for 
> the unwinding and use that right in new arch_stack_walk() instead of 
> walk_stackframe()? I mean, take the unbounded loop, "inline" it to 
> arch_stack_walk() and replace the loop with the iterator. The body of the 
> iterator would call to unwind_frame() and consume_entry() and that's it. 
> It would make arm64 implementation very similar to x86 and s390 and thus 
> easier to follow when one switches between architectures all the time.

That's definitely on the radar, the unwinding stuff needs other changes
for the reliable stack trace (if nothing else we need to distinguish
between "errors" due to reaching the bottom of the stack and errors due
to bogosity) which so far looked sensible to bundle up together.

> Tangential to this patch, but another idea for improvement is in 
> unwind_frame(). If I am not missing something, everything in 
> CONFIG_FUNCTION_GRAPH_TRACER could be replaced by a simple call to 
> ftrace_graph_ret_addr(). Again see for example unwind_next_frame() in
> arch/s390/kernel/unwind_bc.c (x86 has it too).

Yes, I'd noticed some divergence there and was going to look into it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]