diff mbox

[RFC,tip/core/rcu,28/28] rcu: Fix idle-task checks

Message ID 1320265849-5744-28-git-send-email-paulmck@linux.vnet.ibm.com
State New
Headers show

Commit Message

Paul E. McKenney Nov. 2, 2011, 8:30 p.m. UTC 
From: Paul E. McKenney <paul.mckenney@linaro.org>

RCU has traditionally relied on idle_cpu() to determine whether a given
CPU is running in the context of an idle task, but recent changes have
invalidated this approach.  This commit therefore switches from idle_cpu
to "current->pid != 0".

Reported-by: Wu Fengguang <fengguang.wu@intel.com>
Suggested-by: Carsten Emde <C.Emde@osadl.org>
Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
Tested-by: Wu Fengguang <fengguang.wu@intel.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 kernel/rcutiny.c |    4 ++--
 kernel/rcutree.c |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

Comments

Josh Triplett Nov. 3, 2011, 4:55 a.m. UTC | #1 
On Wed, Nov 02, 2011 at 01:30:49PM -0700, Paul E. McKenney wrote:
> From: Paul E. McKenney <paul.mckenney@linaro.org>
> 
> RCU has traditionally relied on idle_cpu() to determine whether a given
> CPU is running in the context of an idle task, but recent changes have
> invalidated this approach.  This commit therefore switches from idle_cpu
> to "current->pid != 0".

Could you elaborate a bit on "recent changes"?  It looks like you mean
commit 908a3283728d92df36e0c7cd63304fd35e93a8a9; if so, could you add
that reference to the commit message?

Also, the hard-coded use of "current->pid != 0" concerns me.  Could this
use some existing function?  Does idle_task() help?  If no appropriate
predicate exists, perhaps it should.  is_idle_task(current)?

- Josh Triplett
Paul E. McKenney Nov. 3, 2011, 9 p.m. UTC | #2 
On Wed, Nov 02, 2011 at 09:55:09PM -0700, Josh Triplett wrote:
> On Wed, Nov 02, 2011 at 01:30:49PM -0700, Paul E. McKenney wrote:
> > From: Paul E. McKenney <paul.mckenney@linaro.org>
> > 
> > RCU has traditionally relied on idle_cpu() to determine whether a given
> > CPU is running in the context of an idle task, but recent changes have
> > invalidated this approach.  This commit therefore switches from idle_cpu
> > to "current->pid != 0".
> 
> Could you elaborate a bit on "recent changes"?  It looks like you mean
> commit 908a3283728d92df36e0c7cd63304fd35e93a8a9; if so, could you add
> that reference to the commit message?

Will do!

> Also, the hard-coded use of "current->pid != 0" concerns me.  Could this
> use some existing function?  Does idle_task() help?  If no appropriate
> predicate exists, perhaps it should.  is_idle_task(current)?

I could use idle_task(), but that does quite a bit more work.
The hard-coded "current->pid != 0" is used in a number of other places
in the kernel, so there is precedent.  Might be worth fixing globally
as a separate fix, though.

							Thanx, Paul
Josh Triplett Nov. 3, 2011, 11:05 p.m. UTC | #3 
On Thu, Nov 03, 2011 at 02:00:05PM -0700, Paul E. McKenney wrote:
> On Wed, Nov 02, 2011 at 09:55:09PM -0700, Josh Triplett wrote:
> > On Wed, Nov 02, 2011 at 01:30:49PM -0700, Paul E. McKenney wrote:
> > > From: Paul E. McKenney <paul.mckenney@linaro.org>
> > > 
> > > RCU has traditionally relied on idle_cpu() to determine whether a given
> > > CPU is running in the context of an idle task, but recent changes have
> > > invalidated this approach.  This commit therefore switches from idle_cpu
> > > to "current->pid != 0".
> > 
> > Could you elaborate a bit on "recent changes"?  It looks like you mean
> > commit 908a3283728d92df36e0c7cd63304fd35e93a8a9; if so, could you add
> > that reference to the commit message?
> 
> Will do!
> 
> > Also, the hard-coded use of "current->pid != 0" concerns me.  Could this
> > use some existing function?  Does idle_task() help?  If no appropriate
> > predicate exists, perhaps it should.  is_idle_task(current)?
> 
> I could use idle_task(), but that does quite a bit more work.

Doesn't seem that high-overhead, but *shrug*.

> The hard-coded "current->pid != 0" is used in a number of other places
> in the kernel, so there is precedent. 

Well, 2 is a number, yes:

arch/sparc/kernel/setup_32.c:   if(current->pid != 0) {
kernel/events/core.c:           if (!(event->attr.exclude_idle && current->pid == 0))

> Might be worth fixing globally
> as a separate fix, though.

Fair enough.

- Josh Triplett
Peter Zijlstra Nov. 9, 2011, 2:52 p.m. UTC | #4 
On Wed, 2011-11-02 at 21:55 -0700, Josh Triplett wrote:
> On Wed, Nov 02, 2011 at 01:30:49PM -0700, Paul E. McKenney wrote:
> > From: Paul E. McKenney <paul.mckenney@linaro.org>
> > 
> > RCU has traditionally relied on idle_cpu() to determine whether a given
> > CPU is running in the context of an idle task, but recent changes have
> > invalidated this approach.  This commit therefore switches from idle_cpu
> > to "current->pid != 0".
> 
> Could you elaborate a bit on "recent changes"?  It looks like you mean
> commit 908a3283728d92df36e0c7cd63304fd35e93a8a9; if so, could you add
> that reference to the commit message?

Oh, that was unintended fallout, idle_cpu() was taken to mean is this
cpu currently idle, and was changed to not return true when there's
pending wakeups, since in that case the cpu isn't actually idle, even
though it might still be running the idle task.

> Also, the hard-coded use of "current->pid != 0" concerns me.  Could this
> use some existing function?  Does idle_task() help?  If no appropriate
> predicate exists, perhaps it should.  is_idle_task(current)?

Right, current == idle_task(smp_processor_id()) will test if the current
task is the idle task for the current cpu, regardless of whether the cpu
is actually idle or not.

Then again, the ->pid == 0 thing seems to be fairly solid as well,
having just looked at the fork_idle() code etc..
diff mbox

Patch

diff --git a/kernel/rcutiny.c b/kernel/rcutiny.c
index f4e7bc3..35f8a07 100644
--- a/kernel/rcutiny.c
+++ b/kernel/rcutiny.c
@@ -65,7 +65,7 @@  static void rcu_idle_enter_common(long long oldval)
 		return;
 	}
 	RCU_TRACE(trace_rcu_dyntick("Start", oldval, rcu_dynticks_nesting));
-	if (!idle_cpu(smp_processor_id())) {
+	if (current->pid != 0) {
 		struct task_struct *idle = idle_task(smp_processor_id());
 
 		RCU_TRACE(trace_rcu_dyntick("Error on entry: not idle task",
@@ -119,7 +119,7 @@  static void rcu_idle_exit_common(long long oldval)
 		return;
 	}
 	RCU_TRACE(trace_rcu_dyntick("End", oldval, rcu_dynticks_nesting));
-	if (!idle_cpu(smp_processor_id())) {
+	if (!current->pid != 0) {
 		struct task_struct *idle = idle_task(smp_processor_id());
 
 		RCU_TRACE(trace_rcu_dyntick("Error on exit: not idle task",
diff --git a/kernel/rcutree.c b/kernel/rcutree.c
index 3d7b474..414af68 100644
--- a/kernel/rcutree.c
+++ b/kernel/rcutree.c
@@ -355,7 +355,7 @@  static void rcu_idle_enter_common(struct rcu_dynticks *rdtp, long long oldval)
 		return;
 	}
 	trace_rcu_dyntick("Start", oldval, rdtp->dynticks_nesting);
-	if (!idle_cpu(smp_processor_id())) {
+	if (current->pid != 0) {
 		struct task_struct *idle = idle_task(smp_processor_id());
 
 		trace_rcu_dyntick("Error on entry: not idle task",
@@ -449,7 +449,7 @@  static void rcu_idle_exit_common(struct rcu_dynticks *rdtp, long long oldval)
 	smp_mb__after_atomic_inc();  /* See above. */
 	WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
 	trace_rcu_dyntick("End", oldval, rdtp->dynticks_nesting);
-	if (!idle_cpu(smp_processor_id())) {
+	if (current->pid != 0) {
 		struct task_struct *idle = idle_task(smp_processor_id());
 
 		trace_rcu_dyntick("Error on exit: not idle task",