| Message ID | 20110601154259.12487694@tom-ThinkPad-T410 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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
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.
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) /*