Skip to contentSkip to conversation

ftrace/x86: Ftrace cleanup and add support for -mfentry on x86_32

GIT PULL In review Steven Rostedt 3 participants 10 messages Mar 22, 2017 – Mar 23, 2017

Subsystems (3)
lore
Share & tools
lore

Steven Rostedt

author maintainer
#0 lore raw

[
Ingo, Thomas or H.Peter,

I believe this is all set to go now. I updated those patches that Linus commented on and I don't believe there are any more issues. I ran this through several tests (although some of my tests are failing due to bugs introduced by others in 4.11-rc2). You can take this as a patch series, or you can pull from my tree defined below. It's based on 4.11-rc2 as I noticed that tip/x86/core is rather outdated, and Linus is fine with basing off of his tagged releases.
]

With the issues of gcc screwing around with the mcount stack frame causing function graph tracer to panic on x86_32, and with Linus saying that we should start deprecating mcount (at least on x86), I figured that x86_32 needs to support fentry.

First, I renamed mcount_64.S to ftrace_64.S. As we want to get away from mcount, having the ftrace code in a file called mcount seems rather backwards.

Next I moved the ftrace code out of entry_32.S. It's not in entry_64.S and it does not belong in entry_32.S.

I noticed that the x86_32 code has the same issue as the x86_64 did in the past with respect to a stack frame. I fixed that just for the main ftrace_caller. The ftrace_regs_caller is rather special, and so is function graph tracing.

I realized the ftrace_regs_caller code was complex due to me aggressively saving flags, even though I could still do push, lea and mov without changing them. That made the logic a little nicer.

Finally I added the fentry code.

I tested this with an older compiler (for mcount) with and without FRAME_POINTER set. I also did it with a new compiler (with fentry), with and without FRAME_POINTER. I tested function tracing, stack tracing, function_graph tracing, and kprobes (as that uses the ftrace_regs_caller).

Please pull (or take the patch series) from:

git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
tip/x86/ftrace

Head SHA1: e086b1da846b8e53f4de11e38d02d05b1ec5db58

Steven Rostedt (VMware) (6):
ftrace/x86_64: Rename mcount_64.S to ftrace_64.S ftrace/x86_32: Move the ftrace specific code out of entry_32.S ftrace/x86_32: Add stack frame pointer to ftrace_caller ftrace/x86_32: Clean up ftrace_regs_caller
ftrace/x86_32: Add -mfentry support to x86_32 with DYNAMIC_FTRACE set ftrace/x86: Use Makefile logic instead of #ifdef for compiling ftrace_*.o

----
Changes since v3:

* Changed comment about regs being skipped (Josh Poimboeuf)

* Change subject line to fix typo (Josh Poimboeuf)

 arch/x86/Kconfig                             |   2 +-
 arch/x86/entry/entry_32.S                    | 169 ------------------
 arch/x86/kernel/Makefile                     |   5 +-
 arch/x86/kernel/ftrace_32.S                  | 246 +++++++++++++++++++++++++++
 arch/x86/kernel/{mcount_64.S => ftrace_64.S} |   4 -
 5 files changed, 250 insertions(+), 176 deletions(-)
 create mode 100644 arch/x86/kernel/ftrace_32.S
 rename arch/x86/kernel/{mcount_64.S => ftrace_64.S} (99%)

Diff against v3:

arch/x86/kernel/ftrace_32.S +1 -1
index 4d52e0d49e26..de50c9084d16 100644
@@ -167,7 +167,7 @@ GLOBAL(ftrace_regs_call)
 	popl	%gs
 
 	/* use lea to not affect flags */
-	lea	3*4(%esp), %esp			/* Skip orig_ax, ip and flags */
+	lea	3*4(%esp), %esp			/* Skip orig_ax, ip and cs */
 
 	jmp	.Lftrace_ret
 #else /* ! CONFIG_DYNAMIC_FTRACE */
author maintainer
#1 lore raw

PATCH 1/6 ftrace/x86_64: Rename mcount_64.S to ftrace_64.S

From: "Steven Rostedt (VMware)" <[email protected]>

With the advent of -mfentry that uses the new "fentry" hook over mcount, the mcount name is obsolete. Having the code file that ftrace hooks into called "mcount*.S" is rather misleading. Rename it to ftrace_64.S

---
 arch/x86/kernel/Makefile                     | 4 ++--
 arch/x86/kernel/{mcount_64.S => ftrace_64.S} | 0
 2 files changed, 2 insertions(+), 2 deletions(-)
 rename arch/x86/kernel/{mcount_64.S => ftrace_64.S} (100%)
arch/x86/kernel/Makefile +2 -2
index 84c00592d359..d3743a37e990 100644
@@ -27,7 +27,7 @@ KASAN_SANITIZE_stacktrace.o := n
 
 OBJECT_FILES_NON_STANDARD_head_$(BITS).o		:= y
 OBJECT_FILES_NON_STANDARD_relocate_kernel_$(BITS).o	:= y
-OBJECT_FILES_NON_STANDARD_mcount_$(BITS).o		:= y
+OBJECT_FILES_NON_STANDARD_ftrace_$(BITS).o		:= y
 OBJECT_FILES_NON_STANDARD_test_nx.o			:= y
 
 # If instrumentation of this dir is enabled, boot hangs during first second.
@@ -46,7 +46,7 @@ obj-$(CONFIG_MODIFY_LDT_SYSCALL)	+= ldt.o
 obj-y			+= setup.o x86_init.o i8259.o irqinit.o jump_label.o
 obj-$(CONFIG_IRQ_WORK)  += irq_work.o
 obj-y			+= probe_roms.o
-obj-$(CONFIG_X86_64)	+= sys_x86_64.o mcount_64.o
+obj-$(CONFIG_X86_64)	+= sys_x86_64.o ftrace_64.o
 obj-$(CONFIG_X86_ESPFIX64)	+= espfix_64.o
 obj-$(CONFIG_SYSFS)	+= ksysfs.o
 obj-y			+= bootflag.o e820.o
arch/x86/kernel/ftrace_64.S renamed from arch/x86/kernel/mcount_64.S +0 -0
similarity index 100%
rename from arch/x86/kernel/mcount_64.S
rename to arch/x86/kernel/ftrace_64.S
Signature · 1 line
2.10.2
author maintainer
#2 lore raw

PATCH 2/6 ftrace/x86_32: Move the ftrace specific code out of entry_32.S

In reply to Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>

The function tracing hook code for ftrace is not an entry point from userspace and does not belong in the entry_*.S files. It has already been moved out of entry_64.S. This moves it out of entry_32.S into its own ftrace_32.S file.

---
 arch/x86/entry/entry_32.S   | 169 ------------------------------------------
 arch/x86/kernel/Makefile    |   1 +
 arch/x86/kernel/ftrace_32.S | 177 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 178 insertions(+), 169 deletions(-)
 create mode 100644 arch/x86/kernel/ftrace_32.S
arch/x86/entry/entry_32.S +0 -169
index 57f7ec35216e..169b3b0c5ec6 100644
@@ -35,16 +35,13 @@
 #include <asm/errno.h>
 #include <asm/segment.h>
 #include <asm/smp.h>
-#include <asm/page_types.h>
 #include <asm/percpu.h>
 #include <asm/processor-flags.h>
-#include <asm/ftrace.h>
 #include <asm/irq_vectors.h>
 #include <asm/cpufeatures.h>
 #include <asm/alternative-asm.h>
 #include <asm/asm.h>
 #include <asm/smap.h>
-#include <asm/export.h>
 #include <asm/frame.h>
 
 	.section .entry.text, "ax"
@@ -886,172 +883,6 @@ BUILD_INTERRUPT3(hyperv_callback_vector, HYPERVISOR_CALLBACK_VECTOR,
 
 #endif /* CONFIG_HYPERV */
 
-#ifdef CONFIG_FUNCTION_TRACER
-#ifdef CONFIG_DYNAMIC_FTRACE
-
-ENTRY(mcount)
-	ret
-END(mcount)
-
-ENTRY(ftrace_caller)
-	pushl	%eax
-	pushl	%ecx
-	pushl	%edx
-	pushl	$0				/* Pass NULL as regs pointer */
-	movl	4*4(%esp), %eax
-	movl	0x4(%ebp), %edx
-	movl	function_trace_op, %ecx
-	subl	$MCOUNT_INSN_SIZE, %eax
-
-.globl ftrace_call
-ftrace_call:
-	call	ftrace_stub
-
-	addl	$4, %esp			/* skip NULL pointer */
-	popl	%edx
-	popl	%ecx
-	popl	%eax
-.Lftrace_ret:
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-.globl ftrace_graph_call
-ftrace_graph_call:
-	jmp	ftrace_stub
-#endif
-
-/* This is weak to keep gas from relaxing the jumps */
-WEAK(ftrace_stub)
-	ret
-END(ftrace_caller)
-
-ENTRY(ftrace_regs_caller)
-	pushf	/* push flags before compare (in cs location) */
-
-	/*
-	 * i386 does not save SS and ESP when coming from kernel.
-	 * Instead, to get sp, &regs->sp is used (see ptrace.h).
-	 * Unfortunately, that means eflags must be at the same location
-	 * as the current return ip is. We move the return ip into the
-	 * ip location, and move flags into the return ip location.
-	 */
-	pushl	4(%esp)				/* save return ip into ip slot */
-
-	pushl	$0				/* Load 0 into orig_ax */
-	pushl	%gs
-	pushl	%fs
-	pushl	%es
-	pushl	%ds
-	pushl	%eax
-	pushl	%ebp
-	pushl	%edi
-	pushl	%esi
-	pushl	%edx
-	pushl	%ecx
-	pushl	%ebx
-
-	movl	13*4(%esp), %eax		/* Get the saved flags */
-	movl	%eax, 14*4(%esp)		/* Move saved flags into regs->flags location */
-						/* clobbering return ip */
-	movl	$__KERNEL_CS, 13*4(%esp)
-
-	movl	12*4(%esp), %eax		/* Load ip (1st parameter) */
-	subl	$MCOUNT_INSN_SIZE, %eax		/* Adjust ip */
-	movl	0x4(%ebp), %edx			/* Load parent ip (2nd parameter) */
-	movl	function_trace_op, %ecx		/* Save ftrace_pos in 3rd parameter */
-	pushl	%esp				/* Save pt_regs as 4th parameter */
-
-GLOBAL(ftrace_regs_call)
-	call	ftrace_stub
-
-	addl	$4, %esp			/* Skip pt_regs */
-	movl	14*4(%esp), %eax		/* Move flags back into cs */
-	movl	%eax, 13*4(%esp)		/* Needed to keep addl	from modifying flags */
-	movl	12*4(%esp), %eax		/* Get return ip from regs->ip */
-	movl	%eax, 14*4(%esp)		/* Put return ip back for ret */
-
-	popl	%ebx
-	popl	%ecx
-	popl	%edx
-	popl	%esi
-	popl	%edi
-	popl	%ebp
-	popl	%eax
-	popl	%ds
-	popl	%es
-	popl	%fs
-	popl	%gs
-	addl	$8, %esp			/* Skip orig_ax and ip */
-	popf					/* Pop flags at end (no addl to corrupt flags) */
-	jmp	.Lftrace_ret
-
-	popf
-	jmp	ftrace_stub
-#else /* ! CONFIG_DYNAMIC_FTRACE */
-
-ENTRY(mcount)
-	cmpl	$__PAGE_OFFSET, %esp
-	jb	ftrace_stub			/* Paging not enabled yet? */
-
-	cmpl	$ftrace_stub, ftrace_trace_function
-	jnz	.Ltrace
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-	cmpl	$ftrace_stub, ftrace_graph_return
-	jnz	ftrace_graph_caller
-
-	cmpl	$ftrace_graph_entry_stub, ftrace_graph_entry
-	jnz	ftrace_graph_caller
-#endif
-.globl ftrace_stub
-ftrace_stub:
-	ret
-
-	/* taken from glibc */
-.Ltrace:
-	pushl	%eax
-	pushl	%ecx
-	pushl	%edx
-	movl	0xc(%esp), %eax
-	movl	0x4(%ebp), %edx
-	subl	$MCOUNT_INSN_SIZE, %eax
-
-	call	*ftrace_trace_function
-
-	popl	%edx
-	popl	%ecx
-	popl	%eax
-	jmp	ftrace_stub
-END(mcount)
-#endif /* CONFIG_DYNAMIC_FTRACE */
-EXPORT_SYMBOL(mcount)
-#endif /* CONFIG_FUNCTION_TRACER */
-
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-ENTRY(ftrace_graph_caller)
-	pushl	%eax
-	pushl	%ecx
-	pushl	%edx
-	movl	0xc(%esp), %eax
-	lea	0x4(%ebp), %edx
-	movl	(%ebp), %ecx
-	subl	$MCOUNT_INSN_SIZE, %eax
-	call	prepare_ftrace_return
-	popl	%edx
-	popl	%ecx
-	popl	%eax
-	ret
-END(ftrace_graph_caller)
-
-.globl return_to_handler
-return_to_handler:
-	pushl	%eax
-	pushl	%edx
-	movl	%ebp, %eax
-	call	ftrace_return_to_handler
-	movl	%eax, %ecx
-	popl	%edx
-	popl	%eax
-	jmp	*%ecx
-#endif
-
 #ifdef CONFIG_TRACING
 ENTRY(trace_page_fault)
 	ASM_CLAC
arch/x86/kernel/Makefile +1 -0
index d3743a37e990..55e8902c461f 100644
@@ -47,6 +47,7 @@ obj-y			+= setup.o x86_init.o i8259.o irqinit.o jump_label.o
 obj-$(CONFIG_IRQ_WORK)  += irq_work.o
 obj-y			+= probe_roms.o
 obj-$(CONFIG_X86_64)	+= sys_x86_64.o ftrace_64.o
+obj-$(CONFIG_X86_32)	+= ftrace_32.o
 obj-$(CONFIG_X86_ESPFIX64)	+= espfix_64.o
 obj-$(CONFIG_SYSFS)	+= ksysfs.o
 obj-y			+= bootflag.o e820.o
arch/x86/kernel/ftrace_32.S new file +177 -0

177 lines not on this page · raw message

Signature · 1 line
2.10.2
#3 lore raw

Re: [PATCH 2/6] ftrace/x86_32: Move the ftrace specific code out of entry_32.S

Hi Steve,

Quoted 9 lines · Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>

The function tracing hook code for ftrace is not an entry point from userspace and does not belong in the entry_*.S files. It has already been moved out of entry_64.S. This moves it out of entry_32.S into its own ftrace_32.S file.

---

[SNIP]

arch/x86/kernel/ftrace_32.S line 2

Quoted 8 lines
arch/x86/kernel/ftrace_32.S new file +2 -0
new file mode 100644
index 000000000000..1889a74823ce
@@ -0,0 +1,177 @@
+/*
+ *  linux/arch/x86_64/mcount_64.S

You may want to change this line.. :)

Thanks,
Namhyung

Quoted 179 lines
arch/x86/kernel/ftrace_32.S new file +175 -1
+ *
+ *  Copyright (C) 2017  Steven Rostedt, VMware Inc.
+ */
+
+#include <linux/linkage.h>
+#include <asm/page_types.h>
+#include <asm/segment.h>
+#include <asm/export.h>
+#include <asm/ftrace.h>
+
+#ifdef CONFIG_FUNCTION_TRACER
+#ifdef CONFIG_DYNAMIC_FTRACE
+
+ENTRY(mcount)
+       ret
+END(mcount)
+
+ENTRY(ftrace_caller)
+       pushl   %eax
+       pushl   %ecx
+       pushl   %edx
+       pushl   $0                              /* Pass NULL as regs pointer */
+       movl    4*4(%esp), %eax
+       movl    0x4(%ebp), %edx
+       movl    function_trace_op, %ecx
+       subl    $MCOUNT_INSN_SIZE, %eax
+
+.globl ftrace_call
+ftrace_call:
+       call    ftrace_stub
+
+       addl    $4, %esp                        /* skip NULL pointer */
+       popl    %edx
+       popl    %ecx
+       popl    %eax
+.Lftrace_ret:
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+.globl ftrace_graph_call
+ftrace_graph_call:
+       jmp     ftrace_stub
+#endif
+
+/* This is weak to keep gas from relaxing the jumps */
+WEAK(ftrace_stub)
+       ret
+END(ftrace_caller)
+
+ENTRY(ftrace_regs_caller)
+       pushf   /* push flags before compare (in cs location) */
+
+       /*
+        * i386 does not save SS and ESP when coming from kernel.
+        * Instead, to get sp, &regs->sp is used (see ptrace.h).
+        * Unfortunately, that means eflags must be at the same location
+        * as the current return ip is. We move the return ip into the
+        * ip location, and move flags into the return ip location.
+        */
+       pushl   4(%esp)                         /* save return ip into ip slot */
+
+       pushl   $0                              /* Load 0 into orig_ax */
+       pushl   %gs
+       pushl   %fs
+       pushl   %es
+       pushl   %ds
+       pushl   %eax
+       pushl   %ebp
+       pushl   %edi
+       pushl   %esi
+       pushl   %edx
+       pushl   %ecx
+       pushl   %ebx
+
+       movl    13*4(%esp), %eax                /* Get the saved flags */
+       movl    %eax, 14*4(%esp)                /* Move saved flags into regs->flags location */
+                                               /* clobbering return ip */
+       movl    $__KERNEL_CS, 13*4(%esp)
+
+       movl    12*4(%esp), %eax                /* Load ip (1st parameter) */
+       subl    $MCOUNT_INSN_SIZE, %eax         /* Adjust ip */
+       movl    0x4(%ebp), %edx                 /* Load parent ip (2nd parameter) */
+       movl    function_trace_op, %ecx         /* Save ftrace_pos in 3rd parameter */
+       pushl   %esp                            /* Save pt_regs as 4th parameter */
+
+GLOBAL(ftrace_regs_call)
+       call    ftrace_stub
+
+       addl    $4, %esp                        /* Skip pt_regs */
+       movl    14*4(%esp), %eax                /* Move flags back into cs */
+       movl    %eax, 13*4(%esp)                /* Needed to keep addl  from modifying flags */
+       movl    12*4(%esp), %eax                /* Get return ip from regs->ip */
+       movl    %eax, 14*4(%esp)                /* Put return ip back for ret */
+
+       popl    %ebx
+       popl    %ecx
+       popl    %edx
+       popl    %esi
+       popl    %edi
+       popl    %ebp
+       popl    %eax
+       popl    %ds
+       popl    %es
+       popl    %fs
+       popl    %gs
+       addl    $8, %esp                        /* Skip orig_ax and ip */
+       popf                                    /* Pop flags at end (no addl to corrupt flags) */
+       jmp     .Lftrace_ret
+
+       popf
+       jmp     ftrace_stub
+#else /* ! CONFIG_DYNAMIC_FTRACE */
+
+ENTRY(mcount)
+       cmpl    $__PAGE_OFFSET, %esp
+       jb      ftrace_stub                     /* Paging not enabled yet? */
+
+       cmpl    $ftrace_stub, ftrace_trace_function
+       jnz     .Ltrace
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+       cmpl    $ftrace_stub, ftrace_graph_return
+       jnz     ftrace_graph_caller
+
+       cmpl    $ftrace_graph_entry_stub, ftrace_graph_entry
+       jnz     ftrace_graph_caller
+#endif
+.globl ftrace_stub
+ftrace_stub:
+       ret
+
+       /* taken from glibc */
+.Ltrace:
+       pushl   %eax
+       pushl   %ecx
+       pushl   %edx
+       movl    0xc(%esp), %eax
+       movl    0x4(%ebp), %edx
+       subl    $MCOUNT_INSN_SIZE, %eax
+
+       call    *ftrace_trace_function
+
+       popl    %edx
+       popl    %ecx
+       popl    %eax
+       jmp     ftrace_stub
+END(mcount)
+#endif /* CONFIG_DYNAMIC_FTRACE */
+EXPORT_SYMBOL(mcount)
+#endif /* CONFIG_FUNCTION_TRACER */
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+ENTRY(ftrace_graph_caller)
+       pushl   %eax
+       pushl   %ecx
+       pushl   %edx
+       movl    0xc(%esp), %eax
+       lea     0x4(%ebp), %edx
+       movl    (%ebp), %ecx
+       subl    $MCOUNT_INSN_SIZE, %eax
+       call    prepare_ftrace_return
+       popl    %edx
+       popl    %ecx
+       popl    %eax
+       ret
+END(ftrace_graph_caller)
+
+.globl return_to_handler
+return_to_handler:
+       pushl   %eax
+       pushl   %edx
+       movl    %ebp, %eax
+       call    ftrace_return_to_handler
+       movl    %eax, %ecx
+       popl    %edx
+       popl    %eax
+       jmp     *%ecx
+#endif
--

2.10.2

author maintainer
#4 lore raw

PATCH 3/6 ftrace/x86_32: Add stack frame pointer to ftrace_caller

In reply to Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>

The function hook ftrace_caller does not create its own stack frame, and this causes the ftrace stack trace to miss the first function when doing stack traces.

 # echo schedule:stacktrace > /sys/kernel/tracing/set_ftrace_filter

Before:

         <idle>-0     [002] .N..    29.865807: <stack trace>
 => cpu_startup_entry
 => start_secondary
 => startup_32_smp
           <...>-7     [001] ....    29.866509: <stack trace>
 => kthread
 => ret_from_fork
           <...>-1     [000] ....    29.865377: <stack trace>

=> poll_schedule_timeout
=> do_select
=> core_sys_select
=> SyS_select
=> do_fast_syscall_32
=> entry_SYSENTER_32

After:

          <idle>-0     [002] .N..    31.234853: <stack trace>
 => do_idle
 => cpu_startup_entry
 => start_secondary
 => startup_32_smp
           <...>-7     [003] ....    31.235140: <stack trace>
 => rcu_gp_kthread
 => kthread
 => ret_from_fork
           <...>-1819  [000] ....    31.264172: <stack trace>

=> schedule_hrtimeout_range
=> poll_schedule_timeout
=> do_sys_poll
=> SyS_ppoll
=> do_fast_syscall_32
=> entry_SYSENTER_32

---
 arch/x86/kernel/ftrace_32.S | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
arch/x86/kernel/ftrace_32.S +10 -2
index 1889a74823ce..f991e723c3e4 100644
@@ -18,12 +18,19 @@ ENTRY(mcount)
 END(mcount)
 
 ENTRY(ftrace_caller)
+
+	pushl	%ebp
+	movl	%esp, %ebp
+
 	pushl	%eax
 	pushl	%ecx
 	pushl	%edx
 	pushl	$0				/* Pass NULL as regs pointer */
-	movl	4*4(%esp), %eax
-	movl	0x4(%ebp), %edx
+	movl	5*4(%esp), %eax
+	/* Copy original ebp into %edx */
+	movl	4*4(%esp), %edx
+	/* Get the parent ip */
+	movl	0x4(%edx), %edx
 	movl	function_trace_op, %ecx
 	subl	$MCOUNT_INSN_SIZE, %eax
 
@@ -35,6 +42,7 @@ ftrace_call:
 	popl	%edx
 	popl	%ecx
 	popl	%eax
+	popl	%ebp
 .Lftrace_ret:
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 .globl ftrace_graph_call
Signature · 1 line
2.10.2
maintainer Reviewed-by on v1 3/6
#5 lore raw

Re: [PATCH 3/6] ftrace/x86_32: Add stack frame pointer to ftrace_caller

Quoted 35 lines · On Tue, 21 Mar 2017 21:35:05 -0400 Steven Rostedt <[email protected]> wrote:

From: "Steven Rostedt (VMware)" <[email protected]>

The function hook ftrace_caller does not create its own stack frame, and this causes the ftrace stack trace to miss the first function when doing stack traces.

 # echo schedule:stacktrace > /sys/kernel/tracing/set_ftrace_filter

Before:

         <idle>-0     [002] .N..    29.865807: <stack trace>
 => cpu_startup_entry
 => start_secondary
 => startup_32_smp
           <...>-7     [001] ....    29.866509: <stack trace>
 => kthread
 => ret_from_fork
           <...>-1     [000] ....    29.865377: <stack trace>

=> poll_schedule_timeout
=> do_select
=> core_sys_select
=> SyS_select
=> do_fast_syscall_32
=> entry_SYSENTER_32

After:

          <idle>-0     [002] .N..    31.234853: <stack trace>
 => do_idle
 => cpu_startup_entry
 => start_secondary
 => startup_32_smp
           <...>-7     [003] ....    31.235140: <stack trace>
 => rcu_gp_kthread
 => kthread
 => ret_from_fork
           <...>-1819  [000] ....    31.264172: <stack trace>
Quoted 7 lines

=> schedule_hrtimeout_range
=> poll_schedule_timeout
=> do_sys_poll
=> SyS_ppoll
=> do_fast_syscall_32
=> entry_SYSENTER_32

Looks good to me.

Thank you,

Quoted 44 lines
---
 arch/x86/kernel/ftrace_32.S | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
arch/x86/kernel/ftrace_32.S +10 -2
index 1889a74823ce..f991e723c3e4 100644
@@ -18,12 +18,19 @@ ENTRY(mcount)
 END(mcount)
 
 ENTRY(ftrace_caller)
+
+	pushl	%ebp
+	movl	%esp, %ebp
+
 	pushl	%eax
 	pushl	%ecx
 	pushl	%edx
 	pushl	$0				/* Pass NULL as regs pointer */
-	movl	4*4(%esp), %eax
-	movl	0x4(%ebp), %edx
+	movl	5*4(%esp), %eax
+	/* Copy original ebp into %edx */
+	movl	4*4(%esp), %edx
+	/* Get the parent ip */
+	movl	0x4(%edx), %edx
 	movl	function_trace_op, %ecx
 	subl	$MCOUNT_INSN_SIZE, %eax
 
@@ -35,6 +42,7 @@ ftrace_call:
 	popl	%edx
 	popl	%ecx
 	popl	%eax
+	popl	%ebp
 .Lftrace_ret:
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 .globl ftrace_graph_call
Signature · 1 line
2.10.2
Signature · 1 line
Masami Hiramatsu <[email protected]>
author maintainer
#6 lore raw

PATCH 4/6 ftrace/x86_32: Clean up ftrace_regs_caller

In reply to Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>

When ftrace_regs_caller was created, it was designed to preserve flags as much as possible as it needed to act just like a breakpoint triggered on the same location. But the design is over complicated as it treated all operations as modifying flags. But push, mov and lea do not modify flags. This means the code can become more simplified by allowing flags to be stored further down.

Making ftrace_regs_caller simpler will also be useful in implementing fentry logic.

---
 arch/x86/kernel/ftrace_32.S | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)
arch/x86/kernel/ftrace_32.S +21 -19
index f991e723c3e4..9e998d44abd8 100644
@@ -56,23 +56,27 @@ WEAK(ftrace_stub)
 END(ftrace_caller)
 
 ENTRY(ftrace_regs_caller)
-	pushf	/* push flags before compare (in cs location) */
-
 	/*
 	 * i386 does not save SS and ESP when coming from kernel.
 	 * Instead, to get sp, &regs->sp is used (see ptrace.h).
 	 * Unfortunately, that means eflags must be at the same location
 	 * as the current return ip is. We move the return ip into the
-	 * ip location, and move flags into the return ip location.
+	 * regs->ip location, and move flags into the return ip location.
 	 */
-	pushl	4(%esp)				/* save return ip into ip slot */
-
+	pushl	$__KERNEL_CS
+	pushl	4(%esp)				/* Save the return ip */
 	pushl	$0				/* Load 0 into orig_ax */
 	pushl	%gs
 	pushl	%fs
 	pushl	%es
 	pushl	%ds
 	pushl	%eax
+
+	/* Get flags and place them into the return ip slot */
+	pushf
+	popl	%eax
+	movl	%eax, 8*4(%esp)
+
 	pushl	%ebp
 	pushl	%edi
 	pushl	%esi
@@ -80,11 +84,6 @@ ENTRY(ftrace_regs_caller)
 	pushl	%ecx
 	pushl	%ebx
 
-	movl	13*4(%esp), %eax		/* Get the saved flags */
-	movl	%eax, 14*4(%esp)		/* Move saved flags into regs->flags location */
-						/* clobbering return ip */
-	movl	$__KERNEL_CS, 13*4(%esp)
-
 	movl	12*4(%esp), %eax		/* Load ip (1st parameter) */
 	subl	$MCOUNT_INSN_SIZE, %eax		/* Adjust ip */
 	movl	0x4(%ebp), %edx			/* Load parent ip (2nd parameter) */
@@ -95,10 +94,14 @@ GLOBAL(ftrace_regs_call)
 	call	ftrace_stub
 
 	addl	$4, %esp			/* Skip pt_regs */
-	movl	14*4(%esp), %eax		/* Move flags back into cs */
-	movl	%eax, 13*4(%esp)		/* Needed to keep addl	from modifying flags */
-	movl	12*4(%esp), %eax		/* Get return ip from regs->ip */
-	movl	%eax, 14*4(%esp)		/* Put return ip back for ret */
+
+	/* restore flags */
+	push	14*4(%esp)
+	popf
+
+	/* Move return ip back to its original location */
+	movl	12*4(%esp), %eax
+	movl	%eax, 14*4(%esp)
 
 	popl	%ebx
 	popl	%ecx
@@ -111,12 +114,11 @@ GLOBAL(ftrace_regs_call)
 	popl	%es
 	popl	%fs
 	popl	%gs
-	addl	$8, %esp			/* Skip orig_ax and ip */
-	popf					/* Pop flags at end (no addl to corrupt flags) */
-	jmp	.Lftrace_ret
 
-	popf
-	jmp	ftrace_stub
+	/* use lea to not affect flags */
+	lea	3*4(%esp), %esp			/* Skip orig_ax, ip and cs */
+
+	jmp	.Lftrace_ret
 #else /* ! CONFIG_DYNAMIC_FTRACE */
 
 ENTRY(mcount)
Signature · 1 line
2.10.2
author maintainer
#7 lore raw

PATCH 5/6 ftrace/x86_32: Add -mfentry support to x86_32 with DYNAMIC_FTRACE set

In reply to Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>

x86_64 has had fentry support for some time. I did not add support to x86_32 as I was unsure if it will be used much in the future. It is still very much used, and there's issues with function graph tracing with gcc playing around with the mcount frames, causing function graph to panic. The fentry code does not have this issue, and is able to cope as there is no frame to mess up.

Note, this only add support for fentry when DYNAMIC_FTRACE is set. There's really no reason to not have that set, because the performance of the machine drops significantly when it's not enabled. I only keep !DYNAMIC_FTRACE around to test it off, as there's still some archs that have FTRACE but not DYNAMIC_FTRACE.

---
 arch/x86/Kconfig            |  2 +-
 arch/x86/kernel/ftrace_32.S | 82 +++++++++++++++++++++++++++++++++++++++------
 2 files changed, 73 insertions(+), 11 deletions(-)
arch/x86/Kconfig +1 -1
index cc98d5a294ee..8c17146427ca 100644
@@ -127,7 +127,7 @@ config X86
 	select HAVE_EBPF_JIT			if X86_64
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS
 	select HAVE_EXIT_THREAD
-	select HAVE_FENTRY			if X86_64
+	select HAVE_FENTRY			if X86_64 || DYNAMIC_FTRACE
 	select HAVE_FTRACE_MCOUNT_RECORD
 	select HAVE_FUNCTION_GRAPH_TRACER
 	select HAVE_FUNCTION_TRACER
arch/x86/kernel/ftrace_32.S +72 -10

143 lines not on this page · raw message

Signature · 1 line
2.10.2
author maintainer
#8 lore raw

PATCH 6/6 ftrace/x86: Use Makefile logic instead of #ifdef for compiling ftrace_*.o

In reply to Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>

Currently ftrace_32.S and ftrace_64.S are compiled even when CONFIG_FUNCTION_TRACER is not set. This means there's an unnecessary #ifdef to protect the code. Instead of using preprocessor directives, only compile those files when FUNCTION_TRACER is defined.

---
 arch/x86/kernel/Makefile    | 4 ++--
 arch/x86/kernel/ftrace_32.S | 3 ---
 arch/x86/kernel/ftrace_64.S | 4 ----
 3 files changed, 2 insertions(+), 9 deletions(-)
arch/x86/kernel/Makefile +2 -2
index 55e8902c461f..4b994232cb57 100644
@@ -46,8 +46,7 @@ obj-$(CONFIG_MODIFY_LDT_SYSCALL)	+= ldt.o
 obj-y			+= setup.o x86_init.o i8259.o irqinit.o jump_label.o
 obj-$(CONFIG_IRQ_WORK)  += irq_work.o
 obj-y			+= probe_roms.o
-obj-$(CONFIG_X86_64)	+= sys_x86_64.o ftrace_64.o
-obj-$(CONFIG_X86_32)	+= ftrace_32.o
+obj-$(CONFIG_X86_64)	+= sys_x86_64.o
 obj-$(CONFIG_X86_ESPFIX64)	+= espfix_64.o
 obj-$(CONFIG_SYSFS)	+= ksysfs.o
 obj-y			+= bootflag.o e820.o
@@ -83,6 +82,7 @@ obj-y				+= apic/
 obj-$(CONFIG_X86_REBOOTFIXUPS)	+= reboot_fixups_32.o
 obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o
 obj-$(CONFIG_LIVEPATCH)	+= livepatch.o
+obj-$(CONFIG_FUNCTION_TRACER)	+= ftrace_$(BITS).o
 obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
 obj-$(CONFIG_FTRACE_SYSCALLS)	+= ftrace.o
 obj-$(CONFIG_X86_TSC)		+= trace_clock.o
arch/x86/kernel/ftrace_32.S +0 -3
index 4c1dc90ba381..de50c9084d16 100644
@@ -10,8 +10,6 @@
 #include <asm/export.h>
 #include <asm/ftrace.h>
 
-#ifdef CONFIG_FUNCTION_TRACER
-
 #ifdef CC_USING_FENTRY
 # define function_hook	__fentry__
 EXPORT_SYMBOL(__fentry__)
@@ -208,7 +206,6 @@ ftrace_stub:
 	jmp	ftrace_stub
 END(function_hook)
 #endif /* CONFIG_DYNAMIC_FTRACE */
-#endif /* CONFIG_FUNCTION_TRACER */
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 ENTRY(ftrace_graph_caller)
arch/x86/kernel/ftrace_64.S +0 -4
index 7b0d3da52fb4..b9c46919d4fc 100644
@@ -13,9 +13,6 @@
 	.code64
 	.section .entry.text, "ax"
 
-
-#ifdef CONFIG_FUNCTION_TRACER
-
 #ifdef CC_USING_FENTRY
 # define function_hook	__fentry__
 EXPORT_SYMBOL(__fentry__)
@@ -297,7 +294,6 @@ trace:
 	jmp fgraph_trace
 END(function_hook)
 #endif /* CONFIG_DYNAMIC_FTRACE */
-#endif /* CONFIG_FUNCTION_TRACER */
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 ENTRY(ftrace_graph_caller)
Signature · 1 line
2.10.2
author maintainer
#9 lore raw
In reply to Steven Rostedt

Bah, I forgot to add the "v4" in the subject (like I did for this email).

-- Steve

Quoted 94 lines · On Tue, 21 Mar 2017 21:35:02 -0400 Steven Rostedt <[email protected]> wrote:

[
Ingo, Thomas or H.Peter,

I believe this is all set to go now. I updated those patches that
Linus commented on and I don't believe there are any more issues. I ran this through several tests (although some of my tests are failing due to bugs introduced by others in 4.11-rc2). You can take this as a patch series, or you can pull from my tree defined below. It's based on 4.11-rc2 as I noticed that tip/x86/core is rather outdated, and Linus is fine with basing off of his tagged releases.
]

With the issues of gcc screwing around with the mcount stack frame causing function graph tracer to panic on x86_32, and with Linus saying that we should start deprecating mcount (at least on x86), I figured that x86_32 needs to support fentry.

First, I renamed mcount_64.S to ftrace_64.S. As we want to get away from mcount, having the ftrace code in a file called mcount seems rather backwards.

Next I moved the ftrace code out of entry_32.S. It's not in entry_64.S and it does not belong in entry_32.S.

I noticed that the x86_32 code has the same issue as the x86_64 did in the past with respect to a stack frame. I fixed that just for the main ftrace_caller. The ftrace_regs_caller is rather special, and so is function graph tracing.

I realized the ftrace_regs_caller code was complex due to me aggressively saving flags, even though I could still do push, lea and mov without changing them. That made the logic a little nicer.

Finally I added the fentry code.

I tested this with an older compiler (for mcount) with and without FRAME_POINTER set. I also did it with a new compiler (with fentry), with and without FRAME_POINTER. I tested function tracing, stack tracing, function_graph tracing, and kprobes (as that uses the ftrace_regs_caller).

Please pull (or take the patch series) from:

git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
tip/x86/ftrace

Head SHA1: e086b1da846b8e53f4de11e38d02d05b1ec5db58

Steven Rostedt (VMware) (6):
ftrace/x86_64: Rename mcount_64.S to ftrace_64.S ftrace/x86_32: Move the ftrace specific code out of entry_32.S ftrace/x86_32: Add stack frame pointer to ftrace_caller ftrace/x86_32: Clean up ftrace_regs_caller ftrace/x86_32: Add -mfentry support to x86_32 with
DYNAMIC_FTRACE set ftrace/x86: Use Makefile logic instead of #ifdef for compiling ftrace_*.o

----
Changes since v3:

* Changed comment about regs being skipped (Josh Poimboeuf)

* Change subject line to fix typo (Josh Poimboeuf)

 arch/x86/Kconfig                             |   2 +-
 arch/x86/entry/entry_32.S                    | 169 ------------------
 arch/x86/kernel/Makefile                     |   5 +-
 arch/x86/kernel/ftrace_32.S                  | 246
+++++++++++++++++++++++++++ arch/x86/kernel/{mcount_64.S =>
ftrace_64.S} |   4 - 5 files changed, 250 insertions(+), 176
deletions(-) create mode 100644 arch/x86/kernel/ftrace_32.S
 rename arch/x86/kernel/{mcount_64.S => ftrace_64.S} (99%)

Diff against v3:

arch/x86/kernel/ftrace_32.S +0 -1
index 4d52e0d49e26..de50c9084d16 100644
@@ -167,7 +167,7 @@ GLOBAL(ftrace_regs_call)
 	popl	%gs
 
 	/* use lea to not affect flags */
-	lea	3*4(%esp), %esp			/* Skip

orig_ax, ip and flags */

arch/x86/kernel/ftrace_32.S +1 -0
+	lea	3*4(%esp), %esp			/* Skip

orig_ax, ip and cs */
jmp .Lftrace_ret
#else /* ! CONFIG_DYNAMIC_FTRACE */