Re: linux-next-20110923: warning kernel/rcutree.c:1833

[Frederic Weisbecker <fweisbec@gmail.com>]

On Fri, Sep 30, 2011 at 12:24:38PM -0700, Paul E. McKenney wrote:
> And here is a first cut, probably totally broken, but a start.
> 
> With this change, I am wondering about tick_nohz_stop_sched_tick()'s
> invocation of rcu_idle_enter() -- this now needs to be called regardless
> of whether or not tick_nohz_stop_sched_tick() actually stops the tick.
> Except that if tick_nohz_stop_sched_tick() is invoked with inidle==0,
> it looks like we should -not- call rcu_idle_enter().

Because of the new check in rcu_check_callbacks()? Yeah.

If you think it's fine to call rcu_enter_nohz() unconditionally
everytime we enter the idle loop then yeah. I just don't know
the overhead it adds, as it adds an unconditional tiny piece of
code before we can finally save the power.

Either entering idle involves extended quiescent state as in this
patch, or you separate both and then rcu_enter_nohz() is only
called when the tick is stopped.

If you choose to merge both, you indeed need to call rcu_idle_enter()
and rcu_idle_exit() whether the tick is stopped or not.

> I eventually just left the rcu_idle_enter() calls in their current
> places due to paranoia about messing up and ending up with unbalanced
> rcu_idle_enter() and rcu_idle_exit() calls.  Any thoughts on how to
> make this work better?

Yeah something like this (untested):

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index d5097c4..ad3ecad 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -273,9 +273,12 @@ void tick_nohz_stop_sched_tick(int inidle)
 	 * updated. Thus, it must not be called in the event we are called from
 	 * irq_exit() with the prior state different than idle.
 	 */
-	if (!inidle && !ts->inidle)
+	if (inidle)
+		rcu_idle_enter();
+	else if (!ts->inidle)
 		goto end;
 
+
 	/*
 	 * Set ts->inidle unconditionally. Even if the system did not
 	 * switch to NOHZ mode the cpu frequency governers rely on the
@@ -409,7 +412,6 @@ void tick_nohz_stop_sched_tick(int inidle)
 			ts->idle_tick = hrtimer_get_expires(&ts->sched_timer);
 			ts->tick_stopped = 1;
 			ts->idle_jiffies = last_jiffies;
-			rcu_enter_nohz();
 		}
 
 		ts->idle_sleeps++;
@@ -505,6 +507,9 @@ void tick_nohz_restart_sched_tick(void)
 	ktime_t now;
 
 	local_irq_disable();
+
+	rcu_idle_exit();
+
 	if (ts->idle_active || (ts->inidle && ts->tick_stopped))
 		now = ktime_get();
 
@@ -519,8 +524,6 @@ void tick_nohz_restart_sched_tick(void)
 
 	ts->inidle = 0;
 
-	rcu_exit_nohz();
-
 	/* Update jiffies first */
 	select_nohz_load_balancer(0);
 	tick_do_update_jiffies64(now);




More things about your patch below:

> --- a/kernel/rcutiny.c
> +++ b/kernel/rcutiny.c
> @@ -54,31 +54,47 @@ static void __call_rcu(struct rcu_head *head,
>  
>  #include "rcutiny_plugin.h"
>  
> -#ifdef CONFIG_NO_HZ
> -
>  static long rcu_dynticks_nesting = 1;
>  
>  /*
> - * Enter dynticks-idle mode, which is an extended quiescent state
> - * if we have fully entered that mode (i.e., if the new value of
> - * dynticks_nesting is zero).
> + * Enter idle, which is an extended quiescent state if we have fully
> + * entered that mode (i.e., if the new value of dynticks_nesting is zero).
>   */
> -void rcu_enter_nohz(void)
> +void rcu_idle_enter(void)
>  {
>  	if (--rcu_dynticks_nesting == 0)
>  		rcu_sched_qs(0); /* implies rcu_bh_qsctr_inc(0) */
>  }
>  
>  /*
> - * Exit dynticks-idle mode, so that we are no longer in an extended
> - * quiescent state.
> + * Exit idle, so that we are no longer in an extended quiescent state.
>   */
> -void rcu_exit_nohz(void)
> +void rcu_idle_exit(void)
>  {
>  	rcu_dynticks_nesting++;
>  }
>  
> -#endif /* #ifdef CONFIG_NO_HZ */
> +#ifdef CONFIG_PROVE_RCU
> +
> +/*
> + * Test whether the current CPU is idle.
> + */

Is idle from an RCU point of view yeah.

> +int rcu_is_cpu_idle(void)
> +{
> +	return !rcu_dynticks_nesting;
> +}
> +
> +#endif /* #ifdef CONFIG_PROVE_RCU */
> +
> +/*
> + * Test whether the current CPU was interrupted from idle.  Nested
> + * interrupts don't count, we must be running at the first interrupt
> + * level.
> + */
> +int rcu_is_cpu_rrupt_from_idle(void)
> +{
> +	return rcu_dynticks_nesting <= 0;
> +}
>  
>  /*
>   * Helper function for rcu_sched_qs() and rcu_bh_qs().
> @@ -131,10 +147,7 @@ void rcu_bh_qs(int cpu)
>   */
>  void rcu_check_callbacks(int cpu, int user)
>  {
> -	if (user ||
> -	    (idle_cpu(cpu) &&
> -	     !in_softirq() &&
> -	     hardirq_count() <= (1 << HARDIRQ_SHIFT)))
> +	if (user || rcu_is_cpu_rrupt_from_idle())
>  		rcu_sched_qs(cpu);

It wasn't obvious to me in the first shot. This might need a comment
that tells rcu_check_callbacks() is called from an interrupt
and thus need to handle that first level in the check.

Other than that, looks good overall.