Re: [PATCH 4/5] sched: Mark the balance type for use in need_active_balance()

[Peter Zijlstra <peterz@infradead.org>]

On Fri, 2010-04-09 at 16:21 +1000, Michael Neuling wrote:
> need_active_balance() gates the asymmetric packing based due to power
> save logic, but for packing we don't care.

This explanation lacks a how/why.

So the problem is that need_active_balance() ends up returning false and
prevents the active balance from pulling a task to a lower available SMT
sibling?

> This marks the type of balanace we are attempting to do perform from
> f_b_g() and stops need_active_balance() power save logic gating a
> balance in the asymmetric packing case.

At the very least this wants more comments in the code. I'm not really
charmed by having to add yet another variable to pass around that mess,
but I can't seem to come up with something cleaner either.

> Signed-off-by: Michael Neuling <mikey@neuling.org>
>=20
> ---
>=20
>  kernel/sched_fair.c |   24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
>=20
> Index: linux-2.6-ozlabs/kernel/sched_fair.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- linux-2.6-ozlabs.orig/kernel/sched_fair.c
> +++ linux-2.6-ozlabs/kernel/sched_fair.c
> @@ -91,6 +91,13 @@ const_debug unsigned int sysctl_sched_mi
> =20
>  static const struct sched_class fair_sched_class;
> =20
> +enum balance_type {
> +	BALANCE_NONE =3D 0,
> +	BALANCE_LOAD,
> +	BALANCE_POWER,
> +	BALANCE_PACKING
> +};
> +
>  /**************************************************************
>   * CFS operations on generic schedulable entities:
>   */
> @@ -2783,7 +2790,8 @@ static inline void calculate_imbalance(s
>  static struct sched_group *
>  find_busiest_group(struct sched_domain *sd, int this_cpu,
>  		   unsigned long *imbalance, enum cpu_idle_type idle,
> -		   int *sd_idle, const struct cpumask *cpus, int *balance)
> +		   int *sd_idle, const struct cpumask *cpus, int *balance,
> +		   enum balance_type *bt)
>  {
>  	struct sd_lb_stats sds;
> =20
> @@ -2808,6 +2816,7 @@ find_busiest_group(struct sched_domain *
>  	if (!(*balance))
>  		goto ret;
> =20
> +	*bt =3D BALANCE_PACKING;
>  	if ((idle =3D=3D CPU_IDLE || idle =3D=3D CPU_NEWLY_IDLE) &&
>  	    check_asym_packing(sd, &sds, this_cpu, imbalance))
>  		return sds.busiest;
> @@ -2828,6 +2837,7 @@ find_busiest_group(struct sched_domain *
> =20
>  	/* Looks like there is an imbalance. Compute it */
>  	calculate_imbalance(&sds, this_cpu, imbalance);
> +	*bt =3D BALANCE_LOAD;
>  	return sds.busiest;
> =20
>  out_balanced:
> @@ -2835,10 +2845,12 @@ out_balanced:
>  	 * There is no obvious imbalance. But check if we can do some balancing
>  	 * to save power.
>  	 */
> +	*bt =3D BALANCE_POWER;
>  	if (check_power_save_busiest_group(&sds, this_cpu, imbalance))
>  		return sds.busiest;
>  ret:
>  	*imbalance =3D 0;
> +	*bt =3D BALANCE_NONE;
>  	return NULL;
>  }
> =20
> @@ -2899,9 +2911,10 @@ find_busiest_queue(struct sched_group *g
>  /* Working cpumask for load_balance and load_balance_newidle. */
>  static DEFINE_PER_CPU(cpumask_var_t, load_balance_tmpmask);
> =20
> -static int need_active_balance(struct sched_domain *sd, int sd_idle, int=
 idle)
> +static int need_active_balance(struct sched_domain *sd, int sd_idle, int=
 idle,
> +			       enum balance_type *bt)
>  {
> -	if (idle =3D=3D CPU_NEWLY_IDLE) {
> +	if (idle =3D=3D CPU_NEWLY_IDLE && *bt !=3D BALANCE_PACKING) {
>  		/*
>  		 * The only task running in a non-idle cpu can be moved to this
>  		 * cpu in an attempt to completely freeup the other CPU
> @@ -2946,6 +2959,7 @@ static int load_balance(int this_cpu, st
>  	struct rq *busiest;
>  	unsigned long flags;
>  	struct cpumask *cpus =3D __get_cpu_var(load_balance_tmpmask);
> +	enum balance_type bt;
> =20
>  	cpumask_copy(cpus, cpu_active_mask);
> =20
> @@ -2964,7 +2978,7 @@ static int load_balance(int this_cpu, st
>  redo:
>  	update_shares(sd);
>  	group =3D find_busiest_group(sd, this_cpu, &imbalance, idle, &sd_idle,
> -				   cpus, balance);
> +				   cpus, balance, &bt);
> =20
>  	if (*balance =3D=3D 0)
>  		goto out_balanced;
> @@ -3018,7 +3032,7 @@ redo:
>  		schedstat_inc(sd, lb_failed[idle]);
>  		sd->nr_balance_failed++;
> =20
> -		if (need_active_balance(sd, sd_idle, idle)) {
> +		if (need_active_balance(sd, sd_idle, idle, &bt)) {
>  			raw_spin_lock_irqsave(&busiest->lock, flags);
> =20
>  			/* don't kick the migration_thread, if the curr