* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.