ftrace: Fixes for 6.13

[for-linus][PATCH 3/3] ftrace/microblaze: Do not find "true_parent" for return address

Posted by Steven Rostedt 1 year, 1 month ago

From: Steven Rostedt <rostedt@goodmis.org>

When function tracing and function graph tracing are both enabled (in
different instances) the "parent" of some of the function tracing events
is "return_to_handler" which is the trampoline used by function graph
tracing. To fix this, ftrace_get_true_parent_ip() was introduced that
returns the "true" parent ip instead of the trampoline.

To do this, the ftrace_regs_get_stack_pointer() is used, which uses
kernel_stack_pointer(). The problem is that microblaze does not implement
kerenl_stack_pointer() so when function graph tracing is enabled, the
build fails.

Modify the #ifdef check to the code around ftrace_get_true_parent_ip() to
include !defined(CONFIG_MICROBLAZE) which will default it to just return
the parent ip passed in, which may still be the ip of the function garph
trampoline.

Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Michal Simek <monstr@monstr.eu>
Link: https://lore.kernel.org/20241211153634.69c75afa@batman.local.home
Fixes: 60b1f578b578 ("ftrace: Get the true parent ip for function tracer")
Reported-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace_functions.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
index 74c353164ca1..a75d107a45f8 100644
--- a/kernel/trace/trace_functions.c
+++ b/kernel/trace/trace_functions.c
@@ -176,7 +176,8 @@ static void function_trace_start(struct trace_array *tr)
 	tracing_reset_online_cpus(&tr->array_buffer);
 }
 
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+/* Microblaze currently doesn't implement kernel_stack_pointer() */
+#if defined(CONFIG_FUNCTION_GRAPH_TRACER) && !defined(CONFIG_MICROBLAZE)
 static __always_inline unsigned long
 function_get_true_parent_ip(unsigned long parent_ip, struct ftrace_regs *fregs)
 {
-- 
2.45.2

Re: [for-linus][PATCH 3/3] ftrace/microblaze: Do not find "true_parent" for return address

Posted by Michal Simek 1 year, 1 month ago


On 12/13/24 16:26, Steven Rostedt wrote:
> From: Steven Rostedt <rostedt@goodmis.org>
> 
> When function tracing and function graph tracing are both enabled (in
> different instances) 

What does this mean different instances? Two processes or two cores?


the "parent" of some of the function tracing events
> is "return_to_handler" which is the trampoline used by function graph
> tracing. To fix this, ftrace_get_true_parent_ip() was introduced that
> returns the "true" parent ip instead of the trampoline.
> 
> To do this, the ftrace_regs_get_stack_pointer() is used, which uses
> kernel_stack_pointer(). The problem is that microblaze does not implement
> kerenl_stack_pointer() so when function graph tracing is enabled, the
> build fails.
> 
> Modify the #ifdef check to the code around ftrace_get_true_parent_ip() to
> include !defined(CONFIG_MICROBLAZE) which will default it to just return
> the parent ip passed in, which may still be the ip of the function garph

here is typo.

> trampoline.
> 
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Al Viro <viro@ZenIV.linux.org.uk>
> Cc: Michal Simek <monstr@monstr.eu>
> Link: https://lore.kernel.org/20241211153634.69c75afa@batman.local.home
> Fixes: 60b1f578b578 ("ftrace: Get the true parent ip for function tracer")
> Reported-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>   kernel/trace/trace_functions.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> index 74c353164ca1..a75d107a45f8 100644
> --- a/kernel/trace/trace_functions.c
> +++ b/kernel/trace/trace_functions.c
> @@ -176,7 +176,8 @@ static void function_trace_start(struct trace_array *tr)
>   	tracing_reset_online_cpus(&tr->array_buffer);
>   }
>   
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +/* Microblaze currently doesn't implement kernel_stack_pointer() */

Does it mean that this function should depends on ARCH_HAS_CURRENT_STACK_POINTER 
instead of name the architecture?


> +#if defined(CONFIG_FUNCTION_GRAPH_TRACER) && !defined(CONFIG_MICROBLAZE)
>   static __always_inline unsigned long
>   function_get_true_parent_ip(unsigned long parent_ip, struct ftrace_regs *fregs)
>   {

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP/Versal ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal/Versal NET SoCs
TF-A maintainer - Xilinx ZynqMP/Versal/Versal NET SoCs

Re: [for-linus][PATCH 3/3] ftrace/microblaze: Do not find "true_parent" for return address

Posted by Al Viro 1 year, 1 month ago

On Fri, Dec 13, 2024 at 04:39:29PM +0100, Michal Simek wrote:

> > diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> > index 74c353164ca1..a75d107a45f8 100644
> > --- a/kernel/trace/trace_functions.c
> > +++ b/kernel/trace/trace_functions.c
> > @@ -176,7 +176,8 @@ static void function_trace_start(struct trace_array *tr)
> >   	tracing_reset_online_cpus(&tr->array_buffer);
> >   }
> > -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > +/* Microblaze currently doesn't implement kernel_stack_pointer() */
> 
> Does it mean that this function should depends on
> ARCH_HAS_CURRENT_STACK_POINTER instead of name the architecture?

Nope.  ARCH_HAS_CURRENT_STACK_POINTER == "there's a current_stack_pointer variable"
(presumably something like register unsigned long current_stack_pointer asm("r1");
in case of microblaze).  kernel_stack_pointer() is "here's pt_regs, give me the
kernel stack pointer stored in it (assuming it _is_ stored there)".

And what ftrace code really want is "here's the structure formed by _mcount();
give me the kernel stack pointer at the time of _mcount() entry".  _IF_ that
structure is pt_regs (fairly common) and if there's kernel_stack_pointer(),
we get the default implementation of that helper in linux/ftrace_regs.h:

#define ftrace_regs_get_stack_pointer(fregs) \
        kernel_stack_pointer(&arch_ftrace_regs(fregs)->regs)

If it's not pt_regs, you are expected to define HAVE_ARCH_FTRACE_REGS, define
struct __arch_ftrace_regs to match whatever layout you are using and provide
the set of ftrace_regs_...() helpers.

From my reading of your mcount.S, the layout is, indeed, different and
r1 is not stored there at all - something like

struct __arch_ftrace_regs {
	unsigned long r2, r3, r4, r6;
	unsigned long r7, r8, r9, r10;
	unsigned long r11, r12, r13, r14;
	unsigned long r16, r17, r18, r19;
	unsigned long r20, r21, r22, r23;
	unsigned long r24, r25, r26, r27;
	unsigned long r28, r29, r30, r31;
	unsigned long r5;
}

static inline unsigned long ftrace_regs_get_stack_pointer(struct ftrace_regs *regs)
{
	return (unsigned long)regs + sizeof(struct __arch_ftrace_regs) + 4;
}

Re: [for-linus][PATCH 3/3] ftrace/microblaze: Do not find "true_parent" for return address

Posted by Steven Rostedt 1 year, 1 month ago

On Fri, 13 Dec 2024 17:29:47 +0000
Al Viro <viro@zeniv.linux.org.uk> wrote:

> On Fri, Dec 13, 2024 at 04:39:29PM +0100, Michal Simek wrote:
> 
> > > diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> > > index 74c353164ca1..a75d107a45f8 100644
> > > --- a/kernel/trace/trace_functions.c
> > > +++ b/kernel/trace/trace_functions.c
> > > @@ -176,7 +176,8 @@ static void function_trace_start(struct trace_array *tr)
> > >   	tracing_reset_online_cpus(&tr->array_buffer);
> > >   }
> > > -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > > +/* Microblaze currently doesn't implement kernel_stack_pointer() */  
> > 
> > Does it mean that this function should depends on
> > ARCH_HAS_CURRENT_STACK_POINTER instead of name the architecture?  
> 
> Nope.  ARCH_HAS_CURRENT_STACK_POINTER == "there's a current_stack_pointer variable"
> (presumably something like register unsigned long current_stack_pointer asm("r1");
> in case of microblaze).  kernel_stack_pointer() is "here's pt_regs, give me the
> kernel stack pointer stored in it (assuming it _is_ stored there)".
> 
> And what ftrace code really want is "here's the structure formed by _mcount();
> give me the kernel stack pointer at the time of _mcount() entry".  _IF_ that
> structure is pt_regs (fairly common) and if there's kernel_stack_pointer(),
> we get the default implementation of that helper in linux/ftrace_regs.h:
> 
> #define ftrace_regs_get_stack_pointer(fregs) \
>         kernel_stack_pointer(&arch_ftrace_regs(fregs)->regs)
> 
> If it's not pt_regs, you are expected to define HAVE_ARCH_FTRACE_REGS, define
> struct __arch_ftrace_regs to match whatever layout you are using and provide
> the set of ftrace_regs_...() helpers.
> 
> >From my reading of your mcount.S, the layout is, indeed, different and  
> r1 is not stored there at all - something like
> 
> struct __arch_ftrace_regs {
> 	unsigned long r2, r3, r4, r6;
> 	unsigned long r7, r8, r9, r10;
> 	unsigned long r11, r12, r13, r14;
> 	unsigned long r16, r17, r18, r19;
> 	unsigned long r20, r21, r22, r23;
> 	unsigned long r24, r25, r26, r27;
> 	unsigned long r28, r29, r30, r31;
> 	unsigned long r5;
> }
> 
> static inline unsigned long ftrace_regs_get_stack_pointer(struct ftrace_regs *regs)
> {
> 	return (unsigned long)regs + sizeof(struct __arch_ftrace_regs) + 4;
> }

OK, so this is still unique for Microblaze. I'll keep the patch, but fix
the typo in the change log.

Michal,

Any objections?

-- Steve

Re: [for-linus][PATCH 3/3] ftrace/microblaze: Do not find "true_parent" for return address

Posted by Steven Rostedt 1 year, 1 month ago

On Fri, 13 Dec 2024 16:39:29 +0100
Michal Simek <monstr@monstr.eu> wrote:

> On 12/13/24 16:26, Steven Rostedt wrote:
> > From: Steven Rostedt <rostedt@goodmis.org>
> > 
> > When function tracing and function graph tracing are both enabled (in
> > different instances)   
> 
> What does this mean different instances? Two processes or two cores?

Trace instances:

 # mkdir /sys/kernel/tracing/instances/foo
 # echo function > /sys/kernel/tracing/instances/foo/current_tracer

 # mkdir /sys/kernel/tracing/instances/bar
 # echo function_graph > /sys/kernel/tracing/instances/bar/current_tracer

> 
> 
> the "parent" of some of the function tracing events
> > is "return_to_handler" which is the trampoline used by function graph
> > tracing. To fix this, ftrace_get_true_parent_ip() was introduced that
> > returns the "true" parent ip instead of the trampoline.
> > 
> > To do this, the ftrace_regs_get_stack_pointer() is used, which uses
> > kernel_stack_pointer(). The problem is that microblaze does not implement
> > kerenl_stack_pointer() so when function graph tracing is enabled, the
> > build fails.
> > 
> > Modify the #ifdef check to the code around ftrace_get_true_parent_ip() to
> > include !defined(CONFIG_MICROBLAZE) which will default it to just return
> > the parent ip passed in, which may still be the ip of the function garph  
> 
> here is typo.

Oops, thanks!

> 
> > trampoline.
> > 
> > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Cc: Al Viro <viro@ZenIV.linux.org.uk>
> > Cc: Michal Simek <monstr@monstr.eu>
> > Link: https://lore.kernel.org/20241211153634.69c75afa@batman.local.home
> > Fixes: 60b1f578b578 ("ftrace: Get the true parent ip for function tracer")
> > Reported-by: Al Viro <viro@zeniv.linux.org.uk>
> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> > ---
> >   kernel/trace/trace_functions.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> > index 74c353164ca1..a75d107a45f8 100644
> > --- a/kernel/trace/trace_functions.c
> > +++ b/kernel/trace/trace_functions.c
> > @@ -176,7 +176,8 @@ static void function_trace_start(struct trace_array *tr)
> >   	tracing_reset_online_cpus(&tr->array_buffer);
> >   }
> >   
> > -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > +/* Microblaze currently doesn't implement kernel_stack_pointer() */  
> 
> Does it mean that this function should depends on ARCH_HAS_CURRENT_STACK_POINTER 
> instead of name the architecture?

Hmm, I was looking for a more generic option and didn't find one. I don't
think I looked well enough as that could be what we want. Let me check.

I'll drop this patch for now then.

> 
> 
> > +#if defined(CONFIG_FUNCTION_GRAPH_TRACER) && !defined(CONFIG_MICROBLAZE)
> >   static __always_inline unsigned long
> >   function_get_true_parent_ip(unsigned long parent_ip, struct ftrace_regs *fregs)
> >   {  
> 
> Thanks,
> Michal
> 

Thanks for the review,

-- Steve