sched: more sched_domain iterations fix

PATCH Superseded Xiaotian Feng 2 participants 6 messages Apr 21, 2011 – May 28, 2011

Share & tools

Xiaotian Feng

author

2011-04-21 11:07 UTC #0 lore raw

PATCH 1/1 sched: more sched_domain iterations fix

sched_domain iterations needs to be protected by rcu_read_lock() now, this patch adds another two places which needs the rcu lock.

Signed-off-by Xiaotian Feng <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

---
 kernel/sched_rt.c    |    2 ++
 kernel/sched_stats.h |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

kernel/sched_rt.c +2 -0

index 19ecb31..901ed2a 100644
@@ -1282,7 +1282,9 @@ static struct rq *find_lock_lowest_rq(struct task_struct *task, struct rq *rq)
 	int cpu;
 
 	for (tries = 0; tries < RT_MAX_TRIES; tries++) {
+		rcu_read_lock();
 		cpu = find_lowest_rq(task);
+		rcu_read_unlock();
 
 		if ((cpu == -1) || (cpu == rq->cpu))
 			break;

kernel/sched_stats.h +2 -0

index 48ddf43..d25c8c1 100644
@@ -38,6 +38,7 @@ static int show_schedstat(struct seq_file *seq, void *v)
 #ifdef CONFIG_SMP
 		/* domain-specific stats */
 		preempt_disable();
+		rcu_read_lock();
 		for_each_domain(cpu, sd) {
 			enum cpu_idle_type itype;

@@ -64,6 +65,7 @@ static int show_schedstat(struct seq_file *seq, void *v)
 			    sd->ttwu_wake_remote, sd->ttwu_move_affine,
 			    sd->ttwu_move_balance);
 		}
+		rcu_read_unlock();
 		preempt_enable();
 #endif
 	}

Signature · 1 line

1.7.1

Peter Zijlstra

2011-04-21 11:18 UTC #1 lore raw

Quoted 2 lines · Xiaotian Feng

sched_domain iterations needs to be protected by rcu_read_lock() now, this patch adds another two places which needs the rcu lock.

Changelog fails to mention how you found out about these.

Quoted 13 lines

Signed-off-by Xiaotian Feng <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Peter Zijlstra <[email protected]>
---
 kernel/sched_rt.c    |    2 ++
 kernel/sched_stats.h |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)
kernel/sched_rt.c +0 -0
index 19ecb31..901ed2a 100644

kernel/sched_rt.c static struct rq *find_lock_lowest_rq(struct task_struct *task, struct rq *rq) · line 1287

Quoted 6 lines

kernel/sched_rt.c +2 -0

@@ -1282,7 +1282,9 @@ static struct rq *find_lock_lowest_rq(struct task_struct *task, struct rq *rq)
 	int cpu;
 
 	for (tries = 0; tries < RT_MAX_TRIES; tries++) {
+		rcu_read_lock();
 		cpu = find_lowest_rq(task);
+		rcu_read_unlock();

I would put that inside find_lowest_rq() instead of around.

Quoted 15 lines

kernel/sched_rt.c +0 -0

 		if ((cpu == -1) || (cpu == rq->cpu))
 			break;

kernel/sched_stats.h +1 -0

index 48ddf43..d25c8c1 100644
@@ -38,6 +38,7 @@ static int show_schedstat(struct seq_file *seq, void *v)
 #ifdef CONFIG_SMP
 		/* domain-specific stats */
 		preempt_disable();
+		rcu_read_lock();
 		for_each_domain(cpu, sd) {
 			enum cpu_idle_type itype;

kernel/sched_stats.h static int show_schedstat(struct seq_file *seq, void *v) · line 69

Quoted 5 lines

kernel/sched_stats.h +1 -0

@@ -64,6 +65,7 @@ static int show_schedstat(struct seq_file *seq, void *v)
 			    sd->ttwu_wake_remote, sd->ttwu_move_affine,
 			    sd->ttwu_move_balance);
 		}
+		rcu_read_unlock();
 		preempt_enable();

I suspect that those preempt_disable/enable are an attempt at rcu_read_lock_sched() before that existed, which would suggest they are now redundant.

Quoted 2 lines

kernel/sched_stats.h +0 -0
 #endif
 	}

Xiaotian feng

author

2011-04-22 10:54 UTC #2 lore raw

PATCH v2 1/1 sched: more sched_domain iterations fix

From: Xiaotian Feng <[email protected]>

sched_domain iterations needs to be protected by rcu_read_lock() now, this patch adds another two places which needs the rcu lock, which is spotted by following suspicious rcu_dereference_check() usage warnings.

kernel/sched_rt.c:1244 invoked rcu_dereference_check() without protection! kernel/sched_stats.h:41 invoked rcu_dereference_check() without protection!

Signed-off-by Xiaotian Feng <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

---
 kernel/sched_rt.c    |   10 ++++++++--
 kernel/sched_stats.h |    4 ++--
 2 files changed, 10 insertions(+), 4 deletions(-)

kernel/sched_rt.c +8 -2

index 19ecb31..46a9533 100644
@@ -1241,6 +1241,7 @@ static int find_lowest_rq(struct task_struct *task)
 	if (!cpumask_test_cpu(this_cpu, lowest_mask))
 		this_cpu = -1; /* Skip this_cpu opt if not among lowest */
 
+	rcu_read_lock();
 	for_each_domain(cpu, sd) {
 		if (sd->flags & SD_WAKE_AFFINE) {
 			int best_cpu;

@@ -1250,15 +1251,20 @@ static int find_lowest_rq(struct task_struct *task)
 			 * remote processor.
 			 */
 			if (this_cpu != -1 &&
-			    cpumask_test_cpu(this_cpu, sched_domain_span(sd)))
+			    cpumask_test_cpu(this_cpu, sched_domain_span(sd))) {
+				rcu_read_unlock();
 				return this_cpu;
+			}
 
 			best_cpu = cpumask_first_and(lowest_mask,
 						     sched_domain_span(sd));
-			if (best_cpu < nr_cpu_ids)
+			if (best_cpu < nr_cpu_ids) {
+				rcu_read_unlock();
 				return best_cpu;
+			}
 		}
 	}
+	rcu_read_unlock();
 
 	/*
 	 * And finally, if there were no matches within the domains

kernel/sched_stats.h +2 -2

index 48ddf43..331e01b 100644
@@ -37,7 +37,7 @@ static int show_schedstat(struct seq_file *seq, void *v)
 
 #ifdef CONFIG_SMP
 		/* domain-specific stats */
-		preempt_disable();
+		rcu_read_lock();
 		for_each_domain(cpu, sd) {
 			enum cpu_idle_type itype;

@@ -64,7 +64,7 @@ static int show_schedstat(struct seq_file *seq, void *v)
 			    sd->ttwu_wake_remote, sd->ttwu_move_affine,
 			    sd->ttwu_move_balance);
 		}
-		preempt_enable();
+		rcu_read_unlock();
 #endif
 	}
 	kfree(mask_str);

Signature · 1 line

1.7.1

Peter Zijlstra

2011-04-26 09:27 UTC #3 lore raw

Quoted 8 lines · Xiaotian feng

From: Xiaotian Feng <[email protected]>
sched_domain iterations needs to be protected by rcu_read_lock() now, this patch adds another two places which needs the rcu lock, which is spotted by following suspicious rcu_dereference_check() usage warnings.
kernel/sched_rt.c:1244 invoked rcu_dereference_check() without protection! kernel/sched_stats.h:41 invoked rcu_dereference_check() without protection!

Much better, one worry:

Quoted 19 lines

Signed-off-by Xiaotian Feng <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Peter Zijlstra <[email protected]>
---
kernel/sched_stats.h +1 -1
index 48ddf43..331e01b 100644
@@ -37,7 +37,7 @@ static int show_schedstat(struct seq_file *seq, void *v)
 
 #ifdef CONFIG_SMP
 		/* domain-specific stats */
-		preempt_disable();
+		rcu_read_lock();
 		for_each_domain(cpu, sd) {
 			enum cpu_idle_type itype;
 

kernel/sched_stats.h static int show_schedstat(struct seq_file *seq, void *v) · line 70

Quoted 8 lines

kernel/sched_stats.h +1 -1

@@ -64,7 +64,7 @@ static int show_schedstat(struct seq_file *seq, void *v)
 			    sd->ttwu_wake_remote, sd->ttwu_move_affine,
 			    sd->ttwu_move_balance);
 		}
-		preempt_enable();
+		rcu_read_unlock();
 #endif
 	}
 	kfree(mask_str);

Did you indeed validate that the preempt_disable() wasn't needed for anything else? Your changelog doesn't mention and I didn't check, just noticed the possibility on the first posting.

Xiaotian Feng

author

2011-04-26 10:40 UTC #4 lore raw

Quoted 30 lines · Peter Zijlstra

Quoted 8 lines · Xiaotian feng
From: Xiaotian Feng<[email protected]>
sched_domain iterations needs to be protected by rcu_read_lock() now, this patch adds another two places which needs the rcu lock, which is spotted by following suspicious rcu_dereference_check() usage warnings.
kernel/sched_rt.c:1244 invoked rcu_dereference_check() without protection! kernel/sched_stats.h:41 invoked rcu_dereference_check() without protection!
Much better, one worry:
Quoted 18 lines
Signed-off-by Xiaotian Feng<[email protected]>
Cc Ingo Molnar<[email protected]>
Cc Peter Zijlstra<[email protected]>
---
kernel/sched_stats.h +1 -1
index 48ddf43..331e01b 100644
@@ -37,7 +37,7 @@ static int show_schedstat(struct seq_file *seq, void *v)
 
  #ifdef CONFIG_SMP
  		/* domain-specific stats */
-		preempt_disable();
+		rcu_read_lock();
  		for_each_domain(cpu, sd) {
  			enum cpu_idle_type itype;
 

Quoted 14 lines

kernel/sched_stats.h static int show_schedstat(struct seq_file *seq, void *v) · line 70
Quoted 9 lines
kernel/sched_stats.h +1 -1
@@ -64,7 +64,7 @@ static int show_schedstat(struct seq_file *seq, void *v)
  			    sd->ttwu_wake_remote, sd->ttwu_move_affine,
  			    sd->ttwu_move_balance);
  		}
-		preempt_enable();
+		rcu_read_unlock();
  #endif
  	}
  	kfree(mask_str);
Did you indeed validate that the preempt_disable() wasn't needed for anything else? Your changelog doesn't mention and I didn't check, just noticed the possibility on the first posting.

Sorry, I just checked them, preempt_disable/enable was introduced by commit 674311d, the rcu_read_lock_sched is not existed at that time.

btw, as for_each_domain is protected by rcu_read_lock() and preempt_disable is not suffice any more, could we also update comments in for_each_domain?

/*
* The domain tree (rq->sd) is protected by RCU's quiescent state
transition.
* See detach_destroy_domains: synchronize_sched for details. *
* The domain tree of any CPU may only be accessed from within
* preempt-disabled sections.
*/

tip-bot for Xiaotian Feng

author

2011-05-28 16:34 UTC #5 lore raw

[tip:sched/urgent] sched: More sched_domain iterations fixes

In reply to Xiaotian feng

Commit-ID:  cd4ae6adf8b1c21d88e83ed56afeeef97b28f356
Gitweb:     http://git.kernel.org/tip/cd4ae6adf8b1c21d88e83ed56afeeef97b28f356
Author:     Xiaotian Feng <[email protected]>
AuthorDate: Fri, 22 Apr 2011 18:53:54 +0800
Committer:  Ingo Molnar <[email protected]>
CommitDate: Sat, 28 May 2011 17:02:54 +0200

sched: More sched_domain iterations fixes

kernel/sched_rt.c:1244 invoked rcu_dereference_check() without protection! kernel/sched_stats.h:41 invoked rcu_dereference_check() without protection!

Signed-off-by Xiaotian Feng <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/sched_rt.c    |   10 ++++++++--
 kernel/sched_stats.h |    4 ++--
 2 files changed, 10 insertions(+), 4 deletions(-)

kernel/sched_rt.c +8 -2

index 64b2a37..88725c9 100644
@@ -1263,6 +1263,7 @@ static int find_lowest_rq(struct task_struct *task)
 	if (!cpumask_test_cpu(this_cpu, lowest_mask))
 		this_cpu = -1; /* Skip this_cpu opt if not among lowest */
 
+	rcu_read_lock();
 	for_each_domain(cpu, sd) {
 		if (sd->flags & SD_WAKE_AFFINE) {
 			int best_cpu;

@@ -1272,15 +1273,20 @@ static int find_lowest_rq(struct task_struct *task)
 			 * remote processor.
 			 */
 			if (this_cpu != -1 &&
-			    cpumask_test_cpu(this_cpu, sched_domain_span(sd)))
+			    cpumask_test_cpu(this_cpu, sched_domain_span(sd))) {
+				rcu_read_unlock();
 				return this_cpu;
+			}
 
 			best_cpu = cpumask_first_and(lowest_mask,
 						     sched_domain_span(sd));
-			if (best_cpu < nr_cpu_ids)
+			if (best_cpu < nr_cpu_ids) {
+				rcu_read_unlock();
 				return best_cpu;
+			}
 		}
 	}
+	rcu_read_unlock();
 
 	/*
 	 * And finally, if there were no matches within the domains

kernel/sched_stats.h +2 -2

index 48ddf43..331e01b 100644
@@ -37,7 +37,7 @@ static int show_schedstat(struct seq_file *seq, void *v)
 
 #ifdef CONFIG_SMP
 		/* domain-specific stats */
-		preempt_disable();
+		rcu_read_lock();
 		for_each_domain(cpu, sd) {
 			enum cpu_idle_type itype;

@@ -64,7 +64,7 @@ static int show_schedstat(struct seq_file *seq, void *v)
 			    sd->ttwu_wake_remote, sd->ttwu_move_affine,
 			    sd->ttwu_move_balance);
 		}
-		preempt_enable();
+		rcu_read_unlock();
 #endif
 	}
 	kfree(mask_str);