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

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

[Jorge Ramirez-Ortiz]

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.

[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)))
 		middle_capacity = (min_capacity + max_capacity)
 				>> (SCHED_CAPACITY_SHIFT+1);
 	else
-- 
2.1.4

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

[Vincent Guittot]

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 ?

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

Regards,
Vincent


>                 middle_capacity = (min_capacity + max_capacity)
>                                 >> (SCHED_CAPACITY_SHIFT+1);
>         else
> --
> 2.1.4
>

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

[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