[RFC] ftrace/x86: Remove mcount support
diff mbox series

Message ID 20190509154902.34ea14f8@gandalf.local.home
State New, archived
Headers show
Series
  • [RFC] ftrace/x86: Remove mcount support
Related show

Commit Message

Steven Rostedt May 9, 2019, 7:49 p.m. UTC
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

There's two methods of enabling function tracing in Linux on x86. One is
with just "gcc -pg" and the other is "gcc -pg -mfentry". The former will use
calls to a special function "mcount" after the frame is set up in all C
functions. The latter will add calls to a special function called "fentry"
as the very first instruction of all C functions.

At compile time, there is a check to see if gcc supports, -mfentry, and if
it does, it will use that, because it is more versatile and less error prone
for function tracing.

Starting with v4.19, the minimum gcc supported to build the Linux kernel,
was raised to version 4.6. That also happens to be the first gcc version to
support -mfentry. Since on x86, using gcc versions from 4.6 and beyond will
unconditionally enable the -mfentry, it will no longer use mcount as the
method for inserting calls into the C functions of the kernel. This means
that there is no point in continuing to maintain mcount in x86.

Remove support for using mcount. This makes the code less complex, and will
also allow it to be simplified in the future.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/x86/include/asm/ftrace.h    |  8 +++----
 arch/x86/include/asm/livepatch.h |  3 ---
 arch/x86/kernel/ftrace_32.S      | 36 +++++---------------------------
 arch/x86/kernel/ftrace_64.S      | 28 +------------------------
 4 files changed, 9 insertions(+), 66 deletions(-)

Comments

Peter Zijlstra May 9, 2019, 7:59 p.m. UTC | #1
On Thu, May 09, 2019 at 03:49:02PM -0400, Steven Rostedt wrote:
> 
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> There's two methods of enabling function tracing in Linux on x86. One is
> with just "gcc -pg" and the other is "gcc -pg -mfentry". The former will use
> calls to a special function "mcount" after the frame is set up in all C
> functions. The latter will add calls to a special function called "fentry"
> as the very first instruction of all C functions.
> 
> At compile time, there is a check to see if gcc supports, -mfentry, and if
> it does, it will use that, because it is more versatile and less error prone
> for function tracing.
> 
> Starting with v4.19, the minimum gcc supported to build the Linux kernel,
> was raised to version 4.6. That also happens to be the first gcc version to
> support -mfentry. Since on x86, using gcc versions from 4.6 and beyond will
> unconditionally enable the -mfentry, it will no longer use mcount as the
> method for inserting calls into the C functions of the kernel. This means
> that there is no point in continuing to maintain mcount in x86.
> 
> Remove support for using mcount. This makes the code less complex, and will
> also allow it to be simplified in the future.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Jiri Kosina May 9, 2019, 8:03 p.m. UTC | #2
On Thu, 9 May 2019, Steven Rostedt wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> There's two methods of enabling function tracing in Linux on x86. One is
> with just "gcc -pg" and the other is "gcc -pg -mfentry". The former will use
> calls to a special function "mcount" after the frame is set up in all C
> functions. The latter will add calls to a special function called "fentry"
> as the very first instruction of all C functions.
> 
> At compile time, there is a check to see if gcc supports, -mfentry, and if
> it does, it will use that, because it is more versatile and less error prone
> for function tracing.
> 
> Starting with v4.19, the minimum gcc supported to build the Linux kernel,
> was raised to version 4.6. That also happens to be the first gcc version to
> support -mfentry. Since on x86, using gcc versions from 4.6 and beyond will
> unconditionally enable the -mfentry, it will no longer use mcount as the
> method for inserting calls into the C functions of the kernel. This means
> that there is no point in continuing to maintain mcount in x86.
> 
> Remove support for using mcount. This makes the code less complex, and will
> also allow it to be simplified in the future.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Thanks; the fact that mcount happens only after the prologue has already 
happened makes it unusuable anyway for anything nontrivial.

	Acked-by: Jiri Kosina <jkosina@suse.cz>
Linus Torvalds May 9, 2019, 8:12 p.m. UTC | #3
On Thu, May 9, 2019 at 12:49 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
> index ed80003ce3e2..2f2bdf0662f8 100644
> --- a/arch/x86/include/asm/livepatch.h
> +++ b/arch/x86/include/asm/livepatch.h
> @@ -26,9 +26,6 @@
>
>  static inline int klp_check_compiler_support(void)
>  {
> -#ifndef CC_USING_FENTRY
> -       return 1;
> -#endif
>         return 0;
>  }

Please remove this entirely.

There are now three copies of klp_check_compiler_support(), and all
three are now trivial "return 0" functions.

Remove the whole thing, and remove the single use in kernel/livepatch/core.c.

The only reason for this function existing was literally "mcount isn't
good enough", so with mcount removed, the function should be removed
too.

                     Linus
Josh Poimboeuf May 9, 2019, 8:14 p.m. UTC | #4
On Thu, May 09, 2019 at 03:49:02PM -0400, Steven Rostedt wrote:
> 
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> There's two methods of enabling function tracing in Linux on x86. One is
> with just "gcc -pg" and the other is "gcc -pg -mfentry". The former will use
> calls to a special function "mcount" after the frame is set up in all C
> functions. The latter will add calls to a special function called "fentry"
> as the very first instruction of all C functions.
> 
> At compile time, there is a check to see if gcc supports, -mfentry, and if
> it does, it will use that, because it is more versatile and less error prone
> for function tracing.
> 
> Starting with v4.19, the minimum gcc supported to build the Linux kernel,
> was raised to version 4.6. That also happens to be the first gcc version to
> support -mfentry. Since on x86, using gcc versions from 4.6 and beyond will
> unconditionally enable the -mfentry, it will no longer use mcount as the
> method for inserting calls into the C functions of the kernel. This means
> that there is no point in continuing to maintain mcount in x86.
> 
> Remove support for using mcount. This makes the code less complex, and will
> also allow it to be simplified in the future.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  arch/x86/include/asm/ftrace.h    |  8 +++----
>  arch/x86/include/asm/livepatch.h |  3 ---
>  arch/x86/kernel/ftrace_32.S      | 36 +++++---------------------------
>  arch/x86/kernel/ftrace_64.S      | 28 +------------------------
>  4 files changed, 9 insertions(+), 66 deletions(-)

I was wondering why you didn't do s/mcount/fentry/ everywhere, but I
guess it's because mcount is still used by other arches, so it still has
a generic meaning tree-wide, right?

Anyway it's nice to finally see this cruft disappear.

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
Steven Rostedt May 10, 2019, 2:28 a.m. UTC | #5
On Thu, 9 May 2019 13:12:55 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, May 9, 2019 at 12:49 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
> > index ed80003ce3e2..2f2bdf0662f8 100644
> > --- a/arch/x86/include/asm/livepatch.h
> > +++ b/arch/x86/include/asm/livepatch.h
> > @@ -26,9 +26,6 @@
> >
> >  static inline int klp_check_compiler_support(void)
> >  {
> > -#ifndef CC_USING_FENTRY
> > -       return 1;
> > -#endif
> >         return 0;
> >  }  
> 
> Please remove this entirely.
> 
> There are now three copies of klp_check_compiler_support(), and all
> three are now trivial "return 0" functions.
> 
> Remove the whole thing, and remove the single use in kernel/livepatch/core.c.
> 
> The only reason for this function existing was literally "mcount isn't
> good enough", so with mcount removed, the function should be removed
> too.
>

As this patch is simply a "remove mcount" patch, I'd like to have the
removal of klp_check_compiler_support() be a separate patch.

Jiri or Josh, care to send a patch on top of this one?

Thanks!

-- Steve
Steven Rostedt May 10, 2019, 2:30 a.m. UTC | #6
On Thu, 9 May 2019 15:14:30 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > ---
> >  arch/x86/include/asm/ftrace.h    |  8 +++----
> >  arch/x86/include/asm/livepatch.h |  3 ---
> >  arch/x86/kernel/ftrace_32.S      | 36 +++++---------------------------
> >  arch/x86/kernel/ftrace_64.S      | 28 +------------------------
> >  4 files changed, 9 insertions(+), 66 deletions(-)  
> 
> I was wondering why you didn't do s/mcount/fentry/ everywhere, but I
> guess it's because mcount is still used by other arches, so it still has
> a generic meaning tree-wide, right?

Yes, fentry works nicely when you have a single instruction that pushes
the return address on the stack and then jumps to another location.
It's much trickier to implement with link registers. There's a few
different implementations for other archs, but mcount happens to be the
one supported by most.

> 
> Anyway it's nice to finally see this cruft disappear.
> 
> Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

Thanks!

-- Steve
Steven Rostedt May 10, 2019, 2:37 a.m. UTC | #7
On Thu, 9 May 2019 15:49:02 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> index cf350639e76d..287f1f7b2e52 100644
> --- a/arch/x86/include/asm/ftrace.h
> +++ b/arch/x86/include/asm/ftrace.h
> @@ -3,12 +3,10 @@
>  #define _ASM_X86_FTRACE_H
>  
>  #ifdef CONFIG_FUNCTION_TRACER
> -#ifdef CC_USING_FENTRY
> -# define MCOUNT_ADDR		((unsigned long)(__fentry__))
> -#else
> -# define MCOUNT_ADDR		((unsigned long)(mcount))
> -# define HAVE_FUNCTION_GRAPH_FP_TEST
> +#ifndef CC_USING_FENTRY
> +# error Compiler does not support fentry?
>  #endif
> +# define MCOUNT_ADDR		((unsigned long)(__fentry__))
>  #define MCOUNT_INSN_SIZE	5 /* sizeof mcount call */
>  
>  #ifdef CONFIG_DYNAMIC_FTRACE

Failed my tests. :-(

In arch/x86/Kconfig ...

	select HAVE_FENTRY                      if X86_64 || DYNAMIC_FTRACE


Bah! I need to work a little on this patch.

I need to implement fentry in the !DYNAMIC_FTRACE code of x86_32 first.
Shouldn't be too hard, but still.

I could also just force DYNAMIC_FTRACE to be 'y' for x86_32 if
CONFIG_FUNCTION_TRACER is set. The only reason I still support static
FTRACE on x86 is because I use it to test !DYNAMIC_FTRACE generic code,
because there's still some archs that only support the !DYNAMIC_FTRACE
function tracer.

-- Steve
Jiri Kosina May 10, 2019, 7:30 a.m. UTC | #8
On Thu, 9 May 2019, Steven Rostedt wrote:

> As this patch is simply a "remove mcount" patch, I'd like to have the 
> removal of klp_check_compiler_support() be a separate patch.
> 
> Jiri or Josh, care to send a patch on top of this one?

Sure thing, I'll do that once you send v2 fixing x86_32 of your patch.

Thanks,

Patch
diff mbox series

diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index cf350639e76d..287f1f7b2e52 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -3,12 +3,10 @@ 
 #define _ASM_X86_FTRACE_H
 
 #ifdef CONFIG_FUNCTION_TRACER
-#ifdef CC_USING_FENTRY
-# define MCOUNT_ADDR		((unsigned long)(__fentry__))
-#else
-# define MCOUNT_ADDR		((unsigned long)(mcount))
-# define HAVE_FUNCTION_GRAPH_FP_TEST
+#ifndef CC_USING_FENTRY
+# error Compiler does not support fentry?
 #endif
+# define MCOUNT_ADDR		((unsigned long)(__fentry__))
 #define MCOUNT_INSN_SIZE	5 /* sizeof mcount call */
 
 #ifdef CONFIG_DYNAMIC_FTRACE
diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
index ed80003ce3e2..2f2bdf0662f8 100644
--- a/arch/x86/include/asm/livepatch.h
+++ b/arch/x86/include/asm/livepatch.h
@@ -26,9 +26,6 @@ 
 
 static inline int klp_check_compiler_support(void)
 {
-#ifndef CC_USING_FENTRY
-	return 1;
-#endif
 	return 0;
 }
 
diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S
index 4c8440de3355..79dcfdf9e96a 100644
--- a/arch/x86/kernel/ftrace_32.S
+++ b/arch/x86/kernel/ftrace_32.S
@@ -10,22 +10,12 @@ 
 #include <asm/ftrace.h>
 #include <asm/nospec-branch.h>
 
-#ifdef CC_USING_FENTRY
 # define function_hook	__fentry__
 EXPORT_SYMBOL(__fentry__)
-#else
-# define function_hook	mcount
-EXPORT_SYMBOL(mcount)
-#endif
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 
-/* mcount uses a frame pointer even if CONFIG_FRAME_POINTER is not set */
-#if !defined(CC_USING_FENTRY) || defined(CONFIG_FRAME_POINTER)
-# define USING_FRAME_POINTER
-#endif
-
-#ifdef USING_FRAME_POINTER
+#ifdef CONFIG_FRAME_POINTER
 # define MCOUNT_FRAME			1	/* using frame = true  */
 #else
 # define MCOUNT_FRAME			0	/* using frame = false */
@@ -37,8 +27,7 @@  END(function_hook)
 
 ENTRY(ftrace_caller)
 
-#ifdef USING_FRAME_POINTER
-# ifdef CC_USING_FENTRY
+#ifdef CONFIG_FRAME_POINTER
 	/*
 	 * Frame pointers are of ip followed by bp.
 	 * Since fentry is an immediate jump, we are left with
@@ -49,7 +38,7 @@  ENTRY(ftrace_caller)
 	pushl	%ebp
 	movl	%esp, %ebp
 	pushl	2*4(%esp)			/* function ip */
-# endif
+
 	/* For mcount, the function ip is directly above */
 	pushl	%ebp
 	movl	%esp, %ebp
@@ -59,7 +48,7 @@  ENTRY(ftrace_caller)
 	pushl	%edx
 	pushl	$0				/* Pass NULL as regs pointer */
 
-#ifdef USING_FRAME_POINTER
+#ifdef CONFIG_FRAME_POINTER
 	/* Load parent ebp into edx */
 	movl	4*4(%esp), %edx
 #else
@@ -82,13 +71,11 @@  ftrace_call:
 	popl	%edx
 	popl	%ecx
 	popl	%eax
-#ifdef USING_FRAME_POINTER
+#ifdef CONFIG_FRAME_POINTER
 	popl	%ebp
-# ifdef CC_USING_FENTRY
 	addl	$4,%esp				/* skip function ip */
 	popl	%ebp				/* this is the orig bp */
 	addl	$4, %esp			/* skip parent ip */
-# endif
 #endif
 .Lftrace_ret:
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
@@ -133,11 +120,7 @@  ENTRY(ftrace_regs_caller)
 
 	movl	12*4(%esp), %eax		/* Load ip (1st parameter) */
 	subl	$MCOUNT_INSN_SIZE, %eax		/* Adjust ip */
-#ifdef CC_USING_FENTRY
 	movl	15*4(%esp), %edx		/* Load parent ip (2nd parameter) */
-#else
-	movl	0x4(%ebp), %edx			/* Load parent ip (2nd parameter) */
-#endif
 	movl	function_trace_op, %ecx		/* Save ftrace_pos in 3rd parameter */
 	pushl	%esp				/* Save pt_regs as 4th parameter */
 
@@ -215,13 +198,8 @@  ENTRY(ftrace_graph_caller)
 	pushl	%edx
 	movl	3*4(%esp), %eax
 	/* Even with frame pointers, fentry doesn't have one here */
-#ifdef CC_USING_FENTRY
 	lea	4*4(%esp), %edx
 	movl	$0, %ecx
-#else
-	lea	0x4(%ebp), %edx
-	movl	(%ebp), %ecx
-#endif
 	subl	$MCOUNT_INSN_SIZE, %eax
 	call	prepare_ftrace_return
 	popl	%edx
@@ -234,11 +212,7 @@  END(ftrace_graph_caller)
 return_to_handler:
 	pushl	%eax
 	pushl	%edx
-#ifdef CC_USING_FENTRY
 	movl	$0, %eax
-#else
-	movl	%ebp, %eax
-#endif
 	call	ftrace_return_to_handler
 	movl	%eax, %ecx
 	popl	%edx
diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index 75f2b36b41a6..10eb2760ef2c 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -13,22 +13,12 @@ 
 	.code64
 	.section .entry.text, "ax"
 
-#ifdef CC_USING_FENTRY
 # define function_hook	__fentry__
 EXPORT_SYMBOL(__fentry__)
-#else
-# define function_hook	mcount
-EXPORT_SYMBOL(mcount)
-#endif
 
 #ifdef CONFIG_FRAME_POINTER
-# ifdef CC_USING_FENTRY
 /* Save parent and function stack frames (rip and rbp) */
 #  define MCOUNT_FRAME_SIZE	(8+16*2)
-# else
-/* Save just function stack frame (rip and rbp) */
-#  define MCOUNT_FRAME_SIZE	(8+16)
-# endif
 #else
 /* No need to save a stack frame */
 # define MCOUNT_FRAME_SIZE	0
@@ -75,17 +65,13 @@  EXPORT_SYMBOL(mcount)
 	 * fentry is called before the stack frame is set up, where as mcount
 	 * is called afterward.
 	 */
-#ifdef CC_USING_FENTRY
+
 	/* Save the parent pointer (skip orig rbp and our return address) */
 	pushq \added+8*2(%rsp)
 	pushq %rbp
 	movq %rsp, %rbp
 	/* Save the return address (now skip orig rbp, rbp and parent) */
 	pushq \added+8*3(%rsp)
-#else
-	/* Can't assume that rip is before this (unless added was zero) */
-	pushq \added+8(%rsp)
-#endif
 	pushq %rbp
 	movq %rsp, %rbp
 #endif /* CONFIG_FRAME_POINTER */
@@ -113,12 +99,7 @@  EXPORT_SYMBOL(mcount)
 	movq %rdx, RBP(%rsp)
 
 	/* Copy the parent address into %rsi (second parameter) */
-#ifdef CC_USING_FENTRY
 	movq MCOUNT_REG_SIZE+8+\added(%rsp), %rsi
-#else
-	/* %rdx contains original %rbp */
-	movq 8(%rdx), %rsi
-#endif
 
 	 /* Move RIP to its proper location */
 	movq MCOUNT_REG_SIZE+\added(%rsp), %rdi
@@ -303,15 +284,8 @@  ENTRY(ftrace_graph_caller)
 	/* Saves rbp into %rdx and fills first parameter  */
 	save_mcount_regs
 
-#ifdef CC_USING_FENTRY
 	leaq MCOUNT_REG_SIZE+8(%rsp), %rsi
 	movq $0, %rdx	/* No framepointers needed */
-#else
-	/* Save address of the return address of traced function */
-	leaq 8(%rdx), %rsi
-	/* ftrace does sanity checks against frame pointers */
-	movq (%rdx), %rdx
-#endif
 	call	prepare_ftrace_return
 
 	restore_mcount_regs