For a more accurate (i.e. frequency- and cpu-invariant) load-tracking
the task scheduler needs a frequency-scaling and on a heterogeneous
system a cpu-scaling correction factor.
This patch-set implements a Frequency Invariance Engine (FIE)
(topology_get_freq_scale()) in drivers/base/arch_topology.c to provide
a frequency-scaling correction factor.
The Cpu Invariance Engine (CIE) (topology_get_cpu_scale()) providing a
cpu-scaling correction factor was already introduced by the "Fix issues
and factorize arm/arm64 capacity information code" patch-set [1].
This patch-set also enables the frequency- and cpu-invariant accounting
support. Enabling here means to associate (wire) the task scheduler
cname arch_scale_freq_capacity and arch_scale_cpu_capacity with the FIE
and CIE function names from drivers/base/arch_topology.c. This replaces
the task scheduler's default FIE and CIE in kernel/sched/sched.h.
Patch high level description:
[ 01/06] Rework cpufreq policy notifier for frequency-invariant
accounting support
[ 02/06] Frequency Invariance Engine (FIE)
[03,04/06] Enable frequency- and cpu-invariant accounting support on
arm
[05,06/06] Enable frequency- and cpu-invariant accounting support on
arm64
The patch-set is based on top of linux-next/master (tag: next-20170607)
and it is also available from:
git://linux-arm.org/linux-de.git upstream/freq_and_cpu_inv
It has been tested on TC2 (arm) and JUNO (arm64) by running a ramp-up
rt-app task pinned to a cpu with the ondemand cpufreq governor and
checking the load-tracking signals of this task.
[1] https://marc.info/?l=linux-kernel&m=149625018223002&w=2
Dietmar Eggemann (6):
drivers base/arch_topology: prepare cpufreq policy notifier for
frequency-invariant load-tracking support
drivers base/arch_topology: frequency-invariant load-tracking support
arm: wire frequency-invariant accounting support up to the task
scheduler
arm: wire cpu-invariant accounting support up to the task scheduler
arm64: wire frequency-invariant accounting support up to the task
scheduler
arm64: wire cpu-invariant accounting support up to the task scheduler
arch/arm/include/asm/topology.h | 8 +++++
arch/arm/kernel/topology.c | 1 -
arch/arm64/include/asm/topology.h | 8 +++++
arch/arm64/kernel/topology.c | 1 -
drivers/base/arch_topology.c | 64 ++++++++++++++++++++++++++++++++++-----
include/linux/arch_topology.h | 2 ++
6 files changed, 75 insertions(+), 9 deletions(-)
--
2.11.0
This patch prepares the actual implementation of the frequency-invariant
load-tracking support provided in the next patch ("drivers
base/arch_topology: frequency-invariant load-tracking support").
The maximum supported frequency of a cpu (policy->cpuinfo.max_freq) has
to be retrieved for frequency-invariant load-tracking.
This can be achieved by coding this functionality into the existing
cpufreq policy notifier (init_cpu_capacity_notifier) which is currently
only used for setting up dt-based cpu capacities (cpu node property
capacity-dmips-mhz).
But frequency-invariant load-tracking has to work whether cpu capacity
dt-parsing succeeded or not.
Change init_cpu_capacity_notifier in such a way that even if the parsing
of the cpu capacity information failed the notifier is called for each
cpufreq policy to be able to set the maximum supported frequency.
The exit condition in register_cpufreq_notifier() now only tests for
!acpi_disabled because for frequency invariance the cpufreq policy
notifier has to be enabled even if u32 *raw_capacity is NULL which
occurs when there is no capacity-dmips-mhz property in the dt file or
when the allocation for raw_capacity[cpu] has failed.
The continue statement in init_cpu_capacity_callback() makes sure that
we don't go on calculating capacity_scale in case the capacity parsing
failed. It should be a break rather a continue here but the next patch
introduces code to set the per-cpu variable max_freq in this
for_each_cpu loop before the check if cap_parsing_failed so it has to
be a continue.
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Russell King <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Juri Lelli <[email protected]>
Signed-off-by: Dietmar Eggemann <[email protected]>
---
drivers/base/arch_topology.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index d1c33a85059e..272831c89feb 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -173,7 +173,7 @@ init_cpu_capacity_callback(struct notifier_block *nb,
struct cpufreq_policy *policy = data;
int cpu;
- if (cap_parsing_failed || cap_parsing_done)
+ if (cap_parsing_done)
return 0;
switch (val) {
@@ -185,13 +185,17 @@ init_cpu_capacity_callback(struct notifier_block *nb,
cpus_to_visit,
policy->related_cpus);
for_each_cpu(cpu, policy->related_cpus) {
+ if (cap_parsing_failed)
+ continue;
raw_capacity[cpu] = topology_get_cpu_scale(NULL, cpu) *
policy->cpuinfo.max_freq / 1000UL;
capacity_scale = max(raw_capacity[cpu], capacity_scale);
}
if (cpumask_empty(cpus_to_visit)) {
- topology_normalize_cpu_scale();
- kfree(raw_capacity);
+ if (!cap_parsing_failed) {
+ topology_normalize_cpu_scale();
+ kfree(raw_capacity);
+ }
pr_debug("cpu_capacity: parsing done\n");
cap_parsing_done = true;
schedule_work(&parsing_done_work);
@@ -211,7 +215,7 @@ static int __init register_cpufreq_notifier(void)
* until we have the necessary code to parse the cpu capacity, so
* skip registering cpufreq notifier.
*/
- if (!acpi_disabled || !raw_capacity)
+ if (!acpi_disabled)
return -EINVAL;
if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL)) {
--
2.11.0
Commit dfbca41f3479 ("sched: Optimize freq invariant accounting")
changed the wiring which now has to be done by associating
arch_scale_freq_capacity with the actual implementation provided
by the architecture.
Define arch_scale_freq_capacity to use the arch_topology "driver"
function topology_get_freq_scale() for the task scheduler's
frequency-invariant accounting instead of the default
arch_scale_freq_capacity() in kernel/sched/sched.h.
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Juri Lelli <[email protected]>
Signed-off-by: Dietmar Eggemann <[email protected]>
---
arch/arm64/include/asm/topology.h | 5 +++++
arch/arm64/kernel/topology.c | 1 -
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
index 8b57339823e9..44598a86ec4a 100644
--- a/arch/arm64/include/asm/topology.h
+++ b/arch/arm64/include/asm/topology.h
@@ -32,6 +32,11 @@ int pcibus_to_node(struct pci_bus *bus);
#endif /* CONFIG_NUMA */
+#include <linux/arch_topology.h>
+
+/* Replace task scheduler's default frequency-invariant accounting */
+#define arch_scale_freq_capacity topology_get_freq_scale
+
#include <asm-generic/topology.h>
#endif /* _ASM_ARM_TOPOLOGY_H */
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 79244c75eaec..6cbb6315e493 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -11,7 +11,6 @@
* for more details.
*/
-#include <linux/arch_topology.h>
#include <linux/cpu.h>
#include <linux/cpumask.h>
#include <linux/init.h>
--
2.11.0
Commit 8cd5601c5060 ("sched/fair: Convert arch_scale_cpu_capacity() from
weak function to #define") changed the wiring which now has to be done
by associating arch_scale_cpu_capacity with the actual implementation
provided by the architecture.
Define arch_scale_cpu_capacity to use the arch_topology "driver"
function topology_get_cpu_scale() for the task scheduler's cpu-invariant
accounting instead of the default arch_scale_cpu_capacity() in
kernel/sched/sched.h.
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Juri Lelli <[email protected]>
Signed-off-by: Dietmar Eggemann <[email protected]>
---
arch/arm64/include/asm/topology.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
index 44598a86ec4a..e313eeb10756 100644
--- a/arch/arm64/include/asm/topology.h
+++ b/arch/arm64/include/asm/topology.h
@@ -37,6 +37,9 @@ int pcibus_to_node(struct pci_bus *bus);
/* Replace task scheduler's default frequency-invariant accounting */
#define arch_scale_freq_capacity topology_get_freq_scale
+/* Replace task scheduler's default cpu-invariant accounting */
+#define arch_scale_cpu_capacity topology_get_cpu_scale
+
#include <asm-generic/topology.h>
#endif /* _ASM_ARM_TOPOLOGY_H */
--
2.11.0
Implements an arch-specific frequency-scaling function
topology_get_freq_scale() which provides the following frequency
scaling factor:
current_freq(cpu) << SCHED_CAPACITY_SHIFT / max_supported_freq(cpu)
The debug output in init_cpu_capacity_callback() has been changed to be
able to distinguish whether cpu capacity and max frequency or only max
frequency has been set. The latter case happens on systems where there
is no or broken cpu capacity binding (cpu node property
capacity-dmips-mhz) information.
One possible consumer of this is the Per-Entity Load Tracking (PELT)
mechanism of the task scheduler.
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Russell King <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Juri Lelli <[email protected]>
Signed-off-by: Dietmar Eggemann <[email protected]>
---
drivers/base/arch_topology.c | 52 ++++++++++++++++++++++++++++++++++++++++---
include/linux/arch_topology.h | 2 ++
2 files changed, 51 insertions(+), 3 deletions(-)
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 272831c89feb..f6f14670bdab 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -24,12 +24,18 @@
static DEFINE_MUTEX(cpu_scale_mutex);
static DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
+static DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE;
unsigned long topology_get_cpu_scale(struct sched_domain *sd, int cpu)
{
return per_cpu(cpu_scale, cpu);
}
+unsigned long topology_get_freq_scale(struct sched_domain *sd, int cpu)
+{
+ return per_cpu(freq_scale, cpu);
+}
+
void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity)
{
per_cpu(cpu_scale, cpu) = capacity;
@@ -164,6 +170,7 @@ static cpumask_var_t cpus_to_visit;
static bool cap_parsing_done;
static void parsing_done_workfn(struct work_struct *work);
static DECLARE_WORK(parsing_done_work, parsing_done_workfn);
+static DEFINE_PER_CPU(unsigned long, max_freq);
static int
init_cpu_capacity_callback(struct notifier_block *nb,
@@ -185,6 +192,7 @@ init_cpu_capacity_callback(struct notifier_block *nb,
cpus_to_visit,
policy->related_cpus);
for_each_cpu(cpu, policy->related_cpus) {
+ per_cpu(max_freq, cpu) = policy->cpuinfo.max_freq;
if (cap_parsing_failed)
continue;
raw_capacity[cpu] = topology_get_cpu_scale(NULL, cpu) *
@@ -195,8 +203,10 @@ init_cpu_capacity_callback(struct notifier_block *nb,
if (!cap_parsing_failed) {
topology_normalize_cpu_scale();
kfree(raw_capacity);
+ pr_debug("cpu_capacity: parsing done\n");
+ } else {
+ pr_debug("cpu_capacity: max frequency parsing done\n");
}
- pr_debug("cpu_capacity: parsing done\n");
cap_parsing_done = true;
schedule_work(&parsing_done_work);
}
@@ -208,8 +218,38 @@ static struct notifier_block init_cpu_capacity_notifier = {
.notifier_call = init_cpu_capacity_callback,
};
+static void set_freq_scale(unsigned int cpu, unsigned long freq)
+{
+ unsigned long max = per_cpu(max_freq, cpu);
+
+ if (!max)
+ return;
+
+ per_cpu(freq_scale, cpu) = (freq << SCHED_CAPACITY_SHIFT) / max;
+}
+
+static int set_freq_scale_callback(struct notifier_block *nb,
+ unsigned long val,
+ void *data)
+{
+ struct cpufreq_freqs *freq = data;
+
+ switch (val) {
+ case CPUFREQ_PRECHANGE:
+ set_freq_scale(freq->cpu, freq->new);
+ }
+
+ return 0;
+}
+
+static struct notifier_block set_freq_scale_notifier = {
+ .notifier_call = set_freq_scale_callback,
+};
+
static int __init register_cpufreq_notifier(void)
{
+ int ret;
+
/*
* on ACPI-based systems we need to use the default cpu capacity
* until we have the necessary code to parse the cpu capacity, so
@@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void)
cpumask_copy(cpus_to_visit, cpu_possible_mask);
- return cpufreq_register_notifier(&init_cpu_capacity_notifier,
- CPUFREQ_POLICY_NOTIFIER);
+ ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
+ CPUFREQ_POLICY_NOTIFIER);
+
+ if (ret)
+ return ret;
+
+ return cpufreq_register_notifier(&set_freq_scale_notifier,
+ CPUFREQ_TRANSITION_NOTIFIER);
}
core_initcall(register_cpufreq_notifier);
diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
index 9af3c174c03a..3fb4d8ccb179 100644
--- a/include/linux/arch_topology.h
+++ b/include/linux/arch_topology.h
@@ -12,6 +12,8 @@ int topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu);
struct sched_domain;
unsigned long topology_get_cpu_scale(struct sched_domain *sd, int cpu);
+unsigned long topology_get_freq_scale(struct sched_domain *sd, int cpu);
+
void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity);
#endif /* _LINUX_ARCH_TOPOLOGY_H_ */
--
2.11.0
Commit dfbca41f3479 ("sched: Optimize freq invariant accounting")
changed the wiring which now has to be done by associating
arch_scale_freq_capacity with the actual implementation provided
by the architecture.
Define arch_scale_freq_capacity to use the arch_topology "driver"
function topology_get_freq_scale() for the task scheduler's
frequency-invariant accounting instead of the default
arch_scale_freq_capacity() in kernel/sched/sched.h.
Cc: Russell King <[email protected]>
Cc: Juri Lelli <[email protected]>
Signed-off-by: Dietmar Eggemann <[email protected]>
---
arch/arm/include/asm/topology.h | 5 +++++
arch/arm/kernel/topology.c | 1 -
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
index 370f7a732900..a56a9e24f4c0 100644
--- a/arch/arm/include/asm/topology.h
+++ b/arch/arm/include/asm/topology.h
@@ -24,6 +24,11 @@ void init_cpu_topology(void);
void store_cpu_topology(unsigned int cpuid);
const struct cpumask *cpu_coregroup_mask(int cpu);
+#include <linux/arch_topology.h>
+
+/* Replace task scheduler's default frequency-invariant accounting */
+#define arch_scale_freq_capacity topology_get_freq_scale
+
#else
static inline void init_cpu_topology(void) { }
diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
index bf949a763dbe..2c47a76c67b0 100644
--- a/arch/arm/kernel/topology.c
+++ b/arch/arm/kernel/topology.c
@@ -11,7 +11,6 @@
* for more details.
*/
-#include <linux/arch_topology.h>
#include <linux/cpu.h>
#include <linux/cpufreq.h>
#include <linux/cpumask.h>
--
2.11.0
Commit 8cd5601c5060 ("sched/fair: Convert arch_scale_cpu_capacity() from
weak function to #define") changed the wiring which now has to be done
by associating arch_scale_cpu_capacity with the actual implementation
provided by the architecture.
Define arch_scale_cpu_capacity to use the arch_topology "driver"
function topology_get_cpu_scale() for the task scheduler's cpu-invariant
accounting instead of the default arch_scale_cpu_capacity() in
kernel/sched/sched.h.
Cc: Russell King <[email protected]>
Cc: Juri Lelli <[email protected]>
Signed-off-by: Dietmar Eggemann <[email protected]>
---
arch/arm/include/asm/topology.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
index a56a9e24f4c0..b713e7223bc4 100644
--- a/arch/arm/include/asm/topology.h
+++ b/arch/arm/include/asm/topology.h
@@ -29,6 +29,9 @@ const struct cpumask *cpu_coregroup_mask(int cpu);
/* Replace task scheduler's default frequency-invariant accounting */
#define arch_scale_freq_capacity topology_get_freq_scale
+/* Replace task scheduler's default cpu-invariant accounting */
+#define arch_scale_cpu_capacity topology_get_cpu_scale
+
#else
static inline void init_cpu_topology(void) { }
--
2.11.0
Hi Dietmar,
On 08/06/17 08:55, Dietmar Eggemann wrote:
> For a more accurate (i.e. frequency- and cpu-invariant) load-tracking
> the task scheduler needs a frequency-scaling and on a heterogeneous
> system a cpu-scaling correction factor.
>
> This patch-set implements a Frequency Invariance Engine (FIE)
> (topology_get_freq_scale()) in drivers/base/arch_topology.c to provide
> a frequency-scaling correction factor.
>
> The Cpu Invariance Engine (CIE) (topology_get_cpu_scale()) providing a
> cpu-scaling correction factor was already introduced by the "Fix issues
> and factorize arm/arm64 capacity information code" patch-set [1].
>
> This patch-set also enables the frequency- and cpu-invariant accounting
> support. Enabling here means to associate (wire) the task scheduler
> cname arch_scale_freq_capacity and arch_scale_cpu_capacity with the FIE
> and CIE function names from drivers/base/arch_topology.c. This replaces
> the task scheduler's default FIE and CIE in kernel/sched/sched.h.
>
> Patch high level description:
>
> [ 01/06] Rework cpufreq policy notifier for frequency-invariant
> accounting support
> [ 02/06] Frequency Invariance Engine (FIE)
> [03,04/06] Enable frequency- and cpu-invariant accounting support on
> arm
> [05,06/06] Enable frequency- and cpu-invariant accounting support on
> arm64
>
> The patch-set is based on top of linux-next/master (tag: next-20170607)
> and it is also available from:
>
> git://linux-arm.org/linux-de.git upstream/freq_and_cpu_inv
>
> It has been tested on TC2 (arm) and JUNO (arm64) by running a ramp-up
> rt-app task pinned to a cpu with the ondemand cpufreq governor and
> checking the load-tracking signals of this task.
>
The whole set looks OK to me, and I tested it as well.
Feel free to add my
Reviewed-and-tested-by: Juri Lelli <[email protected]>
to it.
Best,
- Juri
This time hopefully fixing Vincent's email address..
On 12/06/17 14:00, Juri Lelli wrote:
> Hi Dietmar,
>
> On 08/06/17 08:55, Dietmar Eggemann wrote:
> > For a more accurate (i.e. frequency- and cpu-invariant) load-tracking
> > the task scheduler needs a frequency-scaling and on a heterogeneous
> > system a cpu-scaling correction factor.
> >
> > This patch-set implements a Frequency Invariance Engine (FIE)
> > (topology_get_freq_scale()) in drivers/base/arch_topology.c to provide
> > a frequency-scaling correction factor.
> >
> > The Cpu Invariance Engine (CIE) (topology_get_cpu_scale()) providing a
> > cpu-scaling correction factor was already introduced by the "Fix issues
> > and factorize arm/arm64 capacity information code" patch-set [1].
> >
> > This patch-set also enables the frequency- and cpu-invariant accounting
> > support. Enabling here means to associate (wire) the task scheduler
> > cname arch_scale_freq_capacity and arch_scale_cpu_capacity with the FIE
> > and CIE function names from drivers/base/arch_topology.c. This replaces
> > the task scheduler's default FIE and CIE in kernel/sched/sched.h.
> >
> > Patch high level description:
> >
> > [ 01/06] Rework cpufreq policy notifier for frequency-invariant
> > accounting support
> > [ 02/06] Frequency Invariance Engine (FIE)
> > [03,04/06] Enable frequency- and cpu-invariant accounting support on
> > arm
> > [05,06/06] Enable frequency- and cpu-invariant accounting support on
> > arm64
> >
> > The patch-set is based on top of linux-next/master (tag: next-20170607)
> > and it is also available from:
> >
> > git://linux-arm.org/linux-de.git upstream/freq_and_cpu_inv
> >
> > It has been tested on TC2 (arm) and JUNO (arm64) by running a ramp-up
> > rt-app task pinned to a cpu with the ondemand cpufreq governor and
> > checking the load-tracking signals of this task.
> >
>
> The whole set looks OK to me, and I tested it as well.
>
> Feel free to add my
>
> Reviewed-and-tested-by: Juri Lelli <[email protected]>
>
> to it.
>
> Best,
>
> - Juri
On Thu, Jun 08, 2017 at 08:55:12AM +0100, Dietmar Eggemann wrote:
> Commit dfbca41f3479 ("sched: Optimize freq invariant accounting")
> changed the wiring which now has to be done by associating
> arch_scale_freq_capacity with the actual implementation provided
> by the architecture.
>
> Define arch_scale_freq_capacity to use the arch_topology "driver"
> function topology_get_freq_scale() for the task scheduler's
> frequency-invariant accounting instead of the default
> arch_scale_freq_capacity() in kernel/sched/sched.h.
>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Juri Lelli <[email protected]>
> Signed-off-by: Dietmar Eggemann <[email protected]>
> ---
> arch/arm64/include/asm/topology.h | 5 +++++
> arch/arm64/kernel/topology.c | 1 -
> 2 files changed, 5 insertions(+), 1 deletion(-)
Acked-by: Catalin Marinas <[email protected]>
On Thu, Jun 08, 2017 at 08:55:13AM +0100, Dietmar Eggemann wrote:
> Commit 8cd5601c5060 ("sched/fair: Convert arch_scale_cpu_capacity() from
> weak function to #define") changed the wiring which now has to be done
> by associating arch_scale_cpu_capacity with the actual implementation
> provided by the architecture.
>
> Define arch_scale_cpu_capacity to use the arch_topology "driver"
> function topology_get_cpu_scale() for the task scheduler's cpu-invariant
> accounting instead of the default arch_scale_cpu_capacity() in
> kernel/sched/sched.h.
>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Juri Lelli <[email protected]>
> Signed-off-by: Dietmar Eggemann <[email protected]>
> ---
> arch/arm64/include/asm/topology.h | 3 +++
> 1 file changed, 3 insertions(+)
Acked-by: Catalin Marinas <[email protected]>
On 8 June 2017 at 09:55, Dietmar Eggemann <[email protected]> wrote:
> Implements an arch-specific frequency-scaling function
> topology_get_freq_scale() which provides the following frequency
> scaling factor:
>
> current_freq(cpu) << SCHED_CAPACITY_SHIFT / max_supported_freq(cpu)
>
> The debug output in init_cpu_capacity_callback() has been changed to be
> able to distinguish whether cpu capacity and max frequency or only max
> frequency has been set. The latter case happens on systems where there
> is no or broken cpu capacity binding (cpu node property
> capacity-dmips-mhz) information.
>
> One possible consumer of this is the Per-Entity Load Tracking (PELT)
> mechanism of the task scheduler.
>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Juri Lelli <[email protected]>
> Signed-off-by: Dietmar Eggemann <[email protected]>
> ---
> drivers/base/arch_topology.c | 52 ++++++++++++++++++++++++++++++++++++++++---
> include/linux/arch_topology.h | 2 ++
> 2 files changed, 51 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index 272831c89feb..f6f14670bdab 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -24,12 +24,18 @@
>
> static DEFINE_MUTEX(cpu_scale_mutex);
> static DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
> +static DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE;
>
> unsigned long topology_get_cpu_scale(struct sched_domain *sd, int cpu)
> {
> return per_cpu(cpu_scale, cpu);
> }
>
> +unsigned long topology_get_freq_scale(struct sched_domain *sd, int cpu)
> +{
> + return per_cpu(freq_scale, cpu);
> +}
> +
> void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity)
> {
> per_cpu(cpu_scale, cpu) = capacity;
> @@ -164,6 +170,7 @@ static cpumask_var_t cpus_to_visit;
> static bool cap_parsing_done;
> static void parsing_done_workfn(struct work_struct *work);
> static DECLARE_WORK(parsing_done_work, parsing_done_workfn);
> +static DEFINE_PER_CPU(unsigned long, max_freq);
>
> static int
> init_cpu_capacity_callback(struct notifier_block *nb,
> @@ -185,6 +192,7 @@ init_cpu_capacity_callback(struct notifier_block *nb,
> cpus_to_visit,
> policy->related_cpus);
> for_each_cpu(cpu, policy->related_cpus) {
> + per_cpu(max_freq, cpu) = policy->cpuinfo.max_freq;
> if (cap_parsing_failed)
> continue;
> raw_capacity[cpu] = topology_get_cpu_scale(NULL, cpu) *
> @@ -195,8 +203,10 @@ init_cpu_capacity_callback(struct notifier_block *nb,
> if (!cap_parsing_failed) {
> topology_normalize_cpu_scale();
> kfree(raw_capacity);
> + pr_debug("cpu_capacity: parsing done\n");
> + } else {
> + pr_debug("cpu_capacity: max frequency parsing done\n");
> }
> - pr_debug("cpu_capacity: parsing done\n");
> cap_parsing_done = true;
> schedule_work(&parsing_done_work);
> }
> @@ -208,8 +218,38 @@ static struct notifier_block init_cpu_capacity_notifier = {
> .notifier_call = init_cpu_capacity_callback,
> };
>
> +static void set_freq_scale(unsigned int cpu, unsigned long freq)
> +{
> + unsigned long max = per_cpu(max_freq, cpu);
> +
> + if (!max)
> + return;
> +
> + per_cpu(freq_scale, cpu) = (freq << SCHED_CAPACITY_SHIFT) / max;
> +}
> +
> +static int set_freq_scale_callback(struct notifier_block *nb,
> + unsigned long val,
> + void *data)
> +{
> + struct cpufreq_freqs *freq = data;
> +
> + switch (val) {
> + case CPUFREQ_PRECHANGE:
> + set_freq_scale(freq->cpu, freq->new);
> + }
> +
> + return 0;
> +}
> +
> +static struct notifier_block set_freq_scale_notifier = {
> + .notifier_call = set_freq_scale_callback,
> +};
> +
> static int __init register_cpufreq_notifier(void)
> {
> + int ret;
> +
> /*
> * on ACPI-based systems we need to use the default cpu capacity
> * until we have the necessary code to parse the cpu capacity, so
> @@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void)
>
> cpumask_copy(cpus_to_visit, cpu_possible_mask);
>
> - return cpufreq_register_notifier(&init_cpu_capacity_notifier,
> - CPUFREQ_POLICY_NOTIFIER);
> + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
> + CPUFREQ_POLICY_NOTIFIER);
> +
> + if (ret)
Don't you have to free memory allocated for cpus_to_visit in case of
errot ? it was not done before your patch as well
> + return ret;
> +
> + return cpufreq_register_notifier(&set_freq_scale_notifier,
> + CPUFREQ_TRANSITION_NOTIFIER);
Don't you have to unregister the other cpufreq notifier if an error is
returned and free the mem allocated for cpus_to_visit ?
> }
> core_initcall(register_cpufreq_notifier);
>
> diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
> index 9af3c174c03a..3fb4d8ccb179 100644
> --- a/include/linux/arch_topology.h
> +++ b/include/linux/arch_topology.h
> @@ -12,6 +12,8 @@ int topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu);
> struct sched_domain;
> unsigned long topology_get_cpu_scale(struct sched_domain *sd, int cpu);
>
> +unsigned long topology_get_freq_scale(struct sched_domain *sd, int cpu);
> +
> void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity);
>
> #endif /* _LINUX_ARCH_TOPOLOGY_H_ */
> --
> 2.11.0
>
On 8 June 2017 at 09:55, Dietmar Eggemann <[email protected]> wrote:
> Commit dfbca41f3479 ("sched: Optimize freq invariant accounting")
> changed the wiring which now has to be done by associating
> arch_scale_freq_capacity with the actual implementation provided
> by the architecture.
>
> Define arch_scale_freq_capacity to use the arch_topology "driver"
> function topology_get_freq_scale() for the task scheduler's
> frequency-invariant accounting instead of the default
> arch_scale_freq_capacity() in kernel/sched/sched.h.
>
> Cc: Russell King <[email protected]>
> Cc: Juri Lelli <[email protected]>
> Signed-off-by: Dietmar Eggemann <[email protected]>
Acked-by: Vincent Guittot <[email protected]>
> ---
> arch/arm/include/asm/topology.h | 5 +++++
> arch/arm/kernel/topology.c | 1 -
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
> index 370f7a732900..a56a9e24f4c0 100644
> --- a/arch/arm/include/asm/topology.h
> +++ b/arch/arm/include/asm/topology.h
> @@ -24,6 +24,11 @@ void init_cpu_topology(void);
> void store_cpu_topology(unsigned int cpuid);
> const struct cpumask *cpu_coregroup_mask(int cpu);
>
> +#include <linux/arch_topology.h>
> +
> +/* Replace task scheduler's default frequency-invariant accounting */
> +#define arch_scale_freq_capacity topology_get_freq_scale
> +
> #else
>
> static inline void init_cpu_topology(void) { }
> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
> index bf949a763dbe..2c47a76c67b0 100644
> --- a/arch/arm/kernel/topology.c
> +++ b/arch/arm/kernel/topology.c
> @@ -11,7 +11,6 @@
> * for more details.
> */
>
> -#include <linux/arch_topology.h>
> #include <linux/cpu.h>
> #include <linux/cpufreq.h>
> #include <linux/cpumask.h>
> --
> 2.11.0
>
On 8 June 2017 at 09:55, Dietmar Eggemann <[email protected]> wrote:
> Commit 8cd5601c5060 ("sched/fair: Convert arch_scale_cpu_capacity() from
> weak function to #define") changed the wiring which now has to be done
> by associating arch_scale_cpu_capacity with the actual implementation
> provided by the architecture.
>
> Define arch_scale_cpu_capacity to use the arch_topology "driver"
> function topology_get_cpu_scale() for the task scheduler's cpu-invariant
> accounting instead of the default arch_scale_cpu_capacity() in
> kernel/sched/sched.h.
>
> Cc: Russell King <[email protected]>
> Cc: Juri Lelli <[email protected]>
> Signed-off-by: Dietmar Eggemann <[email protected]>
Acked-by: Vincent Guittot <[email protected]>
> ---
> arch/arm/include/asm/topology.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
> index a56a9e24f4c0..b713e7223bc4 100644
> --- a/arch/arm/include/asm/topology.h
> +++ b/arch/arm/include/asm/topology.h
> @@ -29,6 +29,9 @@ const struct cpumask *cpu_coregroup_mask(int cpu);
> /* Replace task scheduler's default frequency-invariant accounting */
> #define arch_scale_freq_capacity topology_get_freq_scale
>
> +/* Replace task scheduler's default cpu-invariant accounting */
> +#define arch_scale_cpu_capacity topology_get_cpu_scale
> +
> #else
>
> static inline void init_cpu_topology(void) { }
> --
> 2.11.0
>
On 8 June 2017 at 09:55, Dietmar Eggemann <[email protected]> wrote:
> Commit dfbca41f3479 ("sched: Optimize freq invariant accounting")
> changed the wiring which now has to be done by associating
> arch_scale_freq_capacity with the actual implementation provided
> by the architecture.
>
> Define arch_scale_freq_capacity to use the arch_topology "driver"
> function topology_get_freq_scale() for the task scheduler's
> frequency-invariant accounting instead of the default
> arch_scale_freq_capacity() in kernel/sched/sched.h.
>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Juri Lelli <[email protected]>
> Signed-off-by: Dietmar Eggemann <[email protected]>
Acked-by: Vincent Guittot <[email protected]>
On 8 June 2017 at 09:55, Dietmar Eggemann <[email protected]> wrote:
> Commit 8cd5601c5060 ("sched/fair: Convert arch_scale_cpu_capacity() from
> weak function to #define") changed the wiring which now has to be done
> by associating arch_scale_cpu_capacity with the actual implementation
> provided by the architecture.
>
> Define arch_scale_cpu_capacity to use the arch_topology "driver"
> function topology_get_cpu_scale() for the task scheduler's cpu-invariant
> accounting instead of the default arch_scale_cpu_capacity() in
> kernel/sched/sched.h.
>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Juri Lelli <[email protected]>
> Signed-off-by: Dietmar Eggemann <[email protected]>
Acked-by: Vincent Guittot <[email protected]>
On 8 June 2017 at 09:55, Dietmar Eggemann <[email protected]> wrote:
> This patch prepares the actual implementation of the frequency-invariant
> load-tracking support provided in the next patch ("drivers
> base/arch_topology: frequency-invariant load-tracking support").
>
> The maximum supported frequency of a cpu (policy->cpuinfo.max_freq) has
> to be retrieved for frequency-invariant load-tracking.
>
> This can be achieved by coding this functionality into the existing
> cpufreq policy notifier (init_cpu_capacity_notifier) which is currently
> only used for setting up dt-based cpu capacities (cpu node property
> capacity-dmips-mhz).
>
> But frequency-invariant load-tracking has to work whether cpu capacity
> dt-parsing succeeded or not.
>
> Change init_cpu_capacity_notifier in such a way that even if the parsing
> of the cpu capacity information failed the notifier is called for each
> cpufreq policy to be able to set the maximum supported frequency.
>
> The exit condition in register_cpufreq_notifier() now only tests for
> !acpi_disabled because for frequency invariance the cpufreq policy
> notifier has to be enabled even if u32 *raw_capacity is NULL which
> occurs when there is no capacity-dmips-mhz property in the dt file or
> when the allocation for raw_capacity[cpu] has failed.
>
> The continue statement in init_cpu_capacity_callback() makes sure that
> we don't go on calculating capacity_scale in case the capacity parsing
> failed. It should be a break rather a continue here but the next patch
> introduces code to set the per-cpu variable max_freq in this
> for_each_cpu loop before the check if cap_parsing_failed so it has to
> be a continue.
>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Juri Lelli <[email protected]>
> Signed-off-by: Dietmar Eggemann <[email protected]>
Acked-by: Vincent Guittot <[email protected]>
On 06/12/2017 04:27 PM, Vincent Guittot wrote:
> On 8 June 2017 at 09:55, Dietmar Eggemann <[email protected]> wrote:
Hi Vincent,
Thanks for the review!
[...]
>> @@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void)
>>
>> cpumask_copy(cpus_to_visit, cpu_possible_mask);
>>
>> - return cpufreq_register_notifier(&init_cpu_capacity_notifier,
>> - CPUFREQ_POLICY_NOTIFIER);
>> + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
>> + CPUFREQ_POLICY_NOTIFIER);
>> +
>> + if (ret)
>
> Don't you have to free memory allocated for cpus_to_visit in case of
> errot ? it was not done before your patch as well
Yes, we should free cpus_to_visit if the policy notifier registration
fails. But IMHO also, once the parsing of the capacity-dmips-mhz property
is done. free cpus_to_visit is only used in the notifier call
init_cpu_capacity_callback() after being allocated and initialized in
register_cpufreq_notifier().
We could add something like this as the first patch of this set. Only
mildly tested on Juno. Juri, what do you think?
Author: Dietmar Eggemann <[email protected]>
Date: Tue Jun 13 23:21:59 2017 +0100
drivers base/arch_topology: free cpumask cpus_to_visit
Free cpumask cpus_to_visit in case registering
init_cpu_capacity_notifier has failed or the parsing of the cpu
capacity-dmips-mhz property is done. The cpumask cpus_to_visit is
only used inside the notifier call init_cpu_capacity_callback.
Reported-by: Vincent Guittot <[email protected]>
Signed-off-by: Dietmar Eggemann <[email protected]>
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index d1c33a85059e..f4832c662762 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -206,6 +206,8 @@ static struct notifier_block init_cpu_capacity_notifier = {
static int __init register_cpufreq_notifier(void)
{
+ int ret;
+
/*
* on ACPI-based systems we need to use the default cpu capacity
* until we have the necessary code to parse the cpu capacity, so
@@ -221,13 +223,19 @@ static int __init register_cpufreq_notifier(void)
cpumask_copy(cpus_to_visit, cpu_possible_mask);
- return cpufreq_register_notifier(&init_cpu_capacity_notifier,
- CPUFREQ_POLICY_NOTIFIER);
+ ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
+ CPUFREQ_POLICY_NOTIFIER);
+
+ if (ret)
+ free_cpumask_var(cpus_to_visit);
+
+ return ret;
}
core_initcall(register_cpufreq_notifier);
static void parsing_done_workfn(struct work_struct *work)
{
+ free_cpumask_var(cpus_to_visit);
cpufreq_unregister_notifier(&init_cpu_capacity_notifier,
CPUFREQ_POLICY_NOTIFIER);
}
>> + return ret;
>> +
>> + return cpufreq_register_notifier(&set_freq_scale_notifier,
>> + CPUFREQ_TRANSITION_NOTIFIER);
>
> Don't you have to unregister the other cpufreq notifier if an error is
> returned and free the mem allocated for cpus_to_visit ?
IMHO, that's not necessary.
The transition notifier works completely independent from the policy
notifier. In case the latter gets registered correctly and the registration
of the former fails, the notifier call of the policy notifier still parses
the capacity-dmips-mhz property information and sets per_cpu(max_freq, cpu).
The notifier call set_freq_scale_callback() of the transition notifier will
not be called so that frequency invariance always returns
SCHED_CAPACITY_SCALE.
After the policy notifier has finished its work, it schedules
parsing_done_work() in which it gets unregistered.
[...]
On 14 June 2017 at 09:55, Dietmar Eggemann <[email protected]> wrote:
>
> On 06/12/2017 04:27 PM, Vincent Guittot wrote:
> > On 8 June 2017 at 09:55, Dietmar Eggemann <[email protected]> wrote:
>
> Hi Vincent,
>
> Thanks for the review!
>
> [...]
>
> >> @@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void)
> >>
> >> cpumask_copy(cpus_to_visit, cpu_possible_mask);
> >>
> >> - return cpufreq_register_notifier(&init_cpu_capacity_notifier,
> >> - CPUFREQ_POLICY_NOTIFIER);
> >> + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
> >> + CPUFREQ_POLICY_NOTIFIER);
> >> +
> >> + if (ret)
> >
> > Don't you have to free memory allocated for cpus_to_visit in case of
> > errot ? it was not done before your patch as well
>
> Yes, we should free cpus_to_visit if the policy notifier registration
> fails. But IMHO also, once the parsing of the capacity-dmips-mhz property
> is done. free cpus_to_visit is only used in the notifier call
> init_cpu_capacity_callback() after being allocated and initialized in
> register_cpufreq_notifier().
>
> We could add something like this as the first patch of this set. Only
> mildly tested on Juno. Juri, what do you think?
>
> Author: Dietmar Eggemann <[email protected]>
> Date: Tue Jun 13 23:21:59 2017 +0100
>
> drivers base/arch_topology: free cpumask cpus_to_visit
>
> Free cpumask cpus_to_visit in case registering
> init_cpu_capacity_notifier has failed or the parsing of the cpu
> capacity-dmips-mhz property is done. The cpumask cpus_to_visit is
> only used inside the notifier call init_cpu_capacity_callback.
>
> Reported-by: Vincent Guittot <[email protected]>
> Signed-off-by: Dietmar Eggemann <[email protected]>
your proposal for freeing cpus_to_visit looks good for me
Acked-by: Vincent Guittot <[email protected]>
>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index d1c33a85059e..f4832c662762 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -206,6 +206,8 @@ static struct notifier_block init_cpu_capacity_notifier = {
>
> static int __init register_cpufreq_notifier(void)
> {
> + int ret;
> +
> /*
> * on ACPI-based systems we need to use the default cpu capacity
> * until we have the necessary code to parse the cpu capacity, so
> @@ -221,13 +223,19 @@ static int __init register_cpufreq_notifier(void)
>
> cpumask_copy(cpus_to_visit, cpu_possible_mask);
>
> - return cpufreq_register_notifier(&init_cpu_capacity_notifier,
> - CPUFREQ_POLICY_NOTIFIER);
> + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
> + CPUFREQ_POLICY_NOTIFIER);
> +
> + if (ret)
> + free_cpumask_var(cpus_to_visit);
> +
> + return ret;
> }
> core_initcall(register_cpufreq_notifier);
>
> static void parsing_done_workfn(struct work_struct *work)
> {
> + free_cpumask_var(cpus_to_visit);
> cpufreq_unregister_notifier(&init_cpu_capacity_notifier,
> CPUFREQ_POLICY_NOTIFIER);
> }
>
> >> + return ret;
> >> +
> >> + return cpufreq_register_notifier(&set_freq_scale_notifier,
> >> + CPUFREQ_TRANSITION_NOTIFIER);
> >
> > Don't you have to unregister the other cpufreq notifier if an error is
> > returned and free the mem allocated for cpus_to_visit ?
>
> IMHO, that's not necessary.
>
> The transition notifier works completely independent from the policy
> notifier. In case the latter gets registered correctly and the registration
> of the former fails, the notifier call of the policy notifier still parses
> the capacity-dmips-mhz property information and sets per_cpu(max_freq, cpu).
>
> The notifier call set_freq_scale_callback() of the transition notifier will
> not be called so that frequency invariance always returns
> SCHED_CAPACITY_SCALE.
>
> After the policy notifier has finished its work, it schedules
> parsing_done_work() in which it gets unregistered.
Ok so IIUC, the transition notifier is somehow optional and we still
have the cpu invariance.
In this case, you should not return the error code of
cpufreq_register_notifier(&set_freq_scale_notifier,
CPUFREQ_TRANSITION_NOTIFIER) as the error code of the
register_cpufreq_notifier function.
you should better print a warning like " failed to init frequency
invariance" and return 0 for register_cpufreq_notifier()
>
> [...]
Hi,
On 14/06/17 15:08, Vincent Guittot wrote:
> On 14 June 2017 at 09:55, Dietmar Eggemann <[email protected]> wrote:
> >
> > On 06/12/2017 04:27 PM, Vincent Guittot wrote:
> > > On 8 June 2017 at 09:55, Dietmar Eggemann <[email protected]> wrote:
> >
> > Hi Vincent,
> >
> > Thanks for the review!
> >
> > [...]
> >
> > >> @@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void)
> > >>
> > >> cpumask_copy(cpus_to_visit, cpu_possible_mask);
> > >>
> > >> - return cpufreq_register_notifier(&init_cpu_capacity_notifier,
> > >> - CPUFREQ_POLICY_NOTIFIER);
> > >> + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
> > >> + CPUFREQ_POLICY_NOTIFIER);
> > >> +
> > >> + if (ret)
> > >
> > > Don't you have to free memory allocated for cpus_to_visit in case of
> > > errot ? it was not done before your patch as well
> >
> > Yes, we should free cpus_to_visit if the policy notifier registration
> > fails. But IMHO also, once the parsing of the capacity-dmips-mhz property
> > is done. free cpus_to_visit is only used in the notifier call
> > init_cpu_capacity_callback() after being allocated and initialized in
> > register_cpufreq_notifier().
> >
> > We could add something like this as the first patch of this set. Only
> > mildly tested on Juno. Juri, what do you think?
> >
> > Author: Dietmar Eggemann <[email protected]>
> > Date: Tue Jun 13 23:21:59 2017 +0100
> >
> > drivers base/arch_topology: free cpumask cpus_to_visit
> >
> > Free cpumask cpus_to_visit in case registering
> > init_cpu_capacity_notifier has failed or the parsing of the cpu
> > capacity-dmips-mhz property is done. The cpumask cpus_to_visit is
> > only used inside the notifier call init_cpu_capacity_callback.
> >
> > Reported-by: Vincent Guittot <[email protected]>
> > Signed-off-by: Dietmar Eggemann <[email protected]>
>
> your proposal for freeing cpus_to_visit looks good for me
>
> Acked-by: Vincent Guittot <[email protected]>
>
Yep, looks good to me too. Thanks for fixing!
Best,
- Juri
On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
<[email protected]> wrote:
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> static int
> init_cpu_capacity_callback(struct notifier_block *nb,
> @@ -185,6 +192,7 @@ init_cpu_capacity_callback(struct notifier_block *nb,
> cpus_to_visit,
> policy->related_cpus);
> for_each_cpu(cpu, policy->related_cpus) {
> + per_cpu(max_freq, cpu) = policy->cpuinfo.max_freq;
I am not sure about this but why shouldn't we use policy->max here ?
As that is the
max, we can set the frequency to right now.
> if (cap_parsing_failed)
> continue;
> raw_capacity[cpu] = topology_get_cpu_scale(NULL, cpu) *
> @@ -195,8 +203,10 @@ init_cpu_capacity_callback(struct notifier_block *nb,
> if (!cap_parsing_failed) {
> topology_normalize_cpu_scale();
> kfree(raw_capacity);
> + pr_debug("cpu_capacity: parsing done\n");
> + } else {
> + pr_debug("cpu_capacity: max frequency parsing done\n");
> }
> - pr_debug("cpu_capacity: parsing done\n");
> cap_parsing_done = true;
> schedule_work(&parsing_done_work);
> }
> @@ -208,8 +218,38 @@ static struct notifier_block init_cpu_capacity_notifier = {
> .notifier_call = init_cpu_capacity_callback,
> };
>
> +static void set_freq_scale(unsigned int cpu, unsigned long freq)
> +{
> + unsigned long max = per_cpu(max_freq, cpu);
> +
> + if (!max)
> + return;
> +
> + per_cpu(freq_scale, cpu) = (freq << SCHED_CAPACITY_SHIFT) / max;
> +}
> +
> +static int set_freq_scale_callback(struct notifier_block *nb,
> + unsigned long val,
> + void *data)
> +{
> + struct cpufreq_freqs *freq = data;
> +
> + switch (val) {
> + case CPUFREQ_PRECHANGE:
> + set_freq_scale(freq->cpu, freq->new);
Any specific reason on why are we doing this from PRECHANGE and
not POSTCHANGE ? i.e. we are doing this before the frequency is
really updated.
> + }
> +
> + return 0;
> +}
> +
> +static struct notifier_block set_freq_scale_notifier = {
> + .notifier_call = set_freq_scale_callback,
> +};
> +
> static int __init register_cpufreq_notifier(void)
> {
> + int ret;
> +
> /*
> * on ACPI-based systems we need to use the default cpu capacity
> * until we have the necessary code to parse the cpu capacity, so
> @@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void)
>
> cpumask_copy(cpus_to_visit, cpu_possible_mask);
>
> - return cpufreq_register_notifier(&init_cpu_capacity_notifier,
> - CPUFREQ_POLICY_NOTIFIER);
> + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
> + CPUFREQ_POLICY_NOTIFIER);
Wanted to make sure that we all understand the constraints this is going to add
for the ARM64 platforms.
With the introduction of this transition notifier, we would not be able to use
the fast-switch path in the schedutil governor. I am not sure if there are any
ARM platforms that can actually use the fast-switch path in future or not
though. The requirement of fast-switch path is that the freq can be changed
without sleeping in the hot-path.
So, will we ever want fast-switching for ARM platforms ?
--
viresh
On 06/19/2017 11:17 PM, Viresh Kumar wrote:
> On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
> <[email protected]> wrote:
>
>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
>
>> static int __init register_cpufreq_notifier(void)
>> {
>> + int ret;
>> +
>> /*
>> * on ACPI-based systems we need to use the default cpu capacity
>> * until we have the necessary code to parse the cpu capacity, so
>> @@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void)
>>
>> cpumask_copy(cpus_to_visit, cpu_possible_mask);
>>
>> - return cpufreq_register_notifier(&init_cpu_capacity_notifier,
>> - CPUFREQ_POLICY_NOTIFIER);
>> + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
>> + CPUFREQ_POLICY_NOTIFIER);
>
> Wanted to make sure that we all understand the constraints this is going to add
> for the ARM64 platforms.
>
> With the introduction of this transition notifier, we would not be able to use
> the fast-switch path in the schedutil governor. I am not sure if there are any
> ARM platforms that can actually use the fast-switch path in future or not
> though. The requirement of fast-switch path is that the freq can be changed
> without sleeping in the hot-path.
>
> So, will we ever want fast-switching for ARM platforms ?
>
I don't think we should go down a path that'll prevent ARM platform from
switching over to fast-switching in the future.
Having said that, I'm not sure I fully agree with the decision to
completely disable notifiers in the fast-switching case. How many of the
current users of notifiers truly need support for sleeping in the
notifier? Why not make all the transition notifiers atomic? Or at least
add atomic transition notifiers that can be registered for separately if
the client doesn't need the ability to sleep?
Most of the clients don't seem like ones that'll need to sleep.
There are a bunch of generic off-tree drivers (can't upstream them yet
because it depends on the bus scaling framework) that also depend on
CPUfreq transition notifiers that are going to stop working if fast
switching becomes available in the future. So, this decision to disallow
transition notifiers is painful for other reasons too.
-Saravana
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 20-06-17, 17:31, Saravana Kannan wrote:
> On 06/19/2017 11:17 PM, Viresh Kumar wrote:
> >On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
> ><[email protected]> wrote:
> >
> >>diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> >
> >> static int __init register_cpufreq_notifier(void)
> >> {
> >>+ int ret;
> >>+
> >> /*
> >> * on ACPI-based systems we need to use the default cpu capacity
> >> * until we have the necessary code to parse the cpu capacity, so
> >>@@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void)
> >>
> >> cpumask_copy(cpus_to_visit, cpu_possible_mask);
> >>
> >>- return cpufreq_register_notifier(&init_cpu_capacity_notifier,
> >>- CPUFREQ_POLICY_NOTIFIER);
> >>+ ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
> >>+ CPUFREQ_POLICY_NOTIFIER);
> >
> >Wanted to make sure that we all understand the constraints this is going to add
> >for the ARM64 platforms.
> >
> >With the introduction of this transition notifier, we would not be able to use
> >the fast-switch path in the schedutil governor. I am not sure if there are any
> >ARM platforms that can actually use the fast-switch path in future or not
> >though. The requirement of fast-switch path is that the freq can be changed
> >without sleeping in the hot-path.
> >
> >So, will we ever want fast-switching for ARM platforms ?
> >
>
> I don't think we should go down a path that'll prevent ARM platform from
> switching over to fast-switching in the future.
Yeah, that's why brought attention to this stuff.
I think this patch doesn't really need to go down the notifiers way.
We can do something like this in the implementation of
topology_get_freq_scale():
return (policy->cur << SCHED_CAPACITY_SHIFT) / max;
Though, we would be required to take care of policy structure in this
case somehow.
> Having said that, I'm not sure I fully agree with the decision to completely
> disable notifiers in the fast-switching case. How many of the current users
> of notifiers truly need support for sleeping in the notifier?
Its not just about sleeping here. We do not wish to call too much
stuff from scheduler hot path. Even if it doesn't sleep.
> Why not make
> all the transition notifiers atomic? Or at least add atomic transition
> notifiers that can be registered for separately if the client doesn't need
> the ability to sleep?
>
> Most of the clients don't seem like ones that'll need to sleep.
Only if the scheduler maintainers agree to getting these notifiers
called from hot path, which I don't think is going to happen.
> There are a bunch of generic off-tree drivers (can't upstream them yet
> because it depends on the bus scaling framework) that also depend on CPUfreq
> transition notifiers that are going to stop working if fast switching
> becomes available in the future. So, this decision to disallow transition
> notifiers is painful for other reasons too.
I think its kind of fine to work without fast switch in those cases,
as we are anyway ready to call notifiers which may end up taking any
amount of time.
This case was special as it is affecting entire arch here and so I
pointed it out.
--
viresh
On 20/06/17 07:17, Viresh Kumar wrote:
> On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
> <[email protected]> wrote:
>
>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
>
>> static int
>> init_cpu_capacity_callback(struct notifier_block *nb,
>> @@ -185,6 +192,7 @@ init_cpu_capacity_callback(struct notifier_block *nb,
>> cpus_to_visit,
>> policy->related_cpus);
>> for_each_cpu(cpu, policy->related_cpus) {
>> + per_cpu(max_freq, cpu) = policy->cpuinfo.max_freq;
>
> I am not sure about this but why shouldn't we use policy->max here ?
> As that is the
> max, we can set the frequency to right now.
>
No, frequency invariance is defined by:
current_freq(cpu) << SCHED_CAPACITY_SHIFT / max_supported_freq(cpu)
We don't want to scale against a value which might be restricted e.g.
by thermal capping.
>> if (cap_parsing_failed)
>> continue;
>> raw_capacity[cpu] = topology_get_cpu_scale(NULL, cpu) *
[...]
>> +static int set_freq_scale_callback(struct notifier_block *nb,
>> + unsigned long val,
>> + void *data)
>> +{
>> + struct cpufreq_freqs *freq = data;
>> +
>> + switch (val) {
>> + case CPUFREQ_PRECHANGE:
>> + set_freq_scale(freq->cpu, freq->new);
>
> Any specific reason on why are we doing this from PRECHANGE and
> not POSTCHANGE ? i.e. we are doing this before the frequency is
> really updated.
Not really. In case I get a CPUFREQ_POSTCHANGE all the time the
frequency actually changed I can switch to CPUFREQ_POSTCHANGE.
[...]
>> @@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void)
>>
>> cpumask_copy(cpus_to_visit, cpu_possible_mask);
>>
>> - return cpufreq_register_notifier(&init_cpu_capacity_notifier,
>> - CPUFREQ_POLICY_NOTIFIER);
>> + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
>> + CPUFREQ_POLICY_NOTIFIER);
>
> Wanted to make sure that we all understand the constraints this is going to add
> for the ARM64 platforms.
>
> With the introduction of this transition notifier, we would not be able to use
> the fast-switch path in the schedutil governor. I am not sure if there are any
> ARM platforms that can actually use the fast-switch path in future or not
> though. The requirement of fast-switch path is that the freq can be changed
> without sleeping in the hot-path.
That's a good point. The cpufreq transition notifier based Frequency
Invariance Engine (FIE) can only work if none of the cpufreq policies
support fast frequency switching.
What about we still enable cpufreq transition notifier based FIE for
systems where this is true. This will cover 100% of all arm/arm64
systems today.
In case one day we have a cpufreq driver which allows fast frequency
switching we would need a FIE based on something else than cpufreq
transition notifier. Maybe based on performance counters (something
similar to x86 APERF/MPERF) or cpufreq core could provide a function
which provides the avg frequency value.
I could make the current implementation more future-proof by only
using the notifier based FIE in case all policies use slow frequency
switching:
>From afe64b5c0606cad4304b77fc5cff819d3083a88d Mon Sep 17 00:00:00 2001
From: Dietmar Eggemann <[email protected]>
Date: Wed, 21 Jun 2017 14:53:26 +0100
Subject: [PATCH] drivers base/arch_topology: enable cpufreq transistion
notifier based FIE only for slow frequency switching
Fast frequency switching is incompatible with cpufreq transition
notifiers.
Enable the cpufreq transition notifier based Frequency Invariance Engine
(FIE) only in case there are no cpufreq policies able to use fast
frequency switching.
Currently there are no cpufreq drivers for arm/arm64 which support fast
frequency switching. In case such a driver will appear the FEI
topology_get_freq_scale() has to be extended to provide frequency
invariance based on something else than cpufreq transition notifiers,
e.g. performance counters.
Signed-off-by: Dietmar Eggemann <[email protected]>
---
drivers/base/arch_topology.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index c2539dc584d5..bd14c5e81f63 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -171,6 +171,7 @@ static bool cap_parsing_done;
static void parsing_done_workfn(struct work_struct *work);
static DECLARE_WORK(parsing_done_work, parsing_done_workfn);
static DEFINE_PER_CPU(unsigned long, max_freq);
+static bool enable_freq_inv = true;
static int
init_cpu_capacity_callback(struct notifier_block *nb,
@@ -199,6 +200,8 @@ init_cpu_capacity_callback(struct notifier_block *nb,
policy->cpuinfo.max_freq / 1000UL;
capacity_scale = max(raw_capacity[cpu], capacity_scale);
}
+ if (policy->fast_switch_possible)
+ enable_freq_inv = false;
if (cpumask_empty(cpus_to_visit)) {
if (!cap_parsing_failed) {
topology_normalize_cpu_scale();
@@ -268,21 +271,23 @@ static int __init register_cpufreq_notifier(void)
ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
CPUFREQ_POLICY_NOTIFIER);
- if (ret) {
+ if (ret)
free_cpumask_var(cpus_to_visit);
- return ret;
- }
- return cpufreq_register_notifier(&set_freq_scale_notifier,
- CPUFREQ_TRANSITION_NOTIFIER);
+ return ret;
}
core_initcall(register_cpufreq_notifier);
static void parsing_done_workfn(struct work_struct *work)
{
+
free_cpumask_var(cpus_to_visit);
cpufreq_unregister_notifier(&init_cpu_capacity_notifier,
CPUFREQ_POLICY_NOTIFIER);
+
+ if (enable_freq_inv)
+ cpufreq_register_notifier(&set_freq_scale_notifier,
+ CPUFREQ_TRANSITION_NOTIFIER);
}
#else
--
2.11.0
On 14/06/17 14:08, Vincent Guittot wrote:
> On 14 June 2017 at 09:55, Dietmar Eggemann <[email protected]> wrote:
>>
>> On 06/12/2017 04:27 PM, Vincent Guittot wrote:
>>> On 8 June 2017 at 09:55, Dietmar Eggemann <[email protected]> wrote:
[...]
>>
>> Yes, we should free cpus_to_visit if the policy notifier registration
>> fails. But IMHO also, once the parsing of the capacity-dmips-mhz property
>> is done. free cpus_to_visit is only used in the notifier call
>> init_cpu_capacity_callback() after being allocated and initialized in
>> register_cpufreq_notifier().
>>
>> We could add something like this as the first patch of this set. Only
>> mildly tested on Juno. Juri, what do you think?
>>
>> Author: Dietmar Eggemann <[email protected]>
>> Date: Tue Jun 13 23:21:59 2017 +0100
>>
>> drivers base/arch_topology: free cpumask cpus_to_visit
>>
>> Free cpumask cpus_to_visit in case registering
>> init_cpu_capacity_notifier has failed or the parsing of the cpu
>> capacity-dmips-mhz property is done. The cpumask cpus_to_visit is
>> only used inside the notifier call init_cpu_capacity_callback.
>>
>> Reported-by: Vincent Guittot <[email protected]>
>> Signed-off-by: Dietmar Eggemann <[email protected]>
>
> your proposal for freeing cpus_to_visit looks good for me
>
> Acked-by: Vincent Guittot <[email protected]>
Thanks.
[...]
>> IMHO, that's not necessary.
>>
>> The transition notifier works completely independent from the policy
>> notifier. In case the latter gets registered correctly and the registration
>> of the former fails, the notifier call of the policy notifier still parses
>> the capacity-dmips-mhz property information and sets per_cpu(max_freq, cpu).
>>
>> The notifier call set_freq_scale_callback() of the transition notifier will
>> not be called so that frequency invariance always returns
>> SCHED_CAPACITY_SCALE.
>>
>> After the policy notifier has finished its work, it schedules
>> parsing_done_work() in which it gets unregistered.
>
> Ok so IIUC, the transition notifier is somehow optional and we still
> have the cpu invariance.
> In this case, you should not return the error code of
> cpufreq_register_notifier(&set_freq_scale_notifier,
> CPUFREQ_TRANSITION_NOTIFIER) as the error code of the
> register_cpufreq_notifier function.
> you should better print a warning like " failed to init frequency
> invariance" and return 0 for register_cpufreq_notifier()
Makes sense. Will change this.
[...]
On Wed, Jun 21, 2017 at 11:07:35AM +0530, Viresh Kumar wrote:
> On 20-06-17, 17:31, Saravana Kannan wrote:
> > On 06/19/2017 11:17 PM, Viresh Kumar wrote:
> > >On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
> > ><[email protected]> wrote:
> > >
> > >>diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> > >
> > >> static int __init register_cpufreq_notifier(void)
> > >> {
> > >>+ int ret;
> > >>+
> > >> /*
> > >> * on ACPI-based systems we need to use the default cpu capacity
> > >> * until we have the necessary code to parse the cpu capacity, so
> > >>@@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void)
> > >>
> > >> cpumask_copy(cpus_to_visit, cpu_possible_mask);
> > >>
> > >>- return cpufreq_register_notifier(&init_cpu_capacity_notifier,
> > >>- CPUFREQ_POLICY_NOTIFIER);
> > >>+ ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
> > >>+ CPUFREQ_POLICY_NOTIFIER);
> > >
> > >Wanted to make sure that we all understand the constraints this is going to add
> > >for the ARM64 platforms.
> > >
> > >With the introduction of this transition notifier, we would not be able to use
> > >the fast-switch path in the schedutil governor. I am not sure if there are any
> > >ARM platforms that can actually use the fast-switch path in future or not
> > >though. The requirement of fast-switch path is that the freq can be changed
> > >without sleeping in the hot-path.
> > >
> > >So, will we ever want fast-switching for ARM platforms ?
I hope that one day we will have such platforms.
> > >
> >
> > I don't think we should go down a path that'll prevent ARM platform from
> > switching over to fast-switching in the future.
>
> Yeah, that's why brought attention to this stuff.
It is true that this patch relies on the notifiers, but I don't see how
that prevents us from adding a non-notifier based solution for
fast-switch enabled platforms later?
>
> I think this patch doesn't really need to go down the notifiers way.
>
> We can do something like this in the implementation of
> topology_get_freq_scale():
>
> return (policy->cur << SCHED_CAPACITY_SHIFT) / max;
>
> Though, we would be required to take care of policy structure in this
> case somehow.
This is exactly what this patch implements. Unfortunately we can't be
sure that there is a valid policy data structure where we can read the
information from. Isn't the policy protected by a lock as well?
I don't quite see how you would solve that problem without having some
cached version of the scaling factor that is safe to read without
locking and is always available, even before cpufreq has come up.
Another thing is that I don't think a transition notifier based solution
or any other solution based on the cur/max ratio is really the right way
to go for fast-switching platforms. If we can do very frequent frequency
switching it makes less sense to use the current ratio whenever we
update the PELT averages as the frequency might have changed multiple
times since the last update. So it would make more sense to have an
average ratio instead.
If the platform has HW counters (e.g. APERF/MPERF) that can provide the
ratio then we should of course use those, if not, one solution could be
to let cpufreq track the average frequency for each cpu over a suitable
time window (around one sched period I think). It should be fairly low
overhead to maintain. In the topology driver, we would then choose
whether the scaling factor is provided by the cpufreq average frequency
ratio or the current transition notifier based approach based on the
capabilities of the platform.
Morten
On 21/06/17 01:31, Saravana Kannan wrote:
> On 06/19/2017 11:17 PM, Viresh Kumar wrote:
>> On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
>> <[email protected]> wrote:
>>
>>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
>>
>>> static int __init register_cpufreq_notifier(void)
>>> {
>>> + int ret;
>>> +
>>> /*
>>> * on ACPI-based systems we need to use the default cpu
>>> capacity
>>> * until we have the necessary code to parse the cpu
>>> capacity, so
>>> @@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void)
>>>
>>> cpumask_copy(cpus_to_visit, cpu_possible_mask);
>>>
>>> - return cpufreq_register_notifier(&init_cpu_capacity_notifier,
>>> - CPUFREQ_POLICY_NOTIFIER);
>>> + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
>>> + CPUFREQ_POLICY_NOTIFIER);
>>
>> Wanted to make sure that we all understand the constraints this is
>> going to add
>> for the ARM64 platforms.
>>
>> With the introduction of this transition notifier, we would not be
>> able to use
>> the fast-switch path in the schedutil governor. I am not sure if there
>> are any
>> ARM platforms that can actually use the fast-switch path in future or not
>> though. The requirement of fast-switch path is that the freq can be
>> changed
>> without sleeping in the hot-path.
>>
>> So, will we ever want fast-switching for ARM platforms ?
>>
>
> I don't think we should go down a path that'll prevent ARM platform from
> switching over to fast-switching in the future.
Understood. But IMHO implementing a cpufreq transition notifier based
Frequency Invariance Engine (FIE) which provides frequency-invariant
accounting for 100% of today's arm/arm64 system is legitimate.
Like I said in the other email in this thread today, I can make sure
that I only register the cpufreq transition notifier if none of the
policies support fast frequency switching. In this case people can use
mainline to experiment with cpufreq drivers supporting fast frequency
switching (without FIE support).
> Having said that, I'm not sure I fully agree with the decision to
> completely disable notifiers in the fast-switching case. How many of the
> current users of notifiers truly need support for sleeping in the
> notifier? Why not make all the transition notifiers atomic? Or at least
> add atomic transition notifiers that can be registered for separately if
> the client doesn't need the ability to sleep?
IMHO, that's a different construction side inside the cpufreq framework.
Patches which introduced the fast frequency switching support in cpufreq
clearly state that "... fast frequency switching is inherently
incompatible with cpufreq transition notifiers ...".
If we can get rid of this restriction, the cpufreq transition notifier
based FIE implementation could stay. Otherwise we need a FIE for systems
with fast frequency switching based on something else, e.g. performance
counters.
> Most of the clients don't seem like ones that'll need to sleep.
>
> There are a bunch of generic off-tree drivers (can't upstream them yet
> because it depends on the bus scaling framework) that also depend on
> CPUfreq transition notifiers that are going to stop working if fast
> switching becomes available in the future. So, this decision to disallow
> transition notifiers is painful for other reasons too.
Falls into the same bucket for me ... you have a requirement against the
cpufreq framework to let cpufreq transition notifier work for fast
frequency switching drivers.
[...]
On 21-06-17, 17:38, Dietmar Eggemann wrote:
> On 20/06/17 07:17, Viresh Kumar wrote:
> > Any specific reason on why are we doing this from PRECHANGE and
> > not POSTCHANGE ? i.e. we are doing this before the frequency is
> > really updated.
>
> Not really. In case I get a CPUFREQ_POSTCHANGE all the time the
> frequency actually changed I can switch to CPUFREQ_POSTCHANGE.
Yes, you should always get that. And its not right to do any such
change in PRECHANGE notifier as we may fail to change the frequency as
well..
> > Wanted to make sure that we all understand the constraints this is going to add
> > for the ARM64 platforms.
> >
> > With the introduction of this transition notifier, we would not be able to use
> > the fast-switch path in the schedutil governor. I am not sure if there are any
> > ARM platforms that can actually use the fast-switch path in future or not
> > though. The requirement of fast-switch path is that the freq can be changed
> > without sleeping in the hot-path.
>
> That's a good point. The cpufreq transition notifier based Frequency
> Invariance Engine (FIE) can only work if none of the cpufreq policies
> support fast frequency switching.
At least with the current design, yes.
> What about we still enable cpufreq transition notifier based FIE for
> systems where this is true. This will cover 100% of all arm/arm64
> systems today.
I would suggest having a single solution for everyone if we can.
> In case one day we have a cpufreq driver which allows fast frequency
> switching we would need a FIE based on something else than cpufreq
> transition notifier. Maybe based on performance counters (something
> similar to x86 APERF/MPERF) or cpufreq core could provide a function
> which provides the avg frequency value.
>
> I could make the current implementation more future-proof by only
> using the notifier based FIE in case all policies use slow frequency
> switching:
>
> >From afe64b5c0606cad4304b77fc5cff819d3083a88d Mon Sep 17 00:00:00 2001
> From: Dietmar Eggemann <[email protected]>
> Date: Wed, 21 Jun 2017 14:53:26 +0100
> Subject: [PATCH] drivers base/arch_topology: enable cpufreq transistion
> notifier based FIE only for slow frequency switching
>
> Fast frequency switching is incompatible with cpufreq transition
> notifiers.
>
> Enable the cpufreq transition notifier based Frequency Invariance Engine
> (FIE) only in case there are no cpufreq policies able to use fast
> frequency switching.
>
> Currently there are no cpufreq drivers for arm/arm64 which support fast
> frequency switching. In case such a driver will appear the FEI
> topology_get_freq_scale() has to be extended to provide frequency
> invariance based on something else than cpufreq transition notifiers,
> e.g. performance counters.
>
> Signed-off-by: Dietmar Eggemann <[email protected]>
> ---
> drivers/base/arch_topology.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index c2539dc584d5..bd14c5e81f63 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -171,6 +171,7 @@ static bool cap_parsing_done;
> static void parsing_done_workfn(struct work_struct *work);
> static DECLARE_WORK(parsing_done_work, parsing_done_workfn);
> static DEFINE_PER_CPU(unsigned long, max_freq);
> +static bool enable_freq_inv = true;
>
> static int
> init_cpu_capacity_callback(struct notifier_block *nb,
> @@ -199,6 +200,8 @@ init_cpu_capacity_callback(struct notifier_block *nb,
> policy->cpuinfo.max_freq / 1000UL;
> capacity_scale = max(raw_capacity[cpu], capacity_scale);
> }
> + if (policy->fast_switch_possible)
> + enable_freq_inv = false;
> if (cpumask_empty(cpus_to_visit)) {
> if (!cap_parsing_failed) {
> topology_normalize_cpu_scale();
> @@ -268,21 +271,23 @@ static int __init register_cpufreq_notifier(void)
> ret = cpufreq_register_notifier(&init_cpu_capacity_notifier,
> CPUFREQ_POLICY_NOTIFIER);
>
> - if (ret) {
> + if (ret)
> free_cpumask_var(cpus_to_visit);
> - return ret;
> - }
>
> - return cpufreq_register_notifier(&set_freq_scale_notifier,
> - CPUFREQ_TRANSITION_NOTIFIER);
> + return ret;
> }
> core_initcall(register_cpufreq_notifier);
>
> static void parsing_done_workfn(struct work_struct *work)
> {
> +
> free_cpumask_var(cpus_to_visit);
> cpufreq_unregister_notifier(&init_cpu_capacity_notifier,
> CPUFREQ_POLICY_NOTIFIER);
> +
> + if (enable_freq_inv)
> + cpufreq_register_notifier(&set_freq_scale_notifier,
> + CPUFREQ_TRANSITION_NOTIFIER);
> }
This may work, but lets see if we can find a way of doing this for
everyone at once.
(I will continue to reply on Morten's email now)..
--
viresh
On 21-06-17, 17:57, Morten Rasmussen wrote:
> It is true that this patch relies on the notifiers, but I don't see how
> that prevents us from adding a non-notifier based solution for
> fast-switch enabled platforms later?
No it doesn't, but I thought it would be better to have a single
solution (if possible) for all the cases here.
> > I think this patch doesn't really need to go down the notifiers way.
> >
> > We can do something like this in the implementation of
> > topology_get_freq_scale():
> >
> > return (policy->cur << SCHED_CAPACITY_SHIFT) / max;
> >
> > Though, we would be required to take care of policy structure in this
> > case somehow.
>
> This is exactly what this patch implements. Unfortunately we can't be
> sure that there is a valid policy data structure where we can read the
> information from.
Actually there is a way around that.
- Revert one of my patches:
commit f9f41e3ef99a ("cpufreq: Remove policy create/remove notifiers")
- Use those notifiers in init_cpu_capacity_callback() instead of
CPUFREQ_NOTIFY and set/reset a local policy pointer.
- And this pointer we can use safely/reliably in
topology_get_freq_scale(). We may need to use RCU read side
protection in topology_get_freq_scale() though, to make sure the
local policy pointer isn't getting updated simultaneously.
- If the policy pointer isn't set, then we can use
SCHED_CAPACITY_SCALE value instead.
> Isn't the policy protected by a lock as well?
There are locks, but you don't need any to read policy->cur.
> Another thing is that I don't think a transition notifier based solution
> or any other solution based on the cur/max ratio is really the right way
> to go for fast-switching platforms. If we can do very frequent frequency
> switching it makes less sense to use the current ratio whenever we
> update the PELT averages as the frequency might have changed multiple
> times since the last update. So it would make more sense to have an
> average ratio instead.
> If the platform has HW counters (e.g. APERF/MPERF) that can provide the
> ratio then we should of course use those, if not, one solution could be
> to let cpufreq track the average frequency for each cpu over a suitable
> time window (around one sched period I think). It should be fairly low
> overhead to maintain. In the topology driver, we would then choose
> whether the scaling factor is provided by the cpufreq average frequency
> ratio or the current transition notifier based approach based on the
> capabilities of the platform.
Hmm, maybe.
--
viresh
On Thu, Jun 22, 2017 at 09:36:43AM +0530, Viresh Kumar wrote:
> On 21-06-17, 17:57, Morten Rasmussen wrote:
> > It is true that this patch relies on the notifiers, but I don't see how
> > that prevents us from adding a non-notifier based solution for
> > fast-switch enabled platforms later?
>
> No it doesn't, but I thought it would be better to have a single
> solution (if possible) for all the cases here.
Right. As I mentioned further down in my reply. There is no single
solution that fits all. Smart platforms with HW counters, like x86,
would want to use those. IIUC, cpufreq has no idea what the true
delivered performance is anyway on those platforms.
>
> > > I think this patch doesn't really need to go down the notifiers way.
> > >
> > > We can do something like this in the implementation of
> > > topology_get_freq_scale():
> > >
> > > return (policy->cur << SCHED_CAPACITY_SHIFT) / max;
> > >
> > > Though, we would be required to take care of policy structure in this
> > > case somehow.
> >
> > This is exactly what this patch implements. Unfortunately we can't be
> > sure that there is a valid policy data structure where we can read the
> > information from.
>
> Actually there is a way around that.
>
> - Revert one of my patches:
> commit f9f41e3ef99a ("cpufreq: Remove policy create/remove notifiers")
>
> - Use those notifiers in init_cpu_capacity_callback() instead of
> CPUFREQ_NOTIFY and set/reset a local policy pointer.
>
> - And this pointer we can use safely/reliably in
> topology_get_freq_scale(). We may need to use RCU read side
> protection in topology_get_freq_scale() though, to make sure the
> local policy pointer isn't getting updated simultaneously.
>
> - If the policy pointer isn't set, then we can use
> SCHED_CAPACITY_SCALE value instead.
IIUC, you are proposing to maintain an RCU protected pointer in the
topology driver to the policy data structure inside cpufreq and keep it
up to date through cpufreq notifiers. So instead of getting notified
when the frequency changes so we can recompute the scaling ratio, we
have to poll the value and recompute the ratio on each access.
If we are modifying cpufreq, why not just make cpufreq responsible for
providing the scaling factor? It seems easier, cleaner, and a lot
less fragile.
>
>
> > Isn't the policy protected by a lock as well?
>
> There are locks, but you don't need any to read policy->cur.
Okay, but you need to rely on notifiers to know when it is valid.
>
> > Another thing is that I don't think a transition notifier based solution
> > or any other solution based on the cur/max ratio is really the right way
> > to go for fast-switching platforms. If we can do very frequent frequency
> > switching it makes less sense to use the current ratio whenever we
> > update the PELT averages as the frequency might have changed multiple
> > times since the last update. So it would make more sense to have an
> > average ratio instead.
>
> > If the platform has HW counters (e.g. APERF/MPERF) that can provide the
> > ratio then we should of course use those, if not, one solution could be
> > to let cpufreq track the average frequency for each cpu over a suitable
> > time window (around one sched period I think). It should be fairly low
> > overhead to maintain. In the topology driver, we would then choose
> > whether the scaling factor is provided by the cpufreq average frequency
> > ratio or the current transition notifier based approach based on the
> > capabilities of the platform.
>
> Hmm, maybe.
You said you wanted a solution that works for fast-switch enabled
platforms ;-)
The cur/max ratio isn't sufficient for those. PeterZ has already
proposed to use APERF/MPERF for x86 to use the average frequency for
PELT updates. I think other fast-switch platforms would want something
similar, as it makes much more sense.
Morten
On 08/06/17 08:55, Dietmar Eggemann wrote:
> Implements an arch-specific frequency-scaling function
> topology_get_freq_scale() which provides the following frequency
> scaling factor:
>
> current_freq(cpu) << SCHED_CAPACITY_SHIFT / max_supported_freq(cpu)
[...]
Frequency and cpu-invariant load tracking are part of the task
schedulers hot path:
e.g.:
__update_load_avg_se()-> ___update_load_avg() -> accumulate_sum()
That's why function calls should be avoided here.
I would like to fold the following changes into patch 2/6 in v2:
commit 1397770fe47ce5d34511e7062bd3a8bc96a74590
Author: Dietmar Eggemann <[email protected]>
Date: Sat Jun 24 16:46:45 2017 +0100
drivers base/arch_topology: eliminate function call for cpu and
frequency-invariant accounting
topology_get_cpu_scale() and topology_get_freq_scale() are the arm/arm64
architecture specific implementations to provide cpu-invariant and
frequency-invariant accounting support up to the task scheduler.
Define them as static inline functions to allow cpu-invariant and
frequency-invariant accounting to happen without an extra function call
involved.
Test results on JUNO (arm64):
root@juno:~# grep
"__update_load_avg_\|update_group_capacity\|topology_get"
available_filter_functions > set_ftrace_filter
root@juno:~# echo function_graph > current_tracer
root@juno:~# cat trace | tail -50
w/ this patch:
...
3) 0.700 us | __update_load_avg_se.isra.5();
...
3) 0.750 us | __update_load_avg_cfs_rq();
...
3) 0.780 us | update_group_capacity();
...
w/o this patch:
4) | __update_load_avg_cfs_rq() {
4) 0.380 us | topology_get_freq_scale();
4) 0.340 us | topology_get_cpu_scale();
4) 6.420 us | }
...
4) | __update_load_avg_se.isra.4() {
4) 0.300 us | topology_get_freq_scale();
4) 0.260 us | topology_get_cpu_scale();
4) 5.800 us | }
...
4) | update_group_capacity() {
4) 0.260 us | topology_get_cpu_scale();
4) 3.540 us | }
...
So these extra function calls cost ~2.5us each (on Cortex A53,
cpu0,3,4,5). Since this happens in the task scheduler hot-path,
they have to be avoided.
Signed-off-by: Dietmar Eggemann <[email protected]>
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index d7e130c268fb..8dfa4c3dbfc2 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -23,18 +23,8 @@
#include <linux/sched/topology.h>
static DEFINE_MUTEX(cpu_scale_mutex);
-static DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
-static DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE;
-
-unsigned long topology_get_cpu_scale(struct sched_domain *sd, int cpu)
-{
- return per_cpu(cpu_scale, cpu);
-}
-
-unsigned long topology_get_freq_scale(struct sched_domain *sd, int cpu)
-{
- return per_cpu(freq_scale, cpu);
-}
+DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
+DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE;
void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity)
{
diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
index 3fb4d8ccb179..cf22631e6765 100644
--- a/include/linux/arch_topology.h
+++ b/include/linux/arch_topology.h
@@ -9,10 +9,21 @@ void topology_normalize_cpu_scale(void);
struct device_node;
int topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu);
+DECLARE_PER_CPU(unsigned long, cpu_scale);
+DECLARE_PER_CPU(unsigned long, freq_scale);
+
struct sched_domain;
-unsigned long topology_get_cpu_scale(struct sched_domain *sd, int cpu);
+static inline
+unsigned long topology_get_cpu_scale(struct sched_domain *sd, int cpu)
+{
+ return per_cpu(cpu_scale, cpu);
+}
-unsigned long topology_get_freq_scale(struct sched_domain *sd, int cpu);
+static inline
+unsigned long topology_get_freq_scale(struct sched_domain *sd, int cpu)
+{
+ return per_cpu(freq_scale, cpu);
+}
void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity);
[...]