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

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

On 08/09/15 15:01, Vincent Guittot wrote:
> On 8 September 2015 at 14:50, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
>> On 08/09/15 08:22, Vincent Guittot wrote:
>>> On 7 September 2015 at 20:54, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
>>>> 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?
>>>
>>> It's both a unit and a SCALE/SHIFT problem, SCHED_LOAD_SHIFT and
>>> SCHED_CAPACITY_SHIFT are defined separately so we must be sure to
>>> scale the value in the right range. In the case of cpu_usage which
>>> returns sa->util_avg , it's the capacity range not the load range.
>>
>> Still don't understand why it's a unit problem. IMHO LOAD/UTIL and
>> CAPACITY have no unit.
> 
> If you set 2 different values to SCHED_LOAD_SHIFT and
> SCHED_CAPACITY_SHIFT for test purpose, you will see that util_avg will
> not use the right range of value
> 
> If we don't take into account freq and cpu invariance in a 1st step
> 
> sa->util_sum is a load in the range [0..LOAD_AVG_MAX]. I say load
> because of the max value
> 
> the current implementation of util_avg is
> sa->util_avg = (sa->util_sum << SCHED_LOAD_SHIFT) / LOAD_AVG_MAX
> 
> so sa->util_avg is a load in the range [0..SCHED_LOAD_SCALE]
> 
> the current implementation of get_cpu_usage is
> return (sa->util_avg * capacity_orig_of(cpu)) >> SCHED_LOAD_SHIFT
> 
> so the usage has the same unit and range as capacity of the cpu and
> can be compared with another capacity value
> 
> Your patchset returns directly sa->util_avg which is a load to compare
> it with capacity value
> 
> So you have to convert sa->util_avg from load to capacity so if you have
> sa->util_avg = (sa->util_sum << SCHED_CAPACITY_SHIFT) / LOAD_AVG_MAX
> 
> sa->util_avg is now a capacity with the same range as you cpu thanks
> to the cpu invariance factor that the patch 3 has added.
> 
> the << SCHED_CAPACITY_SHIFT above can be optimized with the >>
> SCHED_CAPACITY_SHIFT included in
> sa->util_sum += scale(contrib, scale_cpu);
> as mentioned by Peter
> 
> At now, SCHED_CAPACITY_SHIFT is set to 10 as well as SCHED_LOAD_SHIFT
> so using one instead of the other doesn't change the result but if
> it's no more the case, we need to take care of the range/unit that we
> use

No arguing here, I just called this a SHIFT/SCALE problem.

[...]