* [PATCH -tip] sched: more sched_domain iterations fix
@ 2011-04-21 11:07 Xiaotian Feng
2011-04-21 11:21 ` Peter Zijlstra
0 siblings, 1 reply; 6+ messages in thread
From: Xiaotian Feng @ 2011-04-21 11:07 UTC (permalink / raw)
To: linux-kernel; +Cc: Xiaotian Feng, Ingo Molnar, Peter Zijlstra
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 <dfeng@redhat.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <peterz@infradead.org>
---
kernel/sched_rt.c | 2 ++
kernel/sched_stats.h | 2 ++
2 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
index 19ecb31..901ed2a 100644
--- a/kernel/sched_rt.c
+++ b/kernel/sched_rt.c
@@ -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;
diff --git a/kernel/sched_stats.h b/kernel/sched_stats.h
index 48ddf43..d25c8c1 100644
--- a/kernel/sched_stats.h
+++ b/kernel/sched_stats.h
@@ -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
}
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH -tip] sched: more sched_domain iterations fix
2011-04-21 11:07 [PATCH -tip] sched: more sched_domain iterations fix Xiaotian Feng
@ 2011-04-21 11:21 ` Peter Zijlstra
2011-04-22 10:53 ` [PATCH -tip v2] " Xiaotian feng
0 siblings, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2011-04-21 11:21 UTC (permalink / raw)
To: Xiaotian Feng; +Cc: linux-kernel, Ingo Molnar
On Thu, 2011-04-21 at 19:07 +0800, Xiaotian Feng wrote:
> 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.
> Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Peter Zijlstra <peterz@infradead.org>
> ---
> kernel/sched_rt.c | 2 ++
> kernel/sched_stats.h | 2 ++
> 2 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
> index 19ecb31..901ed2a 100644
> --- a/kernel/sched_rt.c
> +++ b/kernel/sched_rt.c
> @@ -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.
> if ((cpu == -1) || (cpu == rq->cpu))
> break;
> diff --git a/kernel/sched_stats.h b/kernel/sched_stats.h
> index 48ddf43..d25c8c1 100644
> --- a/kernel/sched_stats.h
> +++ b/kernel/sched_stats.h
> @@ -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();
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.
> #endif
> }
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH -tip v2] sched: more sched_domain iterations fix
2011-04-21 11:21 ` Peter Zijlstra
@ 2011-04-22 10:53 ` Xiaotian feng
2011-04-26 9:27 ` Peter Zijlstra
2011-05-28 16:34 ` [tip:sched/urgent] sched: More sched_domain iterations fixes tip-bot for Xiaotian Feng
0 siblings, 2 replies; 6+ messages in thread
From: Xiaotian feng @ 2011-04-22 10:53 UTC (permalink / raw)
To: linux-kernel; +Cc: Xiaotian Feng, Ingo Molnar, Peter Zijlstra
From: Xiaotian Feng <dfeng@redhat.com>
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 <dfeng@redhat.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <peterz@infradead.org>
---
kernel/sched_rt.c | 10 ++++++++--
kernel/sched_stats.h | 4 ++--
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
index 19ecb31..46a9533 100644
--- a/kernel/sched_rt.c
+++ b/kernel/sched_rt.c
@@ -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
diff --git a/kernel/sched_stats.h b/kernel/sched_stats.h
index 48ddf43..331e01b 100644
--- a/kernel/sched_stats.h
+++ b/kernel/sched_stats.h
@@ -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);
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH -tip v2] sched: more sched_domain iterations fix
2011-04-22 10:53 ` [PATCH -tip v2] " Xiaotian feng
@ 2011-04-26 9:27 ` Peter Zijlstra
2011-04-26 10:40 ` Xiaotian Feng
2011-05-28 16:34 ` [tip:sched/urgent] sched: More sched_domain iterations fixes tip-bot for Xiaotian Feng
1 sibling, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2011-04-26 9:27 UTC (permalink / raw)
To: Xiaotian feng; +Cc: linux-kernel, Ingo Molnar
On Fri, 2011-04-22 at 18:53 +0800, Xiaotian feng wrote:
> From: Xiaotian Feng <dfeng@redhat.com>
>
> 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:
> Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Peter Zijlstra <peterz@infradead.org>
> ---
> diff --git a/kernel/sched_stats.h b/kernel/sched_stats.h
> index 48ddf43..331e01b 100644
> --- a/kernel/sched_stats.h
> +++ b/kernel/sched_stats.h
> @@ -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);
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH -tip v2] sched: more sched_domain iterations fix
2011-04-26 9:27 ` Peter Zijlstra
@ 2011-04-26 10:40 ` Xiaotian Feng
0 siblings, 0 replies; 6+ messages in thread
From: Xiaotian Feng @ 2011-04-26 10:40 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: linux-kernel, Ingo Molnar
On 04/26/2011 05:27 PM, Peter Zijlstra wrote:
> On Fri, 2011-04-22 at 18:53 +0800, Xiaotian feng wrote:
>> From: Xiaotian Feng<dfeng@redhat.com>
>>
>> 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:
>
>> Signed-off-by: Xiaotian Feng<dfeng@redhat.com>
>> Cc: Ingo Molnar<mingo@elte.hu>
>> Cc: Peter Zijlstra<peterz@infradead.org>
>> ---
>
>> diff --git a/kernel/sched_stats.h b/kernel/sched_stats.h
>> index 48ddf43..331e01b 100644
>> --- a/kernel/sched_stats.h
>> +++ b/kernel/sched_stats.h
>> @@ -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);
>
> 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.
*/
^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip:sched/urgent] sched: More sched_domain iterations fixes
2011-04-22 10:53 ` [PATCH -tip v2] " Xiaotian feng
2011-04-26 9:27 ` Peter Zijlstra
@ 2011-05-28 16:34 ` tip-bot for Xiaotian Feng
1 sibling, 0 replies; 6+ messages in thread
From: tip-bot for Xiaotian Feng @ 2011-05-28 16:34 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, hpa, mingo, a.p.zijlstra, tglx, dfeng, mingo
Commit-ID: cd4ae6adf8b1c21d88e83ed56afeeef97b28f356
Gitweb: http://git.kernel.org/tip/cd4ae6adf8b1c21d88e83ed56afeeef97b28f356
Author: Xiaotian Feng <dfeng@redhat.com>
AuthorDate: Fri, 22 Apr 2011 18:53:54 +0800
Committer: Ingo Molnar <mingo@elte.hu>
CommitDate: Sat, 28 May 2011 17:02:54 +0200
sched: More sched_domain iterations fixes
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 <dfeng@redhat.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1303469634-11678-1-git-send-email-dfeng@redhat.com
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
kernel/sched_rt.c | 10 ++++++++--
kernel/sched_stats.h | 4 ++--
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
index 64b2a37..88725c9 100644
--- a/kernel/sched_rt.c
+++ b/kernel/sched_rt.c
@@ -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
diff --git a/kernel/sched_stats.h b/kernel/sched_stats.h
index 48ddf43..331e01b 100644
--- a/kernel/sched_stats.h
+++ b/kernel/sched_stats.h
@@ -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);
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-05-28 16:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-04-21 11:07 [PATCH -tip] sched: more sched_domain iterations fix Xiaotian Feng
2011-04-21 11:21 ` Peter Zijlstra
2011-04-22 10:53 ` [PATCH -tip v2] " Xiaotian feng
2011-04-26 9:27 ` Peter Zijlstra
2011-04-26 10:40 ` Xiaotian Feng
2011-05-28 16:34 ` [tip:sched/urgent] sched: More sched_domain iterations fixes tip-bot for Xiaotian Feng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).