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

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

[Thomas Gleixner]

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
 }
 
 /*

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

[Frederic Weisbecker]

2013/2/20 Thomas Gleixner <tglx@linutronix.de>:
> 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.

I know it looks that way. That's because it's a pure regression fix,
minimal for backportability.

I'm distinguishing two different things here: the fact that some archs
can call irq_exit() with interrupts enabled which is a global design
problem, and the fact that tick_nohz_irq_exit() was safe against that
until 3.2 when I broke it with a commit of mine.

My goal was basically to restore that protection in a minimal commit
such that we can backport the regression fix, then deal with
__ARCH_IRQ_EXIT_IRQS_DISABLED afterward, since it requires some more
invasive changes.

>> 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()?

I need a fix that I can backport. Is the below fine with a stable tag?
It looks a bit too invasive for the single regression involved.

Thanks.

>
> 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
>  }
>
>  /*

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

[Thomas Gleixner]

On Wed, 20 Feb 2013, Frederic Weisbecker wrote:
> 2013/2/20 Thomas Gleixner <tglx@linutronix.de>:
> > That's not a fix. That's an hack.
> 
> I know it looks that way. That's because it's a pure regression fix,
> minimal for backportability.
> 
> I'm distinguishing two different things here: the fact that some archs
> can call irq_exit() with interrupts enabled which is a global design
> problem, and the fact that tick_nohz_irq_exit() was safe against that
> until 3.2 when I broke it with a commit of mine.
> 
> My goal was basically to restore that protection in a minimal commit
> such that we can backport the regression fix, then deal with
> __ARCH_IRQ_EXIT_IRQS_DISABLED afterward, since it requires some more
> invasive changes.
> 
> >> 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()?
> 
> I need a fix that I can backport. Is the below fine with a stable tag?
> It looks a bit too invasive for the single regression involved.

I think that's fine as it's obviously correct and not diluting the
real underlying issue of the __ARCH_IRQ_EXIT_IRQS_DISABLED insanity.

Thanks,

	tglx

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

[Frederic Weisbecker]

2013/2/21 Thomas Gleixner <tglx@linutronix.de>:
> On Wed, 20 Feb 2013, Frederic Weisbecker wrote:
>> 2013/2/20 Thomas Gleixner <tglx@linutronix.de>:
>> > That's not a fix. That's an hack.
>>
>> I know it looks that way. That's because it's a pure regression fix,
>> minimal for backportability.
>>
>> I'm distinguishing two different things here: the fact that some archs
>> can call irq_exit() with interrupts enabled which is a global design
>> problem, and the fact that tick_nohz_irq_exit() was safe against that
>> until 3.2 when I broke it with a commit of mine.
>>
>> My goal was basically to restore that protection in a minimal commit
>> such that we can backport the regression fix, then deal with
>> __ARCH_IRQ_EXIT_IRQS_DISABLED afterward, since it requires some more
>> invasive changes.
>>
>> >> 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()?
>>
>> I need a fix that I can backport. Is the below fine with a stable tag?
>> It looks a bit too invasive for the single regression involved.
>
> I think that's fine as it's obviously correct and not diluting the
> real underlying issue of the __ARCH_IRQ_EXIT_IRQS_DISABLED insanity.

Ok fine. Do you plan to commit your proposed change then?

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

[Thomas Gleixner]

On Thu, 21 Feb 2013, Frederic Weisbecker wrote:
> 2013/2/21 Thomas Gleixner <tglx@linutronix.de>:
> > On Wed, 20 Feb 2013, Frederic Weisbecker wrote:
> >> 2013/2/20 Thomas Gleixner <tglx@linutronix.de>:
> >> > That's not a fix. That's an hack.
> >>
> >> I know it looks that way. That's because it's a pure regression fix,
> >> minimal for backportability.
> >>
> >> I'm distinguishing two different things here: the fact that some archs
> >> can call irq_exit() with interrupts enabled which is a global design
> >> problem, and the fact that tick_nohz_irq_exit() was safe against that
> >> until 3.2 when I broke it with a commit of mine.
> >>
> >> My goal was basically to restore that protection in a minimal commit
> >> such that we can backport the regression fix, then deal with
> >> __ARCH_IRQ_EXIT_IRQS_DISABLED afterward, since it requires some more
> >> invasive changes.
> >>
> >> >> 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()?
> >>
> >> I need a fix that I can backport. Is the below fine with a stable tag?
> >> It looks a bit too invasive for the single regression involved.
> >
> > I think that's fine as it's obviously correct and not diluting the
> > real underlying issue of the __ARCH_IRQ_EXIT_IRQS_DISABLED insanity.
> 
> Ok fine. Do you plan to commit your proposed change then?

Second thoughts. I probably go for your minimal fix for stable and
then push my version on top of it to Linus only.

Thanks,

	tglx

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

[Frederic Weisbecker]

2013/2/21 Thomas Gleixner <tglx@linutronix.de>:
> On Thu, 21 Feb 2013, Frederic Weisbecker wrote:
> Second thoughts. I probably go for your minimal fix for stable and
> then push my version on top of it to Linus only.

Thanks, I feel more comfortable with that. Then I'll try to iterate
over archs to finally remove that damn macro.

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

[Linus Torvalds]

On Wed, Feb 20, 2013 at 1:00 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>   */
>  void irq_exit(void)
>  {
> +#ifndef __ARCH_IRQ_EXIT_IRQS_DISABLED
> +       unsigned long flags;
> +
> +       local_irq_save(flags);
> +#else
> +       BUG_ON(!irqs_disabled();
> +#endif

Guys, STOP DOING THIS!

Adding BUG_ON()'s just makes things much much much worse. There is
*never* a reason to add a BUG_ON(). And doing it in an interrupt path
is totally unacceptable. BUG_ON() makes it almost impossible to debug
something, because you just killed the machine. So using BUG_ON() for
"please notice this" is stupid as hell, because the most common end
result is: "Oh, the machine just hung with no messages".

Make it WARN_ON_ONCE() if you absolutely have to let people know, but
for something like this, why would you do even that?

                          Linus

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

[Thomas Gleixner]

On Thu, 21 Feb 2013, Linus Torvalds wrote:

> On Wed, Feb 20, 2013 at 1:00 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >   */
> >  void irq_exit(void)
> >  {
> > +#ifndef __ARCH_IRQ_EXIT_IRQS_DISABLED
> > +       unsigned long flags;
> > +
> > +       local_irq_save(flags);
> > +#else
> > +       BUG_ON(!irqs_disabled();
> > +#endif
> 
> Guys, STOP DOING THIS!
> 
> Adding BUG_ON()'s just makes things much much much worse. There is
> *never* a reason to add a BUG_ON(). And doing it in an interrupt path
> is totally unacceptable. BUG_ON() makes it almost impossible to debug
> something, because you just killed the machine. So using BUG_ON() for
> "please notice this" is stupid as hell, because the most common end
> result is: "Oh, the machine just hung with no messages".
> 
> Make it WARN_ON_ONCE() if you absolutely have to let people know, but
> for something like this, why would you do even that?

This was a draft patch. I made it a WARN_ON_ONCE() already. 

We really want to enforce that irq_exit() is called with interrupts
disabled.

Thanks,

	tglx

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

[Linus Torvalds]

On Thu, Feb 21, 2013 at 10:21 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> This was a draft patch. I made it a WARN_ON_ONCE() already.

Ok, good.

I really wish we could just get rid of BUG_ON(). It was a bad idea,
and it makes it easy for people to do the wrong thing. Sadly, we have
tons of them.

                        Linus

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

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, Feb 21, 2013 at 10:21 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > This was a draft patch. I made it a WARN_ON_ONCE() already.
> 
> Ok, good.
> 
> I really wish we could just get rid of BUG_ON(). It was a bad 
> idea, and it makes it easy for people to do the wrong thing. 
> Sadly, we have tons of them.

So my old plan was to use a little bit of psychology.

Firstly, we could just turn BUG_ON() into a WARN() variant that 
emits:

  BUG: ...

while a WARN()ings emit:

  WARNING: ...

and then we could introduce a new primitive:

  CRASH_ON();

which would be used in the (few) places that really, really 
cannot continue sanely and need to crash the box.

This naming alone would inhibit its use through two channels:

 - Putting the word 'CRASH' into your code feels risky, 
   dissonant and wrong (perfect code does not crash) and thus 
   needs conscious frontal lobe effort to justify it - while 
   BUG_ON() really feels more like a harmless assert to most 
   kernel developers, which is in our muscle memory through 
   years training.

 - CRASH_ON() takes one character more typing than WARN_ON(), 
   and we know good kernel developers are fundamentally lazy.

[ This is an arguably lazy plan that does not involve changing 
  the 10,000+ BUG_ON() call sites and does not involve the
  re-training of thousands of mis-trained kernel developers who 
  introduced over 900 new BUG_ON()s in the v3.7->v3.8 cycle 
  alone (!). ]

So while I don't think we can win the war against BUG_ON(), I 
think we can fight the lazy general's war: turn the enemy over 
to our side and declare victory?

Thanks,

	Ingo