Re: [PATCH 1/3] sched/fair: Fix asym packing to select correct cpu

[Peter Zijlstra]

On Wed, Mar 23, 2016 at 05:04:40PM +0530, Srikar Dronamraju wrote:
> If asymmetric packing is used when target cpu is busy,
> update_sd_pick_busiest(), can select a lightly loaded cpu.
> find_busiest_group() has checks to ensure asym packing is only used
> when target cpu is not busy.  However it may not be able to avoid a
> lightly loaded cpu selected by update_sd_pick_busiest from being
> selected as source cpu for eventual load balancing.

So my brain completely fails to parse. What? Why?

> Also when using asymmetric scheduling, always select higher cpu as
> source cpu for load balancing.
> 
> While doing this change, move the check to see if target cpu is busy
> into check_asym_packing().

> Signed-off-by: Srikar Dronamraju <srikar@xxxxxxxxxxxxxxxxxx>
> ---
>  kernel/sched/fair.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 56b7d4b..9abfb16 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6517,6 +6517,8 @@ static bool update_sd_pick_busiest(struct lb_env *env,
>  	if (!(env->sd->flags & SD_ASYM_PACKING))
>  		return true;
>  
> +	if (env->idle == CPU_NOT_IDLE)
> +		return true;

OK, so this matches check_asym_packing() and makes sense, we don't want
to pull work if we're not idle.

But please add a comment with the condition, its always hard to remember
the intent of these things later.

>  	/*
>  	 * ASYM_PACKING needs to move all the work to the lowest
>  	 * numbered CPUs in the group, therefore mark all groups
> @@ -6526,7 +6528,7 @@ static bool update_sd_pick_busiest(struct lb_env *env,
>  		if (!sds->busiest)
>  			return true;
>  
> -		if (group_first_cpu(sds->busiest) > group_first_cpu(sg))
> +		if (group_first_cpu(sds->busiest) < group_first_cpu(sg))
>  			return true;
>  	}

Right, so you want to start by moving the highest possible cpu's work
down. The end result is the same, but this way you can reach lower power
states quicker.

Again, please add a comment.

> @@ -6672,6 +6674,9 @@ static int check_asym_packing(struct lb_env *env, struct sd_lb_stats *sds)
>  	if (!(env->sd->flags & SD_ASYM_PACKING))
>  		return 0;
>  
> +	if (env->idle == CPU_NOT_IDLE)
> +		return 0;
> +
>  	if (!sds->busiest)
>  		return 0;
>  
> @@ -6864,8 +6869,7 @@ static struct sched_group *find_busiest_group(struct lb_env *env)
>  	busiest = &sds.busiest_stat;
>  
>  	/* ASYM feature bypasses nice load balance check */
> -	if ((env->idle == CPU_IDLE || env->idle == CPU_NEWLY_IDLE) &&
> -	    check_asym_packing(env, &sds))
> +	if (check_asym_packing(env, &sds))
>  		return sds.busiest;
>  
>  	/* There is no busy sibling group to pull tasks from */

OK, this is an effective NOP but results in cleaner code.