Re: [PATCH -tip v2] sched: more sched_domain iterations fix

[Xiaotian Feng]

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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Cc: Ingo Molnar<mingo@xxxxxxx>
> > Cc: Peter Zijlstra<peterz@xxxxxxxxxxxxx>
> > ---
>
> > 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.
 */

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/