Re: [PATCH 5/5 v3] ftrace/x86-32: Add -mfentry support to x86_32 with DYNAMIC_FTRACE set

[Masami Hiramatsu <mhiramat@kernel.org>]

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>