From patchwork Wed Jun 1 15:52:21 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ming Lei X-Patchwork-Id: 840682 Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by demeter1.kernel.org (8.14.4/8.14.3) with ESMTP id p51G2E2W022733 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Wed, 1 Jun 2011 16:02:35 GMT Received: from canuck.infradead.org ([134.117.69.58]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1QRnpc-0001QJ-Cx; Wed, 01 Jun 2011 15:59:40 +0000 Received: from localhost ([127.0.0.1] helo=canuck.infradead.org) by canuck.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1QRnjH-00064I-QD; Wed, 01 Jun 2011 15:53:07 +0000 Received: from casper.infradead.org ([85.118.1.10]) by canuck.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1QRnjF-00064D-Ju for linux-arm-kernel@canuck.infradead.org; Wed, 01 Jun 2011 15:53:05 +0000 Received: from adelie.canonical.com ([91.189.90.139]) by casper.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1QRnjS-0006pe-H9 for linux-arm-kernel@lists.infradead.org; Wed, 01 Jun 2011 15:53:20 +0000 Received: from youngberry.canonical.com ([91.189.89.112]) by adelie.canonical.com with esmtp (Exim 4.71 #1 (Debian)) id 1QRnip-0002DX-LK; Wed, 01 Jun 2011 15:52:39 +0000 Received: from [183.37.193.29] (helo=tom-ThinkPad-T410) by youngberry.canonical.com with esmtpsa (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1QRnio-0003KP-48; Wed, 01 Jun 2011 15:52:39 +0000 Date: Wed, 1 Jun 2011 23:52:21 +0800 From: Ming Lei To: Russell King - ARM Linux Subject: Re: [PATCH] arm: fix lockdep warning of "unannotated irqs-off" Message-ID: <20110601235221.5aab9182@tom-ThinkPad-T410> In-Reply-To: <20110601144630.GL3660@n2100.arm.linux.org.uk> References: <20110601154259.12487694@tom-ThinkPad-T410> <20110601093436.GF3660@n2100.arm.linux.org.uk> <20110601211751.69b85eeb@tom-ThinkPad-T410> <20110601132233.GG3660@n2100.arm.linux.org.uk> <20110601222829.6eaad0e6@tom-ThinkPad-T410> <20110601144630.GL3660@n2100.arm.linux.org.uk> Organization: ubuntu X-Mailer: Claws Mail 3.7.8 (GTK+ 2.24.4; x86_64-pc-linux-gnu) Mime-Version: 1.0 X-CRM114-Version: 20090807-BlameThorstenAndJenny ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20110601_165318_797350_A60FC76E X-CRM114-Status: GOOD ( 30.99 ) X-Spam-Score: -4.2 (----) X-Spam-Report: SpamAssassin version 3.3.2-r929478 on casper.infradead.org summary: Content analysis details: (-4.2 points, 5.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- -2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/, medium trust [91.189.90.139 listed in list.dnswl.org] -0.0 T_RP_MATCHES_RCVD Envelope sender domain matches handover relay domain -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.12 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 X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Wed, 01 Jun 2011 16:02:35 +0000 (UTC) Hi, On Wed, 1 Jun 2011 15:46:30 +0100 Russell King - ARM Linux wrote: > I still don't see what the problem is. Simply speaking, there are two usages which does need to trace irq disable and enable. One is to prove locking correctness in lockdep, such as a lock can't be held in hardirq enable state if the lock was held in hardirq context. Another is to trace the max time of irq disable, which is implemented in ftrace. If the former case is to be supported, CONFIG_PROVE_LOCKING should be selected. If the latter is to be supported, CONFIG_IRQSOFF_TRACER should be enabled. Both the two options will have to select CONFIG_TRACE_IRQFLAGS. If CONFIG_TRACE_IRQFLAGS is enabled, trace_hardirqs_off/on are surely to be defined, but the implementation will be different between CONFIG_PROVE_LOCKING case and non-CONFIG_PROVE_LOCKING case. So you will find there are different implementations of trace_hardirqs_off/on in kernel/lockdep.c and kernel/trace/trace_irqsoff.c. That is why the patch uses CONFIG_TRACE_IRQFLAGS as dependency. > If CONFIG_IRQSOFF_TRACER is > not set, then we do not tell the kernel that IRQs have been enabled > upon exit to user space, and we do not tell the kernel that IRQs have > been disabled upon entry to kernel space. > > So your patch which makes us always report an IRQs off condition on > entry to __irq_usr makes no sense what so ever to me. Except for tracing max time of irq disable, lock proving has to be supported if user needs it. So we should introduces the changes below into the patch: diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S index b2a27b6..9494792 100644 --- a/arch/arm/kernel/entry-common.S +++ b/arch/arm/kernel/entry-common.S @@ -69,7 +69,7 @@ ENTRY(ret_to_user_from_irq) tst r1, #_TIF_WORK_MASK bne work_pending no_work_pending: -#if defined(CONFIG_IRQSOFF_TRACER) +#if defined(CONFIG_TRACE_IRQFLAGS) asm_trace_hardirqs_on #endif /* perform architecture specific actions before user return */ In fact, the old dependency is wrong. > > Please explain how we get to a situation where we're entering > __irq_usr in the CONFIG_IRQSOFF_TRACER unset case where the kernel > incorrectly believes that IRQs are unmasked. Above should explain this. > > Once you've done that, now analyze the case where > CONFIG_IRQSOFF_TRACER is set. There, we tell the kernel that IRQs > are enabled before returning to userspace, and that IRQs are disabled > when entering the kernel. It is only _now_ that we're missing the > irq-off annotation on entry to the interrupt handler. I don't see there are any issue now. > Ergo, it should depend on CONFIG_IRQSOFF_TRACER, not > CONFIG_TRACE_IRQFLAGS. So how about v2 of the patch below? --- arch/arm/kernel/entry-armv.S | 6 +++++- arch/arm/kernel/entry-common.S | 4 +++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S index e8d8856..7780dc9 100644 --- a/arch/arm/kernel/entry-armv.S +++ b/arch/arm/kernel/entry-armv.S @@ -435,6 +435,10 @@ __irq_usr: usr_entry kuser_cmpxchg_check +#ifdef CONFIG_TRACE_IRQFLAGS + bl trace_hardirqs_off +#endif + get_thread_info tsk #ifdef CONFIG_PREEMPT ldr r8, [tsk, #TI_PREEMPT] @ get preempt count @@ -453,7 +457,7 @@ __irq_usr: #endif mov why, #0 - b ret_to_user + b ret_to_user_from_irq UNWIND(.fnend ) ENDPROC(__irq_usr) diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S index 1e7b04a..9494792 100644 --- a/arch/arm/kernel/entry-common.S +++ b/arch/arm/kernel/entry-common.S @@ -64,17 +64,19 @@ work_resched: ENTRY(ret_to_user) ret_slow_syscall: disable_irq @ disable interrupts +ENTRY(ret_to_user_from_irq) ldr r1, [tsk, #TI_FLAGS] tst r1, #_TIF_WORK_MASK bne work_pending no_work_pending: -#if defined(CONFIG_IRQSOFF_TRACER) +#if defined(CONFIG_TRACE_IRQFLAGS) asm_trace_hardirqs_on #endif /* perform architecture specific actions before user return */ arch_ret_to_user r1, lr restore_user_regs fast = 0, offset = 0 +ENDPROC(ret_to_user_from_irq) ENDPROC(ret_to_user) /*