Re: [patch V2 28/29] stacktrace: Provide common infrastructure

Peter Zijlstra
2019-04-19T09:07:56Z

On Fri, Apr 19, 2019 at 10:32:30AM +0200, Thomas Gleixner wrote:
> On Fri, 19 Apr 2019, Peter Zijlstra wrote:
> > On Thu, Apr 18, 2019 at 10:41:47AM +0200, Thomas Gleixner wrote:
> >
> > > +typedef bool (*stack_trace_consume_fn)(void *cookie, unsigned long addr,
> > > + bool reliable);
> >
> > > +void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
> > > + struct task_struct *task, struct pt_regs *regs);
> > > +int arch_stack_walk_reliable(stack_trace_consume_fn consume_entry, void *cookie,
> > > + struct task_struct *task);
> >
> > This bugs me a little; ideally the _reliable() thing would not exists.
> >
> > Thomas said that the existing __save_stack_trace_reliable() is different
> > enough for the unification to be non-trivial, but maybe Josh can help
> > out?
> >
> > >From what I can see the biggest significant differences are:
> >
> > - it looks at the regs sets on the stack and for FP bails early
> > - bails for khreads and idle (after it does all the hard work!?!)
> >
> > The first (FP checking for exceptions) should probably be reflected in
> > consume_fn(.reliable) anyway -- although that would mean a lot of extra
> > '?' entries where there are none today.
> >
> > And the second (KTHREAD/IDLE) is something that the generic code can
> > easily do before calling into the arch unwinder.
>
> And looking at the powerpc version of it, that has even more interesting
> extra checks in that function.

Right, but not fundamentally different from determining @reliable I
think.

Anyway, it would be good if someone knowledgable could have a look at
this.