Re: [patch 1/2] sched: Change the nohz ilb logic from pull to push model

[Peter Zijlstra <peterz@infradead.org>]

On Thu, 2009-12-10 at 17:27 -0800, venkatesh.pallipadi@intel.com wrote:

> @@ -4507,12 +4507,45 @@ static void active_load_balance(struct rq *busiest_rq, int busiest_cpu)
>  }
>  
>  #ifdef CONFIG_NO_HZ
> +
> +/*
> + * idle load balancing details
> + * - One of the idle CPUs nominates itself as idle load_balancer, while
> + *   entering idle.
> + * - With previous logic, this idle load balancer CPU will not go into
> + *   tickless mode when it is idle and does the idle load balancing for
> + *   all the idle CPUs.
> + * - With new logic, this idle load balancer CPU will also go into
> + *   tickless mode when it is idle, just like all other idle CPUs
> + * - When one of the busy CPUs notice that there may be an idle rebalancing
> + *   needed, they will kick the idle load balancer, which then does idle
> + *   load balancing for all the idle CPUs.
> + * - As idle load balancing looks at the load of all the CPUs, not all busy
> + *   CPUs need to do this idle load balancer kick.
> + * - first_pick_cpu is the one of the busy CPUs which will kick
> + *   idle load balancer when it has more than one process active. This
> + *   eliminates the need for idle load balancing altogether when we have
> + *   only one running process in the system (common case).
> + * - If there are more than one busy CPU, idle load balancer may have
> + *   to run for active_load_balance to happen (i.e., two busy CPUs are
> + *   SMT or core siblings and can run better if they move to different
> + *   physical CPUs). So, second_pick_cpu is the second of the busy CPUs
> + *   which will kick idle load balancer as soon as it has any load.
> + * - With previous logic, idle load balancer used to run at every tick.
> + *   With new logic, idle load balancer tracks the rq->next_balance for all
> + *   the idle CPUs and does idle load balancing only when needed.
> + */

Right so like said before, this comments needs a rewrite.

>  static struct {
>  	atomic_t load_balancer;
> -	cpumask_var_t cpu_mask;
> -	cpumask_var_t ilb_grp_nohz_mask;
> +	atomic_t first_pick_cpu;
> +	atomic_t second_pick_cpu;
> +	cpumask_var_t idle_cpus_mask;
> +	cpumask_var_t tmp_nohz_mask;

I don't mind the rename, but tmp_nohz_mask is a really bad name.

> +	unsigned long next_balance;	/* in jiffy units */
>  } nohz ____cacheline_aligned = {
>  	.load_balancer = ATOMIC_INIT(-1),
> +	.first_pick_cpu = ATOMIC_INIT(-1),
> +	.second_pick_cpu = ATOMIC_INIT(-1),
>  };
>  
>  int get_nohz_load_balancer(void)

>  /*
> + * Kick a CPU to do the nohz balancing, if it is time for it. We pick the
> + * nohz_load_balancer CPU (if there is one) otherwise fallback to any idle
> + * CPU (if there is one).
> +*/
> +static void nohz_balancer_kick(int cpu)
> +{
> +	int ilb_cpu;
> +
> +	nohz.next_balance++;
> +
> +	ilb_cpu = get_nohz_load_balancer();
> +	if (ilb_cpu < 0) {
> +		ilb_cpu = cpumask_first(nohz.idle_cpus_mask);
> +		if (ilb_cpu >= nr_cpu_ids)
> +			return;
> +	}
> +
> +	if (!cpu_rq(ilb_cpu)->nohz_balance_kick) {
> +		cpu_rq(ilb_cpu)->nohz_balance_kick = 1;
> +		resched_cpu(ilb_cpu);
> +	}
> +	return;
> +}

So here you simply send an resched-ipi, which requires the below hack in
schedule()?


> @@ -4673,28 +4722,20 @@ int select_nohz_load_balancer(int stop_tick)
>  			if (atomic_cmpxchg(&nohz.load_balancer, cpu, -1) != cpu)
>  				BUG();
>  
> +			return;
>  		}
>  
> +		cpumask_set_cpu(cpu, nohz.idle_cpus_mask);
> +		atomic_cmpxchg(&nohz.first_pick_cpu, cpu, -1);
> +		atomic_cmpxchg(&nohz.second_pick_cpu, cpu, -1);

If you were to use nr_cpu_ids here instead of -1, you get more
consistent code in nohz_balancer_kick().


> +	ret = atomic_cmpxchg(&nohz.first_pick_cpu, -1, cpu);
> +	if (ret == -1 || ret == cpu) {
> +		atomic_cmpxchg(&nohz.second_pick_cpu, cpu, -1);
> +		if (rq->nr_running > 1)
> +			return 1;
> +	} else {
> +		ret = atomic_cmpxchg(&nohz.second_pick_cpu, -1, cpu);
> +		if (ret == -1 || ret == cpu) {
> +			if (rq->nr_running)
> +				return 1;
>  		}
>  	}

Looked very funny, and took a while to understand why you're doing that,
but yeah, I can't see a better way of doing it either.

The comments confused me more than helped me understand it.

> @@ -5446,8 +5490,19 @@ need_resched_nonpreemptible:
>  
>  	pre_schedule(rq, prev);
>  
> -	if (unlikely(!rq->nr_running))
> +	if (unlikely(!rq->nr_running)) {
> +#ifdef CONFIG_NO_HZ
> +		if (rq->nohz_balance_kick) {
> +			spin_unlock_irq(&rq->lock);
> +			nohz_idle_balance(cpu, rq);
> +			spin_lock_irq(&rq->lock);
> +		} else {
> +			idle_balance(cpu, rq);
> +		}
> +#else
>  		idle_balance(cpu, rq);
> +#endif
> +	}

And I think this is the wrong kind of trade-off, complicating the
schedule()/newidle path for nohz idle balancing.

nohz_balancer_kick() seems like the perfect place to use something like
send_remote_softirq().