[PATCH 0/6] arm, arm64: frequency- and cpu-invariant accounting support for task scheduler

[PATCH 0/6] arm, arm64: frequency- and cpu-invariant accounting support for task scheduler

[Dietmar Eggemann]

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

[PATCH 0/6] arm, arm64: frequency- and cpu-invariant accounting support for task scheduler

[Dietmar Eggemann]

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

[PATCH 1/6] drivers base/arch_topology: prepare cpufreq policy notifier for frequency-invariant load-tracking support

[Dietmar Eggemann]

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 <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Juri Lelli <juri.lelli@arm.com>
Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
---
 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

[PATCH 1/6] drivers base/arch_topology: prepare cpufreq policy notifier for frequency-invariant load-tracking support

[Dietmar Eggemann]

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 <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Juri Lelli <juri.lelli@arm.com>
Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
---
 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

[PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Dietmar Eggemann]

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 <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Juri Lelli <juri.lelli@arm.com>
Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
---
 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

[PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Dietmar Eggemann]

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 <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Juri Lelli <juri.lelli@arm.com>
Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
---
 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

[PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Dietmar Eggemann]

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 <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Juri Lelli <juri.lelli@arm.com>
Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
---
 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

[PATCH 3/6] arm: wire frequency-invariant accounting support up to the task scheduler

[Dietmar Eggemann]

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 <linux@arm.linux.org.uk>
Cc: Juri Lelli <juri.lelli@arm.com>
Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
---
 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

[PATCH 3/6] arm: wire frequency-invariant accounting support up to the task scheduler

[Dietmar Eggemann]

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 <linux@arm.linux.org.uk>
Cc: Juri Lelli <juri.lelli@arm.com>
Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
---
 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

[PATCH 4/6] arm: wire cpu-invariant accounting support up to the task scheduler

[Dietmar Eggemann]

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 <linux@arm.linux.org.uk>
Cc: Juri Lelli <juri.lelli@arm.com>
Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
---
 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

[PATCH 4/6] arm: wire cpu-invariant accounting support up to the task scheduler

[Dietmar Eggemann]

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 <linux@arm.linux.org.uk>
Cc: Juri Lelli <juri.lelli@arm.com>
Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
---
 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

[PATCH 5/6] arm64: wire frequency-invariant accounting support up to the task scheduler

[Dietmar Eggemann]

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 <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Juri Lelli <juri.lelli@arm.com>
Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
---
 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

[PATCH 5/6] arm64: wire frequency-invariant accounting support up to the task scheduler

[Dietmar Eggemann]

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 <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Juri Lelli <juri.lelli@arm.com>
Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
---
 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

[PATCH 6/6] arm64: wire cpu-invariant accounting support up to the task scheduler

[Dietmar Eggemann]

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 <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Juri Lelli <juri.lelli@arm.com>
Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
---
 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

[PATCH 6/6] arm64: wire cpu-invariant accounting support up to the task scheduler

[Dietmar Eggemann]

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 <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Juri Lelli <juri.lelli@arm.com>
Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
---
 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

Re: [PATCH 0/6] arm, arm64: frequency- and cpu-invariant accounting support for task scheduler

[Juri Lelli]

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 <juri.lelli@arm.com>

to it.

Best,

- Juri

[PATCH 0/6] arm, arm64: frequency- and cpu-invariant accounting support for task scheduler

[Juri Lelli]

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 <juri.lelli@arm.com>

to it.

Best,

- Juri

Re: [PATCH 0/6] arm, arm64: frequency- and cpu-invariant accounting support for task scheduler

[Juri Lelli]

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 <juri.lelli@arm.com>
> 
> to it.
> 
> Best,
> 
> - Juri

[PATCH 0/6] arm, arm64: frequency- and cpu-invariant accounting support for task scheduler

[Juri Lelli]

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 <juri.lelli@arm.com>
> 
> to it.
> 
> Best,
> 
> - Juri

Re: [PATCH 5/6] arm64: wire frequency-invariant accounting support up to the task scheduler

[Catalin Marinas]

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 <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Juri Lelli <juri.lelli@arm.com>
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> ---
>  arch/arm64/include/asm/topology.h | 5 +++++
>  arch/arm64/kernel/topology.c      | 1 -
>  2 files changed, 5 insertions(+), 1 deletion(-)

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH 5/6] arm64: wire frequency-invariant accounting support up to the task scheduler

[Catalin Marinas]

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 <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Juri Lelli <juri.lelli@arm.com>
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> ---
>  arch/arm64/include/asm/topology.h | 5 +++++
>  arch/arm64/kernel/topology.c      | 1 -
>  2 files changed, 5 insertions(+), 1 deletion(-)

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH 6/6] arm64: wire cpu-invariant accounting support up to the task scheduler

[Catalin Marinas]

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 <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Juri Lelli <juri.lelli@arm.com>
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> ---
>  arch/arm64/include/asm/topology.h | 3 +++
>  1 file changed, 3 insertions(+)

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH 6/6] arm64: wire cpu-invariant accounting support up to the task scheduler

[Catalin Marinas]

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 <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Juri Lelli <juri.lelli@arm.com>
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> ---
>  arch/arm64/include/asm/topology.h | 3 +++
>  1 file changed, 3 insertions(+)

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Vincent Guittot]

On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Juri Lelli <juri.lelli@arm.com>
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> ---
>  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
>

[PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Vincent Guittot]

On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Juri Lelli <juri.lelli@arm.com>
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> ---
>  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
>

Re: [PATCH 3/6] arm: wire frequency-invariant accounting support up to the task scheduler

[Vincent Guittot]

On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <linux@arm.linux.org.uk>
> Cc: Juri Lelli <juri.lelli@arm.com>
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

Acked-by: Vincent Guittot <vincent.guittot@linaro.org>

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

[PATCH 3/6] arm: wire frequency-invariant accounting support up to the task scheduler

[Vincent Guittot]

On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <linux@arm.linux.org.uk>
> Cc: Juri Lelli <juri.lelli@arm.com>
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

Acked-by: Vincent Guittot <vincent.guittot@linaro.org>

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

Re: [PATCH 4/6] arm: wire cpu-invariant accounting support up to the task scheduler

[Vincent Guittot]

On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <linux@arm.linux.org.uk>
> Cc: Juri Lelli <juri.lelli@arm.com>
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

Acked-by: Vincent Guittot <vincent.guittot@linaro.org>

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

[PATCH 4/6] arm: wire cpu-invariant accounting support up to the task scheduler

[Vincent Guittot]

On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <linux@arm.linux.org.uk>
> Cc: Juri Lelli <juri.lelli@arm.com>
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

Acked-by: Vincent Guittot <vincent.guittot@linaro.org>

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

Re: [PATCH 5/6] arm64: wire frequency-invariant accounting support up to the task scheduler

[Vincent Guittot]

On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Juri Lelli <juri.lelli@arm.com>
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

Acked-by: Vincent Guittot <vincent.guittot@linaro.org>

[PATCH 5/6] arm64: wire frequency-invariant accounting support up to the task scheduler

[Vincent Guittot]

On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Juri Lelli <juri.lelli@arm.com>
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

Acked-by: Vincent Guittot <vincent.guittot@linaro.org>

Re: [PATCH 6/6] arm64: wire cpu-invariant accounting support up to the task scheduler

[Vincent Guittot]

On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Juri Lelli <juri.lelli@arm.com>
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

Acked-by: Vincent Guittot <vincent.guittot@linaro.org>

[PATCH 6/6] arm64: wire cpu-invariant accounting support up to the task scheduler

[Vincent Guittot]

On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Juri Lelli <juri.lelli@arm.com>
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

Acked-by: Vincent Guittot <vincent.guittot@linaro.org>

Re: [PATCH 1/6] drivers base/arch_topology: prepare cpufreq policy notifier for frequency-invariant load-tracking support

[Vincent Guittot]

On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Juri Lelli <juri.lelli@arm.com>
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

Acked-by: Vincent Guittot <vincent.guittot@linaro.org>

[PATCH 1/6] drivers base/arch_topology: prepare cpufreq policy notifier for frequency-invariant load-tracking support

[Vincent Guittot]

On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Juri Lelli <juri.lelli@arm.com>
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

Acked-by: Vincent Guittot <vincent.guittot@linaro.org>

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Dietmar Eggemann]

On 06/12/2017 04:27 PM, Vincent Guittot wrote:
> On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <dietmar.eggemann@arm.com>
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 <vincent.guittot@linaro.org>
    Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

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.

[...]

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Dietmar Eggemann]

On 06/12/2017 04:27 PM, Vincent Guittot wrote:
> On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <dietmar.eggemann@arm.com>
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 <vincent.guittot@linaro.org>
    Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

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.

[...]

[PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Dietmar Eggemann]

On 06/12/2017 04:27 PM, Vincent Guittot wrote:
> On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <dietmar.eggemann@arm.com>
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 <vincent.guittot@linaro.org>
    Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

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.

[...]

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Vincent Guittot]

On 14 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
>
> On 06/12/2017 04:27 PM, Vincent Guittot wrote:
> > On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <dietmar.eggemann@arm.com>
> 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 <vincent.guittot@linaro.org>
>     Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

your proposal for freeing cpus_to_visit looks good for me

Acked-by: Vincent Guittot <vincent.guittot@linaro.org>

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

>
> [...]

[PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Vincent Guittot]

On 14 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
>
> On 06/12/2017 04:27 PM, Vincent Guittot wrote:
> > On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <dietmar.eggemann@arm.com>
> 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 <vincent.guittot@linaro.org>
>     Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

your proposal for freeing cpus_to_visit looks good for me

Acked-by: Vincent Guittot <vincent.guittot@linaro.org>

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

>
> [...]

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Juri Lelli]

Hi,

On 14/06/17 15:08, Vincent Guittot wrote:
> On 14 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
> >
> > On 06/12/2017 04:27 PM, Vincent Guittot wrote:
> > > On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <dietmar.eggemann@arm.com>
> > 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 <vincent.guittot@linaro.org>
> >     Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> 
> your proposal for freeing cpus_to_visit looks good for me
> 
> Acked-by: Vincent Guittot <vincent.guittot@linaro.org>
> 

Yep, looks good to me too. Thanks for fixing!

Best,

- Juri

[PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Juri Lelli]

Hi,

On 14/06/17 15:08, Vincent Guittot wrote:
> On 14 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
> >
> > On 06/12/2017 04:27 PM, Vincent Guittot wrote:
> > > On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <dietmar.eggemann@arm.com>
> > 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 <vincent.guittot@linaro.org>
> >     Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> 
> your proposal for freeing cpus_to_visit looks good for me
> 
> Acked-by: Vincent Guittot <vincent.guittot@linaro.org>
> 

Yep, looks good to me too. Thanks for fixing!

Best,

- Juri

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Viresh Kumar]

On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
<dietmar.eggemann@arm.com> 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

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Viresh Kumar]

On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
<dietmar.eggemann@arm.com> 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

[PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Viresh Kumar]

On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
<dietmar.eggemann@arm.com> 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

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Saravana Kannan]

On 06/19/2017 11:17 PM, Viresh Kumar wrote:
> On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
> <dietmar.eggemann@arm.com> 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

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Saravana Kannan]

On 06/19/2017 11:17 PM, Viresh Kumar wrote:
> On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
> <dietmar.eggemann@arm.com> 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

[PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Saravana Kannan]

On 06/19/2017 11:17 PM, Viresh Kumar wrote:
> On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
> <dietmar.eggemann@arm.com> 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

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Viresh Kumar]

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
> ><dietmar.eggemann@arm.com> 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

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Viresh Kumar]

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
> ><dietmar.eggemann@arm.com> 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

[PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Viresh Kumar]

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
> ><dietmar.eggemann@arm.com> 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

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Dietmar Eggemann]

On 20/06/17 07:17, Viresh Kumar wrote:
> On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
> <dietmar.eggemann@arm.com> 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 <dietmar.eggemann@arm.com>
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 <dietmar.eggemann@arm.com>
---
 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

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Dietmar Eggemann]

On 20/06/17 07:17, Viresh Kumar wrote:
> On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
> <dietmar.eggemann@arm.com> 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 <dietmar.eggemann@arm.com>
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 <dietmar.eggemann@arm.com>
---
 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

[PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Dietmar Eggemann]

On 20/06/17 07:17, Viresh Kumar wrote:
> On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann
> <dietmar.eggemann@arm.com> 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 <dietmar.eggemann@arm.com>
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 <dietmar.eggemann@arm.com>
---
 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

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Dietmar Eggemann]

On 14/06/17 14:08, Vincent Guittot wrote:
> On 14 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
>>
>> On 06/12/2017 04:27 PM, Vincent Guittot wrote:
>>> On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <dietmar.eggemann@arm.com>
>> 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 <vincent.guittot@linaro.org>
>>     Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> 
> your proposal for freeing cpus_to_visit looks good for me
> 
> Acked-by: Vincent Guittot <vincent.guittot@linaro.org>

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.

[...]

[PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Dietmar Eggemann]

On 14/06/17 14:08, Vincent Guittot wrote:
> On 14 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
>>
>> On 06/12/2017 04:27 PM, Vincent Guittot wrote:
>>> On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <dietmar.eggemann@arm.com>
>> 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 <vincent.guittot@linaro.org>
>>     Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> 
> your proposal for freeing cpus_to_visit looks good for me
> 
> Acked-by: Vincent Guittot <vincent.guittot@linaro.org>

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.

[...]

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Morten Rasmussen]

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
> > ><dietmar.eggemann@arm.com> 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

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Morten Rasmussen]

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
> > ><dietmar.eggemann@arm.com> 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

[PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Morten Rasmussen]

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
> > ><dietmar.eggemann@arm.com> 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

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Dietmar Eggemann]

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
>> <dietmar.eggemann@arm.com> 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.

[...]

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Dietmar Eggemann]

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
>> <dietmar.eggemann@arm.com> 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.

[...]

[PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Dietmar Eggemann]

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
>> <dietmar.eggemann@arm.com> 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.

[...]

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Viresh Kumar]

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 <dietmar.eggemann@arm.com>
> 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 <dietmar.eggemann@arm.com>
> ---
>  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

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Viresh Kumar]

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 <dietmar.eggemann@arm.com>
> 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 <dietmar.eggemann@arm.com>
> ---
>  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

[PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Viresh Kumar]

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 <dietmar.eggemann@arm.com>
> 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 <dietmar.eggemann@arm.com>
> ---
>  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

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Viresh Kumar]

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

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Viresh Kumar]

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

[PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Viresh Kumar]

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

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Morten Rasmussen]

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

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Morten Rasmussen]

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

[PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Morten Rasmussen]

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

Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Dietmar Eggemann]

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 <dietmar.eggemann@arm.com>
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 <dietmar.eggemann@arm.com>

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

[...]

[PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support

[Dietmar Eggemann]

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 <dietmar.eggemann@arm.com>
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 at juno:~# grep
    "__update_load_avg_\|update_group_capacity\|topology_get"
    available_filter_functions > set_ftrace_filter
    
    root at juno:~# echo function_graph > current_tracer
    
    root at 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 <dietmar.eggemann@arm.com>

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

[...]