2012-06-12 22:54:29

by Steven Rostedt

[permalink] [raw]
Subject: [RFC][PATCH 00/13 v2] kprobes/ftrace: Making ftrace usable for kprobes


This is an RFC of patches that allow ftrace to be used directly by
kprobes.

The first set of patches modify the function tracer to:

1) have ftrace_ops passed to all functions
2) allow regs to be passed to all functions when requested

The first change is not required by kprobes, but is a nice feature
that people have asked for. You can now pass data to different
functions. Well, you can pass the ftrace_ops that registered the function
such that if two ftrace_ops register the same callback, the call back
can do different things depending on what registered it.

The second change adds a second mcount trampoline. That is,
if you request to have pt_regs returned, it will call a different
function that saves those registers. If nothing asks for regs, then
the old way is performed, and there's no slow down in the performance,
as not many functions would ever require regs passed to it.

If an arch supports passing of ftrace ops, it must also pass regs.
But it does not need to support passing regs. By default
an arch can just pass NULL. If it supports regs, then
it can allow tools like kprobes to ask for regs. Otherwise the
regs parameter should just be NULL.

The second set is Masami's patches ported on top of these changes.

This is v2, and I've cleaned the patches up a bit and added a lot more
comments. This is a more serious RFC as this is the current way I plan
on pushing it to mainline. But I'm posting first in case someone spots
something that I missed. I'll also be doing more testing on it and if
someting comes up I'll obviously will fix it before pushing.

Some change since v1:

Added fix for undefined ftrace_location() when compiling with !DYNAMIC_FTRACE.

Added missing regs passed for x86_64 (segment regs, and r11).

Saved flags for when regs are being saved. The cmp in the mcount trampoline
that checks if function tracing is disabled will save flags before
the compare, and restore flags before returning back to the function.

Decided that passing partial regs is a bad idea. Either the arch should
pass all regs or NULL. Even though partial regs are saved for calling the
function, it does't make sense to give that to the callback. By making
regs be full pt_regs or NULL, then the callbacks can simply check if
regs is NULL and if it isn't it can do more functionality, but if it
is NULL then it just does limited work. Having 'partial regs' would just
complicate that for no good reason. If in the future we want partial regs,
we can add another flag to request it.

Note, code can still check if ARCH_SUPPORTS_FTRACE_SAVE_REGS is defined,
and if it is, if the ftrace_ops specifies the SAVE_REGS flag, then the
arch will pass the full pt_regs to it.

These patches are in git and can be found at:

git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
rfc/kprobes/ftrace-v5

Head SHA1: 3cbbedd93e39d1394251825775d913be10d16e4e


Masami Hiramatsu (5):
ftrace: add ftrace_set_filter_ip() for address based filter
kprobes: cleanup to separate probe-able check
kprobes: Move locks into appropriate functions
kprobes: introduce ftrace based optimization
kprobes/x86: ftrace based optimization for x86

Steven Rostedt (8):
ftrace: Pass ftrace_ops as third parameter to function trace callback
ftrace: Consolidate arch dependent functions with 'list' function
ftrace: Return pt_regs to function trace callback
ftrace/x86_32: Push ftrace_ops in as 3rd parameter to function tracer
ftrace/x86: Add separate function to save regs
ftrace/x86: Add save_regs for i386 function calls
kprobes: Inverse taking of module_mutex with kprobe_mutex
ftrace: Make ftrace_location() a nop on !DYNAMIC_FTRACE

----
arch/x86/include/asm/ftrace.h | 47 ++++---
arch/x86/include/asm/kprobes.h | 1 +
arch/x86/kernel/entry_32.S | 56 ++++++++
arch/x86/kernel/entry_64.S | 89 +++++++++++-
arch/x86/kernel/ftrace.c | 73 +++++++++-
arch/x86/kernel/kprobes.c | 48 +++++++
include/linux/ftrace.h | 130 ++++++++++++++++-
include/linux/kprobes.h | 27 ++++
kernel/kprobes.c | 250 +++++++++++++++++++++++----------
kernel/trace/ftrace.c | 281 +++++++++++++++++++++++++++----------
kernel/trace/trace_event_perf.c | 3 +-
kernel/trace/trace_events.c | 3 +-
kernel/trace/trace_functions.c | 10 +-
kernel/trace/trace_irqsoff.c | 3 +-
kernel/trace/trace_sched_wakeup.c | 3 +-
kernel/trace/trace_selftest.c | 20 ++-
kernel/trace/trace_stack.c | 3 +-
17 files changed, 850 insertions(+), 197 deletions(-)


Subject: Re: [RFC][PATCH 00/13 v2] kprobes/ftrace: Making ftrace usable for kprobes

(2012/06/13 7:43), Steven Rostedt wrote:
> If an arch supports passing of ftrace ops, it must also pass regs.
> But it does not need to support passing regs. By default
> an arch can just pass NULL. If it supports regs, then
> it can allow tools like kprobes to ask for regs. Otherwise the
> regs parameter should just be NULL.

Hmm, by default, will the ftrace_ops be also NULL? or NULL only if
the arch doesn't support passing ftrace ops?
I mean, should the generic handler always check if ftrace_ops
isn't NULL before using it?

> The second set is Masami's patches ported on top of these changes.
>
> This is v2, and I've cleaned the patches up a bit and added a lot more
> comments. This is a more serious RFC as this is the current way I plan
> on pushing it to mainline. But I'm posting first in case someone spots
> something that I missed. I'll also be doing more testing on it and if
> someting comes up I'll obviously will fix it before pushing.
>
> Some change since v1:
>
> Added fix for undefined ftrace_location() when compiling with !DYNAMIC_FTRACE.
>
> Added missing regs passed for x86_64 (segment regs, and r11).
>
> Saved flags for when regs are being saved. The cmp in the mcount trampoline
> that checks if function tracing is disabled will save flags before
> the compare, and restore flags before returning back to the function.

Nice! :) this is what I hope in previous thread.

> Decided that passing partial regs is a bad idea. Either the arch should
> pass all regs or NULL. Even though partial regs are saved for calling the
> function, it does't make sense to give that to the callback. By making
> regs be full pt_regs or NULL, then the callbacks can simply check if
> regs is NULL and if it isn't it can do more functionality, but if it
> is NULL then it just does limited work. Having 'partial regs' would just
> complicate that for no good reason. If in the future we want partial regs,
> we can add another flag to request it.

Agreed.

Thank you,

--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

2012-06-13 11:12:12

by Steven Rostedt

[permalink] [raw]
Subject: Re: [RFC][PATCH 00/13 v2] kprobes/ftrace: Making ftrace usable for kprobes

On Wed, 2012-06-13 at 17:25 +0900, Masami Hiramatsu wrote:
> (2012/06/13 7:43), Steven Rostedt wrote:
> > If an arch supports passing of ftrace ops, it must also pass regs.
> > But it does not need to support passing regs. By default
> > an arch can just pass NULL. If it supports regs, then
> > it can allow tools like kprobes to ask for regs. Otherwise the
> > regs parameter should just be NULL.
>
> Hmm, by default, will the ftrace_ops be also NULL? or NULL only if
> the arch doesn't support passing ftrace ops?
> I mean, should the generic handler always check if ftrace_ops
> isn't NULL before using it?

Nope, ftrace_ops will always be set with the ops that registered it for
the callback. As suppose to regs, ftrace_ops is created by the generic
core code and is arch agnostic. With regs, that's very arch specific and
if an arch does not support passing regs, there's nothing that the core
code can do about it but pass a NULL pointer telling the callback
"Sorry!".

Currently there's a few things that an arch needs to do in the mcount
handler to support full dynamic ftrace. One is to check the
function_trace_stop variable is not set before calling the callback. If
an arch does not support this, then ftrace will only let the callback be
a generic function that does the check on behalf of the arch and then
calls the normal routine.

Now the arch also needs to support sending in the 3rd parameter the
ftrace_ops. If it does not support this ftrace generic code will.
Luckily, it already has code to do this. If more than one ftrace_ops is
registered (even if they register to different functions), a generic
handler is called that iterates through all the registered ftrace_ops to
call their callbacks. It also checks if the ftrace_ops has registered
the current function before calling it.

Now if an arch does not pass the ftrace ops we simply call this list
function instead. The list function already has access to what
ftrace_ops is being used and passes that to the callback. Again, this is
what must be done anyway if more than one ftrace_ops is registered to
the function tracer.

As this is another indirect call that must be made, I removed the old
helper function that does the check against function_trace_stop variable
and made the arch call the list code instead. The list code now does the
check. If the arch does not support checking function_trace_stop, it
most likely doesn't support passing ftrace_ops either.

-- Steve