Re: [PATCH] nohz: Make tick_nohz_irq_exit() irq safe

* Re: [PATCH] nohz: Make tick_nohz_irq_exit() irq safe
       [not found] <1361373336-11337-1-git-send-email-fweisbec@gmail.com>
@ 2013-02-20 21:00 ` Thomas Gleixner
  2013-02-20 22:01   ` Frederic Weisbecker
                     ` (5 more replies)
  2013-02-21 17:43 ` [tip:irq/urgent] nohz: Make tick_nohz_irq_exit() irq safe tip-bot for Frederic Weisbecker
  2013-02-21 20:04 ` tip-bot for Frederic Weisbecker
  2 siblings, 6 replies; 18+ messages in thread
From: Thomas Gleixner @ 2013-02-20 21:00 UTC (permalink / raw)
  To: Frederic Weisbecker; +Cc: Ingo Molnar, LKML, Peter Zijlstra, stable

On Wed, 20 Feb 2013, Frederic Weisbecker wrote:

> As it stands, irq_exit() may or may not be called with
> irqs disabled, depending on __ARCH_IRQ_EXIT_IRQS_DISABLED
> that the arch can define.
> 
> It makes tick_nohz_irq_exit() unsafe. For example two
> interrupts can race in tick_nohz_stop_sched_tick(): the inner
> most one computes the expiring time on top of the timer list,
> then it's interrupted right before reprogramming the
> clock. The new interrupt enqueues a new timer list timer,
> it reprogram the clock to take it into account and it exits.
> The CPUs resumes the inner most interrupt and performs the clock
> reprogramming without considering the new timer list timer.
> 
> This regression has been introduced by:
>      280f06774afedf849f0b34248ed6aff57d0f6908
>      ("nohz: Separate out irq exit and idle loop dyntick logic")
> 
> Let's fix it right now with the appropriate protections.

That's not a fix. That's an hack.
 
> A saner long term solution will be to remove
> __ARCH_IRQ_EXIT_IRQS_DISABLED.

We really want to enforce that interrupt disabled condition for
calling irq_exit(). So why make this exclusive to tick_nohz_irq_exit()?

Thanks,

	tglx

Index: linux-2.6/kernel/softirq.c
===================================================================

--- linux-2.6.orig/kernel/softirq.c
+++ linux-2.6/kernel/softirq.c
@@ -322,18 +322,10 @@ void irq_enter(void)
 
 static inline void invoke_softirq(void)
 {
-	if (!force_irqthreads) {
-#ifdef __ARCH_IRQ_EXIT_IRQS_DISABLED
+	if (!force_irqthreads)
 		__do_softirq();
-#else
-		do_softirq();
-#endif
-	} else {
-		__local_bh_disable((unsigned long)__builtin_return_address(0),
-				SOFTIRQ_OFFSET);
+	else
 		wakeup_softirqd();
-		__local_bh_enable(SOFTIRQ_OFFSET);
-	}
 }
 
 /*
@@ -341,6 +333,14 @@ static inline void invoke_softirq(void)
  */
 void irq_exit(void)
 {
+#ifndef __ARCH_IRQ_EXIT_IRQS_DISABLED
+	unsigned long flags;
+
+	local_irq_save(flags);
+#else
+	BUG_ON(!irqs_disabled();
+#endif
+
 	account_irq_exit_time(current);
 	trace_hardirq_exit();
 	sub_preempt_count(IRQ_EXIT_OFFSET);
@@ -354,6 +354,9 @@ void irq_exit(void)
 #endif
 	rcu_irq_exit();
 	sched_preempt_enable_no_resched();
+#ifndef __ARCH_IRQ_EXIT_IRQS_DISABLED
+	local_irq_restore(flags);
+#endif
 }
 
 /*

^ permalink raw reply	[flat|nested] 18+ messages in thread