ftrace, kprobes: Ftrace-based kprobe optimization

struct ftrace_ops {

include/linux/ftrace.h +4 -1

-	ftrace_func_t			func;
+	union {
+		ftrace_func_t			func;
+		ftrace_regs_func_t		regs_func;
+	};
 	struct ftrace_ops		*next;
 	unsigned long			flags;
 	int __percpu			*disabled;

@@ -164,6 +182,7 @@ static inline int ftrace_function_local_disabled(struct ftrace_ops *ops)
 }

static struct ftrace_ops global_ops = {

kernel/trace/ftrace.c +2 -1

-	.func			= ftrace_stub,
+	.regs_func		= ftrace_regs_stub,
 	.notrace_hash		= EMPTY_HASH,
 	.filter_hash		= EMPTY_HASH,
+	.flags			= FTRACE_OPS_FL_SAVE_REGS,
 };
 
 static DEFINE_MUTEX(ftrace_regex_lock);

Quoted 11 lines · Masami Hiramatsu

struct ftrace_ops {

include/linux/ftrace.h +4 -1

-	ftrace_func_t			func;
+	union {
+		ftrace_func_t			func;
+		ftrace_regs_func_t		regs_func;
+	};
 	struct ftrace_ops		*next;
 	unsigned long			flags;
 	int __percpu			*disabled;

@@ -164,6 +182,7 @@ static inline int ftrace_function_local_disabled(struct ftrace_ops *ops)
 }

[..]

Quoted 11 lines

static struct ftrace_ops global_ops = {

kernel/trace/ftrace.c +2 -1

-	.func			= ftrace_stub,
+	.regs_func		= ftrace_regs_stub,
 	.notrace_hash		= EMPTY_HASH,
 	.filter_hash		= EMPTY_HASH,
+	.flags			= FTRACE_OPS_FL_SAVE_REGS,
 };
 
 static DEFINE_MUTEX(ftrace_regex_lock);

kernel/trace/ftrace.c void __init ftrace_init(void) · line 3930

Quoted 8 lines

kernel/trace/ftrace.c +2 -1

@@ -3911,7 +3924,8 @@ void __init ftrace_init(void)
 #else
 
 static struct ftrace_ops global_ops = {
-	.func			= ftrace_stub,
+	.regs_func		= ftrace_regs_stub,
+	.flags			= FTRACE_OPS_FL_SAVE_REGS,
 };
 

Ug, this wont compile with some versions of gcc :-(

The one I stumbled on is gcc 4.5.1 (which I test builds against 4.5.1 and 4.6.0). Then I saw this BZ:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=10676

This can't be a union :-( Then we can not initialize it.

Hmm, how about initializing in __init function ? Or we can make func and regs_func in different members, instead of using a union. (in that case, we can remove FTRACE_OPS_FL_SAVE_REGS.)
I just consider passing uninitialized argument to user function can cause unexpected behavior...

Quoted 6 lines · Masami Hiramatsu

Hmm, how about initializing in __init function ? Or we can make func and regs_func in different members, instead of using a union. (in that case, we can remove FTRACE_OPS_FL_SAVE_REGS.)
I just consider passing uninitialized argument to user function can cause unexpected behavior...

Easy solution:

As I want all functions to pass the ftrace_ops anyway, we can implement the ftrace_ops and the regs passing together. The arch will need to update both at the same time.

But for archs that do not support ftrace_ops (and thus also not regs), we can do (and I will do this):

static inline void
__global_list_func(unsigned long ip, unsigned long parent_ip,
struct ftrace_ops *ops, struct pt_regs *regs)
{
[do the loop, ignoring ops anyway, but passing in regs]
}

#ifndef ARCH_SUPPORTS_FTRACE_OPS
static void
noregs_global_list_func(unsigned long ip, unsigned long parent_ip)

{
	__global_list_func(ip, parent_ip, NULL, NULL);
}

#define global_list_func (ftrace_func_t)noregs_global_list_func #else
static void global_list_func(unsigned long ip, unsigned long parent_ip,
struct ftrace_ops *ops, struct pt_regs *regs)

{
	__global_list_func(ip, parent_ip, ops, regs);
}

#endif

Nothing will be passed uninitialized. If an arch does not support passing ftrace ops and regs, then it will be calling the noregs_global_list_func() (or whatever I name it), which only expects the ip and parent_ip as parameters. Then that will be calling the actual loop function with NULLs in the parameters.

When an arch supports passing of ftrace_ops, then it should also support passing in the regs (as that should be the trivial part).

Note, all funcs will get regs, but it may not get the full regs. That requires the ftrace_ops setting the special flag. The regs are saved for the mcount already. But only a partial set. Those will be sent to all function callbacks. If the function call back requires a full set (like kprobes does), then it must set the flag before registering.

Hows this sound?

Quoted 3 lines

As I want all functions to pass the ftrace_ops anyway, we can implement the ftrace_ops and the regs passing together. The arch will need to update both at the same time.

Hmm, is that ftrace_ops for recursion check? :)

Quoted 5 lines

Nothing will be passed uninitialized. If an arch does not support passing ftrace ops and regs, then it will be calling the noregs_global_list_func() (or whatever I name it), which only expects the ip and parent_ip as parameters. Then that will be calling the actual loop function with NULLs in the parameters.

Yeah, that's safe, but I think dyn_ftrace can't call handler directly from ftrace_call on such archs, can it?
# It depends on performance degradation.

Quoted 2 lines

When an arch supports passing of ftrace_ops, then it should also support passing in the regs (as that should be the trivial part).

Preparing pt_regs by software is hard on some archs, e.g. IA64. But yeah, that's an obsolete arch. We'd better focus on x86 and ARM variants.

Quoted 6 lines

Note, all funcs will get regs, but it may not get the full regs. That requires the ftrace_ops setting the special flag. The regs are saved for the mcount already. But only a partial set. Those will be sent to all function callbacks. If the function call back requires a full set (like kprobes does), then it must set the flag before registering.

Just out of curiously, would you mean that you will allocate full pt_regs frame on stack always?

Quoted 3 lines

Hows this sound?

Sounds better to me, at far as there are non-initialized parameters passed to user handler. :)

BTW, would you like to update ftrace part? I'd like to fix to remove notrace from my previous patchset and resend tomorrow.

Add register saving/restoring sequence in ftrace_caller on x86-64 for enabling SAVE_REGS feature.

Signed-off-by Masami Hiramatsu <[email protected]>

Cc Steven Rostedt <[email protected]>

---

 arch/x86/include/asm/ftrace.h |    4 ++++
 arch/x86/kernel/entry_64.S    |   38 +++++++++++++++++++++++++++++++++-----
 2 files changed, 37 insertions(+), 5 deletions(-)

arch/x86/include/asm/ftrace.h +4 -0

index 18d9005..ffb9564 100644
@@ -32,6 +32,10 @@
 #define MCOUNT_ADDR		((long)(mcount))
 #define MCOUNT_INSN_SIZE	5 /* sizeof mcount call */
 
+#if defined(CONFIG_DYNAMIC_FTRACE) && defined(CONFIG_X86_64)
+#define ARCH_SUPPORTS_FTRACE_SAVE_REGS 1
+#endif
+
 #ifndef __ASSEMBLY__
 extern void mcount(void);
 extern int modifying_ftrace_code;

arch/x86/kernel/entry_64.S +20 -0

index 320852d..6d0545b 100644
@@ -73,21 +73,44 @@ ENTRY(mcount)
 	retq
 END(mcount)
 
+	.macro MCOUNT_SAVE_ALL
+	SAVE_ARGS (RSP-ORIG_RAX)
+	SAVE_REST
+	/* Adjust eflags, rsp and rip */
+	movq RSP(%rsp), %rdx
+	movq %rdx, EFLAGS(%rsp)
+	leaq SS+8(%rsp), %rdx
+	movq %rdx, RSP(%rsp)
+	movq SS(%rsp), %rdi
+	subq $MCOUNT_INSN_SIZE, %rdi
+	movq %rdi, RIP(%rsp)
+	.endm
+
+	.macro MCOUNT_RESTORE_ALL
+	/* store eflags into regs->rsp */
+	movq EFLAGS(%rsp), %rax
+	movq %rax, RSP(%rsp)
+	RESTORE_REST
+	RESTORE_ARGS 1, (RSP-ORIG_RAX)
+	.endm

arch/x86/kernel/entry_64.S +8 -3

-	movq 0x38(%rsp), %rdi
 	movq 8(%rbp), %rsi
-	subq $MCOUNT_INSN_SIZE, %rdi
+	movq %rsp, %rdx
 
 GLOBAL(ftrace_call)
 	call ftrace_stub
 
-	MCOUNT_RESTORE_FRAME
+	MCOUNT_RESTORE_ALL
 
+	popfq_cfi
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 GLOBAL(ftrace_graph_call)
 	jmp ftrace_stub

@@ -95,6 +118,11 @@ GLOBAL(ftrace_graph_call)
 
 GLOBAL(ftrace_stub)
 	retq
+
+ftrace_exit:
+	popfq_cfi
+	retq
+	CFI_ENDPROC
 END(ftrace_caller)
 
 #else /* ! CONFIG_DYNAMIC_FTRACE */

No! Please do not make ftrace save all regs for everyone. This is critical. This will slow down function tracing even more. We do not need to save all regs for every function call, unless it is asked for.

It should only save all regs if the ftrace_ops that is registered asked for it. I had code that actually does that. I can merge your patches with them if you would like.

BTW, that code which dynamically selects trace_caller on your ftrace-v2 tree, seems to be an uncompleted work. Would you already have done it?

Introduce function trace based kprobes optimization.

With using ftrace optimization, kprobes on the mcount calling address, use ftrace's mcount call instead of breakpoint. Farthermore, this optimization works with preemptive kernel not like as current jump-based optimization. Of cource, this feature is limited only if the probe on mcount call.

Quoted 7 lines · Masami Hiramatsu

Introduce function trace based kprobes optimization.

With using ftrace optimization, kprobes on the mcount calling address, use ftrace's mcount call instead of breakpoint. Farthermore, this optimization works with preemptive kernel not like as current jump-based optimization. Of cource, this feature is limited only if the probe on mcount call.

The above paragraph doesn't parse correctly for me. Do you mean to say if the probe is on the mcount calling address, use the jump based approach instead of the breakpoint one? Could you please rephrase?

kernel/kprobes.c static __kprobes struct kprobe *alloc_aggr_kprobe(struct kprobe *p) · line 936

Quoted 8 lines

kernel/kprobes.c +8 -0

+static void __kprobes kprobe_ftrace_init(void)
+{
+	int ret;
+
+	ret = register_ftrace_function(&kprobe_ftrace_ops);
+	WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret);
+
+	kprobe_ftrace_enabled = 1;

Hmm.. is this right? kprobe_ftrace_enabled is 1 even if the init failed.

Also, this makes all __kprobes functions "notrace", because some of those functions are considered as to be called from kprobes handler which is called from function tracer. I think that is another discussion point. Perhaps, we need to introduce another tag which means "don't put kprobe on this function" instead of __kprobes and apply that.

Quoted 6 lines · Masami Hiramatsu

Also, this makes all __kprobes functions "notrace", because some of those functions are considered as to be called from kprobes handler which is called from function tracer. I think that is another discussion point. Perhaps, we need to introduce another tag which means "don't put kprobe on this function" instead of __kprobes and apply that.

Actually, instead, we can force kprobes to have all "__kprobes" functions added to its 'notrace' ftrace_ops. This will just keep kprobes from function tracing these, as I find myself tracing functions marked by kprobes quite a bit.

Hmm, I'm not so sure how the notrace and filter works. What happens if I set a foo function-entry on filter and keep notrace empty?
- only foo's nop is replaced with call?
- or all functions including foo is traced?

From Documentation/trace/ftrace.txt:

Quoted 5 lines · Masami Hiramatsu

Hmm, I'm not so sure how the notrace and filter works. What happens if I set a foo function-entry on filter and keep notrace empty?
- only foo's nop is replaced with call?
- or all functions including foo is traced?

From Documentation/trace/ftrace.txt:

"If a function exists in both set_ftrace_filter
and set_ftrace_notrace, the function will _not_ be traced."

The filters work exactly the same. If notrace always take precedence over filter. If you have foo and bar in filter, and put foo in notrace, then only bar is traced.

"filter" means "limit tracing only to these functions" "notrace" means "do not trace this function"

Think of 'filter' as a way of making the 'available_filter_functions' smaller. It filters the list. But 'notrace' is just like adding a 'notrace' tag. It stops it from being traced regardless.

OK, that's same as what I expected. In that case, all __kprobes functions are already filtered out by kprobes itself. So we don't need to set that anymore.

Hmm, CFLAGS_REMOVE_kprobes.o can also keep kprobes from function tracer. So I'd like to try to use that instead of including notrace into __kprobes.
However, in that case, kprobe users must remove -pg from their kernel modules too, and take care that they must call only notrace kernel APIs...

Perhaps, we'd better introduce new kprobe flag which allow kprobe to accept new probe on ftrace, so that user can explicitly understand what he will do.

Quoted 11 lines · Masami Hiramatsu

OK, that's same as what I expected. In that case, all __kprobes functions are already filtered out by kprobes itself. So we don't need to set that anymore.

Hmm, CFLAGS_REMOVE_kprobes.o can also keep kprobes from function tracer. So I'd like to try to use that instead of including notrace into __kprobes.
However, in that case, kprobe users must remove -pg from their kernel modules too, and take care that they must call only notrace kernel APIs...

Quoted 3 lines

Perhaps, we'd better introduce new kprobe flag which allow kprobe to accept new probe on ftrace, so that user can explicitly understand what he will do.

Please do not make kprobe functions not allow function tracing! I *want* to trace these functions! For example, I trace functions in NMIs all the time, and I know these are prohibited by kprobes.

Why can't we function trace this? If kprobes does not trace functions marked with kprobes already, then it should not have any issue. Kprobes will only use the function tracer for what its allowed to use.

Do not remove -pg from anything to satisfy kprobes. It shouldn't need it.

Quoted 11 lines · Masami Hiramatsu

OK, that's same as what I expected. In that case, all __kprobes functions are already filtered out by kprobes itself. So we don't need to set that anymore.

Hmm, CFLAGS_REMOVE_kprobes.o can also keep kprobes from function tracer. So I'd like to try to use that instead of including notrace into __kprobes.
However, in that case, kprobe users must remove -pg from their kernel modules too, and take care that they must call only notrace kernel APIs...

Quoted 3 lines

Perhaps, we'd better introduce new kprobe flag which allow kprobe to accept new probe on ftrace, so that user can explicitly understand what he will do.

Please do not make kprobe functions not allow function tracing! I *want* to trace these functions! For example, I trace functions in NMIs all the time, and I know these are prohibited by kprobes.

Why can't we function trace this? If kprobes does not trace functions marked with kprobes already, then it should not have any issue. Kprobes will only use the function tracer for what its allowed to use.

OK, so I've introduced new noprobe tag and replaced __kprobes with it. And now __kprobes tag which is a combination of noprobe and notrace, means that the function is not probed and it can be called from kprobe handler. (thus user must use this with their handlers and functions which will be used from the handlers) And also most of __kprobes tags are replaced by noprobe only.

This means that you can trace those by function tracer :)

BTW, currently kprobes allows user cases pagefault in their handler (kprobe.fault_handler will handle it). I guess that can cause some problem with ftrace, isn't it? If so, I need to deny a kprobe using ftrace if it has fault_handler.

Quoted 6 lines · Masami Hiramatsu

OK, so I've introduced new noprobe tag and replaced __kprobes with it. And now __kprobes tag which is a combination of noprobe and notrace, means that the function is not probed and it can be called from kprobe handler. (thus user must use this with their handlers and functions which will be used from the handlers) And also most of __kprobes tags are replaced by noprobe only.

You still haven't answered my question. Why can't function tracer still trace these? If kprobes does not allow it to be probed, it should not interfere with your code. But normal function tracing should still allow these.

I still do not understand why you need to add 'notrace' at all.

Quoted 6 lines

This means that you can trace those by function tracer :)

BTW, currently kprobes allows user cases pagefault in their handler (kprobe.fault_handler will handle it). I guess that can cause some problem with ftrace, isn't it? If so, I need to deny a kprobe using ftrace if it has fault_handler.

As long as there's recursion protection you are fine. In fact, I may add recursion protection within the assembly itself, that will make all function tracing safe. (does not solve the breakpoint bug from the other thread, but will solve most other things). In fact, this may allow us to remove notraces that were added because of recursion issues.

Quoted 9 lines · Steven Rostedt

Quoted 6 lines · Masami Hiramatsu

OK, so I've introduced new noprobe tag and replaced __kprobes with it. And now __kprobes tag which is a combination of noprobe and notrace, means that the function is not probed and it can be called from kprobe handler. (thus user must use this with their handlers and functions which will be used from the handlers) And also most of __kprobes tags are replaced by noprobe only.

Quoted 4 lines

You still haven't answered my question. Why can't function tracer still trace these? If kprobes does not allow it to be probed, it should not interfere with your code. But normal function tracing should still allow these.

Because those are called from ftrace-based kprobe, which means

it is directly invoked from kprobe_ftrace_handler.

I think
that should be handled as a part of ftrace handler.

Currently, I just added notrace on below two kind of functions

- handler functions which can be called intermediately from ftrace

- get_kprobe, set_kprobe_instance, etc. internal utility functions which is called directly from kprobe ftrace handler.

Quoted 3 lines

I still do not understand why you need to add 'notrace' at all.

Because I'd like to solve a recursive call problem.

I saw a problem which I hit some odd function tracer behavior. When I removed notrace from get_kprobe(), which is an essential internal function called directly from kprobe_ftrace_handler, I hit a kernel crash caused by recursive call right after I registered kprobe_ftrace_handler to ftrace. At that time, ftrace_ops.filter was empty so I thought there is no function traced, but the kprobe_ftrace_handler was called from somewhere. So I saw it hit a recursive loop of ftrace_call -> kprobe_ftrace_handler -> get_kprobe -> ftrace_call ...

I think if I just register kprobe's ftrace_ops without start tracing, I think we can just do tracing without "notrace".

Quoted 8 lines

Quoted 7 lines

This means that you can trace those by function tracer :)

BTW, currently kprobes allows user cases pagefault in their handler (kprobe.fault_handler will handle it). I guess that can cause some problem with ftrace, isn't it? If so, I need to deny a kprobe using ftrace if it has fault_handler.

Quoted 5 lines

As long as there's recursion protection you are fine. In fact, I may add recursion protection within the assembly itself, that will make all function tracing safe. (does not solve the breakpoint bug from the other thread, but will solve most other things). In fact, this may allow us to remove notraces that were added because of recursion issues.

OK, I think kprobe already solves that as long as get_kprobe and kprobe_running doesn't cause recursion...

Quoted 2 lines · Masami Hiramatsu

I think if I just register kprobe's ftrace_ops without start tracing, I think we can just do tracing without "notrace".

You should be able to register the filter first, and then register function tracer. If you register without setting the filter, you will start tracing all functions.

I think that may be the problem you are seeing. You want to register the filter first and then the ftrace_ops.