Re: [PATCH 01/16 v3] function_graph: Convert ret_stack to a series of longs

[Steven Rostedt]

On Tue, 28 May 2019 05:50:43 -0400
Joel Fernandes <joel@xxxxxxxxxxxxxxxxx> wrote:

> On Fri, May 24, 2019 at 11:16:34PM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (VMware)" <rostedt@xxxxxxxxxxx>
> > 
> > In order to make it possible to have multiple callbacks registered with the
> > function_graph tracer, the retstack needs to be converted from an array of
> > ftrace_ret_stack structures to an array of longs. This will allow to store
> > the list of callbacks on the stack for the return side of the functions.
> > 
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>
> > ---
> >  include/linux/sched.h |   2 +-
> >  kernel/trace/fgraph.c | 124 ++++++++++++++++++++++++------------------
> >  2 files changed, 71 insertions(+), 55 deletions(-)
> > 
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index 11837410690f..1850d8a3c3f0 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -1113,7 +1113,7 @@ struct task_struct {
> >  	int				curr_ret_depth;
> >  
> >  	/* Stack of return addresses for return function tracing: */
> > -	struct ftrace_ret_stack		*ret_stack;
> > +	unsigned long			*ret_stack;  
> 
> Can it be converted to an array of unsigned int so the shadown stack space
> can be better used? This way stack overflow chance is lesser if there are too
> many registered fgraph users and the function call depth is too deep.
> AFAICS from patch 5/13, you need only 32-bits for the ftrace_ret_stack
> offset, type and array index, so the upper 32-bit would not be used.
> 

We can look to improve that later on. This is complex enough and I kept
some features (like 4 byte minimum) out of this series to keep the
complexity down. I believe there are some archs that require 64bit
aligned access for 64 bit words and pointers. And the retstack
structure still has longs on it. If we need to adapt to making sure we
are aligned, I rather keep that complexity out for now.

That said, I just did a git grep HAVE_64BIT_ALIGNED_ACCESS and only
found the kconfig where it is defined and the ring buffer code that
deals with it. We recently removed a bunch of archs, and it could very
well be that this requirement no longer exists.

Regardless, I've been testing this code quite heavily, and changing the
stack from moving up to moving down already put me behind. I'd like to
pull this code into linux-next soon. Converting to ints can be done for
the release after we get this in.

Thanks for reviewing!

-- Steve