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

[Vincent Guittot]

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.

>
> 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

>
> 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.

Regards,
Vincent

>
> 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?
>>>
>
> [...]
>
--
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/