Messages in this thread | | | Subject | Re: [PATCH v10 04/11] arm64: Make return_address() use arch_stack_walk() | From | "Madhavan T. Venkataraman" <> | Date | Sat, 23 Oct 2021 07:51:50 -0500 |
| |
On 10/22/21 1:51 PM, Mark Rutland wrote: > On Thu, Oct 14, 2021 at 09:58:40PM -0500, madvenka@linux.microsoft.com wrote: >> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> >> >> Currently, return_address() in ARM64 code walks the stack using >> start_backtrace() and walk_stackframe(). Make it use arch_stack_walk() >> instead. This makes maintenance easier. >> >> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com> >> --- >> arch/arm64/kernel/return_address.c | 6 +----- >> 1 file changed, 1 insertion(+), 5 deletions(-) >> >> diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c >> index a6d18755652f..92a0f4d434e4 100644 >> --- a/arch/arm64/kernel/return_address.c >> +++ b/arch/arm64/kernel/return_address.c >> @@ -35,15 +35,11 @@ NOKPROBE_SYMBOL(save_return_addr); >> void *return_address(unsigned int level) >> { >> struct return_address_data data; >> - struct stackframe frame; >> >> data.level = level + 2; >> data.addr = NULL; >> >> - start_backtrace(&frame, >> - (unsigned long)__builtin_frame_address(0), >> - (unsigned long)return_address); >> - walk_stackframe(current, &frame, save_return_addr, &data); >> + arch_stack_walk(save_return_addr, &data, current, NULL); > > This looks equivalent to me. Previously the arguments to > start_backtrace() meant that walk_stackframe would report > return_address(), then the caller of return_address(), and so on. As > arch_stack_walk() starts from its immediate caller (i.e. > return_address()), that should result in the same trace. > > It would be nice if we could note something to that effect in the commit > message. >
Will do.
> I had a play with ftrace, which uses return_address(), and that all > looks sound. >
Thanks a lot!
>> >> if (!data.level) >> return data.addr; > > The end of this function currently does: > > if (!data.level) > return data.addr; > else > return NULL; > > ... but since we initialize data.addr to NULL, and save_return_addr() > only writes to data.addr when called at the correct level, we can > simplify that to: > > return data.addr; >
OK. I will make this change.
> Regardles of that cleanup: > > Reviewed-by: Mark Rutland <mark.rutland@arm.com> > Tested-by: Mark Rutland <mark.rutland@arm.com> >
Thanks a lot!
> I'll continue reviewing the series next week. >
Great!
Madhavan
| |