Re: [PATCH 5/6] sched/fair: Get rid of scaling utilization by capacity_orig

[Dietmar Eggemann <dietmar.eggemann@arm.com>]

On 07/09/15 17:21, Vincent Guittot wrote:
> On 7 September 2015 at 17:37, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
>> On 04/09/15 00:51, Steve Muckle wrote:
>>> Hi Morten, Dietmar,
>>>
>>> On 08/14/2015 09:23 AM, Morten Rasmussen wrote:
>>> ...
>>>> + * cfs_rq.avg.util_avg is the sum of running time of runnable tasks plus the
>>>> + * recent utilization of currently non-runnable tasks on a CPU. It represents
>>>> + * the amount of utilization of a CPU in the range [0..capacity_orig] where
>>>
>>> I see util_sum is scaled by SCHED_LOAD_SHIFT at the end of
>>> __update_load_avg(). If there is now an assumption that util_avg may be
>>> used directly as a capacity value, should it be changed to
>>> SCHED_CAPACITY_SHIFT? These are equal right now, not sure if they will
>>> always be or if they can be combined.
>>
>> You're referring to the code line
>>
>> 2647   sa->util_avg = (sa->util_sum << SCHED_LOAD_SHIFT) / LOAD_AVG_MAX;
>>
>> in __update_load_avg()?
>>
>> Here we actually scale by 'SCHED_LOAD_SCALE/LOAD_AVG_MAX' so both values are
>> load related.
> 
> I agree with Steve that there is an issue from a unit point of view
> 
> sa->util_sum and LOAD_AVG_MAX have the same unit so sa->util_avg is a
> load because of << SCHED_LOAD_SHIFT)
> 
> Before this patch , the translation from load to capacity unit was
> done in get_cpu_usage with "* capacity) >> SCHED_LOAD_SHIFT"
> 
> So you still have to change the unit from load to capacity with a "/
> SCHED_LOAD_SCALE * SCHED_CAPACITY_SCALE" somewhere.
> 
> sa->util_avg = ((sa->util_sum << SCHED_LOAD_SHIFT) /SCHED_LOAD_SCALE *
> SCHED_CAPACITY_SCALE / LOAD_AVG_MAX = (sa->util_sum <<
> SCHED_CAPACITY_SHIFT) / LOAD_AVG_MAX;

I see the point but IMHO this will only be necessary if the SCHED_LOAD_RESOLUTION
stuff gets re-enabled again.

It's not really about utilization or capacity units but rather about using the same
SCALE/SHIFT values for both sides, right?

I always thought that scale_load_down() takes care of that.

So shouldn't:

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 3445d2fb38f4..b80f799aface 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2644,7 +2644,7 @@ __update_load_avg(u64 now, int cpu, struct sched_avg *sa,
                        cfs_rq->runnable_load_avg =
                                div_u64(cfs_rq->runnable_load_sum, LOAD_AVG_MAX);
                }
-               sa->util_avg = (sa->util_sum << SCHED_LOAD_SHIFT) / LOAD_AVG_MAX;
+               sa->util_avg = (sa->util_sum * scale_load_down(SCHED_LOAD_SCALE)) / LOAD_AVG_MAX;
        }
 
        return decayed;

fix that issue in case SCHED_LOAD_RESOLUTION != 0 ?

I would vote for removing this SCHED_LOAD_RESOLUTION thing completely so that we can
assume that load/util and capacity are always using 1024/10.

Cheers,

-- Dietmar

> 
> 
> Regards,
> Vincent
> 
> 
>>
>> LOAD (UTIL) and CAPACITY have the same SCALE and SHIFT values because
>> SCHED_LOAD_RESOLUTION is always defined to 0. scale_load() and
>> scale_load_down() are also NOPs so this area is probably
>> worth a separate clean-up.
>> Beyond that, I'm not sure if the current functionality is
>> broken if we use different SCALE and SHIFT values for LOAD and CAPACITY?
>>

[...]