[PATCH tip/rcu/urgent 0/3] Fix more lockdep-RCU splats

[PATCH tip/rcu/urgent 0/3] Fix more lockdep-RCU splats

[Paul E. McKenney]

Hello!

Three patches to fix RCU-lockdep splats in wake_affine and idr_get_next().

								Thanx, Paul

 b/kernel/sched_fair.c |    2 ++
 b/lib/idr.c           |    4 ++--
 kernel/sched_fair.c   |    2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)

[PATCH tip/core/urgent 1/3] rcu: fix lockdep splat in wake_affine()

[Paul E. McKenney]

From: Daniel J Blueman <daniel.blueman@gmail.com>

With 2.6.35-rc1 and your patch in the context below, we still see
"include/linux/cgroup.h:534 invoked rcu_dereference_check() without
protection!", so need this additional patch:

Acquire read-side RCU lock around task_group() calls, addressing
"include/linux/cgroup.h:534 invoked rcu_dereference_check() without
protection!" warning.

Signed-off-by: Daniel J Blueman <daniel.blueman@gmail.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 kernel/sched_fair.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
index eed35ed..ca56133 100644
--- a/kernel/sched_fair.c
+++ b/kernel/sched_fair.c
@@ -1240,6 +1240,7 @@ static int wake_affine(struct sched_domain *sd, struct task_struct *p, int sync)
 	 * effect of the currently running task from the load
 	 * of the current CPU:
 	 */
+	rcu_read_lock();
 	if (sync) {
 		tg = task_group(current);
 		weight = current->se.load.weight;
@@ -1249,6 +1250,7 @@ static int wake_affine(struct sched_domain *sd, struct task_struct *p, int sync)
 	}
 
 	tg = task_group(p);
+	rcu_read_unlock();
 	weight = p->se.load.weight;
 
 	/*
-- 
1.7.0.6

[PATCH tip/core/urgent 2/3] rcu: fix scope of wake_affine()'s new RCU read-side critical section

[Paul E. McKenney]

The RCU read-side critical section needs to cover all uses of the pointer
returned by task_group().

Located-by: Li Zefan <lizf@cn.fujitsu.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 kernel/sched_fair.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
index ca56133..a878b53 100644
--- a/kernel/sched_fair.c
+++ b/kernel/sched_fair.c
@@ -1250,7 +1250,6 @@ static int wake_affine(struct sched_domain *sd, struct task_struct *p, int sync)
 	}
 
 	tg = task_group(p);
-	rcu_read_unlock();
 	weight = p->se.load.weight;
 
 	/*
@@ -1277,6 +1276,7 @@ static int wake_affine(struct sched_domain *sd, struct task_struct *p, int sync)
 		balanced = this_eff_load <= prev_eff_load;
 	} else
 		balanced = true;
+	rcu_read_unlock();
 
 	/*
 	 * If the currently running task will sleep within
-- 
1.7.0.6

[PATCH tip/core/urgent 3/3] idr: fix RCU lockdep splat in idr_get_next()

[Paul E. McKenney]

Convert to rcu_dereference_raw() given that many callers may have many
different locking models.

Located-by: Miles Lane <miles.lane@gmail.com>
Tested-by: Miles Lane <miles.lane@gmail.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 lib/idr.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/idr.c b/lib/idr.c
index c1a2069..7f1a4f0 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -602,7 +602,7 @@ void *idr_get_next(struct idr *idp, int *nextidp)
 	/* find first ent */
 	n = idp->layers * IDR_BITS;
 	max = 1 << n;
-	p = rcu_dereference(idp->top);
+	p = rcu_dereference_raw(idp->top);
 	if (!p)
 		return NULL;
 
@@ -610,7 +610,7 @@ void *idr_get_next(struct idr *idp, int *nextidp)
 		while (n > 0 && p) {
 			n -= IDR_BITS;
 			*paa++ = p;
-			p = rcu_dereference(p->ary[(id >> n) & IDR_MASK]);
+			p = rcu_dereference_raw(p->ary[(id >> n) & IDR_MASK]);
 		}
 
 		if (p) {
-- 
1.7.0.6

Re: [PATCH tip/core/urgent 1/3] rcu: fix lockdep splat in wake_affine()

[Peter Zijlstra]

On Tue, 2010-06-15 at 15:29 -0700, Paul E. McKenney wrote:
> From: Daniel J Blueman <daniel.blueman@gmail.com>
> 
> With 2.6.35-rc1 and your patch in the context below, we still see
> "include/linux/cgroup.h:534 invoked rcu_dereference_check() without
> protection!", so need this additional patch:
> 
> Acquire read-side RCU lock around task_group() calls, addressing
> "include/linux/cgroup.h:534 invoked rcu_dereference_check() without
> protection!" warning.

Uhm,. this is all just slapping in rcu_read_lock() to make the warning
go away, without explanation of what and why.

Its not obvious what the races is, nor how its handled.

> Signed-off-by: Daniel J Blueman <daniel.blueman@gmail.com>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>  kernel/sched_fair.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
> index eed35ed..ca56133 100644
> --- a/kernel/sched_fair.c
> +++ b/kernel/sched_fair.c
> @@ -1240,6 +1240,7 @@ static int wake_affine(struct sched_domain *sd, struct task_struct *p, int sync)
>  	 * effect of the currently running task from the load
>  	 * of the current CPU:
>  	 */
> +	rcu_read_lock();
>  	if (sync) {
>  		tg = task_group(current);
>  		weight = current->se.load.weight;
> @@ -1249,6 +1250,7 @@ static int wake_affine(struct sched_domain *sd, struct task_struct *p, int sync)
>  	}
>  
>  	tg = task_group(p);
> +	rcu_read_unlock();
>  	weight = p->se.load.weight;
>  
>  	/*

Re: [PATCH tip/core/urgent 1/3] rcu: fix lockdep splat in wake_affine()

[Paul E. McKenney]

On Wed, Jun 16, 2010 at 08:23:11AM +0200, Peter Zijlstra wrote:
> On Tue, 2010-06-15 at 15:29 -0700, Paul E. McKenney wrote:
> > From: Daniel J Blueman <daniel.blueman@gmail.com>
> > 
> > With 2.6.35-rc1 and your patch in the context below, we still see
> > "include/linux/cgroup.h:534 invoked rcu_dereference_check() without
> > protection!", so need this additional patch:
> > 
> > Acquire read-side RCU lock around task_group() calls, addressing
> > "include/linux/cgroup.h:534 invoked rcu_dereference_check() without
> > protection!" warning.
> 
> Uhm,. this is all just slapping in rcu_read_lock() to make the warning
> go away, without explanation of what and why.
> 
> Its not obvious what the races is, nor how its handled.

Please let me know if my explanation in a recent email covers this.
If so, I will add it to the commit log.  If not, well, back to the
drawing board, I guess.  ;-)

							Thanx, Paul

> > Signed-off-by: Daniel J Blueman <daniel.blueman@gmail.com>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> >  kernel/sched_fair.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
> > index eed35ed..ca56133 100644
> > --- a/kernel/sched_fair.c
> > +++ b/kernel/sched_fair.c
> > @@ -1240,6 +1240,7 @@ static int wake_affine(struct sched_domain *sd, struct task_struct *p, int sync)
> >  	 * effect of the currently running task from the load
> >  	 * of the current CPU:
> >  	 */
> > +	rcu_read_lock();
> >  	if (sync) {
> >  		tg = task_group(current);
> >  		weight = current->se.load.weight;
> > @@ -1249,6 +1250,7 @@ static int wake_affine(struct sched_domain *sd, struct task_struct *p, int sync)
> >  	}
> >  
> >  	tg = task_group(p);
> > +	rcu_read_unlock();
> >  	weight = p->se.load.weight;
> >  
> >  	/*
> 
>