[PATCH 0/2] irq: Cleanups in irq_exit()

[PATCH 0/2] irq: Cleanups in irq_exit()

[Frederic Weisbecker]

Hi,

Those are based on tip:/irq/urgent. I'm pretty confident about the
1st patch. The 2nd patch passed through some discussions with Thomas:
it's a nice cleanup but it involves a small unfortunate workaround,
so I tagged it as an RFC. I have mixed feelings about it.

Thanks.

Frederic Weisbecker (2):
  irq: Remove IRQ_EXIT_OFFSET workaround
  irq: Cleanup context state transitions in irq_exit()

 include/linux/hardirq.h |    3 +-
 kernel/softirq.c        |   53 +++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 45 insertions(+), 11 deletions(-)

-- 
1.7.5.4

[PATCH 1/2] irq: Remove IRQ_EXIT_OFFSET workaround

[Frederic Weisbecker]

The IRQ_EXIT_OFFSET trick was used to make sure the irq
doesn't get preempted after we substract the HARDIRQ_OFFSET
until we are entirely done with any code in irq_exit().

This workaround was necessary because some archs may call
irq_exit() with irqs enabled and there is still some code
in the end of this function that is not covered by the
HARDIRQ_OFFSET but want to stay non-preemptible.

Now that irq are always disabled in irq_exit(), the whole code
is guaranteed not to be preempted. We can thus remove this hack.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 include/linux/hardirq.h |    2 --
 kernel/softirq.c        |    3 +--
 2 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h
index 29eb805..c1d6555 100644
--- a/include/linux/hardirq.h
+++ b/include/linux/hardirq.h
@@ -118,10 +118,8 @@
 
 #ifdef CONFIG_PREEMPT_COUNT
 # define preemptible()	(preempt_count() == 0 && !irqs_disabled())
-# define IRQ_EXIT_OFFSET (HARDIRQ_OFFSET-1)
 #else
 # define preemptible()	0
-# define IRQ_EXIT_OFFSET HARDIRQ_OFFSET
 #endif
 
 #if defined(CONFIG_SMP) || defined(CONFIG_GENERIC_HARDIRQS)
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 24a921b..f42ff97 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -343,7 +343,7 @@ void irq_exit(void)
 
 	account_irq_exit_time(current);
 	trace_hardirq_exit();
-	sub_preempt_count(IRQ_EXIT_OFFSET);
+	sub_preempt_count(HARDIRQ_OFFSET);
 	if (!in_interrupt() && local_softirq_pending())
 		invoke_softirq();
 
@@ -353,7 +353,6 @@ void irq_exit(void)
 		tick_nohz_irq_exit();
 #endif
 	rcu_irq_exit();
-	sched_preempt_enable_no_resched();
 #ifndef __ARCH_IRQ_EXIT_IRQS_DISABLED
 	local_irq_restore(flags);
 #endif
-- 
1.7.5.4

[PATCH 2/2] irq: Cleanup context state transitions in irq_exit()

[Frederic Weisbecker]

The irq code is run under HARDIRQ_OFFSET preempt offset until
we reach the softirq code. Then it's substracted, leaving the
preempt count to 0, whether we have pending softirqs or not.

Afterward, if we have softirqs to run, we'll run them under
the SOFTIRQ_OFFSET then set the preempt offset back to 0
after softirqs completion.

The rest of the code in irq_exit(), mainly tick_nohz_irq_exit()
and rcu_irq_exit(), are executed with this NULL preempt offset.

The semantics here are not very intuitive. They leave several portions
of the code into some half-defined context state, where in_interrupt()
returns false while we actually are in an interrupt.

In order to make the context definition less confusing, let's
cover the whole code in irq_exit() under HARDIRQ_OFFSET, except
for the softirq processing where we switch back and forth
from HARDIRQ_OFFSET to SOFTIRQ_OFFSET without leaving a gap
in the context definition.

There is a drawback though: raise_softirq() relies on the previous
semantics considering that as long as we are in_interrupt(), the
pending softirq will be handled in the end of the interrupt. Otherwise
it kicks ksoftirqd so the softirq is always processed.

Now tick_nohz_irq_exit() and rcu_irq_exit() can raise softirqs
themselves. Since these functions were not under the HARDIRQ_OFFSET,
calling raise_softirq() resulted in waking up the ksoftirqd thread.
This is correct because invoke_softirq() has already been invoked at
this stage. But with this patch they are now under HARDIRQ_OFFSET and
raise_softirq() wrongly thinks invoke_softirq() has yet to be called.
In the end, this could leave the softirq unprocessed for a while.

So as Thomas suggested me, this also brings a check in the end of
irq_exit() that looks for pending softirqs raised after invoke_softirq()
and wake up ksoftirqd if needed.

Given that the cleanup on the contexts transition involves that
new unpretty workaround, I have mixed feelings about this patch so I
tagged it as "RFC" and I wait for your opinion.

This is mostly based on a patch written by Linus Torvalds.

Original-patch-by: Linus Torvalds <torvalds@linux-foundation.org>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 include/linux/hardirq.h |    1 +
 kernel/softirq.c        |   52 +++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h
index c1d6555..62e084d 100644
--- a/include/linux/hardirq.h
+++ b/include/linux/hardirq.h
@@ -87,6 +87,7 @@
 #define in_irq()		(hardirq_count())
 #define in_softirq()		(softirq_count())
 #define in_interrupt()		(irq_count())
+#define in_nesting_interrupt()  (irq_count() - HARDIRQ_OFFSET)
 #define in_serving_softirq()	(softirq_count() & SOFTIRQ_OFFSET)
 
 /*
diff --git a/kernel/softirq.c b/kernel/softirq.c
index f42ff97..3ecaee9 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -320,12 +320,34 @@ void irq_enter(void)
 	__irq_enter();
 }
 
+/* Invoke softirq's from irq_exit(). */
 static inline void invoke_softirq(void)
 {
-	if (!force_irqthreads)
-		__do_softirq();
-	else
+	/* Can we run softirq's at all? We might be nesting interrupts */
+	if (in_nesting_interrupt())
+		return;
+
+	/* Are there any pending? */
+	if (!local_softirq_pending())
+		return;
+
+	if (force_irqthreads) {
 		wakeup_softirqd();
+		return;
+	}
+
+	/*
+	 * Ok, do actual softirq handling!
+	 *
+	 * This downgrades us from hardirq context to softirq context.
+	 */
+	preempt_count() += SOFTIRQ_OFFSET - HARDIRQ_OFFSET;
+	trace_softirqs_off((unsigned long)__builtin_return_address(0));
+
+	__do_softirq();
+
+	preempt_count() += HARDIRQ_OFFSET - SOFTIRQ_OFFSET;
+	trace_softirqs_on((unsigned long)__builtin_return_address(0));
 }
 
 /*
@@ -343,16 +365,30 @@ void irq_exit(void)
 
 	account_irq_exit_time(current);
 	trace_hardirq_exit();
-	sub_preempt_count(HARDIRQ_OFFSET);
-	if (!in_interrupt() && local_softirq_pending())
-		invoke_softirq();
+	invoke_softirq();
 
 #ifdef CONFIG_NO_HZ
 	/* Make sure that timer wheel updates are propagated */
-	if (idle_cpu(smp_processor_id()) && !in_interrupt() && !need_resched())
-		tick_nohz_irq_exit();
+	if (idle_cpu(smp_processor_id())) {
+		if (!in_nesting_interrupt() && !need_resched())
+			tick_nohz_irq_exit();
+	}
 #endif
 	rcu_irq_exit();
+	sub_preempt_count(HARDIRQ_OFFSET);
+
+	/*
+	 * Softirqs have been processed already from invoke_softirq().
+	 * However tick_nohz_irq_exit() may raise timer or hrtimer
+	 * softirqs and rcu_irq_exit() may raise the RCU softirq.
+	 * But raise_softirq() won't wakeup ksoftirqd from these callsites:
+	 * it sees we are in_interrupt() so it makes the wrong assumption
+	 * that softirqs can still be processed through invoke_softirq().
+	 * But we already called it. So we need to do the last check here.
+	 */
+	if (local_softirq_pending() && !in_nesting_interrupt())
+		wakeup_softirqd();
+
 #ifndef __ARCH_IRQ_EXIT_IRQS_DISABLED
 	local_irq_restore(flags);
 #endif
-- 
1.7.5.4

Re: [PATCH 2/2] irq: Cleanup context state transitions in irq_exit()

[Thomas Gleixner]

On Fri, 22 Feb 2013, Frederic Weisbecker wrote:
> The irq code is run under HARDIRQ_OFFSET preempt offset until
> we reach the softirq code. Then it's substracted, leaving the
> preempt count to 0, whether we have pending softirqs or not.
> 
> Afterward, if we have softirqs to run, we'll run them under
> the SOFTIRQ_OFFSET then set the preempt offset back to 0
> after softirqs completion.
> 
> The rest of the code in irq_exit(), mainly tick_nohz_irq_exit()
> and rcu_irq_exit(), are executed with this NULL preempt offset.
> 
> The semantics here are not very intuitive. They leave several portions
> of the code into some half-defined context state, where in_interrupt()
> returns false while we actually are in an interrupt.
> 
> In order to make the context definition less confusing, let's
> cover the whole code in irq_exit() under HARDIRQ_OFFSET, except
> for the softirq processing where we switch back and forth
> from HARDIRQ_OFFSET to SOFTIRQ_OFFSET without leaving a gap
> in the context definition.
> 
> There is a drawback though: raise_softirq() relies on the previous
> semantics considering that as long as we are in_interrupt(), the
> pending softirq will be handled in the end of the interrupt. Otherwise
> it kicks ksoftirqd so the softirq is always processed.
> 
> Now tick_nohz_irq_exit() and rcu_irq_exit() can raise softirqs
> themselves. Since these functions were not under the HARDIRQ_OFFSET,
> calling raise_softirq() resulted in waking up the ksoftirqd thread.
> This is correct because invoke_softirq() has already been invoked at
> this stage. But with this patch they are now under HARDIRQ_OFFSET and
> raise_softirq() wrongly thinks invoke_softirq() has yet to be called.
> In the end, this could leave the softirq unprocessed for a while.
> 
> So as Thomas suggested me, this also brings a check in the end of
> irq_exit() that looks for pending softirqs raised after invoke_softirq()
> and wake up ksoftirqd if needed.
> 
> Given that the cleanup on the contexts transition involves that
> new unpretty workaround, I have mixed feelings about this patch so I
> tagged it as "RFC" and I wait for your opinion.

Of course, I'm all for it because I suggested that solution :)

Seriously, I prefer to have in_interrupt() and in_irq() working in the
functions which are called from irq_exit() rather than having special
case workarounds inside of them. We are in interrupt context at this
point and not in some magic virtual place. 

The minimal extra check at the end of irq_exit() is way better than
any other special cased workaround and the softirq stuff is really the
only thing which needs to be taken care of. Anything else just works.

Thanks,

	tglx

Re: [PATCH 2/2] irq: Cleanup context state transitions in irq_exit()

[Frederic Weisbecker]

On Fri, Feb 22, 2013 at 03:33:51PM +0100, Thomas Gleixner wrote:
> On Fri, 22 Feb 2013, Frederic Weisbecker wrote:
> > The irq code is run under HARDIRQ_OFFSET preempt offset until
> > we reach the softirq code. Then it's substracted, leaving the
> > preempt count to 0, whether we have pending softirqs or not.
> > 
> > Afterward, if we have softirqs to run, we'll run them under
> > the SOFTIRQ_OFFSET then set the preempt offset back to 0
> > after softirqs completion.
> > 
> > The rest of the code in irq_exit(), mainly tick_nohz_irq_exit()
> > and rcu_irq_exit(), are executed with this NULL preempt offset.
> > 
> > The semantics here are not very intuitive. They leave several portions
> > of the code into some half-defined context state, where in_interrupt()
> > returns false while we actually are in an interrupt.
> > 
> > In order to make the context definition less confusing, let's
> > cover the whole code in irq_exit() under HARDIRQ_OFFSET, except
> > for the softirq processing where we switch back and forth
> > from HARDIRQ_OFFSET to SOFTIRQ_OFFSET without leaving a gap
> > in the context definition.
> > 
> > There is a drawback though: raise_softirq() relies on the previous
> > semantics considering that as long as we are in_interrupt(), the
> > pending softirq will be handled in the end of the interrupt. Otherwise
> > it kicks ksoftirqd so the softirq is always processed.
> > 
> > Now tick_nohz_irq_exit() and rcu_irq_exit() can raise softirqs
> > themselves. Since these functions were not under the HARDIRQ_OFFSET,
> > calling raise_softirq() resulted in waking up the ksoftirqd thread.
> > This is correct because invoke_softirq() has already been invoked at
> > this stage. But with this patch they are now under HARDIRQ_OFFSET and
> > raise_softirq() wrongly thinks invoke_softirq() has yet to be called.
> > In the end, this could leave the softirq unprocessed for a while.
> > 
> > So as Thomas suggested me, this also brings a check in the end of
> > irq_exit() that looks for pending softirqs raised after invoke_softirq()
> > and wake up ksoftirqd if needed.
> > 
> > Given that the cleanup on the contexts transition involves that
> > new unpretty workaround, I have mixed feelings about this patch so I
> > tagged it as "RFC" and I wait for your opinion.
> 
> Of course, I'm all for it because I suggested that solution :)
> 
> Seriously, I prefer to have in_interrupt() and in_irq() working in the
> functions which are called from irq_exit() rather than having special
> case workarounds inside of them. We are in interrupt context at this
> point and not in some magic virtual place. 
> 
> The minimal extra check at the end of irq_exit() is way better than
> any other special cased workaround and the softirq stuff is really the
> only thing which needs to be taken care of. Anything else just works.

Yeah that's indeed a point in favour of this patch.

I prefer to let you guys have the final word on this patch. Whether you
apply it or not, I fear I'll never be entirely happy either way :)
That's the sad fate of dealing with circular dependencies...

Re: [PATCH 2/2] irq: Cleanup context state transitions in irq_exit()

[Thomas Gleixner]

On Fri, 22 Feb 2013, Frederic Weisbecker wrote:
> On Fri, Feb 22, 2013 at 03:33:51PM +0100, Thomas Gleixner wrote:
> > The minimal extra check at the end of irq_exit() is way better than
> > any other special cased workaround and the softirq stuff is really the
> > only thing which needs to be taken care of. Anything else just works.
> 
> Yeah that's indeed a point in favour of this patch.
> 
> I prefer to let you guys have the final word on this patch. Whether you
> apply it or not, I fear I'll never be entirely happy either way :)
> That's the sad fate of dealing with circular dependencies...

plus the butt ugly softirq semantics or the lack thereof ...

Re: [PATCH 2/2] irq: Cleanup context state transitions in irq_exit()

[Linus Torvalds]

On Fri, Feb 22, 2013 at 7:06 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>> I prefer to let you guys have the final word on this patch. Whether you
>> apply it or not, I fear I'll never be entirely happy either way :)
>> That's the sad fate of dealing with circular dependencies...
>
> plus the butt ugly softirq semantics or the lack thereof ...

The softirq semantics are perfectly fine. Don't blame softirq for the
fact that irq_exit() has had shit-for-brains for a long time.

Just move the whole "invoke_softirq()" thing down to *after* the
tick_nohz_irq_exit() stuff. And that "wakeup_softirqd()" is garbage
too, since the whole thing should only be used for the
"force_irqthreads" case (which invoke_softirq()" got right.

And get rid of that final

 #ifndef __ARCH_IRQ_EXIT_IRQS_DISABLED
        local_irq_restore(flags);
 #endif

because even if the architecture enters irq_exit() with interrupts
enabled, we should damn well exit with them disabled so that there are
no races with new recursive interrupts (other than the ones that
wakeup_softirqd already handled).

In other words, I think all those special cases are indeed indicative
of something being wrong, but that "something" is not the softirq
code. Don't blame that. Blame the fact that irq_exit() is simply
written wrong. The softirq code should be done last (it used to be
done from the asm code), and the whole comment about
tick_nohz_irq_exit() perhaps changing something looks like pure and
utter garbage.

Don't blame the wrong code here.

               Linus

Re: [PATCH 2/2] irq: Cleanup context state transitions in irq_exit()

[Frederic Weisbecker]

On Fri, Feb 22, 2013 at 01:08:17PM -0800, Linus Torvalds wrote:
> On Fri, Feb 22, 2013 at 7:06 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >>
> >> I prefer to let you guys have the final word on this patch. Whether you
> >> apply it or not, I fear I'll never be entirely happy either way :)
> >> That's the sad fate of dealing with circular dependencies...
> >
> > plus the butt ugly softirq semantics or the lack thereof ...
> 
> The softirq semantics are perfectly fine. Don't blame softirq for the
> fact that irq_exit() has had shit-for-brains for a long time.
> 
> Just move the whole "invoke_softirq()" thing down to *after* the
> tick_nohz_irq_exit() stuff. And that "wakeup_softirqd()" is garbage
> too, since the whole thing should only be used for the
> "force_irqthreads" case (which invoke_softirq()" got right.

The issue here is that softirqs may change some timers state (ie: enqueue
new timer list, dequeue, modify...), so tick_nohz_irq_exit() really need to be
called after irq tail time softirq processing in order to propagate the timer
changes to the hardware.

But tick_nohz_irq_exit() may trigger the timer softirq itself.

Another issue is rcu_irq_exit(). softirqs may make use of RCU read side critical
sections, so rcu_irq_exit() has to be called after softirq processing.
Meanwhile it seems that RCU may raise its softirq from rcu_irq_exit(). Perhaps
Paul could confirm that.

So we have a circular dependency problem here. I guess the sanest solution is to
rework tick_nohz_irq_exit() and rcu_irq_exit() to remove the use of softirqs there.
And even trigger a warning if that happens. Or we need some way to force raise
through ksoftirqd but that doesn't sound like a proper long term solution.
 
> And get rid of that final
> 
>  #ifndef __ARCH_IRQ_EXIT_IRQS_DISABLED
>         local_irq_restore(flags);
>  #endif
> 
> because even if the architecture enters irq_exit() with interrupts
> enabled, we should damn well exit with them disabled so that there are
> no races with new recursive interrupts (other than the ones that
> wakeup_softirqd already handled).

Agreed, we always return from irq_exit() with irqs disabled as long as
we processed pending softirqs anyway.

Re: [PATCH 2/2] irq: Cleanup context state transitions in irq_exit()

[Linus Torvalds]

On Sat, Feb 23, 2013 at 10:21 AM, Frederic Weisbecker
<fweisbec@gmail.com> wrote:
>
> But tick_nohz_irq_exit() may trigger the timer softirq itself.

Suggestion: merge it with the whole softirq handler.

The softirq code *already* knows about the whole "oops, one softirq
may trigger another" issue, and has a loop - with protection against
excess - for exactly this reason. See the whole "goto restart" thing.

And tick_nohz_irq_exit() really has very similar semantics to
softiq's, it's just "CPU is idle and no pending reschedule" instead of
a softirq. But the basic rules are the same ("only run this at the
top-level context when exiting the last irq").

So maybe the right thing to do is move the whole "goto restart" one
level up, and do softirq's and tick_nohz_irq_exit both inside that
loop.

              Linus

Re: [PATCH 2/2] irq: Cleanup context state transitions in irq_exit()

[Thomas Gleixner]

On Fri, 22 Feb 2013, Linus Torvalds wrote:
> On Fri, Feb 22, 2013 at 7:06 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >>
> >> I prefer to let you guys have the final word on this patch. Whether you
> >> apply it or not, I fear I'll never be entirely happy either way :)
> >> That's the sad fate of dealing with circular dependencies...
> >
> > plus the butt ugly softirq semantics or the lack thereof ...
> 
> The softirq semantics are perfectly fine. Don't blame softirq for the
> fact that irq_exit() has had shit-for-brains for a long time.
> 
> Just move the whole "invoke_softirq()" thing down to *after* the
> tick_nohz_irq_exit() stuff.

We can't move tick_nohz_irq_exit() before invoke_softirq() simply
because we need to take the timers into account for NOHZ and those can
change when the softirq code runs.

So no, we need an extra check after invoke_softirq() and the same is
true for RCU.

Thanks,

	tglx

Re: [PATCH 2/2] irq: Cleanup context state transitions in irq_exit()

[Frederic Weisbecker]

2013/2/26 Thomas Gleixner <tglx@linutronix.de>:
> On Fri, 22 Feb 2013, Linus Torvalds wrote:
>> On Fri, Feb 22, 2013 at 7:06 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> >>
>> >> I prefer to let you guys have the final word on this patch. Whether you
>> >> apply it or not, I fear I'll never be entirely happy either way :)
>> >> That's the sad fate of dealing with circular dependencies...
>> >
>> > plus the butt ugly softirq semantics or the lack thereof ...
>>
>> The softirq semantics are perfectly fine. Don't blame softirq for the
>> fact that irq_exit() has had shit-for-brains for a long time.
>>
>> Just move the whole "invoke_softirq()" thing down to *after* the
>> tick_nohz_irq_exit() stuff.
>
> We can't move tick_nohz_irq_exit() before invoke_softirq() simply
> because we need to take the timers into account for NOHZ and those can
> change when the softirq code runs.
>
> So no, we need an extra check after invoke_softirq() and the same is
> true for RCU.

And what do you think about Linus's idea to move tick_nohz_irq_exit()
to do_softirq()?
This sounds feasible and a right place to do this, I hope that won't
uglify do_softirq() though.
I can try something.

Re: [PATCH 2/2] irq: Cleanup context state transitions in irq_exit()

[Thomas Gleixner]

On Tue, 26 Feb 2013, Frederic Weisbecker wrote:

> 2013/2/26 Thomas Gleixner <tglx@linutronix.de>:
> > On Fri, 22 Feb 2013, Linus Torvalds wrote:
> >> On Fri, Feb 22, 2013 at 7:06 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >> >>
> >> >> I prefer to let you guys have the final word on this patch. Whether you
> >> >> apply it or not, I fear I'll never be entirely happy either way :)
> >> >> That's the sad fate of dealing with circular dependencies...
> >> >
> >> > plus the butt ugly softirq semantics or the lack thereof ...
> >>
> >> The softirq semantics are perfectly fine. Don't blame softirq for the
> >> fact that irq_exit() has had shit-for-brains for a long time.
> >>
> >> Just move the whole "invoke_softirq()" thing down to *after* the
> >> tick_nohz_irq_exit() stuff.
> >
> > We can't move tick_nohz_irq_exit() before invoke_softirq() simply
> > because we need to take the timers into account for NOHZ and those can
> > change when the softirq code runs.
> >
> > So no, we need an extra check after invoke_softirq() and the same is
> > true for RCU.
> 
> And what do you think about Linus's idea to move tick_nohz_irq_exit()
> to do_softirq()?
> This sounds feasible and a right place to do this, I hope that won't
> uglify do_softirq() though.
> I can try something.

Yeah, looks doable. the rcu stuff needs to go there as well, right?
 

Re: [PATCH 2/2] irq: Cleanup context state transitions in irq_exit()

[Frederic Weisbecker]

2013/2/26 Thomas Gleixner <tglx@linutronix.de>:
>
> On Tue, 26 Feb 2013, Frederic Weisbecker wrote:
>
>> 2013/2/26 Thomas Gleixner <tglx@linutronix.de>:
>> > On Fri, 22 Feb 2013, Linus Torvalds wrote:
>> >> On Fri, Feb 22, 2013 at 7:06 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> >> >>
>> >> >> I prefer to let you guys have the final word on this patch. Whether you
>> >> >> apply it or not, I fear I'll never be entirely happy either way :)
>> >> >> That's the sad fate of dealing with circular dependencies...
>> >> >
>> >> > plus the butt ugly softirq semantics or the lack thereof ...
>> >>
>> >> The softirq semantics are perfectly fine. Don't blame softirq for the
>> >> fact that irq_exit() has had shit-for-brains for a long time.
>> >>
>> >> Just move the whole "invoke_softirq()" thing down to *after* the
>> >> tick_nohz_irq_exit() stuff.
>> >
>> > We can't move tick_nohz_irq_exit() before invoke_softirq() simply
>> > because we need to take the timers into account for NOHZ and those can
>> > change when the softirq code runs.
>> >
>> > So no, we need an extra check after invoke_softirq() and the same is
>> > true for RCU.
>>
>> And what do you think about Linus's idea to move tick_nohz_irq_exit()
>> to do_softirq()?
>> This sounds feasible and a right place to do this, I hope that won't
>> uglify do_softirq() though.
>> I can try something.
>
> Yeah, looks doable. the rcu stuff needs to go there as well, right?

Probably. I'll try something.

Re: [PATCH 2/2] irq: Cleanup context state transitions in irq_exit()

[Paul E. McKenney]

On Tue, Feb 26, 2013 at 04:14:43PM +0100, Thomas Gleixner wrote:
> 
> On Tue, 26 Feb 2013, Frederic Weisbecker wrote:
> 
> > 2013/2/26 Thomas Gleixner <tglx@linutronix.de>:
> > > On Fri, 22 Feb 2013, Linus Torvalds wrote:
> > >> On Fri, Feb 22, 2013 at 7:06 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > >> >>
> > >> >> I prefer to let you guys have the final word on this patch. Whether you
> > >> >> apply it or not, I fear I'll never be entirely happy either way :)
> > >> >> That's the sad fate of dealing with circular dependencies...
> > >> >
> > >> > plus the butt ugly softirq semantics or the lack thereof ...
> > >>
> > >> The softirq semantics are perfectly fine. Don't blame softirq for the
> > >> fact that irq_exit() has had shit-for-brains for a long time.
> > >>
> > >> Just move the whole "invoke_softirq()" thing down to *after* the
> > >> tick_nohz_irq_exit() stuff.
> > >
> > > We can't move tick_nohz_irq_exit() before invoke_softirq() simply
> > > because we need to take the timers into account for NOHZ and those can
> > > change when the softirq code runs.
> > >
> > > So no, we need an extra check after invoke_softirq() and the same is
> > > true for RCU.
> > 
> > And what do you think about Linus's idea to move tick_nohz_irq_exit()
> > to do_softirq()?
> > This sounds feasible and a right place to do this, I hope that won't
> > uglify do_softirq() though.
> > I can try something.
> 
> Yeah, looks doable. the rcu stuff needs to go there as well, right?

If it does, it needs to do so in such a way that rcu_irq_enter() and
rcu_irq_exit() nest properly.  One area of concern is the force_irqthreads
case, skips calling do_softirq().  Another area of concern is the
__ARCH_IRQ_EXIT_IRQS_DISABLED case, which calls __do_softirq() rather
than do_softirq().

Or am I missing some adjustment that is to be made when moving rcu_irq_exit()
to do_softirq()?

							Thanx, Paul

Re: [PATCH 2/2] irq: Cleanup context state transitions in irq_exit()

[Thomas Gleixner]

On Tue, 26 Feb 2013, Paul E. McKenney wrote:
> On Tue, Feb 26, 2013 at 04:14:43PM +0100, Thomas Gleixner wrote:
> > 
> > On Tue, 26 Feb 2013, Frederic Weisbecker wrote:
> > > And what do you think about Linus's idea to move tick_nohz_irq_exit()
> > > to do_softirq()?
> > > This sounds feasible and a right place to do this, I hope that won't
> > > uglify do_softirq() though.
> > > I can try something.
> > 
> > Yeah, looks doable. the rcu stuff needs to go there as well, right?
> 
> If it does, it needs to do so in such a way that rcu_irq_enter() and
> rcu_irq_exit() nest properly.  One area of concern is the force_irqthreads
> case, skips calling do_softirq().  Another area of concern is the
> __ARCH_IRQ_EXIT_IRQS_DISABLED case, which calls __do_softirq() rather
> than do_softirq().

That's sorted already. We disable interrupts in irq_exit().

> Or am I missing some adjustment that is to be made when moving rcu_irq_exit()
> to do_softirq()?

Yeah, we need an extra parameter or such, so the other callers of
__do_softirq() don't mess with it.

I also noticed that rcu_irq_enter/exit needs to be symetric and the
NOHZ code will get confused as well if we call it asymetric.

So that will become even more ugly than the extra check at the end of
irq_exit().

Thanks,

	tglx

Re: [PATCH 2/2] irq: Cleanup context state transitions in irq_exit()

[Paul E. McKenney]

On Tue, Feb 26, 2013 at 07:55:06PM +0100, Thomas Gleixner wrote:
> On Tue, 26 Feb 2013, Paul E. McKenney wrote:
> > On Tue, Feb 26, 2013 at 04:14:43PM +0100, Thomas Gleixner wrote:
> > > 
> > > On Tue, 26 Feb 2013, Frederic Weisbecker wrote:
> > > > And what do you think about Linus's idea to move tick_nohz_irq_exit()
> > > > to do_softirq()?
> > > > This sounds feasible and a right place to do this, I hope that won't
> > > > uglify do_softirq() though.
> > > > I can try something.
> > > 
> > > Yeah, looks doable. the rcu stuff needs to go there as well, right?
> > 
> > If it does, it needs to do so in such a way that rcu_irq_enter() and
> > rcu_irq_exit() nest properly.  One area of concern is the force_irqthreads
> > case, skips calling do_softirq().  Another area of concern is the
> > __ARCH_IRQ_EXIT_IRQS_DISABLED case, which calls __do_softirq() rather
> > than do_softirq().
> 
> That's sorted already. We disable interrupts in irq_exit().
> 
> > Or am I missing some adjustment that is to be made when moving rcu_irq_exit()
> > to do_softirq()?
> 
> Yeah, we need an extra parameter or such, so the other callers of
> __do_softirq() don't mess with it.
> 
> I also noticed that rcu_irq_enter/exit needs to be symetric and the
> NOHZ code will get confused as well if we call it asymetric.

Exactly!

							Thanx, Paul

> So that will become even more ugly than the extra check at the end of
> irq_exit().
> 
> Thanks,
> 
> 	tglx
> 
> 
> 

[GIT PULL] irq: cleanups

[Frederic Weisbecker]

Hi,

Until everyone agrees on https://patchwork.kernel.org/patch/2174211/ ,
here are two commits that seem quite uncontroversial to me.

Please pull from:

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
	irq/urgent-for-tip

Thanks.

Frederic Weisbecker (2):
  irq: Remove IRQ_EXIT_OFFSET workaround
  irq: Don't re-enable interrupts at the end of irq_exit

 include/linux/hardirq.h |    2 --
 kernel/softirq.c        |   10 ++--------
 2 files changed, 2 insertions(+), 10 deletions(-)

-- 
1.7.5.4

[PATCH 1/2] irq: Remove IRQ_EXIT_OFFSET workaround

[Frederic Weisbecker]

The IRQ_EXIT_OFFSET trick was used to make sure the irq
doesn't get preempted after we substract the HARDIRQ_OFFSET
until we are entirely done with any code in irq_exit().

This workaround was necessary because some archs may call
irq_exit() with irqs enabled and there is still some code
in the end of this function that is not covered by the
HARDIRQ_OFFSET but want to stay non-preemptible.

Now that irq are always disabled in irq_exit(), the whole code
is guaranteed not to be preempted. We can thus remove this hack.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 include/linux/hardirq.h |    2 --
 kernel/softirq.c        |    3 +--
 2 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h
index 29eb805..c1d6555 100644
--- a/include/linux/hardirq.h
+++ b/include/linux/hardirq.h
@@ -118,10 +118,8 @@
 
 #ifdef CONFIG_PREEMPT_COUNT
 # define preemptible()	(preempt_count() == 0 && !irqs_disabled())
-# define IRQ_EXIT_OFFSET (HARDIRQ_OFFSET-1)
 #else
 # define preemptible()	0
-# define IRQ_EXIT_OFFSET HARDIRQ_OFFSET
 #endif
 
 #if defined(CONFIG_SMP) || defined(CONFIG_GENERIC_HARDIRQS)
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 24a921b..f42ff97 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -343,7 +343,7 @@ void irq_exit(void)
 
 	account_irq_exit_time(current);
 	trace_hardirq_exit();
-	sub_preempt_count(IRQ_EXIT_OFFSET);
+	sub_preempt_count(HARDIRQ_OFFSET);
 	if (!in_interrupt() && local_softirq_pending())
 		invoke_softirq();
 
@@ -353,7 +353,6 @@ void irq_exit(void)
 		tick_nohz_irq_exit();
 #endif
 	rcu_irq_exit();
-	sched_preempt_enable_no_resched();
 #ifndef __ARCH_IRQ_EXIT_IRQS_DISABLED
 	local_irq_restore(flags);
 #endif
-- 
1.7.5.4