Re: [RFC PATCH 6/6] drivers: base: arch_topology.c: Enable EAS for arm/arm64 platforms

Dietmar Eggemann
2018-03-20T15:20:55Z

On 03/20/2018 10:49 AM, Greg Kroah-Hartman wrote:
> On Tue, Mar 20, 2018 at 09:43:12AM +0000, Dietmar Eggemann wrote:
>> From: Quentin Perret <quentin.perret@arm.com>
>>
>> Energy Aware Scheduling (EAS) has to be started from the arch code.
>
> Ok, but:
>
>> This commit enables it from the arch topology driver for arm/arm64
>> systems, hence enabling a better support for Arm big.LITTLE and future
>> DynamIQ architectures.
>
> Why does this have to be in the driver core code just for those specific
> types of cpus?

The arch_topology driver is shared functionality between arm and arm64
arch. So far it handles the correct setting of per-cpu cpu_scale values
which are then used by the task scheduler to set the correct cpu
capacity values. This allows to provide cpu invariant load tracking for
arm and arm64 big.LITTLE systems.

The driver was initially created so that we didn't had to duplicate the
exact code in the arm and arm64 arch.

Big.Little platforms have to set appropriate cpu node capacity-dmips-mhz
properties (e.g. arch/arm64/boot/dts/arm/juno.dts) to enable the start
the functionality of the driver. The cpu_scale value depends on the
capacity-dmips-mhz property as well as on the max frequency of a logical
cpu (hence the dependency to cpufreq).

A corner case would be a big.little platform purely based on max
frequency differences (e.g. Google Pixel, Qualcomm MSM8996 Snapdragon
821, Quad-core (2x2.15 GHz Kryo & 2x1.6 GHz Kryo)). For such a system
capacity-dmips-mhz should be set to the same value for all logical cpus
(e.g. 1024).

We would like to use the same code (shared between arm and arm64) to
initialize the energy model and start EAS for arm and arm64 systems.

>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Quentin Perret <quentin.perret@arm.com>
>> ---
>> drivers/base/arch_topology.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
>> index 52ec5174bcb1..e2206ea16538 100644
>> --- a/drivers/base/arch_topology.c
>> +++ b/drivers/base/arch_topology.c
>> @@ -15,6 +15,7 @@
>> #include <linux/slab.h>
>> #include <linux/string.h>
>> #include <linux/sched/topology.h>
>> +#include <linux/sched/energy.h>
>>
>> DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE;
>>
>> @@ -204,6 +205,7 @@ init_cpu_capacity_callback(struct notifier_block *nb,
>> free_raw_capacity();
>> pr_debug("cpu_capacity: parsing done\n");
>> schedule_work(&parsing_done_work);
>> + init_sched_energy();
>
> This is not arch-specific code only for arm.

The driver requires CONFIG_GENERIC_ARCH_TOPOLOGY which is currently only
set in arch/arm/Kconfig and arch/arm64/Kconfig.

> Don't you have a ARM cpu bringup code somewhere? Shouldn't this call be
> in there? It feels odd that this scheduler change is buried way down
> here...

The big benefit is that we don't have to duplicate code for arm and
arm64. In the current form, EAS should only be started when cpufreq is
initialized for all possible cpus. And we don't have to signal back from
the driver to the arch that cpufreq is initialized for all possible cpus.

I agree that none of this is obvious so the patch should explain the
requirements and design better.

-- Dietmar …