[PATCH 2/2] ftrace: add function-graph tracer support for ARM v2

[PATCH 2/2] ftrace: add function-graph tracer support for ARM v2

[Tim Bird]

Add ftrace function-graph tracer support for ARM.

This includes adding code in mcount to check for
(and call) a registered function graph trace entry
routine, and adding infrastructure to support a return
trampoline to the threadinfo structure.

The code in arch/arm/kernel/ftrace_return.c was
cloned from arch/x86/kernel/ftrace.c

IRQENTRY_TEXT was added to vmlinux.lds.S (to eliminate
a compiler error on kernel/trace/trace_functions_graph.c),
although no routines were marked as __irq_entry.

This works for me with a gcc 4.1.1 compiler on an
OMAP OSK board.  This is against -tip (or at least,
-tip a few weeks ago - 2.6.30-rc3)

This (v2) patch addresses previously received feedback,
except for one issue.  In this version of the code,
a function_graph tracer overrides regular function
tracers.  I'll try to address that in a subsequent
patch, but I didn't want to go too long without posting
something.

Note: this code is much cleaner now that much of the
common return-handling code was moved into
kernel/trace_functions_graph.c

Signed-off-by: Tim Bird <tim.bird@am.sony.com>
---
 arch/arm/Kconfig                |    1
 arch/arm/kernel/Makefile        |    4 +--
 arch/arm/kernel/entry-common.S  |   34 ++++++++++++++++++++++++++++++
 arch/arm/kernel/ftrace_return.c |   44 ++++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/vmlinux.lds.S   |    1
 5 files changed, 81 insertions(+), 3 deletions(-)

--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -17,6 +17,7 @@ config ARM
 	select HAVE_KPROBES if (!XIP_KERNEL)
 	select HAVE_KRETPROBES if (HAVE_KPROBES)
 	select HAVE_FUNCTION_TRACER if (!XIP_KERNEL)
+	select HAVE_FUNCTION_GRAPH_TRACER if (!XIP_KERNEL)
 	select HAVE_GENERIC_DMA_COHERENT
 	help
 	  The ARM series is a line of low-power-consumption RISC chip designs
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -4,9 +4,8 @@

 AFLAGS_head.o := -DTEXT_OFFSET=$(TEXT_OFFSET)

-ifdef CONFIG_DYNAMIC_FTRACE
 CFLAGS_REMOVE_ftrace.o = -pg
-endif
+CFLAGS_REMOVE_ftrace_return.o = -pg

 # Object file lists.

@@ -23,6 +22,7 @@ obj-$(CONFIG_ISA_DMA)		+= dma-isa.o
 obj-$(CONFIG_PCI)		+= bios32.o isa.o
 obj-$(CONFIG_SMP)		+= smp.o
 obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o
+obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace_return.o
 obj-$(CONFIG_KEXEC)		+= machine_kexec.o relocate_kernel.o
 obj-$(CONFIG_KPROBES)		+= kprobes.o kprobes-decode.o
 obj-$(CONFIG_ATAGS_PROC)	+= atags.o
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -112,6 +112,11 @@ ENTRY(mcount)
 	mov r0, lr
 	sub r0, r0, #MCOUNT_INSN_SIZE

+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	@ FIXTHIS - DYNAMIC_FTRACE does not support function graph tracing
+	@ when the dynamic work is revived, this should be supported as well
+#endif
+
 	.globl mcount_call
 mcount_call:
 	bl ftrace_stub
@@ -139,8 +144,16 @@ ENTRY(mcount)
 	adr r0, ftrace_stub
 	cmp r0, r2
 	bne trace
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	ldr r1, =ftrace_graph_return
+	ldr r2, [r1]
+	cmp r0, r2		@ if *ftrace_graph_return != ftrace_stub
+	bne ftrace_graph_caller
+#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
+
 	ldr lr, [fp, #-4]			@ restore lr
-	ldmia sp!, {r0-r3, pc}
+	ldmia sp!, {r0-r3, pc}			@ return doing nothing

 trace:
 	ldr r1, [fp, #-4]			@ lr of instrumented routine
@@ -151,6 +164,25 @@ trace:
 	mov lr, r1				@ restore lr
 	ldmia sp!, {r0-r3, pc}

+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+ENTRY(ftrace_graph_caller)
+	sub r0, fp, #4			@ &lr of instrumented routine (&parent)
+	mov r1, lr			@ instrumented routine (func)
+	sub r1, r1, #MCOUNT_INSN_SIZE
+	bl prepare_ftrace_return
+	ldr lr, [fp, #-4]		@ restore lr
+	ldmia sp!, {r0-r3, pc}
+
+	.globl return_to_handler
+return_to_handler:
+	stmdb sp!, {r0-r3}
+	bl ftrace_return_to_handler
+	mov lr, r0			@ r0 has real ret addr
+	ldmia sp!, {r0-r3}
+	mov pc, lr
+
+#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
+
 #endif /* CONFIG_DYNAMIC_FTRACE */

 	.globl ftrace_stub
--- /dev/null
+++ b/arch/arm/kernel/ftrace_return.c
@@ -0,0 +1,44 @@
+/*
+ * function return tracing support.
+ *
+ * Copyright (C) 2009 Tim Bird <tim.bird@am.sony.com>
+ *
+ * For licencing details, see COPYING.
+ *
+ * Defines routine needed for ARM return trampoline for tracing
+ * function exits.
+ */
+
+#include <linux/ftrace.h>
+
+/*
+ * Hook the return address and push it in the stack of return addrs
+ * in current thread info.
+ */
+void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
+{
+	unsigned long old;
+
+	struct ftrace_graph_ent trace;
+	unsigned long return_hooker = (unsigned long)
+				&return_to_handler;
+
+	if (unlikely(atomic_read(&current->tracing_graph_pause)))
+		return;
+
+	old = *parent;
+	*parent = return_hooker;
+
+	if (ftrace_push_return_trace(old, self_addr, &trace.depth) == -EBUSY) {
+		*parent = old;
+		return;
+	}
+
+	trace.func = self_addr;
+
+	/* Only trace if the calling function expects to */
+	if (!ftrace_graph_entry(&trace)) {
+		current->curr_ret_stack--;
+		*parent = old;
+	}
+}
--- a/arch/arm/kernel/vmlinux.lds.S
+++ b/arch/arm/kernel/vmlinux.lds.S
@@ -99,6 +99,7 @@ SECTIONS
 			SCHED_TEXT
 			LOCK_TEXT
 			KPROBES_TEXT
+			IRQENTRY_TEXT
 #ifdef CONFIG_MMU
 			*(.fixup)
 #endif

Re: [PATCH 2/2] ftrace: add function-graph tracer support for ARM v2

[Frederic Weisbecker]

On Fri, May 01, 2009 at 03:38:07PM -0700, Tim Bird wrote:
> Add ftrace function-graph tracer support for ARM.
> 
> This includes adding code in mcount to check for
> (and call) a registered function graph trace entry
> routine, and adding infrastructure to support a return
> trampoline to the threadinfo structure.
> 
> The code in arch/arm/kernel/ftrace_return.c was
> cloned from arch/x86/kernel/ftrace.c
> 
> IRQENTRY_TEXT was added to vmlinux.lds.S (to eliminate
> a compiler error on kernel/trace/trace_functions_graph.c),
> although no routines were marked as __irq_entry.
> 
> This works for me with a gcc 4.1.1 compiler on an
> OMAP OSK board.  This is against -tip (or at least,
> -tip a few weeks ago - 2.6.30-rc3)
> 
> This (v2) patch addresses previously received feedback,
> except for one issue.  In this version of the code,
> a function_graph tracer overrides regular function
> tracers.  I'll try to address that in a subsequent
> patch, but I didn't want to go too long without posting
> something.
> 
> Note: this code is much cleaner now that much of the
> common return-handling code was moved into
> kernel/trace_functions_graph.c
> 
> Signed-off-by: Tim Bird <tim.bird@am.sony.com>
> ---
>  arch/arm/Kconfig                |    1
>  arch/arm/kernel/Makefile        |    4 +--
>  arch/arm/kernel/entry-common.S  |   34 ++++++++++++++++++++++++++++++
>  arch/arm/kernel/ftrace_return.c |   44 ++++++++++++++++++++++++++++++++++++++++
>  arch/arm/kernel/vmlinux.lds.S   |    1
>  5 files changed, 81 insertions(+), 3 deletions(-)
> 
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -17,6 +17,7 @@ config ARM
>  	select HAVE_KPROBES if (!XIP_KERNEL)
>  	select HAVE_KRETPROBES if (HAVE_KPROBES)
>  	select HAVE_FUNCTION_TRACER if (!XIP_KERNEL)
> +	select HAVE_FUNCTION_GRAPH_TRACER if (!XIP_KERNEL)
>  	select HAVE_GENERIC_DMA_COHERENT
>  	help
>  	  The ARM series is a line of low-power-consumption RISC chip designs
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -4,9 +4,8 @@
> 
>  AFLAGS_head.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> 
> -ifdef CONFIG_DYNAMIC_FTRACE
>  CFLAGS_REMOVE_ftrace.o = -pg
> -endif
> +CFLAGS_REMOVE_ftrace_return.o = -pg
> 
>  # Object file lists.
> 
> @@ -23,6 +22,7 @@ obj-$(CONFIG_ISA_DMA)		+= dma-isa.o
>  obj-$(CONFIG_PCI)		+= bios32.o isa.o
>  obj-$(CONFIG_SMP)		+= smp.o
>  obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o
> +obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace_return.o
>  obj-$(CONFIG_KEXEC)		+= machine_kexec.o relocate_kernel.o
>  obj-$(CONFIG_KPROBES)		+= kprobes.o kprobes-decode.o
>  obj-$(CONFIG_ATAGS_PROC)	+= atags.o
> --- a/arch/arm/kernel/entry-common.S
> +++ b/arch/arm/kernel/entry-common.S
> @@ -112,6 +112,11 @@ ENTRY(mcount)
>  	mov r0, lr
>  	sub r0, r0, #MCOUNT_INSN_SIZE
> 
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +	@ FIXTHIS - DYNAMIC_FTRACE does not support function graph tracing
> +	@ when the dynamic work is revived, this should be supported as well
> +#endif
> +
>  	.globl mcount_call
>  mcount_call:
>  	bl ftrace_stub
> @@ -139,8 +144,16 @@ ENTRY(mcount)
>  	adr r0, ftrace_stub
>  	cmp r0, r2
>  	bne trace
> +
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +	ldr r1, =ftrace_graph_return
> +	ldr r2, [r1]
> +	cmp r0, r2		@ if *ftrace_graph_return != ftrace_stub
> +	bne ftrace_graph_caller
> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> +
>  	ldr lr, [fp, #-4]			@ restore lr
> -	ldmia sp!, {r0-r3, pc}
> +	ldmia sp!, {r0-r3, pc}			@ return doing nothing
> 
>  trace:
>  	ldr r1, [fp, #-4]			@ lr of instrumented routine
> @@ -151,6 +164,25 @@ trace:
>  	mov lr, r1				@ restore lr
>  	ldmia sp!, {r0-r3, pc}
> 
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +ENTRY(ftrace_graph_caller)
> +	sub r0, fp, #4			@ &lr of instrumented routine (&parent)
> +	mov r1, lr			@ instrumented routine (func)
> +	sub r1, r1, #MCOUNT_INSN_SIZE
> +	bl prepare_ftrace_return
> +	ldr lr, [fp, #-4]		@ restore lr
> +	ldmia sp!, {r0-r3, pc}
> +
> +	.globl return_to_handler
> +return_to_handler:
> +	stmdb sp!, {r0-r3}
> +	bl ftrace_return_to_handler
> +	mov lr, r0			@ r0 has real ret addr
> +	ldmia sp!, {r0-r3}
> +	mov pc, lr
> +
> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */



Looks good.



>  #endif /* CONFIG_DYNAMIC_FTRACE */
> 
>  	.globl ftrace_stub
> --- /dev/null
> +++ b/arch/arm/kernel/ftrace_return.c



Because it is more commonly known as function graph,
I would suggest ftrace_graph.c so that people can
find it more easily.



> @@ -0,0 +1,44 @@
> +/*
> + * function return tracing support.
> + *
> + * Copyright (C) 2009 Tim Bird <tim.bird@am.sony.com>
> + *
> + * For licencing details, see COPYING.
> + *
> + * Defines routine needed for ARM return trampoline for tracing
> + * function exits.
> + */
> +
> +#include <linux/ftrace.h>
> +
> +/*
> + * Hook the return address and push it in the stack of return addrs
> + * in current thread info.
> + */
> +void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
> +{
> +	unsigned long old;
> +
> +	struct ftrace_graph_ent trace;
> +	unsigned long return_hooker = (unsigned long)
> +				&return_to_handler;
> +
> +	if (unlikely(atomic_read(&current->tracing_graph_pause)))
> +		return;
> +
> +	old = *parent;
> +	*parent = return_hooker;
> +
> +	if (ftrace_push_return_trace(old, self_addr, &trace.depth) == -EBUSY) {
> +		*parent = old;
> +		return;
> +	}
> +
> +	trace.func = self_addr;
> +
> +	/* Only trace if the calling function expects to */
> +	if (!ftrace_graph_entry(&trace)) {
> +		current->curr_ret_stack--;
> +		*parent = old;
> +	}
> +}
> --- a/arch/arm/kernel/vmlinux.lds.S
> +++ b/arch/arm/kernel/vmlinux.lds.S
> @@ -99,6 +99,7 @@ SECTIONS
>  			SCHED_TEXT
>  			LOCK_TEXT
>  			KPROBES_TEXT
> +			IRQENTRY_TEXT
>  #ifdef CONFIG_MMU
>  			*(.fixup)
>  #endif


Well, it looks good to me.
May be you can also add the fault protection against the return address,
as a paranoid check. But that can be done later.

Also I wonder how far we are from the dynamic ftrace support, which seems
half implemented or broken or not tested for a while?

Thanks!

Acked-by: Frederic Weisbecker <fweisbec@gmail.com>

Re: [PATCH 2/2] ftrace: add function-graph tracer support for ARM v2

[Uwe Kleine-König]

Hello,

> Also I wonder how far we are from the dynamic ftrace support, which seems
> half implemented or broken or not tested for a while?
Commit 07c4cc1cdaa08fcb6c0275dd7be49eae37260169 suggests that "the new
MCOUNT_RECORD method" needs to be implemented.  I don't know the
details, though (yet).

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

Re: [PATCH 2/2] ftrace: add function-graph tracer support for ARM v2

[Steven Rostedt]

On Sat, 2 May 2009, Uwe Kleine-K?nig wrote:

> Hello,
> 
> > Also I wonder how far we are from the dynamic ftrace support, which seems
> > half implemented or broken or not tested for a while?
> Commit 07c4cc1cdaa08fcb6c0275dd7be49eae37260169 suggests that "the new
> MCOUNT_RECORD method" needs to be implemented.  I don't know the
> details, though (yet).

The new MCOUNT_RECORD method means to update scripts/recordmcount.pl. 
There is an "arm" section in that script added by:

 e58918ab9d4cd375f6d842e6d88cf4d7a55cbfcc

-- Steve

Re: [PATCH 2/2] ftrace: add function-graph tracer support for ARM v2

[Frédéric Weisbecker]

2009/5/2 Frederic Weisbecker <fweisbec@gmail.com>:
> On Fri, May 01, 2009 at 03:38:07PM -0700, Tim Bird wrote:
>> Add ftrace function-graph tracer support for ARM.
>>
>> This includes adding code in mcount to check for
>> (and call) a registered function graph trace entry
>> routine, and adding infrastructure to support a return
>> trampoline to the threadinfo structure.
>>
>> The code in arch/arm/kernel/ftrace_return.c was
>> cloned from arch/x86/kernel/ftrace.c
>>
>> IRQENTRY_TEXT was added to vmlinux.lds.S (to eliminate
>> a compiler error on kernel/trace/trace_functions_graph.c),
>> although no routines were marked as __irq_entry.
>>
>> This works for me with a gcc 4.1.1 compiler on an
>> OMAP OSK board.  This is against -tip (or at least,
>> -tip a few weeks ago - 2.6.30-rc3)
>>
>> This (v2) patch addresses previously received feedback,
>> except for one issue.  In this version of the code,
>> a function_graph tracer overrides regular function
>> tracers.  I'll try to address that in a subsequent
>> patch, but I didn't want to go too long without posting
>> something.
>>
>> Note: this code is much cleaner now that much of the
>> common return-handling code was moved into
>> kernel/trace_functions_graph.c
>>
>> Signed-off-by: Tim Bird <tim.bird@am.sony.com>
>> ---
>>  arch/arm/Kconfig                |    1
>>  arch/arm/kernel/Makefile        |    4 +--
>>  arch/arm/kernel/entry-common.S  |   34 ++++++++++++++++++++++++++++++
>>  arch/arm/kernel/ftrace_return.c |   44 ++++++++++++++++++++++++++++++++++++++++
>>  arch/arm/kernel/vmlinux.lds.S   |    1
>>  5 files changed, 81 insertions(+), 3 deletions(-)


Ingo, Russell.
What do you think about this?

If you accept it to be merged, in which tree is it most suitable?
I guess it would have better chances to be tested in the Arm tree. But
it would be closer to latest changes in -tip, although
the function graph tracer hasn't been changed since -rc1 IIRC.

Frederic.


>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -17,6 +17,7 @@ config ARM
>>       select HAVE_KPROBES if (!XIP_KERNEL)
>>       select HAVE_KRETPROBES if (HAVE_KPROBES)
>>       select HAVE_FUNCTION_TRACER if (!XIP_KERNEL)
>> +     select HAVE_FUNCTION_GRAPH_TRACER if (!XIP_KERNEL)
>>       select HAVE_GENERIC_DMA_COHERENT
>>       help
>>         The ARM series is a line of low-power-consumption RISC chip designs
>> --- a/arch/arm/kernel/Makefile
>> +++ b/arch/arm/kernel/Makefile
>> @@ -4,9 +4,8 @@
>>
>>  AFLAGS_head.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
>>
>> -ifdef CONFIG_DYNAMIC_FTRACE
>>  CFLAGS_REMOVE_ftrace.o = -pg
>> -endif
>> +CFLAGS_REMOVE_ftrace_return.o = -pg
>>
>>  # Object file lists.
>>
>> @@ -23,6 +22,7 @@ obj-$(CONFIG_ISA_DMA)               += dma-isa.o
>>  obj-$(CONFIG_PCI)            += bios32.o isa.o
>>  obj-$(CONFIG_SMP)            += smp.o
>>  obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o
>> +obj-$(CONFIG_FUNCTION_GRAPH_TRACER)  += ftrace_return.o
>>  obj-$(CONFIG_KEXEC)          += machine_kexec.o relocate_kernel.o
>>  obj-$(CONFIG_KPROBES)                += kprobes.o kprobes-decode.o
>>  obj-$(CONFIG_ATAGS_PROC)     += atags.o
>> --- a/arch/arm/kernel/entry-common.S
>> +++ b/arch/arm/kernel/entry-common.S
>> @@ -112,6 +112,11 @@ ENTRY(mcount)
>>       mov r0, lr
>>       sub r0, r0, #MCOUNT_INSN_SIZE
>>
>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> +     @ FIXTHIS - DYNAMIC_FTRACE does not support function graph tracing
>> +     @ when the dynamic work is revived, this should be supported as well
>> +#endif
>> +
>>       .globl mcount_call
>>  mcount_call:
>>       bl ftrace_stub
>> @@ -139,8 +144,16 @@ ENTRY(mcount)
>>       adr r0, ftrace_stub
>>       cmp r0, r2
>>       bne trace
>> +
>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> +     ldr r1, =ftrace_graph_return
>> +     ldr r2, [r1]
>> +     cmp r0, r2              @ if *ftrace_graph_return != ftrace_stub
>> +     bne ftrace_graph_caller
>> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
>> +
>>       ldr lr, [fp, #-4]                       @ restore lr
>> -     ldmia sp!, {r0-r3, pc}
>> +     ldmia sp!, {r0-r3, pc}                  @ return doing nothing
>>
>>  trace:
>>       ldr r1, [fp, #-4]                       @ lr of instrumented routine
>> @@ -151,6 +164,25 @@ trace:
>>       mov lr, r1                              @ restore lr
>>       ldmia sp!, {r0-r3, pc}
>>
>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> +ENTRY(ftrace_graph_caller)
>> +     sub r0, fp, #4                  @ &lr of instrumented routine (&parent)
>> +     mov r1, lr                      @ instrumented routine (func)
>> +     sub r1, r1, #MCOUNT_INSN_SIZE
>> +     bl prepare_ftrace_return
>> +     ldr lr, [fp, #-4]               @ restore lr
>> +     ldmia sp!, {r0-r3, pc}
>> +
>> +     .globl return_to_handler
>> +return_to_handler:
>> +     stmdb sp!, {r0-r3}
>> +     bl ftrace_return_to_handler
>> +     mov lr, r0                      @ r0 has real ret addr
>> +     ldmia sp!, {r0-r3}
>> +     mov pc, lr
>> +
>> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
>
>
>
> Looks good.
>
>
>
>>  #endif /* CONFIG_DYNAMIC_FTRACE */
>>
>>       .globl ftrace_stub
>> --- /dev/null
>> +++ b/arch/arm/kernel/ftrace_return.c
>
>
>
> Because it is more commonly known as function graph,
> I would suggest ftrace_graph.c so that people can
> find it more easily.
>
>
>
>> @@ -0,0 +1,44 @@
>> +/*
>> + * function return tracing support.
>> + *
>> + * Copyright (C) 2009 Tim Bird <tim.bird@am.sony.com>
>> + *
>> + * For licencing details, see COPYING.
>> + *
>> + * Defines routine needed for ARM return trampoline for tracing
>> + * function exits.
>> + */
>> +
>> +#include <linux/ftrace.h>
>> +
>> +/*
>> + * Hook the return address and push it in the stack of return addrs
>> + * in current thread info.
>> + */
>> +void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
>> +{
>> +     unsigned long old;
>> +
>> +     struct ftrace_graph_ent trace;
>> +     unsigned long return_hooker = (unsigned long)
>> +                             &return_to_handler;
>> +
>> +     if (unlikely(atomic_read(&current->tracing_graph_pause)))
>> +             return;
>> +
>> +     old = *parent;
>> +     *parent = return_hooker;
>> +
>> +     if (ftrace_push_return_trace(old, self_addr, &trace.depth) == -EBUSY) {
>> +             *parent = old;
>> +             return;
>> +     }
>> +
>> +     trace.func = self_addr;
>> +
>> +     /* Only trace if the calling function expects to */
>> +     if (!ftrace_graph_entry(&trace)) {
>> +             current->curr_ret_stack--;
>> +             *parent = old;
>> +     }
>> +}
>> --- a/arch/arm/kernel/vmlinux.lds.S
>> +++ b/arch/arm/kernel/vmlinux.lds.S
>> @@ -99,6 +99,7 @@ SECTIONS
>>                       SCHED_TEXT
>>                       LOCK_TEXT
>>                       KPROBES_TEXT
>> +                     IRQENTRY_TEXT
>>  #ifdef CONFIG_MMU
>>                       *(.fixup)
>>  #endif
>
>
> Well, it looks good to me.
> May be you can also add the fault protection against the return address,
> as a paranoid check. But that can be done later.
>
> Also I wonder how far we are from the dynamic ftrace support, which seems
> half implemented or broken or not tested for a while?
>
> Thanks!
>
> Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
>

Re: [PATCH 2/2] ftrace: add function-graph tracer support for ARM v2

[Russell King - ARM Linux]

On Sat, May 09, 2009 at 06:14:32PM +0200, Frédéric Weisbecker wrote:
> Ingo, Russell.
> What do you think about this?

If it's suitable for the next merge window, lets get it queued up for
it.    What are the dependencies for the patch?  Does it rely on
anything queued in anyone elses tree (eg, the addition of
ftrace_return_to_handler) ?

However, I'm not sure that this code is entirely right (and I'm not
sure what's going on with this patch - my editor is randomly changing
the placement of characters in it.  Are you submitting patches using
UTF-8 characters in the code?)

> >> @@ -139,8 +144,16 @@ ENTRY(mcount)
> >>       adr r0, ftrace_stub
> >>       cmp r0, r2
> >>       bne trace

If this is r0 != ftrace_stub, go to trace.  So the next block will
only be executed if r0 /was/ ftrace_stub, in which case it can't be
ftrace_graph_return.

> >> +
> >> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> >> +     ldr r1, =ftrace_graph_return
> >> +     ldr r2, [r1]
> >> +     cmp r0, r2              @ if *ftrace_graph_return != ftrace_stub
> >> +     bne ftrace_graph_caller
> >> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> >> +
> >>       ldr lr, [fp, #-4]                       @ restore lr
> >> -     ldmia sp!, {r0-r3, pc}
> >> +     ldmia sp!, {r0-r3, pc}                  @ return doing nothing
> >>
> >>  trace:

So surely you want your code above placed here?

> >>       ldr r1, [fp, #-4]                       @ lr of instrumented routine
> >> @@ -151,6 +164,25 @@ trace:
> >>       mov lr, r1                              @ restore lr
> >>       ldmia sp!, {r0-r3, pc}
> >>
> >> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> >> +ENTRY(ftrace_graph_caller)
> >> +     sub r0, fp, #4                  @ &lr of instrumented routine (&parent)
> >> +     mov r1, lr                      @ instrumented routine (func)
> >> +     sub r1, r1, #MCOUNT_INSN_SIZE
> >> +     bl prepare_ftrace_return
> >> +     ldr lr, [fp, #-4]               @ restore lr
> >> +     ldmia sp!, {r0-r3, pc}
> >> +
> >> +     .globl return_to_handler
> >> +return_to_handler:
> >> +     stmdb sp!, {r0-r3}
> >> +     bl ftrace_return_to_handler
> >> +     mov lr, r0                      @ r0 has real ret addr
> >> +     ldmia sp!, {r0-r3}
> >> +     mov pc, lr
> >> +
> >> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> >
> >
> >
> > Looks good.
> >
> >
> >
> >>  #endif /* CONFIG_DYNAMIC_FTRACE */
> >>
> >>       .globl ftrace_stub
> >> --- /dev/null
> >> +++ b/arch/arm/kernel/ftrace_return.c
> >
> >
> >
> > Because it is more commonly known as function graph,
> > I would suggest ftrace_graph.c so that people can
> > find it more easily.
> >
> >
> >
> >> @@ -0,0 +1,44 @@
> >> +/*
> >> + * function return tracing support.
> >> + *
> >> + * Copyright (C) 2009 Tim Bird <tim.bird@am.sony.com>
> >> + *
> >> + * For licencing details, see COPYING.
> >> + *
> >> + * Defines routine needed for ARM return trampoline for tracing
> >> + * function exits.
> >> + */
> >> +
> >> +#include <linux/ftrace.h>
> >> +
> >> +/*
> >> + * Hook the return address and push it in the stack of return addrs
> >> + * in current thread info.
> >> + */
> >> +void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
> >> +{
> >> +     unsigned long old;
> >> +
> >> +     struct ftrace_graph_ent trace;
> >> +     unsigned long return_hooker = (unsigned long)
> >> +                             &return_to_handler;
> >> +
> >> +     if (unlikely(atomic_read(&current->tracing_graph_pause)))
> >> +             return;
> >> +
> >> +     old = *parent;
> >> +     *parent = return_hooker;
> >> +
> >> +     if (ftrace_push_return_trace(old, self_addr, &trace.depth) == -EBUSY) {
> >> +             *parent = old;
> >> +             return;
> >> +     }
> >> +
> >> +     trace.func = self_addr;
> >> +
> >> +     /* Only trace if the calling function expects to */
> >> +     if (!ftrace_graph_entry(&trace)) {
> >> +             current->curr_ret_stack--;
> >> +             *parent = old;
> >> +     }
> >> +}
> >> --- a/arch/arm/kernel/vmlinux.lds.S
> >> +++ b/arch/arm/kernel/vmlinux.lds.S
> >> @@ -99,6 +99,7 @@ SECTIONS
> >>                       SCHED_TEXT
> >>                       LOCK_TEXT
> >>                       KPROBES_TEXT
> >> +                     IRQENTRY_TEXT
> >>  #ifdef CONFIG_MMU
> >>                       *(.fixup)
> >>  #endif
> >
> >
> > Well, it looks good to me.
> > May be you can also add the fault protection against the return address,
> > as a paranoid check. But that can be done later.
> >
> > Also I wonder how far we are from the dynamic ftrace support, which seems
> > half implemented or broken or not tested for a while?
> >
> > Thanks!
> >
> > Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
> >
> 
> -------------------------------------------------------------------
> List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
> FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
> Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php

Re: [PATCH 2/2] ftrace: add function-graph tracer support for ARM v2

[Frederic Weisbecker]

On Fri, May 15, 2009 at 10:06:36PM +0100, Russell King - ARM Linux wrote:
> On Sat, May 09, 2009 at 06:14:32PM +0200, Frédéric Weisbecker wrote:
> > Ingo, Russell.
> > What do you think about this?
> 
> If it's suitable for the next merge window, lets get it queued up for
> it.    What are the dependencies for the patch?  Does it rely on
> anything queued in anyone elses tree (eg, the addition of
> ftrace_return_to_handler) ?



No, until now since -rc1 we hadn't any modifications on the function
graph tracer that affects arch code.
It should be fine with upstream tracing code.


 
> However, I'm not sure that this code is entirely right (and I'm not
> sure what's going on with this patch - my editor is randomly changing
> the placement of characters in it.  Are you submitting patches using
> UTF-8 characters in the code?)
> 
> > >> @@ -139,8 +144,16 @@ ENTRY(mcount)
> > >>       adr r0, ftrace_stub
> > >>       cmp r0, r2
> > >>       bne trace
> 
> If this is r0 != ftrace_stub, go to trace.  So the next block will
> only be executed if r0 /was/ ftrace_stub, in which case it can't be
> ftrace_graph_return.



Ah! This part concerns the function tracer.
Let's see how it looks like in the original code: (added more comments inside)


ENTRY(mcount)
	stmdb sp!, {r0-r3, lr}
	@ ftrace_trace_function points to the function tracer handler
	ldr r0, =ftrace_trace_function
	ldr r2, [r0]
	adr r0, ftrace_stub

@- check if the function tracer is running: ftrace_trace_function != ftrace_stub

	cmp r0, r2

@ if so, go to trace where we jump to the handler (mov pc, r2 in trace:)

	bne trace


	ldr lr, [fp, #-4]			@ restore lr
	ldmia sp!, {r0-r3, pc}

trace:
	ldr r1, [fp, #-4]			@ lr of instrumented routine
	mov r0, lr
	sub r0, r0, #MCOUNT_INSN_SIZE
	mov lr, pc
	mov pc, r2
	mov lr, r1				@ restore lr
	ldmia sp!, {r0-r3, pc}


See? trace actually only handles the function tracer, not the function graph tracer.

Now that we have the function graph tracer, the logic remains the same,
with a small difference:

- check if function tracer running (ftrace_trace_function != ftrace_stub)
- if so, then jump to trace
- otherwise, check if function graph tracer is running (ftrace_graph_return != ftrace_stub)
- if so, then jump to ftrace_graph_caller
- otherwise return

Hm?

Frederic.

> > >> +
> > >> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > >> +     ldr r1, =ftrace_graph_return
> > >> +     ldr r2, [r1]
> > >> +     cmp r0, r2              @ if *ftrace_graph_return != ftrace_stub
> > >> +     bne ftrace_graph_caller
> > >> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> > >> +
> > >>       ldr lr, [fp, #-4]                       @ restore lr
> > >> -     ldmia sp!, {r0-r3, pc}
> > >> +     ldmia sp!, {r0-r3, pc}                  @ return doing nothing
> > >>
> > >>  trace:
> 
> So surely you want your code above placed here?
> 
> > >>       ldr r1, [fp, #-4]                       @ lr of instrumented routine
> > >> @@ -151,6 +164,25 @@ trace:
> > >>       mov lr, r1                              @ restore lr
> > >>       ldmia sp!, {r0-r3, pc}
> > >>
> > >> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > >> +ENTRY(ftrace_graph_caller)
> > >> +     sub r0, fp, #4                  @ &lr of instrumented routine (&parent)
> > >> +     mov r1, lr                      @ instrumented routine (func)
> > >> +     sub r1, r1, #MCOUNT_INSN_SIZE
> > >> +     bl prepare_ftrace_return
> > >> +     ldr lr, [fp, #-4]               @ restore lr
> > >> +     ldmia sp!, {r0-r3, pc}
> > >> +
> > >> +     .globl return_to_handler
> > >> +return_to_handler:
> > >> +     stmdb sp!, {r0-r3}
> > >> +     bl ftrace_return_to_handler
> > >> +     mov lr, r0                      @ r0 has real ret addr
> > >> +     ldmia sp!, {r0-r3}
> > >> +     mov pc, lr
> > >> +
> > >> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> > >
> > >
> > >
> > > Looks good.
> > >
> > >
> > >
> > >>  #endif /* CONFIG_DYNAMIC_FTRACE */
> > >>
> > >>       .globl ftrace_stub
> > >> --- /dev/null
> > >> +++ b/arch/arm/kernel/ftrace_return.c
> > >
> > >
> > >
> > > Because it is more commonly known as function graph,
> > > I would suggest ftrace_graph.c so that people can
> > > find it more easily.
> > >
> > >
> > >
> > >> @@ -0,0 +1,44 @@
> > >> +/*
> > >> + * function return tracing support.
> > >> + *
> > >> + * Copyright (C) 2009 Tim Bird <tim.bird@am.sony.com>
> > >> + *
> > >> + * For licencing details, see COPYING.
> > >> + *
> > >> + * Defines routine needed for ARM return trampoline for tracing
> > >> + * function exits.
> > >> + */
> > >> +
> > >> +#include <linux/ftrace.h>
> > >> +
> > >> +/*
> > >> + * Hook the return address and push it in the stack of return addrs
> > >> + * in current thread info.
> > >> + */
> > >> +void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
> > >> +{
> > >> +     unsigned long old;
> > >> +
> > >> +     struct ftrace_graph_ent trace;
> > >> +     unsigned long return_hooker = (unsigned long)
> > >> +                             &return_to_handler;
> > >> +
> > >> +     if (unlikely(atomic_read(&current->tracing_graph_pause)))
> > >> +             return;
> > >> +
> > >> +     old = *parent;
> > >> +     *parent = return_hooker;
> > >> +
> > >> +     if (ftrace_push_return_trace(old, self_addr, &trace.depth) == -EBUSY) {
> > >> +             *parent = old;
> > >> +             return;
> > >> +     }
> > >> +
> > >> +     trace.func = self_addr;
> > >> +
> > >> +     /* Only trace if the calling function expects to */
> > >> +     if (!ftrace_graph_entry(&trace)) {
> > >> +             current->curr_ret_stack--;
> > >> +             *parent = old;
> > >> +     }
> > >> +}
> > >> --- a/arch/arm/kernel/vmlinux.lds.S
> > >> +++ b/arch/arm/kernel/vmlinux.lds.S
> > >> @@ -99,6 +99,7 @@ SECTIONS
> > >>                       SCHED_TEXT
> > >>                       LOCK_TEXT
> > >>                       KPROBES_TEXT
> > >> +                     IRQENTRY_TEXT
> > >>  #ifdef CONFIG_MMU
> > >>                       *(.fixup)
> > >>  #endif
> > >
> > >
> > > Well, it looks good to me.
> > > May be you can also add the fault protection against the return address,
> > > as a paranoid check. But that can be done later.
> > >
> > > Also I wonder how far we are from the dynamic ftrace support, which seems
> > > half implemented or broken or not tested for a while?
> > >
> > > Thanks!
> > >
> > > Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
> > >
> > 
> > -------------------------------------------------------------------
> > List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
> > FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
> > Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php

Re: [PATCH 2/2] ftrace: add function-graph tracer support for ARM v2

[Russell King - ARM Linux]

On Wed, May 20, 2009 at 04:34:35AM +0200, Frederic Weisbecker wrote:
> On Fri, May 15, 2009 at 10:06:36PM +0100, Russell King - ARM Linux wrote:
> > However, I'm not sure that this code is entirely right (and I'm not
> > sure what's going on with this patch - my editor is randomly changing
> > the placement of characters in it.  Are you submitting patches using
> > UTF-8 characters in the code?)
> > 
> > > >> @@ -139,8 +144,16 @@ ENTRY(mcount)
> > > >>       adr r0, ftrace_stub
> > > >>       cmp r0, r2
> > > >>       bne trace
> > 
> > If this is r0 != ftrace_stub, go to trace.  So the next block will
> > only be executed if r0 /was/ ftrace_stub, in which case it can't be
> > ftrace_graph_return.
> 
> Ah! This part concerns the function tracer.
> Let's see how it looks like in the original code: (added more comments
> inside)

You've completely missed the point I was trying to make.  Please go back
and re-read my email and think about precisely what's happening in the
assembly path, particularly which branches will be taken and which code
paths will be taken with various values of r2.