Re: [Patch v2 3/6] sched/fair: Implement prefer sibling imbalance calculation between asymmetric groups

[Tim Chen <tim.c.chen@linux.intel.com>]

On Mon, 2023-06-12 at 14:05 +0200, Peter Zijlstra wrote:
> On Thu, Jun 08, 2023 at 03:32:29PM -0700, Tim Chen wrote:
> 
> > 
> > +	if (env->sd->flags & SD_ASYM_PACKING) {
> > +		int limit;
> > +
> > +		if (!busiest->sum_nr_running)
> > +			goto out;
> 
> This seems out-of-place, shouldn't we have terminate sooner if busiest
> is empty?

Yes.  Should move this check to the beginning.

> 
> > +
> > +		if (sched_asym_prefer(env->dst_cpu, sds->busiest->asym_prefer_cpu)) {
> > +			/* Don't leave preferred core idle */
> > +			if (imbalance == 0 && local->sum_nr_running < ncores_local)
> > +				imbalance = 1;
> > +			goto out;
> > +		}
> > +
> > +		/* Limit tasks moved from preferred group, don't leave cores idle */
> > +		limit = busiest->sum_nr_running;
> > +		lsub_positive(&limit, ncores_busiest);
> > +		if (imbalance > limit)
> > +			imbalance = limit;
> 
> How does this affect the server parts that have larger than single core
> turbo domains?

Are you thinking about the case where the local group is completely empty
so there's turbo headroom and we should move at least one task, even though
CPU in busiest group has higher priority?

In other words, are you suggesting we should add

		if (imbalance == 0 && busiest->sum_nr_running > 0 &&
			local->sum_nr_running == 0)
			imbalance = 1;
		

> 
> > +
> > +		goto out;
> > +	}
> > +
> > +	/* Take advantage of resource in an empty sched group */
> > +	if (imbalance == 0 && local->sum_nr_running == 0 &&
> > +	    busiest->sum_nr_running > 1)
> > +		imbalance = 1;
> > +out:
> > +	return imbalance << 1;
> > +}
> 
> 
> But basically you have:
> 
>         LcBn - BcLn
>   imb = -----------
>            LcBc
> 
> Which makes sense, except you then return:
> 
>   imb * 2
> 
> which then made me wonder about rounding.
> 
> Do we want to to add (LcBc -1) or (LcBc/2) to resp. ceil() or round()
> the thing before division? Because currently it uses floor().
> 
> If you evaludate it like:
> 
> 
>         2 * (LcBn - BcLn)
>   imb = -----------------
>               LcBc
> 
> The result is different from what you have now.

If I do the rounding after multiplying imb by two (call it imb_new),
the difference with imb I am returning now (call it imb_old)
will be at most 1.  Note that imb_old returned is always a multiple of 2.

I will be using imb in calculate_imbalance() and divide it
by 2 there to get the number tasks to move from busiest group.
So when there is a difference of 1 between imb_old and imb_new,
the difference will be trimmed off after the division of 2.

We will get the same number of tasks to move with either
imb_old or imb_new in calculate_imbalance() so the two
computations will arrive at the same result eventually.

> 
> What actual behaviour is desired in these low imbalance cases? and can
> you write a comment as to why we do as we do etc..?

I do not keep imb as 

           2 * (LcBn - BcLn)
   imb = -----------------
               LcBc

as it is easier to leave out the factor of 2
in the middle of sibling_imblalance() computation
so I can directly interpret imb as the number
of tasks to move, and add the factor of two
when I actually need to return the imbalance.

Would you like me to add this reasoning in the comments?

Thanks.

Tim