Re: [RFC PATCH 1/6] trace/stack: Move code to save the stack trace into a separate function

[Naveen N. Rao]

Steven Rostedt wrote:
> On Fri, 21 May 2021 12:18:36 +0530
> "Naveen N. Rao" <naveen.n.rao@xxxxxxxxxxxxxxxxxx> wrote:
>
> > In preparation to add support for stack tracer to powerpc, move code to
> > save stack trace and to calculate the frame sizes into a separate weak
> > function. Also provide access to some of the data structures used by the
> > stack trace code so that architectures can update those.
> >
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@xxxxxxxxxxxxxxxxxx>
> > ---
> >  include/linux/ftrace.h     |  8 ++++
> >  kernel/trace/trace_stack.c | 98 ++++++++++++++++++++------------------
> >  2 files changed, 60 insertions(+), 46 deletions(-)
> >
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index a69f363b61bf73..8263427379f05c 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -368,10 +368,18 @@ static inline void arch_ftrace_set_direct_caller(struct pt_regs *regs,
> >  
> >  #ifdef CONFIG_STACK_TRACER
> >  
> > +#define STACK_TRACE_ENTRIES 500
> > +
> > +extern unsigned long stack_dump_trace[STACK_TRACE_ENTRIES];
> > +extern unsigned stack_trace_index[STACK_TRACE_ENTRIES];
> > +extern unsigned int stack_trace_nr_entries;
> > +extern unsigned long stack_trace_max_size;
> >  extern int stack_tracer_enabled;
> >  
> >  int stack_trace_sysctl(struct ctl_table *table, int write, void *buffer,
> >  		       size_t *lenp, loff_t *ppos);
> > +void stack_get_trace(unsigned long traced_ip, unsigned long *stack_ref,
> > +					unsigned long stack_size, int *tracer_frame);
> >  
> >  /* DO NOT MODIFY THIS VARIABLE DIRECTLY! */
> >  DECLARE_PER_CPU(int, disable_stack_tracer);
> > diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
> > index 63c28504205162..5b63dbd37c8c25 100644
> > --- a/kernel/trace/trace_stack.c
> > +++ b/kernel/trace/trace_stack.c
> > @@ -19,13 +19,11 @@
> >  
> >  #include "trace.h"
> >  
> > -#define STACK_TRACE_ENTRIES 500
> > +unsigned long stack_dump_trace[STACK_TRACE_ENTRIES];
> > +unsigned stack_trace_index[STACK_TRACE_ENTRIES];
> >  
> > -static unsigned long stack_dump_trace[STACK_TRACE_ENTRIES];
> > -static unsigned stack_trace_index[STACK_TRACE_ENTRIES];
> > -
> > -static unsigned int stack_trace_nr_entries;
> > -static unsigned long stack_trace_max_size;
> > +unsigned int stack_trace_nr_entries;
> > +unsigned long stack_trace_max_size;
> >  static arch_spinlock_t stack_trace_max_lock =
> >  	(arch_spinlock_t)__ARCH_SPIN_LOCK_UNLOCKED;
> >  
> > @@ -152,49 +150,19 @@ static void print_max_stack(void)
> >   * Although the entry function is not displayed, the first function (sys_foo)
> >   * will still include the stack size of it.
> >   */
> > -static void check_stack(unsigned long ip, unsigned long *stack)
>
> I just got back from PTO and have a ton of other obligations to attend
> to before I can dig deeper into this.

Thanks for the heads up.

> I'm not opposed to this change,
> but the stack_tracer has not been getting the love that it deserves and
> I think you hit one of the issues that needs to be addressed.

Sure. I started off by just updating arch_stack_walk() to return the 
traced function, but soon realized that the frame size determination can 
be made more robust on powerpc due to the ABI.

> I'm not
> sure this is a PPC only issue, and would like to see if I can get more
> time (or someone else can) to reevaluate the way stack tracer works,
> and see if it can be made a bit more robust.

Sure. If you have specific issues to be looked at, please do elaborate.

It seems to be working fine otherwise. The one limitation though is down 
to how ftrace works on powerpc -- the mcount call is before a function 
sets up its own stackframe. Due to this, we won't ever be able to 
account for the stackframe from a leaf function -- but, that's a fairly 
minor limitation.


Thanks,
Naveen