lkml.org 
[lkml]   [2017]   [Mar]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 5/5 v3] ftrace/x86-32: Add -mfentry support to x86_32 with DYNAMIC_FTRACE set
On Thu, 16 Mar 2017 16:40:36 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> [
> Fengguang Wu's bot told me that it failed to compile, as ftrace_32.S
> always gets compiled (as does ftrace_64.S). And the content needs to
> be protected within the #ifdef.
> ]
>
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> 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.

Looks good to me.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks!


>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> arch/x86/Kconfig | 2 +-
> arch/x86/kernel/ftrace_32.S | 81 +++++++++++++++++++++++++++++++++++++++------
> 2 files changed, 72 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index cc98d5a..8c17146 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -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
> diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S
> index 068a582..8530567 100644
> --- a/arch/x86/kernel/ftrace_32.S
> +++ b/arch/x86/kernel/ftrace_32.S
> @@ -12,24 +12,65 @@
> #ifdef CONFIG_FUNCTION_TRACER
> #ifdef CONFIG_DYNAMIC_FTRACE
>
> -ENTRY(mcount)
> +#ifdef CC_USING_FENTRY
> +# define function_hook __fentry__
> +EXPORT_SYMBOL(__fentry__)
> +#else
> +# define function_hook mcount
> +EXPORT_SYMBOL(mcount)
> +#endif
> +
> +/* 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
> +# define MCOUNT_FRAME 1 /* using frame = true */
> +#else
> +# define MCOUNT_FRAME 0 /* using frame = false */
> +#endif
> +
> +ENTRY(function_hook)
> ret
> -END(mcount)
> +END(function_hook)
>
> ENTRY(ftrace_caller)
>
> +#ifdef USING_FRAME_POINTER
> +# ifdef CC_USING_FENTRY
> + /*
> + * Frame pointers are of ip followed by bp.
> + * Since fentry is an immediate jump, we are left with
> + * parent-ip, function-ip. We need to add a frame with
> + * parent-ip followed by ebp.
> + */
> + pushl 4(%esp) /* parent ip */
> 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
> +#endif
> pushl %eax
> pushl %ecx
> pushl %edx
> pushl $0 /* Pass NULL as regs pointer */
> - movl 5*4(%esp), %eax
> - /* Copy original ebp into %edx */
> +
> +#ifdef USING_FRAME_POINTER
> + /* Load parent ebp into edx */
> movl 4*4(%esp), %edx
> +#else
> + /* There's no frame pointer, load the appropriate stack addr instead */
> + lea 4*4(%esp), %edx
> +#endif
> +
> + movl (MCOUNT_FRAME+4)*4(%esp), %eax /* load the rip */
> /* Get the parent ip */
> - movl 0x4(%edx), %edx
> + movl 4(%edx), %edx /* edx has ebp */
> +
> movl function_trace_op, %ecx
> subl $MCOUNT_INSN_SIZE, %eax
>
> @@ -41,7 +82,14 @@ ftrace_call:
> popl %edx
> popl %ecx
> popl %eax
> +#ifdef USING_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
> .globl ftrace_graph_call
> @@ -82,6 +130,10 @@ ENTRY(ftrace_regs_caller)
> pushl %edx
> pushl %ecx
> pushl %ebx
> +#ifdef CC_USING_FENTRY
> + /* Load 4 off of the parent ip addr into ebp */
> + lea 14*4(%esp), %ebp
> +#endif
>
> movl 12*4(%esp), %eax /* Load ip (1st parameter) */
> subl $MCOUNT_INSN_SIZE, %eax /* Adjust ip */
> @@ -120,7 +172,7 @@ GLOBAL(ftrace_regs_call)
> jmp .Lftrace_ret
> #else /* ! CONFIG_DYNAMIC_FTRACE */
>
> -ENTRY(mcount)
> +ENTRY(function_hook)
> cmpl $__PAGE_OFFSET, %esp
> jb ftrace_stub /* Paging not enabled yet? */
>
> @@ -152,9 +204,8 @@ ftrace_stub:
> popl %ecx
> popl %eax
> jmp ftrace_stub
> -END(mcount)
> +END(function_hook)
> #endif /* CONFIG_DYNAMIC_FTRACE */
> -EXPORT_SYMBOL(mcount)
> #endif /* CONFIG_FUNCTION_TRACER */
>
> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> @@ -162,9 +213,15 @@ ENTRY(ftrace_graph_caller)
> pushl %eax
> pushl %ecx
> pushl %edx
> - movl 0xc(%esp), %eax
> + 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
> @@ -177,7 +234,11 @@ 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
> --
> 2.9.3
>


--
Masami Hiramatsu <mhiramat@kernel.org>

\
 
 \ /
  Last update: 2017-03-17 11:47    [W:0.109 / U:0.124 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site