Re: [patch 4/5] sched: RCU sched domains

[Ingo Molnar <mingo@elte.hu>]

* Nick Piggin <nickpiggin@yahoo.com.au> wrote:

> 4/5

> One of the problems with the multilevel balance-on-fork/exec is that 
> it needs to jump through hoops to satisfy sched-domain's locking 
> semantics (that is, you may traverse your own domain when not 
> preemptable, and you may traverse others' domains when holding their 
> runqueue lock).
> 
> balance-on-exec had to potentially migrate between more than one CPU 
> before finding a final CPU to migrate to, and balance-on-fork needed 
> to potentially take multiple runqueue locks.
> 
> So bite the bullet and make sched-domains go completely RCU. This 
> actually simplifies the code quite a bit.
> 
> Signed-off-by: Nick Piggin <nickpiggin@yahoo.com.au>

i like it conceptually, so:

Acked-by: Ingo Molnar <mingo@elte.hu>

from now on, all domain-tree readonly uses have to be rcu_read_lock()-ed 
(or otherwise have to be in a non-preemptible section). But there's a 
bug in show_shedstats() which does a for_each_domain() from within a 
preemptible section. (It was a bug with the current hotplug logic too i 
think.)

At a minimum i think we need the fix+comment below.

	Ingo

Signed-off-by: Ingo Molnar <mingo@elte.hu>

--- linux/kernel/sched.c.orig
+++ linux/kernel/sched.c
@@ -260,6 +260,10 @@ struct runqueue {
 
 static DEFINE_PER_CPU(struct runqueue, runqueues);
 
+/*
+ * The domain tree (rq->sd) is RCU locked. I.e. it may only be accessed
+ * from within an rcu_read_lock() [or otherwise preempt-disabled] sections.
+ */
 #define for_each_domain(cpu, domain) \
 	for (domain = cpu_rq(cpu)->sd; domain; domain = domain->parent)
 
@@ -338,6 +342,7 @@ static int show_schedstat(struct seq_fil
 
 #ifdef CONFIG_SMP
 		/* domain-specific stats */
+		rcu_read_lock();
 		for_each_domain(cpu, sd) {
 			enum idle_type itype;
 			char mask_str[NR_CPUS];
@@ -361,6 +366,7 @@ static int show_schedstat(struct seq_fil
 			    sd->sbe_pushed, sd->sbe_attempts,
 			    sd->ttwu_wake_remote, sd->ttwu_move_affine, sd->ttwu_move_balance);
 		}
+		rcu_read_unlock();
 #endif
 	}
 	return 0;