Re: [PATCH] Add function graph tracer support for ARM

[Tim Bird]

Uwe ï wrote:
>> Signed-off-by: <tim.bird@xxxxxxxxxxx>
> According to Documentation/SubmittingPatches you need to provide your
> real name in the S-o-b line.

OK -that's embarrassing - I'll fix this.
scripts/checkpatch.pl didn't catch this.  I may look at adding
something to checkpatch.pl to catch empty names.

> For the lazy of us, can you point me^Uus to some documentation how to
> use the graph tracer?

It looks like Documentation/ftrace.txt is missing anything about
function graph tracing.

Should I add a section?

Here are some quick steps:

$ mount -t debugfs none /debug
$ cd /debug/tracing/
$ cat available_tracers
function_graph function sched_switch nop
$ echo function_graph >current_tracer
$ cat trace
# tracer: function_graph
#
# CPU OVERHEAD/DURATION            FUNCTION CALLS
# |   |        |                   |   |   |   |
 ------------------------------------------
 0)      --1       =>   events/-5
 ------------------------------------------

 0)               |                activate_task() {
 0)               |                  enqueue_task() {
 0)               |                    enqueue_task_fair() {
 0)               |                      update_curr() {
 0)   0.000 us    |                        calc_delta_mine();
 0)   0.000 us    |                        update_min_vruntime();
 0) + 61.035 us   |                      }
 0)   0.000 us    |                      place_entity();
 0)   0.000 us    |                      __enqueue_entity();
 0) + 91.552 us   |                    }
 0) ! 122.070 us  |                  }
 0) ! 152.588 us  |                }
 0)               |                check_preempt_wakeup() {
 0)   0.000 us    |                  update_curr();
 0)   0.000 us    |                  wakeup_preempt_entity();
 0) + 61.035 us   |                }
...

Clearly, my clock stinks, but that's a separate issue.

>>...
>> +extern void return_to_handler(void);
> AFAIK extern doesn't change anything for function declarations.  I
> wouldn't write it myself, and not argue if you considered it
> important/correct/whatever.

It just feels a bit more natural to me. My mind reads it as
'this function is not in this file, go look elsewhere'.
You're right that it doesn't change anything.
If this is preferred, I have no problem removing it.

...
>> --- a/arch/arm/kernel/Makefile
>> +++ b/arch/arm/kernel/Makefile
>> @@ -4,7 +4,7 @@
>>
>>  AFLAGS_head.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
>>
>> -ifdef CONFIG_DYNAMIC_FTRACE
>> +ifdef CONFIG_FUNCTION_TRACER
>>  CFLAGS_REMOVE_ftrace.o = -pg
>>  endif
> Why not simply remove the ifdef?  CFLAGS_REMOVE_ftrace.o shouldn't hurt
> if ftrace.o isn't compiled.

Good point.  I'll check this in the latest code, and remove
the #ifdef if it's still there.  I can't imagine
any circumstances when you'd want to trace any of
the routines in ftrace.c


>> @@ -23,6 +23,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.o
> I there a reason that CONFIG_DYNAMIC_FTRACE and
> CONFIG_FUNCTION_GRAPH_TRACER share the same source file?
ftrace.c has stuff from both, so you want it "turned on"
if one or the other (or both) are configed on.

I'll look at whether it makes sense to split these two into
separate files.  This issue may be resolved by the code
movement that Frederic Weisbecker mentioned in his e-mail.


>> --- a/arch/arm/kernel/entry-common.S
>> +++ b/arch/arm/kernel/entry-common.S
>> @@ -135,8 +135,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
> If ftrace_trace_function != ftrace_stub then ftrace_graph_caller isn't
> called.  Is this correct?
My comment is possibly misleading here.  The sense of the
'!=' includes the 'n' portion of the 'bne' on the following line.
So the logic is:
if *ftrace_graph_return != ftrace stub, branch to ftrace_graph_caller
or in other words
if *ftrace_graph_return == ftrace_stub, return doing nothing

Maybe I should just remove the comment and let the code
speak for itself?  (or maybe move the comment down one line?)

...
>> +/* Add a function return address to the trace stack on thread info.*/
> nitpick: add a space between "." and "*/" please.

OK - nitpicks appreciated as well :-)

>> --- a/arch/arm/kernel/vmlinux.lds.S
>> +++ b/arch/arm/kernel/vmlinux.lds.S
>> @@ -95,6 +95,7 @@ SECTIONS
>>  			SCHED_TEXT
>>  			LOCK_TEXT
>>  			KPROBES_TEXT
>> +			IRQENTRY_TEXT
>>  #ifdef CONFIG_MMU
>>  			*(.fixup)
>>  #endif
> Is this only needed for graph tracer?  If not this should be broken out.

As far as I can tell, yes.
This evaporates into nothing if the graph tracer is not present.

Thanks very much for the feedback!  I'll try to update the patch
based on your and Frederic Weisbecker's feedback shortly.
 -- Tim

=============================
Tim Bird
Architecture Group Chair, CE Linux Forum
Senior Staff Engineer, Sony Corporation of America
=============================

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/