rcu: Add missing RCU idle APIs on idle loop

So this fixes some potential RCU stalls in a bunch of architectures.
When rcu_idle_enter()/rcu_idle_exit() became a requirement, we forgot
to handle the architectures that don't support CONFIG_NO_HZ.

I guess the set should be dispatched into arch maintainer trees.

I'm sorry I haven't built tested everywhere. But the changes are
small and need to be at least boot tested anyway.

Also many of these archs use the same kind of idle loop:

void cpu_idle(void)
{
        while (1) {
                rcu_idle_enter();
                while (!need_resched())
                        //power saving function()
                rcu_idle_exit();
                schedule_preempt_disabled();
        }
}

So once the set is merged, I'll probably try to consolidate this with a generic
simple cpu_idle() that does the above and calls the arch power saving
function. This will be only for archs that use this simple idle loop
of course.

Thanks.

Frederic Weisbecker (10):
  alpha: Add missing RCU idle APIs on idle loop
  cris: Add missing RCU idle APIs on idle loop
  frv: Add missing RCU idle APIs on idle loop
  h8300: Add missing RCU idle APIs on idle loop
  m32r: Add missing RCU idle APIs on idle loop
  m68k: Add missing RCU idle APIs on idle loop
  mn10300: Add missing RCU idle APIs on idle loop
  parisc: Add missing RCU idle APIs on idle loop
  score: Add missing RCU idle APIs on idle loop
  xtensa: Add missing RCU idle APIs on idle loop

16 more lines

In the old times, the whole idle task was considered
as an RCU quiescent state. But as RCU became more and
more successful overtime, some RCU read side critical
section have been added even in the code of some
architectures idle tasks, for tracing for example.

So nowadays, rcu_idle_enter() and rcu_idle_exit() must
be called by the architecture to tell RCU about the part
in the idle loop that doesn't make use of rcu read side
critical sections, typically the part that puts the CPU
in low power mode.

This is necessary for RCU to find the quiescent states in
idle in order to complete grace periods.

Add this missing pair of calls in the Alpha's idle loop.

Reported-by: Paul E. McKenney <redacted>
Signed-off-by: Frederic Weisbecker <redacted>
Cc: Richard Henderson <redacted>
Cc: Ivan Kokshaysky <redacted>
Cc: Matt Turner <mattst88@gmail.com>
Cc: alpha <redacted>
Cc: Paul E. McKenney <redacted>
Cc: 3.2.x.. <stable@kernel.org>
---
 arch/alpha/kernel/process.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/arch/alpha/kernel/process.c b/arch/alpha/kernel/process.c
index 153d3fc..2ebf7b5 100644
--- a/arch/alpha/kernel/process.c
+++ b/arch/alpha/kernel/process.c

@@ -28,6 +28,7 @@
 #include <linux/tty.h>
 #include <linux/console.h>
 #include <linux/slab.h>
+#include <linux/rcupdate.h>
 
 #include <asm/reg.h>
 #include <asm/uaccess.h>

@@ -50,13 +51,16 @@ cpu_idle(void)
 {
 	set_thread_flag(TIF_POLLING_NRFLAG);
 
+	preempt_disable();
 	while (1) {
 		/* FIXME -- EV6 and LCA45 know how to power down
 		   the CPU.  */
 
+		rcu_idle_enter();
 		while (!need_resched())
 			cpu_relax();
-		schedule();
+		rcu_idle_exit();
+		schedule_preempt_disabled();
 	}
 }
 

-- 
1.7.5.4

On Wed, Aug 22, 2012 at 6:23 PM, Frederic Weisbecker [off-list ref] wrote:

So this fixes some potential RCU stalls in a bunch of architectures.
When rcu_idle_enter()/rcu_idle_exit() became a requirement, we forgot
to handle the architectures that don't support CONFIG_NO_HZ.

I guess the set should be dispatched into arch maintainer trees.

I can take the m68k version, but are you sure you want it this way?
Each of them must be in mainline before they can enter stable.

I'm sorry I haven't built tested everywhere. But the changes are
small and need to be at least boot tested anyway.

Builds and boots fine on m68k under ARAnyM.
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org> (for m68k)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

On Wed, Aug 22, 2012 at 06:23:39PM +0200, Frederic Weisbecker wrote:

6 more quoted lines

I don't understand the above preempt_disable() not having a matching
preempt_enable() at exit, but the rest of the patches in this series
look good to me.

							Thanx, Paul

 	while (1) {
 		/* FIXME -- EV6 and LCA45 know how to power down
 		   the CPU.  */

+		rcu_idle_enter();
 		while (!need_resched())
 			cpu_relax();
-		schedule();
+		rcu_idle_exit();
+		schedule_preempt_disabled();
 	}
 }

-- 
1.7.5.4

On Wed, Aug 22, 2012 at 10:19:30AM -0700, Paul E. McKenney wrote:

On Wed, Aug 22, 2012 at 06:23:39PM +0200, Frederic Weisbecker wrote:

quoted

In the old times, the whole idle task was considered
as an RCU quiescent state. But as RCU became more and
more successful overtime, some RCU read side critical
section have been added even in the code of some
architectures idle tasks, for tracing for example.

So nowadays, rcu_idle_enter() and rcu_idle_exit() must
be called by the architecture to tell RCU about the part
in the idle loop that doesn't make use of rcu read side
critical sections, typically the part that puts the CPU
in low power mode.

This is necessary for RCU to find the quiescent states in
idle in order to complete grace periods.

Add this missing pair of calls in the Alpha's idle loop.

Reported-by: Paul E. McKenney <redacted>
Signed-off-by: Frederic Weisbecker <redacted>
Cc: Richard Henderson <redacted>
Cc: Ivan Kokshaysky <redacted>
Cc: Matt Turner <mattst88@gmail.com>
Cc: alpha <redacted>
Cc: Paul E. McKenney <redacted>
Cc: 3.2.x.. <stable@kernel.org>
---
 arch/alpha/kernel/process.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/arch/alpha/kernel/process.c b/arch/alpha/kernel/process.c
index 153d3fc..2ebf7b5 100644
--- a/arch/alpha/kernel/process.c
+++ b/arch/alpha/kernel/process.c

@@ -28,6 +28,7 @@
 #include <linux/tty.h>
 #include <linux/console.h>
 #include <linux/slab.h>
+#include <linux/rcupdate.h>

11 more quoted lines

The current code is preemptable, at least it appears so because it calls
schedule() directly. And if I call rcu_idle_enter() in a preemptable section,
I'm in trouble because I'll schedule while in extended QS.

Thus I need to disable preemption here at least until I call rcu_idle_exit().

Now this is an endless loop so there is no need to re-enable
preemption after the loop. And schedule_preempt_disabled()
takes care of enabling preemption before schedule() and redisabling
it afterward.

							Thanx, Paul

quoted

 	while (1) {
 		/* FIXME -- EV6 and LCA45 know how to power down
 		   the CPU.  */

+		rcu_idle_enter();
 		while (!need_resched())
 			cpu_relax();
-		schedule();
+		rcu_idle_exit();
+		schedule_preempt_disabled();
 	}
 }

-- 
1.7.5.4

On Wed, Aug 22, 2012 at 07:35:45PM +0200, Frederic Weisbecker wrote:

On Wed, Aug 22, 2012 at 10:19:30AM -0700, Paul E. McKenney wrote:

quoted

On Wed, Aug 22, 2012 at 06:23:39PM +0200, Frederic Weisbecker wrote:

quoted

In the old times, the whole idle task was considered
as an RCU quiescent state. But as RCU became more and
more successful overtime, some RCU read side critical
section have been added even in the code of some
architectures idle tasks, for tracing for example.

So nowadays, rcu_idle_enter() and rcu_idle_exit() must
be called by the architecture to tell RCU about the part
in the idle loop that doesn't make use of rcu read side
critical sections, typically the part that puts the CPU
in low power mode.

This is necessary for RCU to find the quiescent states in
idle in order to complete grace periods.

Add this missing pair of calls in the Alpha's idle loop.

Reported-by: Paul E. McKenney <redacted>
Signed-off-by: Frederic Weisbecker <redacted>
Cc: Richard Henderson <redacted>
Cc: Ivan Kokshaysky <redacted>
Cc: Matt Turner <mattst88@gmail.com>
Cc: alpha <redacted>
Cc: Paul E. McKenney <redacted>
Cc: 3.2.x.. <stable@kernel.org>
---
 arch/alpha/kernel/process.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/arch/alpha/kernel/process.c b/arch/alpha/kernel/process.c
index 153d3fc..2ebf7b5 100644
--- a/arch/alpha/kernel/process.c
+++ b/arch/alpha/kernel/process.c

@@ -28,6 +28,7 @@
 #include <linux/tty.h>
 #include <linux/console.h>
 #include <linux/slab.h>
+#include <linux/rcupdate.h>

39 more quoted lines

Understood, but what I don't understand is why you don't need a
preempt_enable() right here.

							Thanx, Paul

quoted

quoted

 }

-- 
1.7.5.4

On 23/08/12 04:23, Frederic Weisbecker wrote:

In the old times, the whole idle task was considered
as an RCU quiescent state. But as RCU became more and
more successful overtime, some RCU read side critical
section have been added even in the code of some
architectures idle tasks, for tracing for example.

Fantastic!  It fixes RCU CPU stalls that we were seeing on the SMP
kernel when built for generic Alpha.

A build of glibc and running its test suite reliably triggers RCU CPU
stalls when running a kernel built for generic Alpha.  I have just built
glibc and ran its test suite twice with no RCU CPU stalls with this
patch against a 3.5.2 kernel!  Nice.  Very nice.

I see the stable queue is CCed but I note the patch does not apply
cleanly to the 3.2.y kernel.  It would be nice to have a backport of the
patches for the 3.2 stable kernel.

So feel free to add:

Tested-by:  Michael Cree <redacted>

Cheers
Michael.

On Wed, Aug 22, 2012 at 12:01:09PM -0700, Paul E. McKenney wrote:

quoted

The current code is preemptable, at least it appears so because it calls
schedule() directly. And if I call rcu_idle_enter() in a preemptable section,
I'm in trouble because I'll schedule while in extended QS.

Thus I need to disable preemption here at least until I call rcu_idle_exit().

Now this is an endless loop so there is no need to re-enable
preemption after the loop. And schedule_preempt_disabled()
takes care of enabling preemption before schedule() and redisabling
it afterward.

quoted

							Thanx, Paul

quoted

 	while (1) {
 		/* FIXME -- EV6 and LCA45 know how to power down
 		   the CPU.  */

+		rcu_idle_enter();
 		while (!need_resched())
 			cpu_relax();
-		schedule();
+		rcu_idle_exit();
+		schedule_preempt_disabled();
 	}

Understood, but what I don't understand is why you don't need a
preempt_enable() right here.

Look, let's inline the content of schedule_preempt_disabled(), the code
then looks like:

void cpu_idle(void)
{
	set_thread_flag(TIF_POLLING_NRFLAG);

	preempt_disable();
	while (1) {
		/* FIXME -- EV6 and LCA45 know how to power down
		   the CPU.  */

		rcu_idle_enter();
		while (!need_resched())
			cpu_relax();
		rcu_idle_exit();

		sched_preempt_enable_no_resched();
	        schedule();
	        preempt_disable();
	}
}

So there is a preempt_enable() before we schedule, then we re-disable
preemption after schedule.

Now I realize cpu_idle() is supposed to be called with preemption disabled
already so I shouldn't add an explicit preempt_disable() or it's going to be worse.
But that means there is an existing bug here in alpha, it should call schedule_preempt_disabled()
instead of schedule(). cpu_idle() is called with preemption disabled on the boot CPU.
And it should as well from the secondary CPUs entry but alpha doesn't seem to do that.

So I need to fix that first. I'll respin.

On Thu, Aug 23, 2012 at 09:32:18PM +1200, Michael Cree wrote:

On 23/08/12 04:23, Frederic Weisbecker wrote:

quoted

In the old times, the whole idle task was considered
as an RCU quiescent state. But as RCU became more and
more successful overtime, some RCU read side critical
section have been added even in the code of some
architectures idle tasks, for tracing for example.

Fantastic!  It fixes RCU CPU stalls that we were seeing on the SMP
kernel when built for generic Alpha.

A build of glibc and running its test suite reliably triggers RCU CPU
stalls when running a kernel built for generic Alpha.  I have just built
glibc and ran its test suite twice with no RCU CPU stalls with this
patch against a 3.5.2 kernel!  Nice.  Very nice.

I see the stable queue is CCed but I note the patch does not apply
cleanly to the 3.2.y kernel.  It would be nice to have a backport of the
patches for the 3.2 stable kernel.

Sure.

So feel free to add:

Tested-by:  Michael Cree <redacted>

Thanks, but I need to refactor the patch, I suspect a problem with CONFIG_PREEMPT.

On Wed, Aug 22, 2012 at 07:18:04PM +0200, Geert Uytterhoeven wrote:

On Wed, Aug 22, 2012 at 6:23 PM, Frederic Weisbecker [off-list ref] wrote:

quoted

So this fixes some potential RCU stalls in a bunch of architectures.
When rcu_idle_enter()/rcu_idle_exit() became a requirement, we forgot
to handle the architectures that don't support CONFIG_NO_HZ.

I guess the set should be dispatched into arch maintainer trees.

I can take the m68k version, but are you sure you want it this way?
Each of them must be in mainline before they can enter stable.

Yeah, I was thinking the right route is for these patches to be
carried by arch maintainer who then push to Linus and then this goes
to stable.

Is that ok for you?

Otherwise I can carry the patches myself. In a tree of my own, or
Paul's or mmotm. As long as I have your ack.

Thanks.

quoted

I'm sorry I haven't built tested everywhere. But the changes are
small and need to be at least boot tested anyway.

Builds and boots fine on m68k under ARAnyM.
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org> (for m68k)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

On Thu, Aug 23, 2012 at 12:42:11PM +0200, Frederic Weisbecker wrote:

On Wed, Aug 22, 2012 at 12:01:09PM -0700, Paul E. McKenney wrote:

quoted

quoted

The current code is preemptable, at least it appears so because it calls
schedule() directly. And if I call rcu_idle_enter() in a preemptable section,
I'm in trouble because I'll schedule while in extended QS.

Thus I need to disable preemption here at least until I call rcu_idle_exit().

Now this is an endless loop so there is no need to re-enable
preemption after the loop. And schedule_preempt_disabled()
takes care of enabling preemption before schedule() and redisabling
it afterward.

quoted

							Thanx, Paul

quoted

 	while (1) {
 		/* FIXME -- EV6 and LCA45 know how to power down
 		   the CPU.  */

+		rcu_idle_enter();
 		while (!need_resched())
 			cpu_relax();
-		schedule();
+		rcu_idle_exit();
+		schedule_preempt_disabled();
 	}

Understood, but what I don't understand is why you don't need a
preempt_enable() right here.

Look, let's inline the content of schedule_preempt_disabled(), the code
then looks like:

void cpu_idle(void)
{
	set_thread_flag(TIF_POLLING_NRFLAG);

	preempt_disable();
	while (1) {

12 more quoted lines

	preempt_enable();  /* Why is this not needed? */

}

So there is a preempt_enable() before we schedule, then we re-disable
preemption after schedule.

Now I realize cpu_idle() is supposed to be called with preemption disabled
already so I shouldn't add an explicit preempt_disable() or it's going to be worse.
But that means there is an existing bug here in alpha, it should call schedule_preempt_disabled()
instead of schedule(). cpu_idle() is called with preemption disabled on the boot CPU.
And it should as well from the secondary CPUs entry but alpha doesn't seem to do that.

So I need to fix that first. I'll respin.

OK, look forward to seeing the respin.

							Thanx, Paul

Hi Frederic,

On Thu, Aug 23, 2012 at 1:02 PM, Frederic Weisbecker [off-list ref] wrote:

On Wed, Aug 22, 2012 at 07:18:04PM +0200, Geert Uytterhoeven wrote:

quoted

On Wed, Aug 22, 2012 at 6:23 PM, Frederic Weisbecker [off-list ref] wrote:

quoted

So this fixes some potential RCU stalls in a bunch of architectures.
When rcu_idle_enter()/rcu_idle_exit() became a requirement, we forgot
to handle the architectures that don't support CONFIG_NO_HZ.

I guess the set should be dispatched into arch maintainer trees.

I can take the m68k version, but are you sure you want it this way?
Each of them must be in mainline before they can enter stable.

Yeah, I was thinking the right route is for these patches to be
carried by arch maintainer who then push to Linus and then this goes
to stable.

Is that ok for you?

Otherwise I can carry the patches myself. In a tree of my own, or
Paul's or mmotm. As long as I have your ack.

I applied your patch to the m68k for-3.6/for-linus branch.
I'll ask Linus to pull later in the rc cycle (right now I don't have
anything else
queued for 3.6).
Still, I think it's better to just collect acks and send it to Linus
in one shot,
so it can go into stable in one shot too.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

On Thu, Aug 23, 2012 at 10:23:22PM +0200, Geert Uytterhoeven wrote:

Hi Frederic,

On Thu, Aug 23, 2012 at 1:02 PM, Frederic Weisbecker [off-list ref] wrote:

quoted

On Wed, Aug 22, 2012 at 07:18:04PM +0200, Geert Uytterhoeven wrote:

quoted

On Wed, Aug 22, 2012 at 6:23 PM, Frederic Weisbecker [off-list ref] wrote:

quoted

So this fixes some potential RCU stalls in a bunch of architectures.
When rcu_idle_enter()/rcu_idle_exit() became a requirement, we forgot
to handle the architectures that don't support CONFIG_NO_HZ.

I guess the set should be dispatched into arch maintainer trees.

I can take the m68k version, but are you sure you want it this way?
Each of them must be in mainline before they can enter stable.

Yeah, I was thinking the right route is for these patches to be
carried by arch maintainer who then push to Linus and then this goes
to stable.

Is that ok for you?

Otherwise I can carry the patches myself. In a tree of my own, or
Paul's or mmotm. As long as I have your ack.

I applied your patch to the m68k for-3.6/for-linus branch.
I'll ask Linus to pull later in the rc cycle (right now I don't have
anything else
queued for 3.6).
Still, I think it's better to just collect acks and send it to Linus
in one shot,
so it can go into stable in one shot too.

Sure I can do that if you prefer.

Thanks.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Hi Frederic, Paul,

On Thu, Aug 23, 2012 at 11:50 PM, Frederic Weisbecker
[off-list ref] wrote:

On Thu, Aug 23, 2012 at 10:23:22PM +0200, Geert Uytterhoeven wrote:

quoted

On Thu, Aug 23, 2012 at 1:02 PM, Frederic Weisbecker [off-list ref] wrote:

quoted

On Wed, Aug 22, 2012 at 07:18:04PM +0200, Geert Uytterhoeven wrote:

quoted

On Wed, Aug 22, 2012 at 6:23 PM, Frederic Weisbecker [off-list ref] wrote:

quoted

So this fixes some potential RCU stalls in a bunch of architectures.
When rcu_idle_enter()/rcu_idle_exit() became a requirement, we forgot
to handle the architectures that don't support CONFIG_NO_HZ.

I guess the set should be dispatched into arch maintainer trees.

I can take the m68k version, but are you sure you want it this way?
Each of them must be in mainline before they can enter stable.

Yeah, I was thinking the right route is for these patches to be
carried by arch maintainer who then push to Linus and then this goes
to stable.

Is that ok for you?

Otherwise I can carry the patches myself. In a tree of my own, or
Paul's or mmotm. As long as I have your ack.

I applied your patch to the m68k for-3.6/for-linus branch.
I'll ask Linus to pull later in the rc cycle (right now I don't have
anything else
queued for 3.6).
Still, I think it's better to just collect acks and send it to Linus
in one shot,
so it can go into stable in one shot too.

Sure I can do that if you prefer.

What's the conclusion on this one? I saw it entered tip.

I still have it (as the only commit) on my for-3.6 branch, but I don't
think m68k
is important enough to be the only architecture to have this fix in 3.6 ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

On Mon, Sep 17, 2012 at 10:31:24PM +0200, Geert Uytterhoeven wrote:

Hi Frederic, Paul,

On Thu, Aug 23, 2012 at 11:50 PM, Frederic Weisbecker
[off-list ref] wrote:

quoted

On Thu, Aug 23, 2012 at 10:23:22PM +0200, Geert Uytterhoeven wrote:

quoted

On Thu, Aug 23, 2012 at 1:02 PM, Frederic Weisbecker [off-list ref] wrote:

quoted

On Wed, Aug 22, 2012 at 07:18:04PM +0200, Geert Uytterhoeven wrote:

quoted

On Wed, Aug 22, 2012 at 6:23 PM, Frederic Weisbecker [off-list ref] wrote:

quoted

So this fixes some potential RCU stalls in a bunch of architectures.
When rcu_idle_enter()/rcu_idle_exit() became a requirement, we forgot
to handle the architectures that don't support CONFIG_NO_HZ.

I guess the set should be dispatched into arch maintainer trees.

I can take the m68k version, but are you sure you want it this way?
Each of them must be in mainline before they can enter stable.

Yeah, I was thinking the right route is for these patches to be
carried by arch maintainer who then push to Linus and then this goes
to stable.

Is that ok for you?

Otherwise I can carry the patches myself. In a tree of my own, or
Paul's or mmotm. As long as I have your ack.

I applied your patch to the m68k for-3.6/for-linus branch.
I'll ask Linus to pull later in the rc cycle (right now I don't have
anything else
queued for 3.6).
Still, I think it's better to just collect acks and send it to Linus
in one shot,
so it can go into stable in one shot too.

Sure I can do that if you prefer.

What's the conclusion on this one? I saw it entered tip.

I don't see it at tip/master, but perhaps I am looking at the wrong branch.

I still have it (as the only commit) on my for-3.6 branch, but I don't
think m68k
is important enough to be the only architecture to have this fix in 3.6 ;-)

I got only two acks in addition to yours, plus one Tested-by.  So, no,
there does not appear to be a large groundswell of support for pushing
this into 3.6.  If it doesn't go in by some other path, I will be pushing
it into 3.7.

							Thanx, Paul