Messages in this thread | | | Date | Tue, 26 Apr 2011 18:40:10 +0800 | From | Xiaotian Feng <> | Subject | Re: [PATCH -tip v2] sched: more sched_domain iterations fix |
| |
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. */
| |