From patchwork Fri Jan 11 08:53:57 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: kpark3469@gmail.com X-Patchwork-Id: 1965171 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by patchwork1.kernel.org (Postfix) with ESMTP id D370C40B27 for ; Fri, 11 Jan 2013 08:57:25 +0000 (UTC) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1TtaNI-0003X5-Uo; Fri, 11 Jan 2013 08:54:05 +0000 Received: from mail-ia0-f172.google.com ([209.85.210.172]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1TtaND-0003WI-6F for linux-arm-kernel@lists.infradead.org; Fri, 11 Jan 2013 08:54:00 +0000 Received: by mail-ia0-f172.google.com with SMTP id u8so1353511iag.3 for ; Fri, 11 Jan 2013 00:53:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:x-received:in-reply-to:references:date:message-id :subject:from:to:cc:content-type; bh=33vThYHmKiuk1mu57hvwA5brGkjlrhXoc5Q2/9q2wZg=; b=yeqUBY8E6ITtA+pqhGOnq09SJyaDqVg5ApmgvOP+Q0tvhqgc8XK0aWWANeYjoWQYvc NGQz1DCWRmMiMTb82AYMnePPvuzqRR1qSh78GIPtaoKcTmGMCX/8kVWIe3FcN92DvKc3 hhYo+o6rPohMSKatJBK8o6eBrVfcgxjjUieDQz/ZvZ/RZnXA91OojssR0lclywDZPFzo PJqb0Gb2TPV/GQSPZlauePmboJ/iPt17uCJG2/kJX5/3AzCn6pnda2AhNvQu3nh1aUfw QmWPRw1dnwF6F95NiJaoV7vYawf4104MaLoXlgduUapng/tA0tAMLymlK9QnHGS5NAZv d5mg== MIME-Version: 1.0 X-Received: by 10.50.57.200 with SMTP id k8mr8474368igq.29.1357894438104; Fri, 11 Jan 2013 00:53:58 -0800 (PST) Received: by 10.231.40.208 with HTTP; Fri, 11 Jan 2013 00:53:57 -0800 (PST) In-Reply-To: <1357566094.10284.83.camel@gandalf.local.home> References: <1357207949-3349-1-git-send-email-kpark3469@gmail.com> <1357207949-3349-2-git-send-email-kpark3469@gmail.com> <20130103103815.GL2631@n2100.arm.linux.org.uk> <1357220165.10284.30.camel@gandalf.local.home> <20130103141301.GQ2631@n2100.arm.linux.org.uk> <1357229038.10284.46.camel@gandalf.local.home> <20130103162359.GR2631@n2100.arm.linux.org.uk> <1357232337.10284.67.camel@gandalf.local.home> <1357566094.10284.83.camel@gandalf.local.home> Date: Fri, 11 Jan 2013 17:53:57 +0900 Message-ID: Subject: Re: [PATCH 2/2] arm: make return_address available for ARM_UNWIND From: Keun-O Park To: Steven Rostedt X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20130111_035359_383144_5646B324 X-CRM114-Status: GOOD ( 31.93 ) X-Spam-Score: -2.5 (--) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-2.5 points) pts rule name description ---- ---------------------- -------------------------------------------------- 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider (kpark3469[at]gmail.com) -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.210.172 listed in list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record 0.2 FREEMAIL_ENVFROM_END_DIGIT Envelope-from freemail username ends in digit (kpark3469[at]gmail.com) -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature Cc: sahara , Russell King - ARM Linux , fweisbec@gmail.com, linux-kernel@vger.kernel.org, mingo@redhat.com, linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org On Mon, Jan 7, 2013 at 10:41 PM, Steven Rostedt wrote: > > On Fri, 2013-01-04 at 17:45 +0900, Keun-O Park wrote: > > > With "CFLAGS_REMOVE_unwind.o = -pg" and with CONFIG_PROVE_LOCKING > > turned on, I confirmed that > > there's no trace output like Steve mentioned. > > However, if I turn off CONFIG_PROVE_LOCKING, "irqsoff" and > > "preemptirqsoff" ftracer prints these lines : > > > > kernel_text_address <-unwind_frame > > core_kernel_text <-unwind_frame > > search_index <-unwind_frame > > > > While I investigated the reason, I just found out there's two function > > with same name, trace_hardirqs_off. > > One in kernel/trace/trace_irqsoff.c and another in kernel/lockdep.c. > > And both symbols are exported. > > I am wondering whether it is intentionally maintained code to > > manipulate ftrace and lockdep or not. > > I guess it's probably not. > > Both the irqsoff tracer from ftrace and lockdep came from the -rt patch. > The two were very integrated at the time. When they were ported over to > mainline, they still used the same infrastructure to hook into the > locations of interrupts being disabled or enabled. > > trace_hardirqs_on/off() is the function that is called for both lockdep > and ftrace's irqsoff tracer. So this was intentional. That way we didn't > need to have two different callers at every location. But if lockdep > isn't defined and ftrace irqsoff is, then ftrace would define the > trace_hardirqs_on/off() routines, otherwise lockdep does. (These > routines are within CONFIG_PROVE_LOCKING #ifdefs in > kernel/trace/trace_irqsoff.c) > > When both lockdep and irqsoff tracer are configured, then lockdep > defines the trace_hardirqs_off_caller(), and calls time_hardirqs_off() > in trace_irqsoff.c which does the same thing as the trace_hardirqs_off() > does without lockdep. > > I'm not sure why one would add the annotations while adding > PROVE_LOCKING doesn't. They both seems to use CALLER_ADDR0, but maybe > there's another path that uses CALLER_ADDR1 without it. > > -- Steve > > Hello Steve, Much obliged for your explaining this. As your guess, there's another path that uses CALLER_ADDR1 without CONFIG_PROVE_LOCKING though both lockdep and ftrace's irqsoff tracer is turned on. In this case, ftrace's trace_hardirqs_on/off() would be called. And, as you said to me, if lockdep is off and ftrace's irqsoff is on, ftrace's trace_hardirqs_on/off() would be called. I think the proper way to avoid calling CALLER_ADDR1 will be calling trace_hardirqs_off_caller() instead of calling stop_critical_timing() directly in trace_hardirqs_off(). And, this will suppress the message outputs of unwind_frame. If you think this idea is okay, I will push the patch v2 to you. In my test result, it looks it's functionally correct. Thanks. -- kpark diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h index f89515a..3552ad9 100644 --- a/arch/arm/include/asm/ftrace.h +++ b/arch/arm/include/asm/ftrace.h @@ -32,13 +32,11 @@ extern void ftrace_call_old(void); #ifndef __ASSEMBLY__ -#if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND) +#if defined(CONFIG_FRAME_POINTER) || defined(CONFIG_ARM_UNWIND) /* * return_address uses walk_stackframe to do it's work. If both * CONFIG_FRAME_POINTER=y and CONFIG_ARM_UNWIND=y walk_stackframe uses unwind - * information. For this to work in the function tracer many functions would - * have to be marked with __notrace. So for now just depend on - * !CONFIG_ARM_UNWIND. + * information. */ void *return_address(unsigned int); diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile index 5bbec7b..675fd62 100644 --- a/arch/arm/kernel/Makefile +++ b/arch/arm/kernel/Makefile @@ -9,6 +9,9 @@ ifdef CONFIG_FUNCTION_TRACER CFLAGS_REMOVE_ftrace.o = -pg CFLAGS_REMOVE_insn.o = -pg CFLAGS_REMOVE_patch.o = -pg +ifdef CONFIG_ARM_UNWIND +CFLAGS_REMOVE_unwind.o = -pg +endif endif CFLAGS_REMOVE_return_address.o = -pg diff --git a/arch/arm/kernel/return_address.c b/arch/arm/kernel/return_address.c index fafedd8..f202bc0 100644 --- a/arch/arm/kernel/return_address.c +++ b/arch/arm/kernel/return_address.c @@ -11,7 +11,7 @@ #include #include -#if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND) +#if defined(CONFIG_FRAME_POINTER) || defined(CONFIG_ARM_UNWIND) #include #include @@ -57,17 +57,13 @@ void *return_address(unsigned int level) return NULL; } -#else /* if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND) */ - -#if defined(CONFIG_ARM_UNWIND) -#warning "TODO: return_address should use unwind tables" -#endif +#else /* CONFIG_FRAME_POINTER || CONFIG_ARM_UNWIND */ void *return_address(unsigned int level) { return NULL; } -#endif /* if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND) / else */ +#endif /* CONFIG_FRAME_POINTER || CONFIG_ARM_UNWIND */ EXPORT_SYMBOL_GPL(return_address); diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c index 00f79e5..aab144b 100644 --- a/arch/arm/kernel/stacktrace.c +++ b/arch/arm/kernel/stacktrace.c @@ -6,6 +6,9 @@ #if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND) /* + * If both CONFIG_FRAME_POINTER=y and CONFIG_ARM_UNWIND=y walk_stackframe uses + * unwind information. So for now just depend on !CONFIG_ARM_UNWIND. + * * Unwind the current stack frame and store the new register values in the * structure passed as argument. Unwinding is equivalent to a function return, * hence the new PC value rather than LR should be used for backtrace. diff --git a/kernel/trace/trace_irqsoff.c b/kernel/trace/trace_irqsoff.c index 713a2ca..6f207ed 100644 --- a/kernel/trace/trace_irqsoff.c +++ b/kernel/trace/trace_irqsoff.c @@ -483,20 +483,6 @@ inline void print_irqtrace_events(struct task_struct *curr) /* * We are only interested in hardirq on/off events: */ -void trace_hardirqs_on(void) -{ - if (!preempt_trace() && irq_trace()) - stop_critical_timing(CALLER_ADDR0, CALLER_ADDR1); -} -EXPORT_SYMBOL(trace_hardirqs_on); - -void trace_hardirqs_off(void) -{ - if (!preempt_trace() && irq_trace()) - start_critical_timing(CALLER_ADDR0, CALLER_ADDR1); -} -EXPORT_SYMBOL(trace_hardirqs_off); - void trace_hardirqs_on_caller(unsigned long caller_addr) { if (!preempt_trace() && irq_trace()) @@ -504,6 +490,12 @@ void trace_hardirqs_on_caller(unsigned long caller_addr) } EXPORT_SYMBOL(trace_hardirqs_on_caller); +void trace_hardirqs_on(void) +{ + trace_hardirqs_on_caller(CALLER_ADDR0); +} +EXPORT_SYMBOL(trace_hardirqs_on); + void trace_hardirqs_off_caller(unsigned long caller_addr) { if (!preempt_trace() && irq_trace()) @@ -511,6 +503,12 @@ void trace_hardirqs_off_caller(unsigned long caller_addr) } EXPORT_SYMBOL(trace_hardirqs_off_caller); +void trace_hardirqs_off(void) +{ + trace_hardirqs_off_caller(CALLER_ADDR0); +} +EXPORT_SYMBOL(trace_hardirqs_off); + #endif /* CONFIG_PROVE_LOCKING */ #endif /* CONFIG_IRQSOFF_TRACER */