Received-SPF: pass (google.com: domain of linux-kernel+bounces-71637-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Mon, 19 Feb 2024 11:07:05 -0500
From: Steven Rostedt <rostedt@goodmis.org>
To: "Masami Hiramatsu (Google)" <mhiramat@kernel.org>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>, Florent Revest
 <revest@chromium.org>, linux-trace-kernel@vger.kernel.org, LKML
 <linux-kernel@vger.kernel.org>, Martin KaFai Lau <martin.lau@linux.dev>,
 bpf <bpf@vger.kernel.org>, Sven Schnelle <svens@linux.ibm.com>, Alexei
 Starovoitov <ast@kernel.org>, Jiri Olsa <jolsa@kernel.org>, Arnaldo
 Carvalho de Melo <acme@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>,
 Alan Maguire <alan.maguire@oracle.com>, Mark Rutland
 <mark.rutland@arm.com>, Peter Zijlstra <peterz@infradead.org>, Thomas
 Gleixner <tglx@linutronix.de>, Guo Ren <guoren@kernel.org>
Subject: Re: [PATCH v7 22/36] function_graph: Add a new entry handler with
 parent_ip and ftrace_regs
Message-ID: <20240219110705.0c1ffa9c@gandalf.local.home>
In-Reply-To: <170723229401.502590.8644663781359457778.stgit@devnote2>
References: <170723204881.502590.11906735097521170661.stgit@devnote2>
	<170723229401.502590.8644663781359457778.stgit@devnote2>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Wed,  7 Feb 2024 00:11:34 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Add a new entry handler to fgraph_ops as 'entryregfunc'  which takes
> parent_ip and ftrace_regs. Note that the 'entryfunc' and 'entryregfunc'
> are mutual exclusive. You can set only one of them.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  Changes in v3:
>   - Update for new multiple fgraph.
> ---
>  arch/arm64/kernel/ftrace.c               |    2 +
>  arch/loongarch/kernel/ftrace_dyn.c       |    2 +
>  arch/powerpc/kernel/trace/ftrace.c       |    2 +
>  arch/powerpc/kernel/trace/ftrace_64_pg.c |   10 ++++---
>  arch/x86/kernel/ftrace.c                 |   42 ++++++++++++++++--------------
>  include/linux/ftrace.h                   |   19 +++++++++++---
>  kernel/trace/fgraph.c                    |   30 +++++++++++++++++----
>  7 files changed, 72 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
> index b96740829798..779b975f03f5 100644
> --- a/arch/arm64/kernel/ftrace.c
> +++ b/arch/arm64/kernel/ftrace.c
> @@ -497,7 +497,7 @@ void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>  		return;
>  
>  	if (!function_graph_enter_ops(*parent, ip, frame_pointer,
> -				      (void *)frame_pointer, gops))
> +				      (void *)frame_pointer, fregs, gops))

I would like to replace that second frame_pointer with fregs.


>  		*parent = (unsigned long)&return_to_handler;
>  
>  	ftrace_test_recursion_unlock(bit);
> diff --git a/arch/loongarch/kernel/ftrace_dyn.c b/arch/loongarch/kernel/ftrace_dyn.c
> index 81d18b911cc1..45d26c6e6564 100644
> --- a/arch/loongarch/kernel/ftrace_dyn.c
> +++ b/arch/loongarch/kernel/ftrace_dyn.c
> @@ -250,7 +250,7 @@ void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>  
>  	old = *parent;
>  
> -	if (!function_graph_enter_ops(old, ip, 0, parent, gops))
> +	if (!function_graph_enter_ops(old, ip, 0, parent, fregs, gops))

That is, to replace the parent with fregs, as the parent can be retrieved
from fregs.

We should add a fregs helper (something like):

unsigned long *fregs_caller_addr(fregs) {
	return (unsigned long *)(kernel_stack_pointer(fregs->regs) + PT_R1);
}

That returns the address that points to the parent caller on the stack.

This was on my todo list to do. That is, replace the passing of the parent
of the stack with fregs as it is redundant information.

>  		*parent = return_hooker;
>  }
>  #else
> diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
> index 4ef8bf480279..eeaaa798f4f9 100644
> --- a/arch/powerpc/kernel/trace/ftrace.c
> +++ b/arch/powerpc/kernel/trace/ftrace.c
> @@ -423,7 +423,7 @@ void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>  	if (bit < 0)
>  		goto out;
>  
> -	if (!function_graph_enter_ops(parent_ip, ip, 0, (unsigned long *)sp, gops))
> +	if (!function_graph_enter_ops(parent_ip, ip, 0, (unsigned long *)sp, fregs, gops))
>  		parent_ip = ppc_function_entry(return_to_handler);
>  
>  	ftrace_test_recursion_unlock(bit);
> diff --git a/arch/powerpc/kernel/trace/ftrace_64_pg.c b/arch/powerpc/kernel/trace/ftrace_64_pg.c
> index 7b85c3b460a3..43f6cfaaf7db 100644
> --- a/arch/powerpc/kernel/trace/ftrace_64_pg.c
> +++ b/arch/powerpc/kernel/trace/ftrace_64_pg.c
> @@ -795,7 +795,8 @@ int ftrace_disable_ftrace_graph_caller(void)
>   * in current thread info. Return the address we want to divert to.
>   */
>  static unsigned long
> -__prepare_ftrace_return(unsigned long parent, unsigned long ip, unsigned long sp)
> +__prepare_ftrace_return(unsigned long parent, unsigned long ip, unsigned long sp,
> +			struct ftrace_regs *fregs)

And sp shouldn't need to be passed in either, as hat should be part of the fregs.

I really like to consolidate the parameters and not just keep adding to
them. This all slows down the logic to load the parameters.

-- Steve


>  {
>  	unsigned long return_hooker;
>  	int bit;