Re: [PATCH v3] ftrace: document basic ftracer/ftracer graph needs

[Frederic Weisbecker <fweisbec@gmail.com>]

On Sat, Jun 13, 2009 at 09:52:18PM -0400, Mike Frysinger wrote:
> On Sat, Jun 13, 2009 at 21:24, Frederic Weisbecker wrote:
> > On Sat, Jun 13, 2009 at 08:21:53PM -0400, Mike Frysinger wrote:
> >> +HAVE_FUNCTION_GRAPH_TRACER
> >
> > Should be HAVE_FUNCTION_TRACER, right?
> 
> yes
> 
> > There is one crucial missing thing, I mean "save all state needed by the ABI"
> > can be more detailed. do_trace _must_
> > save the scratch and argument registers to the stack because
> > the traced function may have parameters passed by registers,
> > initialized things on scratch registers, and this state
> > must be left intact before calling ftrace_trace_function()
> 
> parameters passed by registers, yes, but as for scratch registers,
> that depends on the toolchain and where the mcount invocation occurs.
> if it's before the function prolog, then no, it doesnt need to worry
> about any scratch registers.  if it's after, then yes, it probably
> needs to worry about those things.  but this is why i have a paragraph
> saying "go read your abi documentation" and review glibc.



Ok, then in this case, just talk about it as a constraint that
may be or not handled by the arch specific mcount.

It's really a prerequisite to check on every archs so I think
it's worth showing this mandatory stage in the recipe.


 
> >> +For information on how to implement prepare_ftrace_return(), simply look at
> >> +the x86 version.  The only architecture-specific piece in it is the setup of
> >> +the fault recovery table (the asm(...) code).  The rest should be the same
> >> +across architectures.
> >> +
> >> +Here is the pseudo code for the new return_to_handler assembly function.  Note
> >> +that the ABI that applies here is different from what applies to the mcount
> >> +code.  Here you are returning from a function, so you might be able to skimp
> >> +on things saved/restored.
> >
> > It would be nice to add details about that, especially about a constant rule:
> > return_to_handler must save/restore the return value of the current exiting
> > function around ftrace_return_to_handler call.
> >
> > And this return value might be stored in more than one register for
> > 64 bits return values.
> >
> > But we don't need to save/restore the other scratch  registers because the
> > traced function is exiting and won't need anymore values stored in them.
> 
> i'm not familiar with other architectures and crazy shit that might go
> down here which is why i kind of skimped on details.  for the Blackfin
> port, i know what i have to do -- just save/restore the return
> registers (r0 for 32bits, +r1 for 64bits, +p0 for >64bits).  but i
> purposefully tried to avoid ABI details because i dont want this
> turning into "on <arch>, do <whole bunch of details>, on <arch2>, do
> <whole bunch of details>, ....".



That's not what I'm suggesting.
As I explained above, it's about a generic and cross arch constraint.

The details of implementation are only a concern for the developer.


> 
> the scratch register is more because the exit code is coming after the
> function epilog rather than "exiting it" ...


Yeah, that's what I meant by "exiting" :)


> 
> i dont mind adding tips, but the last thing i want is people
> complaining that they did what the docs said and now things crashed
> because they didnt fully grasp the "it's your ABI, so it's your
> problem".



I'm only talking about generic rules here. Every ABI has conventions
about calls, scratch registers, etc...

But here we are using tracing code in very fragile paths that have
common constraints on every archs. Those constraints concern
common properties of every CPU, the implementation which implement
these constraints is only a concern of the arch developer.


> > Also, we had some problems with return_to_handler in x86-64.
> > We needed to allocate a large stack room (0x80 bytes) before calling
> > ftrace_return_to_handler(). The funny thing is that we still don't know
> > why we needed to do that, but omitting that resulted in crashes :-)
> 
> without knowing anything about x86-64 treating of the stack, it does
> seem weird.  with the Blackfin arch, the called function is
> responsible for allocating its own space.


That's also what does x86, really that's a mystery..


> >> +HAVE_FTRACE_SYSCALLS
> >> +---------------------
> >> +
> >> +<details to be filled>
> >
> > This part doesn't need more for now because it may change soon
> > since the syscall tracing is currently reworked.
> > We'll fill it once it reaches a more established state.
> >
> >> +HAVE_DYNAMIC_FTRACE
> >> +---------------------
> >> +
> >> +<details to be filled>
> >
> > But this part is important :)
> 
> i filled in what i could reverse engineer ... and these two bits
> looked way more complicated than was worth me trying to figure out.
> these are what i'd be interested in next though.
> -mike


Never mind, this documentation can be filled through several iterations.
This patch already provides very useful explanations.

Thanks,
Frederic.