From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759752AbZDWSja (ORCPT ); Thu, 23 Apr 2009 14:39:30 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1759660AbZDWSjL (ORCPT ); Thu, 23 Apr 2009 14:39:11 -0400 Received: from metis.ext.pengutronix.de ([92.198.50.35]:57644 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759641AbZDWSjI (ORCPT ); Thu, 23 Apr 2009 14:39:08 -0400 Date: Thu, 23 Apr 2009 20:39:05 +0200 From: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= To: Tim Bird Cc: linux kernel , Steven Rostedt Subject: Re: [PATCH] Add function graph tracer support for ARM Message-ID: <20090423183905.GA8383@pengutronix.de> References: <49F0AEA2.5010309@am.sony.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <49F0AEA2.5010309@am.sony.com> User-Agent: Mutt/1.5.18 (2008-05-17) X-SA-Exim-Connect-IP: 2001:6f8:1178:2:215:17ff:fe12:23b0 X-SA-Exim-Mail-From: ukl@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello, On Thu, Apr 23, 2009 at 11:08:34AM -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 the threadinfo > structure to support a return trampoline. > > I wrote a separate program to scan the kernel assembly > output and validate that callframes for mcount are > generated properly. > > Much of the code in arch/arm/kernel/ftrace.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. > > FIXTHIS indicates questions or areas of uncertainty. > > This works for me with a gcc 4.1.1 compiler on an > OMAP OSK board, with kernel 2.6.29. > > The use of "already_here" works around trace entry nesting > (I believe due to instrumentation of either cpu_clock or > raw_smp_processor_id). Advice on the best way to deal > with this would be appreciated. (that is, whether > to use the guard variable, or just un-instrument the > offending routines). > > Thanks for any feedback. > > Signed-off-by: According to Documentation/SubmittingPatches you need to provide your real name in the S-o-b line. For the lazy of us, can you point me^Uus to some documentation how to use the graph tracer? > --- 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/include/asm/ftrace.h > +++ b/arch/arm/include/asm/ftrace.h > @@ -7,8 +7,32 @@ > > #ifndef __ASSEMBLY__ > extern void mcount(void); > -#endif > +#endif /* __ASSEMBLY__ */ IMHO this belongs into a "clean up first patch" > -#endif > +#endif /* CONFIG_FUNCTION_TRACER */ > + > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER > + > +#ifndef __ASSEMBLY__ > + > +/* > + * Stack of return addresses for functions of a thread. > + * Used in struct thread_info > + */ > +struct ftrace_ret_stack { > + unsigned long ret; > + unsigned long func; > + unsigned long long calltime; > +}; > + > +/* > + * Primary handler of a function return. > + * It relays on ftrace_return_to_handler. > + * Defined in entry-common.S > + */ > +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. > +#endif /* __ASSEMBLY__ */ > +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */ > > #endif /* _ASM_ARM_FTRACE */ > --- 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. > @@ -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? > 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 > @@ -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? > trace: > ldr r1, [fp, #-4] @ lr of instrumented routine > @@ -147,6 +155,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 > --- a/arch/arm/kernel/ftrace.c > +++ b/arch/arm/kernel/ftrace.c > @@ -16,6 +16,8 @@ > #include > #include > > +#ifdef CONFIG_DYNAMIC_FTRACE > + > #define PC_OFFSET 8 > #define BL_OPCODE 0xeb000000 > #define BL_OFFSET_MASK 0x00ffffff > @@ -101,3 +103,147 @@ int __init ftrace_dyn_arch_init(void *da > ftrace_mcount_set(data); > return 0; > } > + > +#endif /* CONFIG_DYNAMIC_FTRACE */ > + > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER > + > +/* Add a function return address to the trace stack on thread info.*/ nitpick: add a space between "." and "*/" please. > +static int push_return_trace(unsigned long ret, unsigned long long time, > + unsigned long func, int *depth) > +{ > + int index; > + > + if (!current->ret_stack) > + return -EBUSY; > + > + /* The return trace stack is full */ > + if (current->curr_ret_stack == FTRACE_RETFUNC_DEPTH - 1) { > + atomic_inc(¤t->trace_overrun); > + return -EBUSY; > + } > + > + index = ++current->curr_ret_stack; > + barrier(); > + current->ret_stack[index].ret = ret; > + current->ret_stack[index].func = func; > + current->ret_stack[index].calltime = time; > + *depth = index; > + > + return 0; > +} > + > +/* > + * Retrieve a function return address from the trace stack on thread info. > + * set the address to return to in *ret > + */ > +static void pop_return_trace(struct ftrace_graph_ret *trace, unsigned long *ret) > +{ > + int index; > + > + index = current->curr_ret_stack; > + > + if (unlikely(index < 0)) { > + ftrace_graph_stop(); > + WARN_ON(1); > + /* Might as well panic, otherwise we have no where to go */ > + *ret = (unsigned long)panic; > + return; > + } > + > + *ret = current->ret_stack[index].ret; > + trace->func = current->ret_stack[index].func; > + trace->calltime = current->ret_stack[index].calltime; > + trace->overrun = atomic_read(¤t->trace_overrun); > + trace->depth = index; > + barrier(); > + current->curr_ret_stack--; > +} > + > +int already_here; /* starts at 0 */ > + > +/* > + * Send the trace to the ring-buffer > + * @return the original return address > + */ > +unsigned long ftrace_return_to_handler(void) > +{ > + struct ftrace_graph_ret trace; > + unsigned long ret; > + > + /* guard against trace entry while handling a trace exit */ > + already_here++; > + > + pop_return_trace(&trace, &ret); > + trace.rettime = cpu_clock(raw_smp_processor_id()); > + ftrace_graph_return(&trace); > + > + if (unlikely(!ret)) { > + ftrace_graph_stop(); > + WARN_ON(1); > + /* Might as well panic. Where else to go? */ > + ret = (unsigned long)panic; > + } > + > + already_here--; > + return ret; > +} > + > +/* > + * 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; > + unsigned long long calltime; > + > + struct ftrace_graph_ent trace; > + unsigned long return_hooker = (unsigned long) > + &return_to_handler; > + > + if (already_here) > + return; > + already_here++; > + > + if (unlikely(atomic_read(¤t->tracing_graph_pause))) { > + already_here--; > + return; > + } > + > + /* FIXTHIS - need a local_irqsave here?, (to fend off ints?) */ > + > + /* FIXTHIS - x86 protects against a fault during this operation!*/ > + old = *parent; > + *parent = return_hooker; > + > + if (unlikely(!__kernel_text_address(old))) { > + ftrace_graph_stop(); > + *parent = old; > + WARN_ON(1); > + already_here--; > + return; > + } > + > + /* FIXTHIS - trace entry appears to nest inside following */ > + calltime = cpu_clock(raw_smp_processor_id()); > + > + if (push_return_trace(old, calltime, > + self_addr, &trace.depth) == -EBUSY) { > + *parent = old; > + already_here--; > + return; > + } > + > + trace.func = self_addr; > + > + /* Only trace if the calling function expects to */ > + if (!ftrace_graph_entry(&trace)) { > + current->curr_ret_stack--; > + *parent = old; > + } > + > + already_here--; > +} > +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */ > --- 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. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ |