Re: Very high CPU load when idle with 3.0-rc1

[Paul E. McKenney]

On Mon, May 30, 2011 at 01:34:51PM +0200, Peter Zijlstra wrote:
> On Mon, 2011-05-30 at 07:59 +0200, Damien Wyart wrote:
> > Hi,
> > 
> > Testing 3.0-rc1 on a core i7 (4 cores + HT), I get a load average of 9.0
> > when idle. No process is shown running or in "D state" in htop. The box
> > is behaving normal, no impression of lag or slowness.
> > 
> > Not sure what other info to include, I guess this should be quite easy
> > to reproduce.
> 
> 
> ---
> Subject: rcu: Cure load woes
> 
> Commit cc3ce5176d83 (rcu: Start RCU kthreads in TASK_INTERRUPTIBLE
> state) fudges a sleeping task' state, resulting in the scheduler seeing
> a TASK_UNINTERRUPTIBLE task going to sleep, but a TASK_INTERRUPTIBLE
> task waking up. The result is unbalanced load calculation.
> 
> The problem that patch tried to address is that the RCU threads could
> stay in UNINTERRUPTIBLE state for quite a while and triggering the hung
> task detector due to on-demand wake-ups.
> 
> Cure the problem differently by always giving the tasks at least one
> wake-up once the CPU is fully up and running, this will kick them out of
> the initial UNINTERRUPTIBLE state and into the regular INTERRUPTIBLE
> wait state.
> 
> The alternative would be teaching kthread_create() to start threads as
> INTERRUPTIBLE but that needs a tad more thought.
> 
> Reported-by: Damien Wyart <damien.wyart@xxxxxxx>
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@xxxxxxxxx>

Very cool!  I do have a few questions below, but am queuing and testing
this in the meantime.

> ---
>  kernel/rcutree.c        |   54 ++++++++++++++++++++++++++++++++++++++++-------
>  kernel/rcutree_plugin.h |   11 ++++++++-
>  2 files changed, 56 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> index 77a7671..89419ff 100644
> --- a/kernel/rcutree.c
> +++ b/kernel/rcutree.c
> @@ -1648,7 +1648,6 @@ static int __cpuinit rcu_spawn_one_cpu_kthread(int cpu)
>  	if (IS_ERR(t))
>  		return PTR_ERR(t);
>  	kthread_bind(t, cpu);
> -	set_task_state(t, TASK_INTERRUPTIBLE);
>  	per_cpu(rcu_cpu_kthread_cpu, cpu) = cpu;
>  	WARN_ON_ONCE(per_cpu(rcu_cpu_kthread_task, cpu) != NULL);
>  	per_cpu(rcu_cpu_kthread_task, cpu) = t;
> @@ -1756,7 +1755,6 @@ static int __cpuinit rcu_spawn_one_node_kthread(struct rcu_state *rsp,
>  		if (IS_ERR(t))
>  			return PTR_ERR(t);
>  		raw_spin_lock_irqsave(&rnp->lock, flags);
> -		set_task_state(t, TASK_INTERRUPTIBLE);
>  		rnp->node_kthread_task = t;
>  		raw_spin_unlock_irqrestore(&rnp->lock, flags);
>  		sp.sched_priority = 99;
> @@ -1765,6 +1763,8 @@ static int __cpuinit rcu_spawn_one_node_kthread(struct rcu_state *rsp,
>  	return rcu_spawn_one_boost_kthread(rsp, rnp, rnp_index);
>  }
>  
> +static void rcu_wake_one_boost_kthread(struct rcu_node *rnp);
> +
>  /*
>   * Spawn all kthreads -- called as soon as the scheduler is running.
>   */
> @@ -1772,18 +1772,30 @@ static int __init rcu_spawn_kthreads(void)
>  {
>  	int cpu;
>  	struct rcu_node *rnp;
> +	struct task_struct *t;
>  
>  	rcu_kthreads_spawnable = 1;
>  	for_each_possible_cpu(cpu) {
>  		per_cpu(rcu_cpu_has_work, cpu) = 0;
> -		if (cpu_online(cpu))
> +		if (cpu_online(cpu)) {
>  			(void)rcu_spawn_one_cpu_kthread(cpu);
> +			t = per_cpu(rcu_cpu_kthread_task, cpu);
> +			if (t)
> +				wake_up_process(t);
> +		}

Would it be OK to simplify the code a bit by doing this initial wakeup
in rcu_spawn_one_cpu_kthread() itself?  My thought would be to rearrange
rcu_spawn_one_cpu_kthread() as follows:

static int __cpuinit rcu_spawn_one_cpu_kthread(int cpu)
{
	struct sched_param sp;
	struct task_struct *t;

	if (!rcu_kthreads_spawnable ||
	    per_cpu(rcu_cpu_kthread_task, cpu) != NULL)
		return 0;
	t = kthread_create(rcu_cpu_kthread, (void *)(long)cpu, "rcuc%d", cpu);
	if (IS_ERR(t))
		return PTR_ERR(t);
	kthread_bind(t, cpu);
	set_task_state(t, TASK_INTERRUPTIBLE);
	per_cpu(rcu_cpu_kthread_cpu, cpu) = cpu;
	WARN_ON_ONCE(per_cpu(rcu_cpu_kthread_task, cpu) != NULL);
	sp.sched_priority = RCU_KTHREAD_PRIO;
	sched_setscheduler_nocheck(t, SCHED_FIFO, &sp);
	wake_up_process(t);
	per_cpu(rcu_cpu_kthread_task, cpu) = t;
	return 0;
}

>  	}
>  	rnp = rcu_get_root(rcu_state);
>  	(void)rcu_spawn_one_node_kthread(rcu_state, rnp);
> +	if (rnp->node_kthread_task)
> +		wake_up_process(rnp->node_kthread_task);

Ditto here -- can this wake_up_process() be pushed into
rcu_spawn_one_node_kthread()?

>  	if (NUM_RCU_NODES > 1) {
> -		rcu_for_each_leaf_node(rcu_state, rnp)
> +		rcu_for_each_leaf_node(rcu_state, rnp) {
>  			(void)rcu_spawn_one_node_kthread(rcu_state, rnp);
> +			t = rnp->node_kthread_task;
> +			if (t)
> +				wake_up_process(t);
> +			rcu_wake_one_boost_kthread(rnp);
> +		}

Analogous question here for rcu_wake_one_boost_kthread being eliminated
in favor of doing the wake_up_process() in rcu_spawn_one_node_kthread().

>  	}
>  	return 0;
>  }
> @@ -2188,14 +2200,14 @@ rcu_init_percpu_data(int cpu, struct rcu_state *rsp, int preemptible)
>  	raw_spin_unlock_irqrestore(&rsp->onofflock, flags);
>  }
>  
> -static void __cpuinit rcu_online_cpu(int cpu)
> +static void __cpuinit rcu_prepare_cpu(int cpu)
>  {
>  	rcu_init_percpu_data(cpu, &rcu_sched_state, 0);
>  	rcu_init_percpu_data(cpu, &rcu_bh_state, 0);
>  	rcu_preempt_init_percpu_data(cpu);
>  }
>  
> -static void __cpuinit rcu_online_kthreads(int cpu)
> +static void __cpuinit rcu_prepare_kthreads(int cpu)

Indeed, this naming is much better than mine.  ;-)

>  {
>  	struct rcu_data *rdp = per_cpu_ptr(rcu_state->rda, cpu);
>  	struct rcu_node *rnp = rdp->mynode;
> @@ -2209,6 +2221,31 @@ static void __cpuinit rcu_online_kthreads(int cpu)
>  }
>  
>  /*
> + * kthread_create() creates threads in TASK_UNINTERRUPTIBLE state,
> + * but the RCU threads are woken on demand, and if demand is low this
> + * could be a while triggering the hung task watchdog.
> + *
> + * In order to avoid this, poke all tasks once the CPU is fully
> + * up and running.
> + */
> +static void __cpuinit rcu_online_kthreads(int cpu)
> +{
> +	struct rcu_data *rdp = per_cpu_ptr(rcu_state->rda, cpu);
> +	struct rcu_node *rnp = rdp->mynode;
> +	struct task_struct *t;
> +
> +	t = per_cpu(rcu_cpu_kthread_task, cpu);
> +	if (t)
> +		wake_up_process(t);
> +
> +	t = rnp->node_kthread_task;
> +	if (t)
> +		wake_up_process(t);
> +
> +	rcu_wake_one_boost_kthread(rnp);

Interesting...  So we are really awakening them twice, once at creation
time to get them to sleep interruptibly, and a second time when the CPU
comes online.

What does this second set of wake_up_process() calls do?

> +}
> +
> +/*
>   * Handle CPU online/offline notification events.
>   */
>  static int __cpuinit rcu_cpu_notify(struct notifier_block *self,
> @@ -2221,10 +2258,11 @@ static int __cpuinit rcu_cpu_notify(struct notifier_block *self,
>  	switch (action) {
>  	case CPU_UP_PREPARE:
>  	case CPU_UP_PREPARE_FROZEN:
> -		rcu_online_cpu(cpu);
> -		rcu_online_kthreads(cpu);
> +		rcu_prepare_cpu(cpu);
> +		rcu_prepare_kthreads(cpu);
>  		break;
>  	case CPU_ONLINE:
> +		rcu_online_kthreads(cpu);
>  	case CPU_DOWN_FAILED:
>  		rcu_node_kthread_setaffinity(rnp, -1);
>  		rcu_cpu_kthread_setrt(cpu, 1);
> diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
> index a767b7d..2910de7 100644
> --- a/kernel/rcutree_plugin.h
> +++ b/kernel/rcutree_plugin.h
> @@ -1295,7 +1295,6 @@ static int __cpuinit rcu_spawn_one_boost_kthread(struct rcu_state *rsp,
>  	if (IS_ERR(t))
>  		return PTR_ERR(t);
>  	raw_spin_lock_irqsave(&rnp->lock, flags);
> -	set_task_state(t, TASK_INTERRUPTIBLE);
>  	rnp->boost_kthread_task = t;
>  	raw_spin_unlock_irqrestore(&rnp->lock, flags);
>  	sp.sched_priority = RCU_KTHREAD_PRIO;
> @@ -1303,6 +1302,12 @@ static int __cpuinit rcu_spawn_one_boost_kthread(struct rcu_state *rsp,
>  	return 0;
>  }
>  
> +static void __cpuinit rcu_wake_one_boost_kthread(struct rcu_node *rnp)
> +{
> +	if (rnp->boost_kthread_task)
> +		wake_up_process(rnp->boost_thread_task);
> +}
> +
>  #else /* #ifdef CONFIG_RCU_BOOST */
>  
>  static void rcu_initiate_boost(struct rcu_node *rnp, unsigned long flags)
> @@ -1326,6 +1331,10 @@ static int __cpuinit rcu_spawn_one_boost_kthread(struct rcu_state *rsp,
>  	return 0;
>  }
>  
> +static void __cpuinit rcu_wake_one_boost_kthread(struct rcu_node *rnp)
> +{
> +}
> +
>  #endif /* #else #ifdef CONFIG_RCU_BOOST */
>  
>  #ifndef CONFIG_SMP
> 
--
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/