* [PATCH] arm: fix lockdep warning of "unannotated irqs-off" @ 2011-06-01 7:42 Ming Lei 2011-06-01 9:34 ` Russell King - ARM Linux 0 siblings, 1 reply; 9+ messages in thread From: Ming Lei @ 2011-06-01 7:42 UTC (permalink / raw) To: linux-arm-kernel >From ad88fff7aaa32b50f77b07dba239bac938fb56c1 Mon Sep 17 00:00:00 2001 From: Ming Lei <ming.lei@canonical.com> Date: Wed, 1 Jun 2011 15:02:24 +0800 Subject: [PATCH] arm: fix lockdep warning of "unannotated irqs-off" This patch fixes the lockdep warning of "unannotated irqs-off"[1]. After entering __irq_usr, arm core will disable interrupt automatically, but __irq_usr does not annotate the irq disable, so lockdep may complain the warning if it has chance to check this in irq handler. This patch adds trace_hardirqs_off in __irq_usr before entering irq_handler to handle the irq, also calls ret_to_user_from_irq to avoid calling disable_irq again. [1], lockdep warning log of "unannotated irqs-off" [ 13.804687] ------------[ cut here ]------------ [ 13.809570] WARNING: at kernel/lockdep.c:3335 check_flags+0x78/0x1d0() [ 13.816467] Modules linked in: [ 13.819732] Backtrace: [ 13.822357] [<c01cb42c>] (dump_backtrace+0x0/0x100) from [<c06abb14>] (dump_stack+0x20/0x24) [ 13.831268] r6:c07d8c2c r5:00000d07 r4:00000000 r3:00000000 [ 13.837280] [<c06abaf4>] (dump_stack+0x0/0x24) from [<c01ffc04>] (warn_slowpath_common+0x5c/0x74) [ 13.846649] [<c01ffba8>] (warn_slowpath_common+0x0/0x74) from [<c01ffc48>] (warn_slowpath_null+0x2c/0x34) [ 13.856781] r8:00000000 r7:00000000 r6:c18b8194 r5:60000093 r4:ef182000 [ 13.863708] r3:00000009 [ 13.866485] [<c01ffc1c>] (warn_slowpath_null+0x0/0x34) from [<c0237d84>] (check_flags+0x78/0x1d0) [ 13.875823] [<c0237d0c>] (check_flags+0x0/0x1d0) from [<c023afc8>] (lock_acquire+0x4c/0x150) [ 13.884704] [<c023af7c>] (lock_acquire+0x0/0x150) from [<c06af638>] (_raw_spin_lock+0x4c/0x84) [ 13.893798] [<c06af5ec>] (_raw_spin_lock+0x0/0x84) from [<c01f9a44>] (sched_ttwu_pending+0x58/0x8c) [ 13.903320] r6:ef92d040 r5:00000003 r4:c18b8180 [ 13.908233] [<c01f99ec>] (sched_ttwu_pending+0x0/0x8c) from [<c01f9a90>] (scheduler_ipi+0x18/0x1c) [ 13.917663] r6:ef183fb0 r5:00000003 r4:00000000 r3:00000001 [ 13.923645] [<c01f9a78>] (scheduler_ipi+0x0/0x1c) from [<c01bc458>] (do_IPI+0x9c/0xfc) [ 13.932006] [<c01bc3bc>] (do_IPI+0x0/0xfc) from [<c06b0888>] (__irq_usr+0x48/0xe0) [ 13.939971] Exception stack(0xef183fb0 to 0xef183ff8) [ 13.945281] 3fa0: ffffffc3 0001500c 00000001 0001500c [ 13.953948] 3fc0: 00000050 400b45f0 400d9000 00000000 00000001 400d9600 6474e552 bea05b3c [ 13.962585] 3fe0: 400d96c0 bea059c0 400b6574 400b65d8 20000010 ffffffff [ 13.969573] r6:00000403 r5:fa240100 r4:ffffffff r3:20000010 [ 13.975585] ---[ end trace efc4896ab0fb62cb ]--- [ 13.980468] possible reason: unannotated irqs-off. [ 13.985534] irq event stamp: 1610 [ 13.989044] hardirqs last enabled at (1610): [<c01c703c>] no_work_pending+0x8/0x2c [ 13.997131] hardirqs last disabled at (1609): [<c01c7024>] ret_slow_syscall+0xc/0x1c [ 14.005371] softirqs last enabled at (0): [<c01fe5e4>] copy_process+0x2cc/0xa24 [ 14.013183] softirqs last disabled at (0): [< (null)>] (null) Signed-off-by: Ming Lei <ming.lei@canonical.com> --- arch/arm/kernel/entry-armv.S | 6 +++++- arch/arm/kernel/entry-common.S | 2 ++ 2 files changed, 7 insertions(+), 1 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..b2a27b6 100644 --- a/arch/arm/kernel/entry-common.S +++ b/arch/arm/kernel/entry-common.S @@ -64,6 +64,7 @@ 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 @@ -75,6 +76,7 @@ no_work_pending: arch_ret_to_user r1, lr restore_user_regs fast = 0, offset = 0 +ENDPROC(ret_to_user_from_irq) ENDPROC(ret_to_user) /* -- 1.7.4.1 -- Ming Lei ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] arm: fix lockdep warning of "unannotated irqs-off" 2011-06-01 7:42 [PATCH] arm: fix lockdep warning of "unannotated irqs-off" Ming Lei @ 2011-06-01 9:34 ` Russell King - ARM Linux 2011-06-01 13:17 ` Ming Lei 0 siblings, 1 reply; 9+ messages in thread From: Russell King - ARM Linux @ 2011-06-01 9:34 UTC (permalink / raw) To: linux-arm-kernel On Wed, Jun 01, 2011 at 03:42:59PM +0800, Ming Lei wrote: > @@ -435,6 +435,10 @@ __irq_usr: > usr_entry > kuser_cmpxchg_check > > +#ifdef CONFIG_TRACE_IRQFLAGS I think this wants to be CONFIG_IRQSOFF_TRACER. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] arm: fix lockdep warning of "unannotated irqs-off" 2011-06-01 9:34 ` Russell King - ARM Linux @ 2011-06-01 13:17 ` Ming Lei 2011-06-01 13:22 ` Russell King - ARM Linux 0 siblings, 1 reply; 9+ messages in thread From: Ming Lei @ 2011-06-01 13:17 UTC (permalink / raw) To: linux-arm-kernel Hi, On Wed, 1 Jun 2011 10:34:36 +0100 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Wed, Jun 01, 2011 at 03:42:59PM +0800, Ming Lei wrote: > > @@ -435,6 +435,10 @@ __irq_usr: > > usr_entry > > kuser_cmpxchg_check > > > > +#ifdef CONFIG_TRACE_IRQFLAGS > > I think this wants to be CONFIG_IRQSOFF_TRACER. IMO, this should be CONFIG_TRACE_IRQFLAGS because we want to trace interrupt disable and enable here, but enabling CONFIG_IRQSOFF_TRACER means interrupt-off latency tracer will be switched on, see kernel/trace/Kconfig. -- Ming Lei ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] arm: fix lockdep warning of "unannotated irqs-off" 2011-06-01 13:17 ` Ming Lei @ 2011-06-01 13:22 ` Russell King - ARM Linux 2011-06-01 14:28 ` Ming Lei 0 siblings, 1 reply; 9+ messages in thread From: Russell King - ARM Linux @ 2011-06-01 13:22 UTC (permalink / raw) To: linux-arm-kernel On Wed, Jun 01, 2011 at 09:17:51PM +0800, Ming Lei wrote: > Hi, > > On Wed, 1 Jun 2011 10:34:36 +0100 > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > > > On Wed, Jun 01, 2011 at 03:42:59PM +0800, Ming Lei wrote: > > > @@ -435,6 +435,10 @@ __irq_usr: > > > usr_entry > > > kuser_cmpxchg_check > > > > > > +#ifdef CONFIG_TRACE_IRQFLAGS > > > > I think this wants to be CONFIG_IRQSOFF_TRACER. > > IMO, this should be CONFIG_TRACE_IRQFLAGS because we > want to trace interrupt disable and enable here, but > enabling CONFIG_IRQSOFF_TRACER means interrupt-off > latency tracer will be switched on, see kernel/trace/Kconfig. If CONFIG_IRQSOFF_TRACER is not set, we don't bother telling the tracer that we've turned IRQs on for userspace when we exit from the kernel, and we don't bother telling the tracer that we've turned IRQs off when entering from userspace back into the kernel. So, making this depend on CONFIG_TRACE_IRQFLAGS looks wrong to me. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] arm: fix lockdep warning of "unannotated irqs-off" 2011-06-01 13:22 ` Russell King - ARM Linux @ 2011-06-01 14:28 ` Ming Lei 2011-06-01 14:46 ` Russell King - ARM Linux 0 siblings, 1 reply; 9+ messages in thread From: Ming Lei @ 2011-06-01 14:28 UTC (permalink / raw) To: linux-arm-kernel Hi, On Wed, 1 Jun 2011 14:22:33 +0100 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > From: Russell King - ARM Linux <linux@arm.linux.org.uk> > If CONFIG_IRQSOFF_TRACER is not set, we don't bother telling the > tracer that we've turned IRQs on for userspace when we exit from the > kernel, and we don't bother telling the tracer that we've turned IRQs > off when entering from userspace back into the kernel. > > So, making this depend on CONFIG_TRACE_IRQFLAGS looks wrong to me. >From lib/Kconfig.debug: config PROVE_LOCKING bool "Lock debugging: prove locking correctness" ...... select TRACE_IRQFLAGS you can find locking prove does need to enable TRACE_IRQFLAGS. Meantime, we may not enable irq off tracer via below: Kernel hacking ---> Tracers ---> [ ] Interrupts-off Latency Tracer so making this depend on CONFIG_TRACE_IRQFLAGS is reasonable. thanks, -- Ming Lei ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] arm: fix lockdep warning of "unannotated irqs-off" 2011-06-01 14:28 ` Ming Lei @ 2011-06-01 14:46 ` Russell King - ARM Linux 2011-06-01 15:52 ` Ming Lei 0 siblings, 1 reply; 9+ messages in thread From: Russell King - ARM Linux @ 2011-06-01 14:46 UTC (permalink / raw) To: linux-arm-kernel On Wed, Jun 01, 2011 at 10:28:29PM +0800, Ming Lei wrote: > From lib/Kconfig.debug: > > config PROVE_LOCKING > bool "Lock debugging: prove locking correctness" > ...... > select TRACE_IRQFLAGS > > you can find locking prove does need to enable TRACE_IRQFLAGS. Meantime, > we may not enable irq off tracer via below: > > Kernel hacking ---> > Tracers ---> > [ ] Interrupts-off Latency Tracer > > so making this depend on CONFIG_TRACE_IRQFLAGS is reasonable. I still don't see what the problem is. 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. 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. 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. Ergo, it should depend on CONFIG_IRQSOFF_TRACER, not CONFIG_TRACE_IRQFLAGS. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] arm: fix lockdep warning of "unannotated irqs-off" 2011-06-01 14:46 ` Russell King - ARM Linux @ 2011-06-01 15:52 ` Ming Lei 2011-06-01 16:01 ` Russell King - ARM Linux 0 siblings, 1 reply; 9+ messages in thread From: Ming Lei @ 2011-06-01 15:52 UTC (permalink / raw) To: linux-arm-kernel Hi, On Wed, 1 Jun 2011 15:46:30 +0100 Russell King - ARM Linux <linux@arm.linux.org.uk> 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) /* -- 1.7.4.1 thanks, -- Ming Lei ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] arm: fix lockdep warning of "unannotated irqs-off" 2011-06-01 15:52 ` Ming Lei @ 2011-06-01 16:01 ` Russell King - ARM Linux 2011-06-02 1:32 ` Ming Lei 0 siblings, 1 reply; 9+ messages in thread From: Russell King - ARM Linux @ 2011-06-01 16:01 UTC (permalink / raw) To: linux-arm-kernel On Wed, Jun 01, 2011 at 11:52:21PM +0800, Ming Lei wrote: > 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: No, you've completely missed the point: when we are not doing latency analysis of IRQs-off regions, there is _no_ _point_ in telling the kernel that IRQs are on while userspace is running - userspace will not be taking any kernel locks itself. So, the way things are setup at the moment on ARM, we "optimize" the overhead of telling lockdep that IRQs are enabled while userspace is running away _provided_ we are not doing IRQs-off latency tracing. IOW, when CONFIG_IRQSOFF_TRACER is not set, we optimize away the trace_hardirq_on call when entering userspace, and the trace_hardirq_off call when leaving userspace. I'm not sure why you're not grasping that, but I've explained it to you several times in telling you that when CONFIG_IRQSOFF_TRACER is not set, the kernel treats userspace as an IRQs-off region. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] arm: fix lockdep warning of "unannotated irqs-off" 2011-06-01 16:01 ` Russell King - ARM Linux @ 2011-06-02 1:32 ` Ming Lei 0 siblings, 0 replies; 9+ messages in thread From: Ming Lei @ 2011-06-02 1:32 UTC (permalink / raw) To: linux-arm-kernel Hi Russell, Thanks for your detailed explain. On Wed, 1 Jun 2011 17:01:44 +0100 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > So, the way things are setup at the moment on ARM, we "optimize" the > overhead of telling lockdep that IRQs are enabled while userspace is > running away _provided_ we are not doing IRQs-off latency tracing. > IOW, when CONFIG_IRQSOFF_TRACER is not set, we optimize away the > trace_hardirq_on call when entering userspace, and the > trace_hardirq_off call when leaving userspace. Sorry, I did not notice this "optimization" before, and will change the dependency and update the patch. BTW, the patch also fixes for irq off tracer. thanks, -- Ming Lei ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-06-02 1:32 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-06-01 7:42 [PATCH] arm: fix lockdep warning of "unannotated irqs-off" Ming Lei 2011-06-01 9:34 ` Russell King - ARM Linux 2011-06-01 13:17 ` Ming Lei 2011-06-01 13:22 ` Russell King - ARM Linux 2011-06-01 14:28 ` Ming Lei 2011-06-01 14:46 ` Russell King - ARM Linux 2011-06-01 15:52 ` Ming Lei 2011-06-01 16:01 ` Russell King - ARM Linux 2011-06-02 1:32 ` Ming Lei
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).