[PATCH v5 0/5] cpufreq: improve frequency invariance support

[PATCH v5 0/5] cpufreq: improve frequency invariance support

[Ionela Voinescu]

Hi,

v4->v5
 - I've applied Viresh's remaining suggestion and Acked-by/s
 - v4 can be found at [4]
 - v5 is based on linux-next 20200828

Thank you,
Ionela.

---
v3->v4:
 - addressing Viresh's comments on patches 1/5 and 3/5, and
 - with his Acked-by applied for the rest of the patches;
 - v3 can be found at [3], and
 - this is based on linux-next 20200827.

v2->v3
 - v2 can be found at [2]
 - 1/5 was introduced to check input frequencies to
   arch_set_freq_scale() as recommended by Rafael
 - The previous 2/7 was squashed into 1/7 - now 2/5, with additions to
   the changelog as suggested by Rafael.
 - The previous 3/7 (BL_SWITCHER handling) was dropped to be handled
   in a separate patch. This does not change the current functionality.
 - The previous 4/7 - now 3/5 is simplified as suggested by Viresh.
 - 3/5 - cpufreq_supports_freq_invariance() replaces
   cpufreq_sets_freq_scale(). The meaning chosen for
   cpufreq_supports_freq_invariance() is whether it can set the frequency
   scale factor, not whether arch_set_freq_scale() actually does.
 - 4/5 - Change after Catalin's Ack: The changes to
   arch_set_thermal_pressure() were dropped as they were done in a separate
   patch. Therefore this patch now has a subset of the previous changes
   at 5/7
 - 5/5 - Change after Catalin's Ack:
   s/cpufreq_sets_freq_scale/cpufreq_supports_freq_invariance
 - v3 is based on linux-next 20200814


v1 -> v2:
 - v1 can be found at [1]
 - No cpufreq flags are introduced
 - Previous patches 2/8 and 3/8 were squashed in this series under 1/7,
   to ensure bisection.
 - 2/7 was introduced as a proposal for Viresh's suggestion to use
   policy->cur in the call to arch_set_freq_scale() and is extended to
   support drivers that implement the target() callback as well
 - Additional commit message changes are added to 1/7 and 2/7, to
   clarify that the definition of arch_set_freq_scale() will filter 
   settings of the scale factor, if unwanted
 - 3/7 disables setting of the scale factor for
   CONFIG_BL_SWITCHER, as Dietmar suggested
 - Small change introduced in 4/7 to disable cpufreq-based frequency
   invariance for the users of the default arch_set_freq_scale() call
   which will not actually set a scale factor
 - build issue solved (reported by 0day test)
 - v2 is based on linux-next 20200716
 - all functional tests in v1 were repeated for v2


[1] https://lore.kernel.org/lkml/20200701090751.7543-1-ionela.voinescu@arm.com/
[2] https://lore.kernel.org/lkml/20200722093732.14297-1-ionela.voinescu@arm.com/
[3] https://lore.kernel.org/lkml/20200824210252.27486-1-ionela.voinescu@arm.com/
[4] https://lore.kernel.org/lkml/20200828173303.11939-1-ionela.voinescu@arm.com/

Ionela Voinescu (3):
  arch_topology: validate input frequencies to arch_set_freq_scale()
  cpufreq: move invariance setter calls in cpufreq core
  cpufreq: report whether cpufreq supports Frequency Invariance (FI)

Valentin Schneider (2):
  arch_topology, cpufreq: constify arch_* cpumasks
  arch_topology, arm, arm64: define arch_scale_freq_invariant()

 arch/arm/include/asm/topology.h        |  1 +
 arch/arm64/include/asm/topology.h      |  1 +
 arch/arm64/kernel/topology.c           |  9 ++++++-
 drivers/base/arch_topology.c           | 13 ++++++++--
 drivers/cpufreq/cpufreq-dt.c           | 10 +-------
 drivers/cpufreq/cpufreq.c              | 33 +++++++++++++++++++++++---
 drivers/cpufreq/qcom-cpufreq-hw.c      |  9 +------
 drivers/cpufreq/scmi-cpufreq.c         | 12 ++--------
 drivers/cpufreq/scpi-cpufreq.c         |  6 +----
 drivers/cpufreq/vexpress-spc-cpufreq.c | 12 ++--------
 include/linux/arch_topology.h          |  4 +++-
 include/linux/cpufreq.h                |  8 ++++++-
 12 files changed, 68 insertions(+), 50 deletions(-)


base-commit: b36c969764ab12faebb74711c942fa3e6eaf1e96
-- 
2.17.1

[PATCH v5 1/5] arch_topology: validate input frequencies to arch_set_freq_scale()

[Ionela Voinescu]

The current frequency passed to arch_set_freq_scale() could end up
being 0, signaling an error in setting a new frequency. Also, if the
maximum frequency in 0, this will result in a division by 0 error.

Therefore, validate these input values before using them for the
setting of the frequency scale factor.

Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
---
 drivers/base/arch_topology.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 75f72d684294..42a08ef693ae 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -33,6 +33,9 @@ void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
 	unsigned long scale;
 	int i;
 
+	if (WARN_ON_ONCE(!cur_freq || !max_freq))
+		return;
+
 	/*
 	 * If the use of counters for FIE is enabled, just return as we don't
 	 * want to update the scale factor with information from CPUFREQ.
-- 
2.17.1

[PATCH v5 2/5] cpufreq: move invariance setter calls in cpufreq core

[Ionela Voinescu]

To properly scale its per-entity load-tracking signals, the task scheduler
needs to be given a frequency scale factor, i.e. some image of the current
frequency the CPU is running at. Currently, this scale can be computed
either by using counters (APERF/MPERF on x86, AMU on arm64), or by
piggy-backing on the frequency selection done by cpufreq.

For the latter, drivers have to explicitly set the scale factor
themselves, despite it being purely boiler-plate code: the required
information depends entirely on the kind of frequency switch callback
implemented by the driver, i.e. either of: target_index(), target(),
fast_switch() and setpolicy().

The fitness of those callbacks with regard to driving the Frequency
Invariance Engine (FIE) is studied below:

target_index()
==============
Documentation states that the chosen frequency "must be determined by
freq_table[index].frequency". It isn't clear if it *has* to be that
frequency, or if it can use that frequency value to do some computation
that ultimately leads to a different frequency selection. All drivers
go for the former, while the vexpress-spc-cpufreq has an atypical
implementation which is handled separately.

Therefore, the hook works on the assumption the core can use
freq_table[index].frequency.

target()
=======
This has been flagged as deprecated since:

  commit 9c0ebcf78fde ("cpufreq: Implement light weight ->target_index() routine")

It also doesn't have that many users:

  gx-suspmod.c:439:       .target = cpufreq_gx_target,
  s3c24xx-cpufreq.c:428:  .target = s3c_cpufreq_target,
  intel_pstate.c:2528:    .target = intel_cpufreq_target,
  cppc_cpufreq.c:401:     .target = cppc_cpufreq_set_target,
  cpufreq-nforce2.c:371:  .target = nforce2_target,
  sh-cpufreq.c:163:       .target = sh_cpufreq_target,
  pcc-cpufreq.c:573:      .target = pcc_cpufreq_target,

Similarly to the path taken for target_index() calls in the cpufreq core
during a frequency change, all of the drivers above will mark the end of a
frequency change by a call to cpufreq_freq_transition_end().

Therefore, cpufreq_freq_transition_end() can be used as the location for
the arch_set_freq_scale() call to potentially inform the scheduler of the
frequency change.

This change maintains the previous functionality for the drivers that
implement the target_index() callback, while also adding support for the
few drivers that implement the deprecated target() callback.

fast_switch()
=============
This callback *has* to return the frequency that was selected.

setpolicy()
===========
This callback does not have any designated way of informing what was the
end choice. But there are only two drivers using setpolicy(), and none
of them have current FIE support:

  drivers/cpufreq/longrun.c:281:	.setpolicy	= longrun_set_policy,
  drivers/cpufreq/intel_pstate.c:2215:	.setpolicy	= intel_pstate_set_policy,

The intel_pstate is known to use counter-driven frequency invariance.

Conclusion
==========

Given that the significant majority of current FIE enabled drivers use
callbacks that lend themselves to triggering the setting of the FIE scale
factor in a generic way, move the invariance setter calls to cpufreq core.

As a result of setting the frequency scale factor in cpufreq core, after
callbacks that lend themselves to trigger it, remove this functionality
from the driver side.

To be noted that despite marking a successful frequency change, many
cpufreq drivers will consider the new frequency as the requested
frequency, although this is might not be the one granted by the hardware.

Therefore, the call to arch_set_freq_scale() is a "best effort" one, and
it is up to the architecture if the new frequency is used in the new
frequency scale factor setting (determined by the implementation of
arch_set_freq_scale()) or eventually used by the scheduler (determined
by the implementation of arch_scale_freq_capacity()). The architecture
is in a better position to decide if it has better methods to obtain
more accurate information regarding the current frequency and use that
information instead (for example, the use of counters).

Also, the implementation to arch_set_freq_scale() will now have to handle
error conditions (current frequency == 0) in order to prevent the
overhead in cpufreq core when the default arch_set_freq_scale()
implementation is used.

Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Suggested-by: Valentin Schneider <valentin.schneider@arm.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq-dt.c           | 10 +---------
 drivers/cpufreq/cpufreq.c              | 12 +++++++++++-
 drivers/cpufreq/qcom-cpufreq-hw.c      |  9 +--------
 drivers/cpufreq/scmi-cpufreq.c         | 12 ++----------
 drivers/cpufreq/scpi-cpufreq.c         |  6 +-----
 drivers/cpufreq/vexpress-spc-cpufreq.c | 12 ++----------
 6 files changed, 18 insertions(+), 43 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index f0c2d3876dec..e363ae04aac6 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -58,16 +58,8 @@ static int set_target(struct cpufreq_policy *policy, unsigned int index)
 {
 	struct private_data *priv = policy->driver_data;
 	unsigned long freq = policy->freq_table[index].frequency;
-	int ret;
-
-	ret = dev_pm_opp_set_rate(priv->cpu_dev, freq * 1000);
 
-	if (!ret) {
-		arch_set_freq_scale(policy->related_cpus, freq,
-				    policy->cpuinfo.max_freq);
-	}
-
-	return ret;
+	return dev_pm_opp_set_rate(priv->cpu_dev, freq * 1000);
 }
 
 /*
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 47aa90f9a7c2..4d5fe777184a 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -446,6 +446,10 @@ void cpufreq_freq_transition_end(struct cpufreq_policy *policy,
 
 	cpufreq_notify_post_transition(policy, freqs, transition_failed);
 
+	arch_set_freq_scale(policy->related_cpus,
+			    policy->cur,
+			    policy->cpuinfo.max_freq);
+
 	policy->transition_ongoing = false;
 	policy->transition_task = NULL;
 
@@ -2056,9 +2060,15 @@ EXPORT_SYMBOL(cpufreq_unregister_notifier);
 unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
 					unsigned int target_freq)
 {
+	unsigned int freq;
+
 	target_freq = clamp_val(target_freq, policy->min, policy->max);
+	freq = cpufreq_driver->fast_switch(policy, target_freq);
+
+	arch_set_freq_scale(policy->related_cpus, freq,
+			    policy->cpuinfo.max_freq);
 
-	return cpufreq_driver->fast_switch(policy, target_freq);
+	return freq;
 }
 EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch);
 
diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 3fb044b907a8..aabe4306d92f 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -85,8 +85,6 @@ static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
 	if (icc_scaling_enabled)
 		qcom_cpufreq_set_bw(policy, freq);
 
-	arch_set_freq_scale(policy->related_cpus, freq,
-			    policy->cpuinfo.max_freq);
 	return 0;
 }
 
@@ -113,16 +111,11 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
 {
 	void __iomem *perf_state_reg = policy->driver_data;
 	unsigned int index;
-	unsigned long freq;
 
 	index = policy->cached_resolved_idx;
 	writel_relaxed(index, perf_state_reg);
 
-	freq = policy->freq_table[index].frequency;
-	arch_set_freq_scale(policy->related_cpus, freq,
-			    policy->cpuinfo.max_freq);
-
-	return freq;
+	return policy->freq_table[index].frequency;
 }
 
 static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
index fb42e3390377..6dd1311660b5 100644
--- a/drivers/cpufreq/scmi-cpufreq.c
+++ b/drivers/cpufreq/scmi-cpufreq.c
@@ -48,16 +48,11 @@ static unsigned int scmi_cpufreq_get_rate(unsigned int cpu)
 static int
 scmi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index)
 {
-	int ret;
 	struct scmi_data *priv = policy->driver_data;
 	struct scmi_perf_ops *perf_ops = handle->perf_ops;
 	u64 freq = policy->freq_table[index].frequency;
 
-	ret = perf_ops->freq_set(handle, priv->domain_id, freq * 1000, false);
-	if (!ret)
-		arch_set_freq_scale(policy->related_cpus, freq,
-				    policy->cpuinfo.max_freq);
-	return ret;
+	return perf_ops->freq_set(handle, priv->domain_id, freq * 1000, false);
 }
 
 static unsigned int scmi_cpufreq_fast_switch(struct cpufreq_policy *policy,
@@ -67,11 +62,8 @@ static unsigned int scmi_cpufreq_fast_switch(struct cpufreq_policy *policy,
 	struct scmi_perf_ops *perf_ops = handle->perf_ops;
 
 	if (!perf_ops->freq_set(handle, priv->domain_id,
-				target_freq * 1000, true)) {
-		arch_set_freq_scale(policy->related_cpus, target_freq,
-				    policy->cpuinfo.max_freq);
+				target_freq * 1000, true))
 		return target_freq;
-	}
 
 	return 0;
 }
diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c
index b0f5388b8854..43db05b949d9 100644
--- a/drivers/cpufreq/scpi-cpufreq.c
+++ b/drivers/cpufreq/scpi-cpufreq.c
@@ -47,9 +47,8 @@ static unsigned int scpi_cpufreq_get_rate(unsigned int cpu)
 static int
 scpi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index)
 {
-	unsigned long freq = policy->freq_table[index].frequency;
+	u64 rate = policy->freq_table[index].frequency * 1000;
 	struct scpi_data *priv = policy->driver_data;
-	u64 rate = freq * 1000;
 	int ret;
 
 	ret = clk_set_rate(priv->clk, rate);
@@ -60,9 +59,6 @@ scpi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index)
 	if (clk_get_rate(priv->clk) != rate)
 		return -EIO;
 
-	arch_set_freq_scale(policy->related_cpus, freq,
-			    policy->cpuinfo.max_freq);
-
 	return 0;
 }
 
diff --git a/drivers/cpufreq/vexpress-spc-cpufreq.c b/drivers/cpufreq/vexpress-spc-cpufreq.c
index 4e8b1dee7c9a..e89b905754d2 100644
--- a/drivers/cpufreq/vexpress-spc-cpufreq.c
+++ b/drivers/cpufreq/vexpress-spc-cpufreq.c
@@ -182,7 +182,6 @@ static int ve_spc_cpufreq_set_target(struct cpufreq_policy *policy,
 {
 	u32 cpu = policy->cpu, cur_cluster, new_cluster, actual_cluster;
 	unsigned int freqs_new;
-	int ret;
 
 	cur_cluster = cpu_to_cluster(cpu);
 	new_cluster = actual_cluster = per_cpu(physical_cluster, cpu);
@@ -197,15 +196,8 @@ static int ve_spc_cpufreq_set_target(struct cpufreq_policy *policy,
 			new_cluster = A15_CLUSTER;
 	}
 
-	ret = ve_spc_cpufreq_set_rate(cpu, actual_cluster, new_cluster,
-				      freqs_new);
-
-	if (!ret) {
-		arch_set_freq_scale(policy->related_cpus, freqs_new,
-				    policy->cpuinfo.max_freq);
-	}
-
-	return ret;
+	return ve_spc_cpufreq_set_rate(cpu, actual_cluster, new_cluster,
+				       freqs_new);
 }
 
 static inline u32 get_table_count(struct cpufreq_frequency_table *table)
-- 
2.17.1

[PATCH v5 3/5] cpufreq: report whether cpufreq supports Frequency Invariance (FI)

[Ionela Voinescu]

Now that the update of the FI scale factor is done in cpufreq core for
selected functions - target(), target_index() and fast_switch(),
we can provide feedback to the task scheduler and architecture code
on whether cpufreq supports FI.

For this purpose provide an external function to expose whether the
cpufreq drivers support FI, by using a static key.

The logic behind the enablement of cpufreq-based invariance is as
follows:
 - cpufreq-based invariance is disabled by default
 - cpufreq-based invariance is enabled if any of the callbacks
   above is implemented while the unsupported setpolicy() is not

The cpufreq_supports_freq_invariance() function only returns whether
cpufreq is instrumented with the arch_set_freq_scale() calls that
result in support for frequency invariance. Due to the lack of knowledge
on whether the implementation of arch_set_freq_scale() actually results
in the setting of a scale factor based on cpufreq information, it is up
to the architecture code to ensure the setting and provision of the
scale factor to the scheduler.

Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 16 ++++++++++++++++
 include/linux/cpufreq.h   |  5 +++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 4d5fe777184a..570bf2ebe9d4 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -61,6 +61,12 @@ static struct cpufreq_driver *cpufreq_driver;
 static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
 static DEFINE_RWLOCK(cpufreq_driver_lock);
 
+static DEFINE_STATIC_KEY_FALSE(cpufreq_freq_invariance);
+bool cpufreq_supports_freq_invariance(void)
+{
+	return static_branch_likely(&cpufreq_freq_invariance);
+}
+
 /* Flag to suspend/resume CPUFreq governors */
 static bool cpufreq_suspended;
 
@@ -2720,6 +2726,15 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
 	cpufreq_driver = driver_data;
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
+	/*
+	 * Mark support for the scheduler's frequency invariance engine for
+	 * drivers that implement target(), target_index() or fast_switch().
+	 */
+	if (!cpufreq_driver->setpolicy) {
+		static_branch_enable_cpuslocked(&cpufreq_freq_invariance);
+		pr_debug("supports frequency invariance");
+	}
+
 	if (driver_data->setpolicy)
 		driver_data->flags |= CPUFREQ_CONST_LOOPS;
 
@@ -2789,6 +2804,7 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver)
 	cpus_read_lock();
 	subsys_interface_unregister(&cpufreq_interface);
 	remove_boost_sysfs_file();
+	static_branch_disable_cpuslocked(&cpufreq_freq_invariance);
 	cpuhp_remove_state_nocalls_cpuslocked(hp_online);
 
 	write_lock_irqsave(&cpufreq_driver_lock, flags);
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index a911e5d06845..e54767e2a68a 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -217,6 +217,7 @@ void refresh_frequency_limits(struct cpufreq_policy *policy);
 void cpufreq_update_policy(unsigned int cpu);
 void cpufreq_update_limits(unsigned int cpu);
 bool have_governor_per_policy(void);
+bool cpufreq_supports_freq_invariance(void);
 struct kobject *get_governor_parent_kobj(struct cpufreq_policy *policy);
 void cpufreq_enable_fast_switch(struct cpufreq_policy *policy);
 void cpufreq_disable_fast_switch(struct cpufreq_policy *policy);
@@ -237,6 +238,10 @@ static inline unsigned int cpufreq_get_hw_max_freq(unsigned int cpu)
 {
 	return 0;
 }
+static inline bool cpufreq_supports_freq_invariance(void)
+{
+	return false;
+}
 static inline void disable_cpufreq(void) { }
 #endif
 
-- 
2.17.1

[PATCH v5 4/5] arch_topology, cpufreq: constify arch_* cpumasks

[Ionela Voinescu]

From: Valentin Schneider <valentin.schneider@arm.com>

The passed cpumask arguments to arch_set_freq_scale() and
arch_freq_counters_available() are only iterated over, so reflect this
in the prototype. This also allows to pass system cpumasks like
cpu_online_mask without getting a warning.

Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
---
 arch/arm64/kernel/topology.c  | 2 +-
 drivers/base/arch_topology.c  | 4 ++--
 drivers/cpufreq/cpufreq.c     | 5 +++--
 include/linux/arch_topology.h | 2 +-
 include/linux/cpufreq.h       | 3 ++-
 5 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 0801a0f3c156..9a9f2b8dedf5 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -253,7 +253,7 @@ static int __init init_amu_fie(void)
 }
 late_initcall_sync(init_amu_fie);
 
-bool arch_freq_counters_available(struct cpumask *cpus)
+bool arch_freq_counters_available(const struct cpumask *cpus)
 {
 	return amu_freq_invariant() &&
 	       cpumask_subset(cpus, amu_fie_cpus);
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 42a08ef693ae..91de5331ac8a 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -21,13 +21,13 @@
 #include <linux/sched.h>
 #include <linux/smp.h>
 
-__weak bool arch_freq_counters_available(struct cpumask *cpus)
+__weak bool arch_freq_counters_available(const struct cpumask *cpus)
 {
 	return false;
 }
 DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE;
 
-void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
+void arch_set_freq_scale(const struct cpumask *cpus, unsigned long cur_freq,
 			 unsigned long max_freq)
 {
 	unsigned long scale;
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 570bf2ebe9d4..2ea245a6c0c0 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -160,8 +160,9 @@ u64 get_cpu_idle_time(unsigned int cpu, u64 *wall, int io_busy)
 }
 EXPORT_SYMBOL_GPL(get_cpu_idle_time);
 
-__weak void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
-		unsigned long max_freq)
+__weak void arch_set_freq_scale(const struct cpumask *cpus,
+				unsigned long cur_freq,
+				unsigned long max_freq)
 {
 }
 EXPORT_SYMBOL_GPL(arch_set_freq_scale);
diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
index 69b1dabe39dc..810c83336257 100644
--- a/include/linux/arch_topology.h
+++ b/include/linux/arch_topology.h
@@ -30,7 +30,7 @@ static inline unsigned long topology_get_freq_scale(int cpu)
 	return per_cpu(freq_scale, cpu);
 }
 
-bool arch_freq_counters_available(struct cpumask *cpus);
+bool arch_freq_counters_available(const struct cpumask *cpus);
 
 DECLARE_PER_CPU(unsigned long, thermal_pressure);
 
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index e54767e2a68a..9f779fbdbe7b 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -1011,7 +1011,8 @@ static inline void sched_cpufreq_governor_change(struct cpufreq_policy *policy,
 extern void arch_freq_prepare_all(void);
 extern unsigned int arch_freq_get_on_cpu(int cpu);
 
-extern void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
+extern void arch_set_freq_scale(const struct cpumask *cpus,
+				unsigned long cur_freq,
 				unsigned long max_freq);
 
 /* the following are really really optional */
-- 
2.17.1

[PATCH v5 5/5] arch_topology, arm, arm64: define arch_scale_freq_invariant()

[Ionela Voinescu]

From: Valentin Schneider <valentin.schneider@arm.com>

arch_scale_freq_invariant() is used by schedutil to determine whether
the scheduler's load-tracking signals are frequency invariant. Its
definition is overridable, though by default it is hardcoded to 'true'
if arch_scale_freq_capacity() is defined ('false' otherwise).

This behaviour is not overridden on arm, arm64 and other users of the
generic arch topology driver, which is somewhat precarious:
arch_scale_freq_capacity() will always be defined, yet not all cpufreq
drivers are guaranteed to drive the frequency invariance scale factor
setting. In other words, the load-tracking signals may very well *not*
be frequency invariant.

Now that cpufreq can be queried on whether the current driver is driving
the Frequency Invariance (FI) scale setting, the current situation can
be improved. This combines the query of whether cpufreq supports the
setting of the frequency scale factor, with whether all online CPUs are
counter-based FI enabled.

While cpufreq FI enablement applies at system level, for all CPUs,
counter-based FI support could also be used for only a subset of CPUs to
set the invariance scale factor. Therefore, if cpufreq-based FI support
is present, we consider the system to be invariant. If missing, we
require all online CPUs to be counter-based FI enabled in order for the
full system to be considered invariant.

If the system ends up not being invariant, a new condition is needed in
the counter initialization code that disables all scale factor setting
based on counters.

Precedence of counters over cpufreq use is not important here. The
invariant status is only given to the system if all CPUs have at least
one method of setting the frequency scale factor.

Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm/include/asm/topology.h   | 1 +
 arch/arm64/include/asm/topology.h | 1 +
 arch/arm64/kernel/topology.c      | 7 +++++++
 drivers/base/arch_topology.c      | 6 ++++++
 include/linux/arch_topology.h     | 2 ++
 5 files changed, 17 insertions(+)

diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
index e0593cf095d0..9219e67befbe 100644
--- a/arch/arm/include/asm/topology.h
+++ b/arch/arm/include/asm/topology.h
@@ -9,6 +9,7 @@
 
 /* Replace task scheduler's default frequency-invariant accounting */
 #define arch_scale_freq_capacity topology_get_freq_scale
+#define arch_scale_freq_invariant topology_scale_freq_invariant
 
 /* Replace task scheduler's default cpu-invariant accounting */
 #define arch_scale_cpu_capacity topology_get_cpu_scale
diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
index e042f6527981..7cb519473fbd 100644
--- a/arch/arm64/include/asm/topology.h
+++ b/arch/arm64/include/asm/topology.h
@@ -27,6 +27,7 @@ void topology_scale_freq_tick(void);
 
 /* Replace task scheduler's default frequency-invariant accounting */
 #define arch_scale_freq_capacity topology_get_freq_scale
+#define arch_scale_freq_invariant topology_scale_freq_invariant
 
 /* Replace task scheduler's default cpu-invariant accounting */
 #define arch_scale_cpu_capacity topology_get_cpu_scale
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 9a9f2b8dedf5..4064d39bb66d 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -246,6 +246,13 @@ static int __init init_amu_fie(void)
 		static_branch_enable(&amu_fie_key);
 	}
 
+	/*
+	 * If the system is not fully invariant after AMU init, disable
+	 * partial use of counters for frequency invariance.
+	 */
+	if (!topology_scale_freq_invariant())
+		static_branch_disable(&amu_fie_key);
+
 free_valid_mask:
 	free_cpumask_var(valid_cpus);
 
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 91de5331ac8a..89cae168b076 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -21,6 +21,12 @@
 #include <linux/sched.h>
 #include <linux/smp.h>
 
+bool topology_scale_freq_invariant(void)
+{
+	return cpufreq_supports_freq_invariance() ||
+	       arch_freq_counters_available(cpu_online_mask);
+}
+
 __weak bool arch_freq_counters_available(const struct cpumask *cpus)
 {
 	return false;
diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
index 810c83336257..083df331a3c9 100644
--- a/include/linux/arch_topology.h
+++ b/include/linux/arch_topology.h
@@ -30,6 +30,8 @@ static inline unsigned long topology_get_freq_scale(int cpu)
 	return per_cpu(freq_scale, cpu);
 }
 
+bool topology_scale_freq_invariant(void);
+
 bool arch_freq_counters_available(const struct cpumask *cpus);
 
 DECLARE_PER_CPU(unsigned long, thermal_pressure);
-- 
2.17.1

Re: [PATCH v5 5/5] arch_topology, arm, arm64: define arch_scale_freq_invariant()

[Sudeep Holla]

On Tue, Sep 01, 2020 at 09:55:49PM +0100, Ionela Voinescu wrote:
> From: Valentin Schneider <valentin.schneider@arm.com>
> 
> arch_scale_freq_invariant() is used by schedutil to determine whether
> the scheduler's load-tracking signals are frequency invariant. Its
> definition is overridable, though by default it is hardcoded to 'true'
> if arch_scale_freq_capacity() is defined ('false' otherwise).
> 
> This behaviour is not overridden on arm, arm64 and other users of the
> generic arch topology driver, which is somewhat precarious:
> arch_scale_freq_capacity() will always be defined, yet not all cpufreq
> drivers are guaranteed to drive the frequency invariance scale factor
> setting. In other words, the load-tracking signals may very well *not*
> be frequency invariant.
> 
> Now that cpufreq can be queried on whether the current driver is driving
> the Frequency Invariance (FI) scale setting, the current situation can
> be improved. This combines the query of whether cpufreq supports the
> setting of the frequency scale factor, with whether all online CPUs are
> counter-based FI enabled.
> 
> While cpufreq FI enablement applies at system level, for all CPUs,
> counter-based FI support could also be used for only a subset of CPUs to
> set the invariance scale factor. Therefore, if cpufreq-based FI support
> is present, we consider the system to be invariant. If missing, we
> require all online CPUs to be counter-based FI enabled in order for the
> full system to be considered invariant.
> 
> If the system ends up not being invariant, a new condition is needed in
> the counter initialization code that disables all scale factor setting
> based on counters.
> 
> Precedence of counters over cpufreq use is not important here. The
> invariant status is only given to the system if all CPUs have at least
> one method of setting the frequency scale factor.
> 
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

-- 
Regards,
Sudeep

Re: [PATCH v5 4/5] arch_topology, cpufreq: constify arch_* cpumasks

[Sudeep Holla]

On Tue, Sep 01, 2020 at 09:55:48PM +0100, Ionela Voinescu wrote:
> From: Valentin Schneider <valentin.schneider@arm.com>
>
> The passed cpumask arguments to arch_set_freq_scale() and
> arch_freq_counters_available() are only iterated over, so reflect this
> in the prototype. This also allows to pass system cpumasks like
> cpu_online_mask without getting a warning.
>
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep

Re: [PATCH v5 3/5] cpufreq: report whether cpufreq supports Frequency Invariance (FI)

[Sudeep Holla]

On Tue, Sep 01, 2020 at 09:55:47PM +0100, Ionela Voinescu wrote:
> Now that the update of the FI scale factor is done in cpufreq core for
> selected functions - target(), target_index() and fast_switch(),
> we can provide feedback to the task scheduler and architecture code
> on whether cpufreq supports FI.
> 
> For this purpose provide an external function to expose whether the
> cpufreq drivers support FI, by using a static key.
> 
> The logic behind the enablement of cpufreq-based invariance is as
> follows:
>  - cpufreq-based invariance is disabled by default
>  - cpufreq-based invariance is enabled if any of the callbacks
>    above is implemented while the unsupported setpolicy() is not
> 
> The cpufreq_supports_freq_invariance() function only returns whether
> cpufreq is instrumented with the arch_set_freq_scale() calls that
> result in support for frequency invariance. Due to the lack of knowledge
> on whether the implementation of arch_set_freq_scale() actually results
> in the setting of a scale factor based on cpufreq information, it is up
> to the architecture code to ensure the setting and provision of the
> scale factor to the scheduler.
> 
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 16 ++++++++++++++++
>  include/linux/cpufreq.h   |  5 +++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 4d5fe777184a..570bf2ebe9d4 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -61,6 +61,12 @@ static struct cpufreq_driver *cpufreq_driver;
>  static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
>  static DEFINE_RWLOCK(cpufreq_driver_lock);
>  
> +static DEFINE_STATIC_KEY_FALSE(cpufreq_freq_invariance);
> +bool cpufreq_supports_freq_invariance(void)
> +{
> +	return static_branch_likely(&cpufreq_freq_invariance);
> +}
> +
>  /* Flag to suspend/resume CPUFreq governors */
>  static bool cpufreq_suspended;
>  
> @@ -2720,6 +2726,15 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
>  	cpufreq_driver = driver_data;
>  	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
>  
> +	/*
> +	 * Mark support for the scheduler's frequency invariance engine for
> +	 * drivers that implement target(), target_index() or fast_switch().
> +	 */
> +	if (!cpufreq_driver->setpolicy) {
> +		static_branch_enable_cpuslocked(&cpufreq_freq_invariance);
> +		pr_debug("supports frequency invariance");
> +	}
> +
>  	if (driver_data->setpolicy)

[super nit] while I understand cpufreq_driver = driver_data, it looks odd
if 2 consecutive statements refer it with different variables. Or am I
confusing myself hugely.

-- 
Regards,
Sudeep

Re: [PATCH v5 2/5] cpufreq: move invariance setter calls in cpufreq core

[Sudeep Holla]

On Tue, Sep 01, 2020 at 09:55:46PM +0100, Ionela Voinescu wrote:
> To properly scale its per-entity load-tracking signals, the task scheduler
> needs to be given a frequency scale factor, i.e. some image of the current
> frequency the CPU is running at. Currently, this scale can be computed
> either by using counters (APERF/MPERF on x86, AMU on arm64), or by
> piggy-backing on the frequency selection done by cpufreq.
> 
> For the latter, drivers have to explicitly set the scale factor
> themselves, despite it being purely boiler-plate code: the required
> information depends entirely on the kind of frequency switch callback
> implemented by the driver, i.e. either of: target_index(), target(),
> fast_switch() and setpolicy().
> 
> The fitness of those callbacks with regard to driving the Frequency
> Invariance Engine (FIE) is studied below:
> 
> target_index()
> ==============
> Documentation states that the chosen frequency "must be determined by
> freq_table[index].frequency". It isn't clear if it *has* to be that
> frequency, or if it can use that frequency value to do some computation
> that ultimately leads to a different frequency selection. All drivers
> go for the former, while the vexpress-spc-cpufreq has an atypical
> implementation which is handled separately.
> 
> Therefore, the hook works on the assumption the core can use
> freq_table[index].frequency.
> 
> target()
> =======
> This has been flagged as deprecated since:
> 
>   commit 9c0ebcf78fde ("cpufreq: Implement light weight ->target_index() routine")
> 
> It also doesn't have that many users:
> 
>   gx-suspmod.c:439:       .target = cpufreq_gx_target,
>   s3c24xx-cpufreq.c:428:  .target = s3c_cpufreq_target,
>   intel_pstate.c:2528:    .target = intel_cpufreq_target,
>   cppc_cpufreq.c:401:     .target = cppc_cpufreq_set_target,
>   cpufreq-nforce2.c:371:  .target = nforce2_target,
>   sh-cpufreq.c:163:       .target = sh_cpufreq_target,
>   pcc-cpufreq.c:573:      .target = pcc_cpufreq_target,
> 
> Similarly to the path taken for target_index() calls in the cpufreq core
> during a frequency change, all of the drivers above will mark the end of a
> frequency change by a call to cpufreq_freq_transition_end().
> 
> Therefore, cpufreq_freq_transition_end() can be used as the location for
> the arch_set_freq_scale() call to potentially inform the scheduler of the
> frequency change.
> 
> This change maintains the previous functionality for the drivers that
> implement the target_index() callback, while also adding support for the
> few drivers that implement the deprecated target() callback.
> 
> fast_switch()
> =============
> This callback *has* to return the frequency that was selected.
> 
> setpolicy()
> ===========
> This callback does not have any designated way of informing what was the
> end choice. But there are only two drivers using setpolicy(), and none
> of them have current FIE support:
> 
>   drivers/cpufreq/longrun.c:281:	.setpolicy	= longrun_set_policy,
>   drivers/cpufreq/intel_pstate.c:2215:	.setpolicy	= intel_pstate_set_policy,
> 
> The intel_pstate is known to use counter-driven frequency invariance.
> 
> Conclusion
> ==========
> 
> Given that the significant majority of current FIE enabled drivers use
> callbacks that lend themselves to triggering the setting of the FIE scale
> factor in a generic way, move the invariance setter calls to cpufreq core.
> 
> As a result of setting the frequency scale factor in cpufreq core, after
> callbacks that lend themselves to trigger it, remove this functionality
> from the driver side.
> 
> To be noted that despite marking a successful frequency change, many
> cpufreq drivers will consider the new frequency as the requested
> frequency, although this is might not be the one granted by the hardware.
> 
> Therefore, the call to arch_set_freq_scale() is a "best effort" one, and
> it is up to the architecture if the new frequency is used in the new
> frequency scale factor setting (determined by the implementation of
> arch_set_freq_scale()) or eventually used by the scheduler (determined
> by the implementation of arch_scale_freq_capacity()). The architecture
> is in a better position to decide if it has better methods to obtain
> more accurate information regarding the current frequency and use that
> information instead (for example, the use of counters).
> 
> Also, the implementation to arch_set_freq_scale() will now have to handle
> error conditions (current frequency == 0) in order to prevent the
> overhead in cpufreq core when the default arch_set_freq_scale()
> implementation is used.
> 
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Suggested-by: Valentin Schneider <valentin.schneider@arm.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq-dt.c           | 10 +---------
>  drivers/cpufreq/cpufreq.c              | 12 +++++++++++-
>  drivers/cpufreq/qcom-cpufreq-hw.c      |  9 +--------
[...]

>  drivers/cpufreq/scmi-cpufreq.c         | 12 ++----------
>  drivers/cpufreq/scpi-cpufreq.c         |  6 +-----
>  drivers/cpufreq/vexpress-spc-cpufreq.c | 12 ++----------

For above 3 files:

Acked-by: Sudeep Holla <sudeep.holla@arm.com>

-- 
Regards,
Sudeep

Re: [PATCH v5 1/5] arch_topology: validate input frequencies to arch_set_freq_scale()

[Sudeep Holla]

On Tue, Sep 01, 2020 at 09:55:45PM +0100, Ionela Voinescu wrote:
> The current frequency passed to arch_set_freq_scale() could end up
> being 0, signaling an error in setting a new frequency. Also, if the
> maximum frequency in 0, this will result in a division by 0 error.
>
> Therefore, validate these input values before using them for the
> setting of the frequency scale factor.
>
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep

Re: [PATCH v5 0/5] cpufreq: improve frequency invariance support

[Ionela Voinescu]

Hi Rafael, Viresh,

Would it be okay for you to apply this series, as the majority of
changes are in cpufreq? For arch_topology and arm64 changes, they have
been reviewed and acked-by Catalin and Sudeep.

Also, please let me know if I should send v6 with Sudeep's Reviewed-by/s
applied.

Thank you,
Ionela.


On Tuesday 01 Sep 2020 at 21:55:44 (+0100), Ionela Voinescu wrote:
> Hi,
> 
> v4->v5
>  - I've applied Viresh's remaining suggestion and Acked-by/s
>  - v4 can be found at [4]
>  - v5 is based on linux-next 20200828
> 
> Thank you,
> Ionela.
> 
> ---
> v3->v4:
>  - addressing Viresh's comments on patches 1/5 and 3/5, and
>  - with his Acked-by applied for the rest of the patches;
>  - v3 can be found at [3], and
>  - this is based on linux-next 20200827.
> 
> v2->v3
>  - v2 can be found at [2]
>  - 1/5 was introduced to check input frequencies to
>    arch_set_freq_scale() as recommended by Rafael
>  - The previous 2/7 was squashed into 1/7 - now 2/5, with additions to
>    the changelog as suggested by Rafael.
>  - The previous 3/7 (BL_SWITCHER handling) was dropped to be handled
>    in a separate patch. This does not change the current functionality.
>  - The previous 4/7 - now 3/5 is simplified as suggested by Viresh.
>  - 3/5 - cpufreq_supports_freq_invariance() replaces
>    cpufreq_sets_freq_scale(). The meaning chosen for
>    cpufreq_supports_freq_invariance() is whether it can set the frequency
>    scale factor, not whether arch_set_freq_scale() actually does.
>  - 4/5 - Change after Catalin's Ack: The changes to
>    arch_set_thermal_pressure() were dropped as they were done in a separate
>    patch. Therefore this patch now has a subset of the previous changes
>    at 5/7
>  - 5/5 - Change after Catalin's Ack:
>    s/cpufreq_sets_freq_scale/cpufreq_supports_freq_invariance
>  - v3 is based on linux-next 20200814
> 
> 
> v1 -> v2:
>  - v1 can be found at [1]
>  - No cpufreq flags are introduced
>  - Previous patches 2/8 and 3/8 were squashed in this series under 1/7,
>    to ensure bisection.
>  - 2/7 was introduced as a proposal for Viresh's suggestion to use
>    policy->cur in the call to arch_set_freq_scale() and is extended to
>    support drivers that implement the target() callback as well
>  - Additional commit message changes are added to 1/7 and 2/7, to
>    clarify that the definition of arch_set_freq_scale() will filter 
>    settings of the scale factor, if unwanted
>  - 3/7 disables setting of the scale factor for
>    CONFIG_BL_SWITCHER, as Dietmar suggested
>  - Small change introduced in 4/7 to disable cpufreq-based frequency
>    invariance for the users of the default arch_set_freq_scale() call
>    which will not actually set a scale factor
>  - build issue solved (reported by 0day test)
>  - v2 is based on linux-next 20200716
>  - all functional tests in v1 were repeated for v2
> 
> 
> [1] https://lore.kernel.org/lkml/20200701090751.7543-1-ionela.voinescu@arm.com/
> [2] https://lore.kernel.org/lkml/20200722093732.14297-1-ionela.voinescu@arm.com/
> [3] https://lore.kernel.org/lkml/20200824210252.27486-1-ionela.voinescu@arm.com/
> [4] https://lore.kernel.org/lkml/20200828173303.11939-1-ionela.voinescu@arm.com/
> 
> Ionela Voinescu (3):
>   arch_topology: validate input frequencies to arch_set_freq_scale()
>   cpufreq: move invariance setter calls in cpufreq core
>   cpufreq: report whether cpufreq supports Frequency Invariance (FI)
> 
> Valentin Schneider (2):
>   arch_topology, cpufreq: constify arch_* cpumasks
>   arch_topology, arm, arm64: define arch_scale_freq_invariant()
> 
>  arch/arm/include/asm/topology.h        |  1 +
>  arch/arm64/include/asm/topology.h      |  1 +
>  arch/arm64/kernel/topology.c           |  9 ++++++-
>  drivers/base/arch_topology.c           | 13 ++++++++--
>  drivers/cpufreq/cpufreq-dt.c           | 10 +-------
>  drivers/cpufreq/cpufreq.c              | 33 +++++++++++++++++++++++---
>  drivers/cpufreq/qcom-cpufreq-hw.c      |  9 +------
>  drivers/cpufreq/scmi-cpufreq.c         | 12 ++--------
>  drivers/cpufreq/scpi-cpufreq.c         |  6 +----
>  drivers/cpufreq/vexpress-spc-cpufreq.c | 12 ++--------
>  include/linux/arch_topology.h          |  4 +++-
>  include/linux/cpufreq.h                |  8 ++++++-
>  12 files changed, 68 insertions(+), 50 deletions(-)
> 
> 
> base-commit: b36c969764ab12faebb74711c942fa3e6eaf1e96
> -- 
> 2.17.1
> 

Re: [PATCH v5 3/5] cpufreq: report whether cpufreq supports Frequency Invariance (FI)

[Ionela Voinescu]

Hi Sudeep,

Thank you for your review here and for the other patches.

On Wednesday 02 Sep 2020 at 14:28:38 (+0100), Sudeep Holla wrote:
[..]
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 4d5fe777184a..570bf2ebe9d4 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -61,6 +61,12 @@ static struct cpufreq_driver *cpufreq_driver;
> >  static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
> >  static DEFINE_RWLOCK(cpufreq_driver_lock);
> >  
> > +static DEFINE_STATIC_KEY_FALSE(cpufreq_freq_invariance);
> > +bool cpufreq_supports_freq_invariance(void)
> > +{
> > +	return static_branch_likely(&cpufreq_freq_invariance);
> > +}
> > +
> >  /* Flag to suspend/resume CPUFreq governors */
> >  static bool cpufreq_suspended;
> >  
> > @@ -2720,6 +2726,15 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
> >  	cpufreq_driver = driver_data;
> >  	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
> >  
> > +	/*
> > +	 * Mark support for the scheduler's frequency invariance engine for
> > +	 * drivers that implement target(), target_index() or fast_switch().
> > +	 */
> > +	if (!cpufreq_driver->setpolicy) {
> > +		static_branch_enable_cpuslocked(&cpufreq_freq_invariance);
> > +		pr_debug("supports frequency invariance");
> > +	}
> > +
> >  	if (driver_data->setpolicy)
> 
> [super nit] while I understand cpufreq_driver = driver_data, it looks odd
> if 2 consecutive statements refer it with different variables. Or am I
> confusing myself hugely.
> 

No, you are right. If you look at the rest of the register function,
after cpufreq_driver = driver_data, both driver_data and cpufreq_driver
are used. For me using cpufreq_driver seemed more natural as after being
assigned driver_data, it will continue to be used after registration.

If it's alright with you I won't make this change for now. It's possible
that a better solution is to change the other occurrences of either
cpufreq_driver or driver_data in a separate patch, to make things
consistent across the function.

Thank you,
Ionela.

> -- 
> Regards,
> Sudeep

Re: [PATCH v5 3/5] cpufreq: report whether cpufreq supports Frequency Invariance (FI)

[Sudeep Holla]

On Thu, Sep 03, 2020 at 02:45:08PM +0100, Ionela Voinescu wrote:
> Hi Sudeep,
> 
> Thank you for your review here and for the other patches.
> 
> On Wednesday 02 Sep 2020 at 14:28:38 (+0100), Sudeep Holla wrote:
> [..]
> > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > index 4d5fe777184a..570bf2ebe9d4 100644
> > > --- a/drivers/cpufreq/cpufreq.c
> > > +++ b/drivers/cpufreq/cpufreq.c
> > > @@ -61,6 +61,12 @@ static struct cpufreq_driver *cpufreq_driver;
> > >  static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
> > >  static DEFINE_RWLOCK(cpufreq_driver_lock);
> > >  
> > > +static DEFINE_STATIC_KEY_FALSE(cpufreq_freq_invariance);
> > > +bool cpufreq_supports_freq_invariance(void)
> > > +{
> > > +	return static_branch_likely(&cpufreq_freq_invariance);
> > > +}
> > > +
> > >  /* Flag to suspend/resume CPUFreq governors */
> > >  static bool cpufreq_suspended;
> > >  
> > > @@ -2720,6 +2726,15 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
> > >  	cpufreq_driver = driver_data;
> > >  	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
> > >  
> > > +	/*
> > > +	 * Mark support for the scheduler's frequency invariance engine for
> > > +	 * drivers that implement target(), target_index() or fast_switch().
> > > +	 */
> > > +	if (!cpufreq_driver->setpolicy) {
> > > +		static_branch_enable_cpuslocked(&cpufreq_freq_invariance);
> > > +		pr_debug("supports frequency invariance");
> > > +	}
> > > +
> > >  	if (driver_data->setpolicy)
> > 
> > [super nit] while I understand cpufreq_driver = driver_data, it looks odd
> > if 2 consecutive statements refer it with different variables. Or am I
> > confusing myself hugely.
> > 
> 
> No, you are right. If you look at the rest of the register function,
> after cpufreq_driver = driver_data, both driver_data and cpufreq_driver
> are used. For me using cpufreq_driver seemed more natural as after being
> assigned driver_data, it will continue to be used after registration.
>

Ah OK, I haven't seen the whole file/function, just looked at the patch.

> If it's alright with you I won't make this change for now. It's possible
> that a better solution is to change the other occurrences of either
> cpufreq_driver or driver_data in a separate patch, to make things
> consistent across the function.
> 

I am fine to keep it as is, hence I mentioned it as super nit. If there
are other occurrences, then better to take it up separately.

-- 
Regards,
Sudeep

Re: [PATCH v5 0/5] cpufreq: improve frequency invariance support

[Viresh Kumar]

On 03-09-20, 14:32, Ionela Voinescu wrote:
> Hi Rafael, Viresh,
> 
> Would it be okay for you to apply this series, as the majority of
> changes are in cpufreq? For arch_topology and arm64 changes, they have
> been reviewed and acked-by Catalin and Sudeep.
> 
> Also, please let me know if I should send v6 with Sudeep's Reviewed-by/s
> applied.

No need to resend. Rafael will apply these with the tags.

-- 
viresh

Re: [PATCH v5 0/5] cpufreq: improve frequency invariance support

[Rafael J. Wysocki]

On Fri, Sep 4, 2020 at 6:39 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 03-09-20, 14:32, Ionela Voinescu wrote:
> > Hi Rafael, Viresh,
> >
> > Would it be okay for you to apply this series, as the majority of
> > changes are in cpufreq? For arch_topology and arm64 changes, they have
> > been reviewed and acked-by Catalin and Sudeep.
> >
> > Also, please let me know if I should send v6 with Sudeep's Reviewed-by/s
> > applied.
>
> No need to resend. Rafael will apply these with the tags.

Right. :-)

So now applied as 5.10 material, sorry for the delay.

Thanks!