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

[Vincent Guittot]

On 8 September 2015 at 14:50, Dietmar Eggemann <dietmar.eggemann@xxxxxxx> wrote:
> On 08/09/15 08:22, Vincent Guittot wrote:
>> On 7 September 2015 at 20:54, Dietmar Eggemann <dietmar.eggemann@xxxxxxx> wrote:
>>> On 07/09/15 17:21, Vincent Guittot wrote:
>>>> On 7 September 2015 at 17:37, Dietmar Eggemann <dietmar.eggemann@xxxxxxx> 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

Regards,
Vincent


>
> 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
>
> [...]
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/