Re: [PATCH 1/6] LoongArch: Get frame info in unwind_start when regs is not supported

[Jinyang He]

On 2022-12-15 13:24, Qing Zhang wrote:

> Hi, Jinyang
>
> On 2022/12/15 下午12:01, Jinyang He wrote:
> > At unwind_start, it is better to try to get its frame info even we not
> > support regs rather than get them outside. So that we can simply use
> > unwind_{start, next_frame, done} outside.
> >
> > Signed-off-by: Jinyang He <hejinyang@xxxxxxxxxxx>
> > ---
> >   arch/loongarch/kernel/process.c         | 12 +++---------
> >   arch/loongarch/kernel/unwind_guess.c    |  6 ++++++
> >   arch/loongarch/kernel/unwind_prologue.c | 16 +++++++++++++---
> >   3 files changed, 22 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/loongarch/kernel/process.c 
> > b/arch/loongarch/kernel/process.c
> > index 502b8b950057..6ef45174ad35 100644
> > --- a/arch/loongarch/kernel/process.c
> > +++ b/arch/loongarch/kernel/process.c
> > @@ -197,20 +197,14 @@ int copy_thread(struct task_struct *p, const 
> > struct kernel_clone_args *args)
> >     unsigned long __get_wchan(struct task_struct *task)
> >   {
> > -    unsigned long pc;
> > +    unsigned long pc = 0;
> >       struct unwind_state state;
> >         if (!try_get_task_stack(task))
> >           return 0;
> >   -    unwind_start(&state, task, NULL);
> > -    state.sp = thread_saved_fp(task);
> > -    get_stack_info(state.sp, state.task, &state.stack_info);
> > -    state.pc = thread_saved_ra(task);
> > -#ifdef CONFIG_UNWINDER_PROLOGUE
> > -    state.type = UNWINDER_PROLOGUE;
> > -#endif
> > -    for (; !unwind_done(&state); unwind_next_frame(&state)) {
> > +    for (unwind_start(&state, task, NULL); !unwind_done(&state);
> > +         unwind_next_frame(&state)) {
> >           pc = unwind_get_return_address(&state);
> >           if (!pc)
> >               break;
> > diff --git a/arch/loongarch/kernel/unwind_guess.c 
> > b/arch/loongarch/kernel/unwind_guess.c
> > index e2d2e4f3001f..e03864511582 100644
> > --- a/arch/loongarch/kernel/unwind_guess.c
> > +++ b/arch/loongarch/kernel/unwind_guess.c
> > @@ -26,6 +26,12 @@ void unwind_start(struct unwind_state *state, 
> > struct task_struct *task,
> >       if (regs) {
> >           state->sp = regs->regs[3];
> >           state->pc = regs->csr_era;
> > +    } else if (task == current) {
> > +        state->sp = (unsigned long)__builtin_frame_address(0);
> > +        state->pc = (unsigned long)__builtin_return_address(0);
> > +    } else {
> > +        state->sp = thread_saved_fp(task);
> > +        state->pc = thread_saved_ra(task);
> The case for tasks is already handled in stacktrace.c, which is 
> reusable code:
> void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
>              struct task_struct *task, struct pt_regs *regs)
> {
> ...
>
>     if (task == current) {
>         regs->regs[3] = (unsigned long)__builtin_frame_address(0);
>         regs->csr_era = (unsigned long)__builtin_return_address(0);
>     } else {
>         regs->regs[3] = thread_saved_fp(task);
>         regs->csr_era = thread_saved_ra(task);
>     }
> ...
>     for (unwind_start(&state, task, regs);
>           !unwind_done(&state); unwind_next_frame(&state)) {
>         addr = unwind_get_return_address(&state);
> ...
> }
>
> >         state->task = task;
> > diff --git a/arch/loongarch/kernel/unwind_prologue.c 
> > b/arch/loongarch/kernel/unwind_prologue.c
> > index 0f8d1451ebb8..9d51ea37782e 100644
> > --- a/arch/loongarch/kernel/unwind_prologue.c
> > +++ b/arch/loongarch/kernel/unwind_prologue.c
> > @@ -141,12 +141,22 @@ void unwind_start(struct unwind_state *state, 
> > struct task_struct *task,
> >               struct pt_regs *regs)
> >   {
> >       memset(state, 0, sizeof(*state));
> > +    state->type = UNWINDER_PROLOGUE;
> >   -    if (regs && __kernel_text_address(regs->csr_era)) {
> > -        state->pc = regs->csr_era;
> > +    if (regs) {
> >           state->sp = regs->regs[3];
> > +        state->pc = regs->csr_era;
> >           state->ra = regs->regs[1];
> > -        state->type = UNWINDER_PROLOGUE;
> > +        if (!__kernel_text_address(state->pc))
> > +            state->type = UNWINDER_GUESS;
> > +    } else if (task == current) {
> > +        state->sp = (unsigned long)__builtin_frame_address(0);
> > +        state->pc = (unsigned long)__builtin_return_address(0);
> > +        state->ra = 0;
> > +    } else {
> > +        state->sp = thread_saved_fp(task);
> > +        state->pc = thread_saved_ra(task);
> > +        state->ra = 0;
> >       }
> also...
> >       state->task = task;
That looks similar, but stacktrace actually provides the regs parameter.
Unwind_{start, next_frame, get_return_address} is API to others, such the
GPL module. Btw, if we want to drop the redundant codes in arch_stack_walk
and keep same result, use "cookie->skip += 1", I think, but beyond this 
series.


Thanks,

Jinyang