[PATCH] arm: topology: fix capacity calculation on SMP SoCs

[jorge.ramirez-ortiz@linaro.org (Jorge Ramirez-Ortiz)]

On 05/14/2015 07:31 AM, Vincent Guittot wrote:
> Hi Jorge,
>
> On 13 May 2015 at 22:38, Jorge Ramirez-Ortiz
> <jorge.ramirez-ortiz@linaro.org> wrote:
>> This commit sets the capacity of the average CPU in SMP systems to
>> SCHED_CAPACITY_SCALE.
>>
>> Ignoring the condition "min_capacity==max_capacity" causes the function
>> update_cpu_capacity( .. ) to generate out of range values [1]. This is
>> because the default value of middle_capacity is used in the final
>> calculation instead of a valid scaling factor.
>>
>> Incidentally, when out of range values are generated and if
>> SCHED_FEAT(ARCH_POWER, true), the load balancing algorithm makes the
>> wrong decisions typically overallocating work on one of the cores
>> while leaving the others unused.
> Have you got an example ?

This was tested on lsk 3.10 which was slightly different (see my comments below).
I can get you the test source code.


>
>> [1] val > SCHED_CAPACITY_SCALE
>>
>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>> ---
>>  arch/arm/kernel/topology.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
>> index 08b7847..509bc9b 100644
>> --- a/arch/arm/kernel/topology.c
>> +++ b/arch/arm/kernel/topology.c
>> @@ -137,14 +137,14 @@ static void __init parse_dt_topology(void)
>>                 cpu_capacity(cpu) = capacity;
>>         }
>>
>> -       /* If min and max capacities are equals, we bypass the update of the
>> -        * cpu_scale because all CPUs have the same capacity. Otherwise, we
>> -        * compute a middle_capacity factor that will ensure that the capacity
>> +       /* Compute a middle_capacity factor that will ensure that the capacity
>>          * of an 'average' CPU of the system will be as close as possible to
>>          * SCHED_CAPACITY_SCALE, which is the default value, but with the
>>          * constraint explained near table_efficiency[].
>>          */
>> -       if (4*max_capacity < (3*(max_capacity + min_capacity)))
>> +       if (min_capacity == max_capacity)
>> +               middle_capacity = min_capacity >> SCHED_CAPACITY_SHIFT;
>> +       else if (4*max_capacity < (3*(max_capacity + min_capacity)))
> if min_capacity == max_capacity then the condition 4*max_capacity <
> (3*(max_capacity + min_capacity)) is true and
> middle_capacity = (min_capacity + max_capacity) >>
> (SCHED_CAPACITY_SHIFT+1) = 2*min_capacity >> (SCHED_CAPACITY_SHIFT+1)
> so middle capacity = min_capacity >> SCHED_CAPACITY_SHIFT
>
> I don't see what your change does that is not already done by current code

ah you are right! I didn't even consider it would be handled under that condition.
My mistake.

It seem the problem is only present in lsk 3.10 then [1]

btw the comments above the condition should still be edited (cpu_scale is always
updated)

ok ignore this patch and let's only fix it on the lsk in the same way it is done
here.

thanks

[1]
https://git.linaro.org/kernel/linux-linaro-stable.git/blob/refs/heads/linux-linaro-lsk:/arch/arm64/kernel/topology.c#l327