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

[Frederic Weisbecker <fweisbec@gmail.com>]

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