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

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

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.

I agree that with the current patch-set we have a SHIFT/SCALE problem
once SCHED_LOAD_RESOLUTION is set to != 0.

> 
>>
>> I always thought that scale_load_down() takes care of that.
> 
> AFAIU, scale_load_down is a way to increase the resolution  of the
> load not to move from load to capacity

IMHO, increasing the resolution of the load is done by re-enabling this
define SCHED_LOAD_RESOLUTION  10 thing (or by setting
SCHED_LOAD_RESOLUTION to something else than 0).

I tried to figure out why we have this issue when comparing UTIL w/
CAPACITY and not LOAD w/ CAPACITY:

Both are initialized like that:

 sa->load_avg = scale_load_down(se->load.weight);
 sa->load_sum = sa->load_avg * LOAD_AVG_MAX;
 sa->util_avg = scale_load_down(SCHED_LOAD_SCALE);
 sa->util_sum = LOAD_AVG_MAX;

and we use 'se->on_rq * scale_load_down(se->load.weight)' as 'unsigned
long weight' argument to call __update_load_avg() making sure the
scaling differences between LOAD and CAPACITY are respected while
updating sa->load_sum (and sa->load_avg).

OTAH, we don't apply a scale_load_down for sa->util_[sum/avg] only a '<<
SCHED_LOAD_SHIFT) / LOAD_AVG_MAX' on sa->util_avg.
So changing '<< SCHED_LOAD_SHIFT' to '*
scale_load_down(SCHED_LOAD_SCALE)' would be the logical thing to do.

I agree that '<< SCHED_CAPACITY_SHIFT' would have the same effect but
why using a CAPACITY related thing on the LOAD/UTIL side? The only
reason would be the unit problem which I don't understand.

> 
>>
>> 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 ?
> 
> 
> No, but
> sa->util_avg = (sa->util_sum << SCHED_CAPACITY_SHIFT) / LOAD_AVG_MAX;
> will fix the unit issue.
> I agree that i don't change the result because both SCHED_LOAD_SHIFT
> and SCHED_CAPACITY_SHIFT are set to 10 but as mentioned above, they
> are set separately so it can make the difference if someone change one
> SHIFT value.

SCHED_LOAD_SHIFT and SCHED_CAPACITY_SHIFT can be set separately but the
way to change SCHED_LOAD_SHIFT is by re-enabling the define
SCHED_LOAD_RESOLUTION 10 in kernel/sched/sched.h. I guess nobody wants
to change SCHED_CAPACITY_[SHIFT/SCALE].

Cheers,

-- Dietmar

[...]