Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/7] cpufreq: improve frequency invariance support
@ 2020-07-22  9:37 Ionela Voinescu
  2020-07-22  9:37 ` [PATCH v2 1/7] cpufreq: move invariance setter calls in cpufreq core Ionela Voinescu
                   ` (6 more replies)
  0 siblings, 7 replies; 35+ messages in thread
From: Ionela Voinescu @ 2020-07-22  9:37 UTC (permalink / raw)
  To: rjw, viresh.kumar, dietmar.eggemann, catalin.marinas,
	sudeep.holla, will, linux
  Cc: linux-pm, peterz, linux-kernel, mingo, ionela.voinescu, linux-arm-kernel

Hi guys,

Please find below the changes to this series:

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/


Let me know what you think!

Thank you,
Ionela.

Ionela Voinescu (3):
  cpufreq: set invariance scale factor on transition end
  arch_topology: disable frequency invariance for CONFIG_BL_SWITCHER
  cpufreq: report whether cpufreq supports Frequency Invariance (FI)

Valentin Schneider (4):
  cpufreq: move invariance setter calls in cpufreq core
  arch_topology,cpufreq,sched/core: constify arch_* cpumasks
  arch_topology,arm,arm64: define arch_scale_freq_invariant()
  cpufreq: make schedutil the default for arm and arm64

 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           | 12 +++++--
 drivers/cpufreq/Kconfig                |  2 +-
 drivers/cpufreq/cpufreq-dt.c           | 10 +-----
 drivers/cpufreq/cpufreq.c              | 46 ++++++++++++++++++++++++--
 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 |  5 ---
 include/linux/arch_topology.h          |  6 ++--
 include/linux/cpufreq.h                |  8 ++++-
 kernel/sched/core.c                    |  2 +-
 14 files changed, 81 insertions(+), 48 deletions(-)


base-commit: 4c43049f19a280329c1d01699f3cc8ad6910cbbe
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2 1/7] cpufreq: move invariance setter calls in cpufreq core
  2020-07-22  9:37 [PATCH v2 0/7] cpufreq: improve frequency invariance support Ionela Voinescu
@ 2020-07-22  9:37 ` Ionela Voinescu
  2020-07-27 13:48   ` Rafael J. Wysocki
  2020-07-22  9:37 ` [PATCH v2 2/7] cpufreq: set invariance scale factor on transition end Ionela Voinescu
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 35+ messages in thread
From: Ionela Voinescu @ 2020-07-22  9:37 UTC (permalink / raw)
  To: rjw, viresh.kumar, dietmar.eggemann, catalin.marinas,
	sudeep.holla, will, linux
  Cc: linux-pm, peterz, linux-kernel, mingo, ionela.voinescu,
	Valentin Schneider, linux-arm-kernel

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

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:

  cpufreq-nforce2.c:371:2:	.target = nforce2_target,
  cppc_cpufreq.c:416:2:		.target = cppc_cpufreq_set_target,
  gx-suspmod.c:439:2:		.target = cpufreq_gx_target,
  pcc-cpufreq.c:573:2:		.target = pcc_cpufreq_target,

Should we care about drivers using this hook, we may be able to exploit
cpufreq_freq_transition_{being, end}(). This is handled in a separate
patch.

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

Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq-dt.c           | 10 +---------
 drivers/cpufreq/cpufreq.c              | 20 +++++++++++++++++---
 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 |  5 -----
 6 files changed, 22 insertions(+), 40 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 944d7b45afe9..9fd4ce774f12 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -40,16 +40,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 036f4cc42ede..bac4101546db 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2058,9 +2058,16 @@ 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);
+
+	if (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);
 
@@ -2152,7 +2159,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 			    unsigned int relation)
 {
 	unsigned int old_target_freq = target_freq;
-	int index;
+	int index, retval;
 
 	if (cpufreq_disabled())
 		return -ENODEV;
@@ -2183,7 +2190,14 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 
 	index = cpufreq_frequency_table_target(policy, target_freq, relation);
 
-	return __target_index(policy, index);
+	retval = __target_index(policy, index);
+
+	if (!retval)
+		arch_set_freq_scale(policy->related_cpus,
+				    policy->freq_table[index].frequency,
+				    policy->cpuinfo.max_freq);
+
+	return retval;
 }
 EXPORT_SYMBOL_GPL(__cpufreq_driver_target);
 
diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 573630c23aca..e5d1ee7746a4 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,7 +111,6 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
 {
 	void __iomem *perf_state_reg = policy->driver_data;
 	int index;
-	unsigned long freq;
 
 	index = policy->cached_resolved_idx;
 	if (index < 0)
@@ -121,11 +118,7 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
 
 	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..313bb9db369b 100644
--- a/drivers/cpufreq/vexpress-spc-cpufreq.c
+++ b/drivers/cpufreq/vexpress-spc-cpufreq.c
@@ -200,11 +200,6 @@ static int ve_spc_cpufreq_set_target(struct cpufreq_policy *policy,
 	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;
 }
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2 2/7] cpufreq: set invariance scale factor on transition end
  2020-07-22  9:37 [PATCH v2 0/7] cpufreq: improve frequency invariance support Ionela Voinescu
  2020-07-22  9:37 ` [PATCH v2 1/7] cpufreq: move invariance setter calls in cpufreq core Ionela Voinescu
@ 2020-07-22  9:37 ` Ionela Voinescu
  2020-07-27 13:52   ` Rafael J. Wysocki
  2020-07-30  4:13   ` Viresh Kumar
  2020-07-22  9:37 ` [PATCH v2 3/7] arch_topology: disable frequency invariance for CONFIG_BL_SWITCHER Ionela Voinescu
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 35+ messages in thread
From: Ionela Voinescu @ 2020-07-22  9:37 UTC (permalink / raw)
  To: rjw, viresh.kumar, dietmar.eggemann, catalin.marinas,
	sudeep.holla, will, linux
  Cc: linux-pm, peterz, linux-kernel, mingo, ionela.voinescu, linux-arm-kernel

While the move of the invariance setter calls (arch_set_freq_scale())
from cpufreq drivers to cpufreq core maintained the previous
functionality for existing drivers that use target_index() and
fast_switch() for frequency switching, it also gives the possibility
of adding support for users of the target() callback, which is exploited
here.

To be noted that the target() callback has been flagged as deprecated
since:

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

It also doesn't have that many users:

  cpufreq-nforce2.c:371:2:      .target = nforce2_target,
  cppc_cpufreq.c:416:2:         .target = cppc_cpufreq_set_target,
  gx-suspmod.c:439:2:           .target = cpufreq_gx_target,
  pcc-cpufreq.c:573:2:          .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.

Two notes are worthwhile here:
 - In __target_index(), cpufreq_freq_transition_end() is called only for
   drivers that have synchronous notifications enabled. There is only one
   driver that disables them,

   drivers/cpufreq/powernow-k8.c:1142: .flags = CPUFREQ_ASYNC_NOTIFICATION,

   which is deprecated.

 - Despite marking a successful frequency change, many cpufreq drivers
   will populate the new policy->cur with the new requested frequency,
   although this 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 or eventually used by the scheduler.
   The architecture is in a better position to decide if it has better
   methods to obtain more accurate information regarding the current
   frequency (for example the use of counters).

Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index bac4101546db..3497c1cd6818 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -448,6 +448,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;
 
@@ -2159,7 +2163,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 			    unsigned int relation)
 {
 	unsigned int old_target_freq = target_freq;
-	int index, retval;
+	int index;
 
 	if (cpufreq_disabled())
 		return -ENODEV;
@@ -2190,14 +2194,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 
 	index = cpufreq_frequency_table_target(policy, target_freq, relation);
 
-	retval = __target_index(policy, index);
-
-	if (!retval)
-		arch_set_freq_scale(policy->related_cpus,
-				    policy->freq_table[index].frequency,
-				    policy->cpuinfo.max_freq);
-
-	return retval;
+	return __target_index(policy, index);
 }
 EXPORT_SYMBOL_GPL(__cpufreq_driver_target);
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2 3/7] arch_topology: disable frequency invariance for CONFIG_BL_SWITCHER
  2020-07-22  9:37 [PATCH v2 0/7] cpufreq: improve frequency invariance support Ionela Voinescu
  2020-07-22  9:37 ` [PATCH v2 1/7] cpufreq: move invariance setter calls in cpufreq core Ionela Voinescu
  2020-07-22  9:37 ` [PATCH v2 2/7] cpufreq: set invariance scale factor on transition end Ionela Voinescu
@ 2020-07-22  9:37 ` Ionela Voinescu
  2020-07-30  4:24   ` Viresh Kumar
  2020-07-22  9:37 ` [PATCH v2 4/7] cpufreq: report whether cpufreq supports Frequency Invariance (FI) Ionela Voinescu
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 35+ messages in thread
From: Ionela Voinescu @ 2020-07-22  9:37 UTC (permalink / raw)
  To: rjw, viresh.kumar, dietmar.eggemann, catalin.marinas,
	sudeep.holla, will, linux
  Cc: linux-pm, peterz, linux-kernel, mingo, ionela.voinescu, linux-arm-kernel

big.LITTLE switching complicates the setting of a correct cpufreq-based
frequency invariance scale factor due to (as observed in
drivers/cpufreq/vexpress-spc-cpufreq.c):
 - Incorrect current and maximum frequencies as a result of the
   exposure of a virtual frequency table to the cpufreq core,
 - Missed updates as a result of asynchronous frequency adjustments
   caused by frequency changes in other CPU pairs.

Given that its functionality is atypical in regards to frequency
invariance and this is an old technology, disable frequency
invariance for when big.LITTLE switching is configured in to prevent
incorrect scale setting.

Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Suggested-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
---
 drivers/base/arch_topology.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 4d0a0038b476..708768f528dc 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -27,6 +27,7 @@ __weak bool arch_freq_counters_available(struct cpumask *cpus)
 }
 DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE;
 
+#ifndef CONFIG_BL_SWITCHER
 void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
 			 unsigned long max_freq)
 {
@@ -46,6 +47,7 @@ void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
 	for_each_cpu(i, cpus)
 		per_cpu(freq_scale, i) = scale;
 }
+#endif
 
 DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2 4/7] cpufreq: report whether cpufreq supports Frequency Invariance (FI)
  2020-07-22  9:37 [PATCH v2 0/7] cpufreq: improve frequency invariance support Ionela Voinescu
                   ` (2 preceding siblings ...)
  2020-07-22  9:37 ` [PATCH v2 3/7] arch_topology: disable frequency invariance for CONFIG_BL_SWITCHER Ionela Voinescu
@ 2020-07-22  9:37 ` Ionela Voinescu
  2020-07-27 14:02   ` Rafael J. Wysocki
  2020-07-30  4:43   ` Viresh Kumar
  2020-07-22  9:37 ` [PATCH v2 5/7] arch_topology, cpufreq, sched/core: constify arch_* cpumasks Ionela Voinescu
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 35+ messages in thread
From: Ionela Voinescu @ 2020-07-22  9:37 UTC (permalink / raw)
  To: rjw, viresh.kumar, dietmar.eggemann, catalin.marinas,
	sudeep.holla, will, linux
  Cc: linux-pm, peterz, linux-kernel, mingo, ionela.voinescu, linux-arm-kernel

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 error and debug messages, together with 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
 - if enabled, cpufreq-based invariance will be disabled during the
   call of the default arch_set_freq_scale() function which does
   not set a scale factor.

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

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 3497c1cd6818..1d0b046fe8e9 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -61,6 +61,9 @@ static struct cpufreq_driver *cpufreq_driver;
 static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
 static DEFINE_RWLOCK(cpufreq_driver_lock);
 
+/* Mark support for the scheduler's frequency invariance engine */
+static DEFINE_STATIC_KEY_FALSE(cpufreq_set_freq_scale);
+
 /* Flag to suspend/resume CPUFreq governors */
 static bool cpufreq_suspended;
 
@@ -69,6 +72,25 @@ static inline bool has_target(void)
 	return cpufreq_driver->target_index || cpufreq_driver->target;
 }
 
+static inline
+void enable_cpufreq_freq_invariance(struct cpufreq_driver *driver)
+{
+	if ((driver->target || driver->target_index || driver->fast_switch) &&
+	    !driver->setpolicy) {
+
+		static_branch_enable_cpuslocked(&cpufreq_set_freq_scale);
+		pr_debug("%s: Driver %s can provide frequency invariance.",
+			 __func__, driver->name);
+	} else
+		pr_err("%s: Driver %s cannot provide frequency invariance.",
+		__func__, driver->name);
+}
+
+bool cpufreq_sets_freq_scale(void)
+{
+	return static_branch_likely(&cpufreq_set_freq_scale);
+}
+
 /* internal prototypes */
 static unsigned int __cpufreq_get(struct cpufreq_policy *policy);
 static int cpufreq_init_governor(struct cpufreq_policy *policy);
@@ -159,6 +181,9 @@ EXPORT_SYMBOL_GPL(get_cpu_idle_time);
 __weak void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
 		unsigned long max_freq)
 {
+	if (cpufreq_sets_freq_scale())
+		static_branch_disable_cpuslocked(&cpufreq_set_freq_scale);
+
 }
 EXPORT_SYMBOL_GPL(arch_set_freq_scale);
 
@@ -2722,6 +2747,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
 	cpufreq_driver = driver_data;
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
+	enable_cpufreq_freq_invariance(cpufreq_driver);
+
 	if (driver_data->setpolicy)
 		driver_data->flags |= CPUFREQ_CONST_LOOPS;
 
@@ -2791,6 +2818,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_set_freq_scale);
 	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 e62b022cb07e..f81215ad76f1 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_sets_freq_scale(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_sets_freq_scale(void)
+{
+	return false;
+}
 static inline void disable_cpufreq(void) { }
 #endif
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2 5/7] arch_topology, cpufreq, sched/core: constify arch_* cpumasks
  2020-07-22  9:37 [PATCH v2 0/7] cpufreq: improve frequency invariance support Ionela Voinescu
                   ` (3 preceding siblings ...)
  2020-07-22  9:37 ` [PATCH v2 4/7] cpufreq: report whether cpufreq supports Frequency Invariance (FI) Ionela Voinescu
@ 2020-07-22  9:37 ` Ionela Voinescu
  2020-07-30 11:43   ` Catalin Marinas
  2020-07-22  9:37 ` [PATCH v2 6/7] arch_topology, arm, arm64: define arch_scale_freq_invariant() Ionela Voinescu
  2020-07-22  9:37 ` [PATCH v2 7/7] cpufreq: make schedutil the default for arm and arm64 Ionela Voinescu
  6 siblings, 1 reply; 35+ messages in thread
From: Ionela Voinescu @ 2020-07-22  9:37 UTC (permalink / raw)
  To: rjw, viresh.kumar, dietmar.eggemann, catalin.marinas,
	sudeep.holla, will, linux
  Cc: linux-pm, peterz, linux-kernel, mingo, ionela.voinescu,
	Valentin Schneider, linux-arm-kernel

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

The passed cpumask arguments to:
 - arch_set_freq_scale(),
 - arch_set_thermal_pressure(), 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>
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>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 arch/arm64/kernel/topology.c  | 2 +-
 drivers/base/arch_topology.c  | 4 ++--
 drivers/cpufreq/cpufreq.c     | 5 +++--
 include/linux/arch_topology.h | 4 ++--
 include/linux/cpufreq.h       | 3 ++-
 kernel/sched/core.c           | 2 +-
 6 files changed, 11 insertions(+), 9 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 708768f528dc..3ad59e38f3f3 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -21,14 +21,14 @@
 #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;
 
 #ifndef CONFIG_BL_SWITCHER
-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 1d0b046fe8e9..7e6452143654 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -178,8 +178,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)
 {
 	if (cpufreq_sets_freq_scale())
 		static_branch_disable_cpuslocked(&cpufreq_set_freq_scale);
diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
index 0566cb3314ef..4be0315700cb 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);
 
@@ -39,7 +39,7 @@ static inline unsigned long topology_get_thermal_pressure(int cpu)
 	return per_cpu(thermal_pressure, cpu);
 }
 
-void arch_set_thermal_pressure(struct cpumask *cpus,
+void arch_set_thermal_pressure(const struct cpumask *cpus,
 			       unsigned long th_pressure);
 
 struct cpu_topology {
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index f81215ad76f1..781f1c5957ff 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -1009,7 +1009,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 */
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index f518af52d0fb..b44a42b1236c 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3645,7 +3645,7 @@ unsigned long long task_sched_runtime(struct task_struct *p)
 
 DEFINE_PER_CPU(unsigned long, thermal_pressure);
 
-void arch_set_thermal_pressure(struct cpumask *cpus,
+void arch_set_thermal_pressure(const struct cpumask *cpus,
 			       unsigned long th_pressure)
 {
 	int cpu;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2 6/7] arch_topology, arm, arm64: define arch_scale_freq_invariant()
  2020-07-22  9:37 [PATCH v2 0/7] cpufreq: improve frequency invariance support Ionela Voinescu
                   ` (4 preceding siblings ...)
  2020-07-22  9:37 ` [PATCH v2 5/7] arch_topology, cpufreq, sched/core: constify arch_* cpumasks Ionela Voinescu
@ 2020-07-22  9:37 ` Ionela Voinescu
  2020-07-30 11:44   ` [PATCH v2 6/7] arch_topology,arm,arm64: " Catalin Marinas
  2020-07-22  9:37 ` [PATCH v2 7/7] cpufreq: make schedutil the default for arm and arm64 Ionela Voinescu
  6 siblings, 1 reply; 35+ messages in thread
From: Ionela Voinescu @ 2020-07-22  9:37 UTC (permalink / raw)
  To: rjw, viresh.kumar, dietmar.eggemann, catalin.marinas,
	sudeep.holla, will, linux
  Cc: linux-pm, peterz, linux-kernel, mingo, ionela.voinescu,
	Valentin Schneider, linux-arm-kernel

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>
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 435aba289fc5..d59458e97c6d 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 0cc835ddfcd1..b67081c0dcfb 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 3ad59e38f3f3..30b67bb820c5 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_sets_freq_scale() ||
+	       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 4be0315700cb..5bc55cfc9399 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v2 7/7] cpufreq: make schedutil the default for arm and arm64
  2020-07-22  9:37 [PATCH v2 0/7] cpufreq: improve frequency invariance support Ionela Voinescu
                   ` (5 preceding siblings ...)
  2020-07-22  9:37 ` [PATCH v2 6/7] arch_topology, arm, arm64: define arch_scale_freq_invariant() Ionela Voinescu
@ 2020-07-22  9:37 ` Ionela Voinescu
  2020-07-30  4:54   ` Viresh Kumar
  6 siblings, 1 reply; 35+ messages in thread
From: Ionela Voinescu @ 2020-07-22  9:37 UTC (permalink / raw)
  To: rjw, viresh.kumar, dietmar.eggemann, catalin.marinas,
	sudeep.holla, will, linux
  Cc: linux-pm, peterz, linux-kernel, mingo, ionela.voinescu,
	Valentin Schneider, linux-arm-kernel

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

schedutil is already a hard-requirement for EAS, which has lead to making
it default on arm (when CONFIG_BIG_LITTLE), see:

  commit 8fdcca8e254a ("cpufreq: Select schedutil when using big.LITTLE")

One thing worth pointing out is that schedutil isn't only relevant for
asymmetric CPU capacity systems; for instance, schedutil is the only
governor that honours util-clamp performance requests. Another good example
of this is x86 switching to using it by default in:

  commit a00ec3874e7d ("cpufreq: intel_pstate: Select schedutil as the default governor")

Arguably it should be made the default for all architectures, but it seems
better to wait for them to also gain frequency invariance powers. Make it
the default for arm && arm64 for now.

Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
index e91750132552..2c7171e0b001 100644
--- a/drivers/cpufreq/Kconfig
+++ b/drivers/cpufreq/Kconfig
@@ -37,7 +37,7 @@ config CPU_FREQ_STAT
 choice
 	prompt "Default CPUFreq governor"
 	default CPU_FREQ_DEFAULT_GOV_USERSPACE if ARM_SA1100_CPUFREQ || ARM_SA1110_CPUFREQ
-	default CPU_FREQ_DEFAULT_GOV_SCHEDUTIL if BIG_LITTLE
+	default CPU_FREQ_DEFAULT_GOV_SCHEDUTIL if ARM64 || ARM
 	default CPU_FREQ_DEFAULT_GOV_SCHEDUTIL if X86_INTEL_PSTATE && SMP
 	default CPU_FREQ_DEFAULT_GOV_PERFORMANCE
 	help
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 1/7] cpufreq: move invariance setter calls in cpufreq core
  2020-07-22  9:37 ` [PATCH v2 1/7] cpufreq: move invariance setter calls in cpufreq core Ionela Voinescu
@ 2020-07-27 13:48   ` Rafael J. Wysocki
  2020-07-29  9:03     ` Ionela Voinescu
  2020-07-30  3:41     ` Viresh Kumar
  0 siblings, 2 replies; 35+ messages in thread
From: Rafael J. Wysocki @ 2020-07-27 13:48 UTC (permalink / raw)
  To: Ionela Voinescu
  Cc: Catalin Marinas, Linux PM, Peter Zijlstra, Viresh Kumar,
	Valentin Schneider, Rafael J. Wysocki, Russell King - ARM Linux,
	Linux Kernel Mailing List, Ingo Molnar, Sudeep Holla,
	Will Deacon, Dietmar Eggemann, Linux ARM

On Wed, Jul 22, 2020 at 11:38 AM Ionela Voinescu
<ionela.voinescu@arm.com> wrote:
>
> From: Valentin Schneider <valentin.schneider@arm.com>
>
> 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:
>
>   cpufreq-nforce2.c:371:2:      .target = nforce2_target,
>   cppc_cpufreq.c:416:2:         .target = cppc_cpufreq_set_target,
>   gx-suspmod.c:439:2:           .target = cpufreq_gx_target,
>   pcc-cpufreq.c:573:2:          .target = pcc_cpufreq_target,

Also intel_pstate in the passive mode.

>
> Should we care about drivers using this hook, we may be able to exploit
> cpufreq_freq_transition_{being, end}(). This is handled in a separate
> patch.
>
> 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).
>
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq-dt.c           | 10 +---------
>  drivers/cpufreq/cpufreq.c              | 20 +++++++++++++++++---
>  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 |  5 -----
>  6 files changed, 22 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> index 944d7b45afe9..9fd4ce774f12 100644
> --- a/drivers/cpufreq/cpufreq-dt.c
> +++ b/drivers/cpufreq/cpufreq-dt.c
> @@ -40,16 +40,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 036f4cc42ede..bac4101546db 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2058,9 +2058,16 @@ 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);
> +
> +       if (freq)
> +               arch_set_freq_scale(policy->related_cpus, freq,
> +                                   policy->cpuinfo.max_freq);

Why can't arch_set_freq_scale() handle freq == 0?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 2/7] cpufreq: set invariance scale factor on transition end
  2020-07-22  9:37 ` [PATCH v2 2/7] cpufreq: set invariance scale factor on transition end Ionela Voinescu
@ 2020-07-27 13:52   ` Rafael J. Wysocki
  2020-07-29  9:14     ` Ionela Voinescu
  2020-07-30  4:13   ` Viresh Kumar
  1 sibling, 1 reply; 35+ messages in thread
From: Rafael J. Wysocki @ 2020-07-27 13:52 UTC (permalink / raw)
  To: Ionela Voinescu
  Cc: Catalin Marinas, Linux PM, Peter Zijlstra, Viresh Kumar,
	Rafael J. Wysocki, Russell King - ARM Linux,
	Linux Kernel Mailing List, Ingo Molnar, Sudeep Holla,
	Will Deacon, Dietmar Eggemann, Linux ARM

On Wed, Jul 22, 2020 at 11:38 AM Ionela Voinescu
<ionela.voinescu@arm.com> wrote:
>
> While the move of the invariance setter calls (arch_set_freq_scale())
> from cpufreq drivers to cpufreq core maintained the previous
> functionality for existing drivers that use target_index() and
> fast_switch() for frequency switching, it also gives the possibility
> of adding support for users of the target() callback, which is exploited
> here.
>
> To be noted that the target() callback has been flagged as deprecated
> since:
>
> commit 9c0ebcf78fde ("cpufreq: Implement light weight ->target_index() routine")
>
> It also doesn't have that many users:
>
>   cpufreq-nforce2.c:371:2:      .target = nforce2_target,
>   cppc_cpufreq.c:416:2:         .target = cppc_cpufreq_set_target,
>   gx-suspmod.c:439:2:           .target = cpufreq_gx_target,
>   pcc-cpufreq.c:573:2:          .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.
>
> Two notes are worthwhile here:
>  - In __target_index(), cpufreq_freq_transition_end() is called only for
>    drivers that have synchronous notifications enabled. There is only one
>    driver that disables them,
>
>    drivers/cpufreq/powernow-k8.c:1142: .flags = CPUFREQ_ASYNC_NOTIFICATION,
>
>    which is deprecated.
>
>  - Despite marking a successful frequency change, many cpufreq drivers
>    will populate the new policy->cur with the new requested frequency,
>    although this 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 or eventually used by the scheduler.
>    The architecture is in a better position to decide if it has better
>    methods to obtain more accurate information regarding the current
>    frequency (for example the use of counters).
>
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index bac4101546db..3497c1cd6818 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -448,6 +448,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;
>
> @@ -2159,7 +2163,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
>                             unsigned int relation)
>  {
>         unsigned int old_target_freq = target_freq;
> -       int index, retval;
> +       int index;
>
>         if (cpufreq_disabled())
>                 return -ENODEV;
> @@ -2190,14 +2194,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
>
>         index = cpufreq_frequency_table_target(policy, target_freq, relation);
>
> -       retval = __target_index(policy, index);
> -
> -       if (!retval)
> -               arch_set_freq_scale(policy->related_cpus,
> -                                   policy->freq_table[index].frequency,
> -                                   policy->cpuinfo.max_freq);
> -
> -       return retval;
> +       return __target_index(policy, index);
>  }
>  EXPORT_SYMBOL_GPL(__cpufreq_driver_target);
>
> --

I would fold this patch into the previous one.

I don't see much reason for it to be separate and it looks like
folding it in would cause the previous patch to be simpler.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 4/7] cpufreq: report whether cpufreq supports Frequency Invariance (FI)
  2020-07-22  9:37 ` [PATCH v2 4/7] cpufreq: report whether cpufreq supports Frequency Invariance (FI) Ionela Voinescu
@ 2020-07-27 14:02   ` Rafael J. Wysocki
  2020-07-29 14:39     ` Ionela Voinescu
  2020-07-30  4:43   ` Viresh Kumar
  1 sibling, 1 reply; 35+ messages in thread
From: Rafael J. Wysocki @ 2020-07-27 14:02 UTC (permalink / raw)
  To: Ionela Voinescu
  Cc: Catalin Marinas, Linux PM, Peter Zijlstra, Viresh Kumar,
	Rafael J. Wysocki, Russell King - ARM Linux,
	Linux Kernel Mailing List, Ingo Molnar, Sudeep Holla,
	Will Deacon, Dietmar Eggemann, Linux ARM

On Wed, Jul 22, 2020 at 11:38 AM Ionela Voinescu
<ionela.voinescu@arm.com> 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 error and debug messages, together with 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
>  - if enabled, cpufreq-based invariance will be disabled during the
>    call of the default arch_set_freq_scale() function which does
>    not set a scale factor.
>
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 28 ++++++++++++++++++++++++++++
>  include/linux/cpufreq.h   |  5 +++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 3497c1cd6818..1d0b046fe8e9 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -61,6 +61,9 @@ static struct cpufreq_driver *cpufreq_driver;
>  static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
>  static DEFINE_RWLOCK(cpufreq_driver_lock);
>
> +/* Mark support for the scheduler's frequency invariance engine */
> +static DEFINE_STATIC_KEY_FALSE(cpufreq_set_freq_scale);
> +
>  /* Flag to suspend/resume CPUFreq governors */
>  static bool cpufreq_suspended;
>
> @@ -69,6 +72,25 @@ static inline bool has_target(void)
>         return cpufreq_driver->target_index || cpufreq_driver->target;
>  }
>
> +static inline
> +void enable_cpufreq_freq_invariance(struct cpufreq_driver *driver)
> +{
> +       if ((driver->target || driver->target_index || driver->fast_switch) &&
> +           !driver->setpolicy) {
> +
> +               static_branch_enable_cpuslocked(&cpufreq_set_freq_scale);
> +               pr_debug("%s: Driver %s can provide frequency invariance.",
> +                        __func__, driver->name);
> +       } else
> +               pr_err("%s: Driver %s cannot provide frequency invariance.",
> +               __func__, driver->name);

This doesn't follow the kernel coding style (the braces around the
pr_err() statement are missing).

Besides, IMO on architectures where arch_set_freq_scale() is empty,
this should be empty as well.

> +}
> +
> +bool cpufreq_sets_freq_scale(void)
> +{
> +       return static_branch_likely(&cpufreq_set_freq_scale);
> +}
> +
>  /* internal prototypes */
>  static unsigned int __cpufreq_get(struct cpufreq_policy *policy);
>  static int cpufreq_init_governor(struct cpufreq_policy *policy);
> @@ -159,6 +181,9 @@ EXPORT_SYMBOL_GPL(get_cpu_idle_time);
>  __weak void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
>                 unsigned long max_freq)
>  {
> +       if (cpufreq_sets_freq_scale())
> +               static_branch_disable_cpuslocked(&cpufreq_set_freq_scale);
> +
>  }
>  EXPORT_SYMBOL_GPL(arch_set_freq_scale);
>
> @@ -2722,6 +2747,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
>         cpufreq_driver = driver_data;
>         write_unlock_irqrestore(&cpufreq_driver_lock, flags);
>
> +       enable_cpufreq_freq_invariance(cpufreq_driver);
> +
>         if (driver_data->setpolicy)
>                 driver_data->flags |= CPUFREQ_CONST_LOOPS;
>
> @@ -2791,6 +2818,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_set_freq_scale);
>         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 e62b022cb07e..f81215ad76f1 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_sets_freq_scale(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_sets_freq_scale(void)
> +{
> +       return false;
> +}
>  static inline void disable_cpufreq(void) { }
>  #endif
>
> --
> 2.17.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 1/7] cpufreq: move invariance setter calls in cpufreq core
  2020-07-27 13:48   ` Rafael J. Wysocki
@ 2020-07-29  9:03     ` Ionela Voinescu
  2020-07-30  3:41     ` Viresh Kumar
  1 sibling, 0 replies; 35+ messages in thread
From: Ionela Voinescu @ 2020-07-29  9:03 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Catalin Marinas, Linux PM, Peter Zijlstra, Viresh Kumar,
	Valentin Schneider, Rafael J. Wysocki, Russell King - ARM Linux,
	Linux Kernel Mailing List, Ingo Molnar, Sudeep Holla,
	Will Deacon, Dietmar Eggemann, Linux ARM

Hi Rafael,

On Monday 27 Jul 2020 at 15:48:39 (+0200), Rafael J. Wysocki wrote:
> On Wed, Jul 22, 2020 at 11:38 AM Ionela Voinescu
> <ionela.voinescu@arm.com> wrote:
[..]
> > 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:
> >
> >   cpufreq-nforce2.c:371:2:      .target = nforce2_target,
> >   cppc_cpufreq.c:416:2:         .target = cppc_cpufreq_set_target,
> >   gx-suspmod.c:439:2:           .target = cpufreq_gx_target,
> >   pcc-cpufreq.c:573:2:          .target = pcc_cpufreq_target,
> 
> Also intel_pstate in the passive mode.
> 

Thanks! I'll update the list. Somehow I missed a few others:

$ grep -E -Hn -r '\.target\s*='

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,

Sorry about that!

Many thanks for the review,
Ionela.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 2/7] cpufreq: set invariance scale factor on transition end
  2020-07-27 13:52   ` Rafael J. Wysocki
@ 2020-07-29  9:14     ` Ionela Voinescu
  0 siblings, 0 replies; 35+ messages in thread
From: Ionela Voinescu @ 2020-07-29  9:14 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Catalin Marinas, Linux PM, Peter Zijlstra, Viresh Kumar,
	Rafael J. Wysocki, Russell King - ARM Linux,
	Linux Kernel Mailing List, Ingo Molnar, Sudeep Holla,
	Will Deacon, Dietmar Eggemann, Linux ARM

On Monday 27 Jul 2020 at 15:52:41 (+0200), Rafael J. Wysocki wrote:
> On Wed, Jul 22, 2020 at 11:38 AM Ionela Voinescu
> <ionela.voinescu@arm.com> wrote:
> >
> > While the move of the invariance setter calls (arch_set_freq_scale())
> > from cpufreq drivers to cpufreq core maintained the previous
> > functionality for existing drivers that use target_index() and
> > fast_switch() for frequency switching, it also gives the possibility
> > of adding support for users of the target() callback, which is exploited
> > here.
> >
> > To be noted that the target() callback has been flagged as deprecated
> > since:
> >
> > commit 9c0ebcf78fde ("cpufreq: Implement light weight ->target_index() routine")
> >
> > It also doesn't have that many users:
> >
> >   cpufreq-nforce2.c:371:2:      .target = nforce2_target,
> >   cppc_cpufreq.c:416:2:         .target = cppc_cpufreq_set_target,
> >   gx-suspmod.c:439:2:           .target = cpufreq_gx_target,
> >   pcc-cpufreq.c:573:2:          .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.
> >
> > Two notes are worthwhile here:
> >  - In __target_index(), cpufreq_freq_transition_end() is called only for
> >    drivers that have synchronous notifications enabled. There is only one
> >    driver that disables them,
> >
> >    drivers/cpufreq/powernow-k8.c:1142: .flags = CPUFREQ_ASYNC_NOTIFICATION,
> >
> >    which is deprecated.
> >
> >  - Despite marking a successful frequency change, many cpufreq drivers
> >    will populate the new policy->cur with the new requested frequency,
> >    although this 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 or eventually used by the scheduler.
> >    The architecture is in a better position to decide if it has better
> >    methods to obtain more accurate information regarding the current
> >    frequency (for example the use of counters).
> >
[..]

> I would fold this patch into the previous one.
> 
> I don't see much reason for it to be separate and it looks like
> folding it in would cause the previous patch to be simpler.

I kept it separate in this version as a proposal to move the call to
cpufreq_freq_transition_end() and properly justify it in the commit
message.

I'll squash it into the previous one, as recommended.

Thanks,
Ionela.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 4/7] cpufreq: report whether cpufreq supports Frequency Invariance (FI)
  2020-07-27 14:02   ` Rafael J. Wysocki
@ 2020-07-29 14:39     ` Ionela Voinescu
  0 siblings, 0 replies; 35+ messages in thread
From: Ionela Voinescu @ 2020-07-29 14:39 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Catalin Marinas, Linux PM, Peter Zijlstra, Viresh Kumar,
	Rafael J. Wysocki, Russell King - ARM Linux,
	Linux Kernel Mailing List, Ingo Molnar, Sudeep Holla,
	Will Deacon, Dietmar Eggemann, Linux ARM

Hi,

On Monday 27 Jul 2020 at 16:02:18 (+0200), Rafael J. Wysocki wrote:
> On Wed, Jul 22, 2020 at 11:38 AM Ionela Voinescu
> <ionela.voinescu@arm.com> wrote:
[..]
> > +static inline
> > +void enable_cpufreq_freq_invariance(struct cpufreq_driver *driver)
> > +{
> > +       if ((driver->target || driver->target_index || driver->fast_switch) &&
> > +           !driver->setpolicy) {
> > +
> > +               static_branch_enable_cpuslocked(&cpufreq_set_freq_scale);
> > +               pr_debug("%s: Driver %s can provide frequency invariance.",
> > +                        __func__, driver->name);
> > +       } else
> > +               pr_err("%s: Driver %s cannot provide frequency invariance.",
> > +               __func__, driver->name);
> 
> This doesn't follow the kernel coding style (the braces around the
> pr_err() statement are missing).
> 

I'll fix this.

Also, depending on the result of the discussion below, it might be best
for this to be a warning, not an error.

> Besides, IMO on architectures where arch_set_freq_scale() is empty,
> this should be empty as well.
>

Yes, you are right, there are two aspects here:
 - (1) Whether a driver *can* provide frequency invariance. IOW, whether
   it implements the callbacks that result in the call to
   arch_set_freq_scale().

 - (2) Whether cpufreq/driver *does* provide frequency invariance. IOW,
   whether the call to arch_set_freq_scale() actually results in the
   setting of the scale factor.

Even when creating this v2 I was going back and forth between the options
for this:

(a) cpufreq should report whether it *can* provide frequency invariance
    (as described at (1)). If we go for this, for clarity I should change

    s/cpufreq_set_freq_scale/cpufreq_can_set_freq_scale_key
    s/cpufreq_sets_freq_scale()/cpufreq_can_set_freq_scale()

    Through this, cpufreq only reports that it calls
    arch_set_freq_scale(), independent on whether that call results in a
    scale factor being set. Then it would be up to the caller to ensure
    this information is used with a proper definition of
    arch_set_freq_scale().

(b) cpufreq should report whether it *does* provide frequency invariance

    A way of doing this is to use a arch_set_freq_scale define (as done
    for other arch functions, for example arch_scale_freq_tick()) and
    guard this enable_cpufreq_freq_invariance() function based on that
    definition.
    Therefore, cpufreq_sets_freq_scale() would report whether
    enable_cpufreq_freq_invariance() was successful and there is an
    external definition of arch_set_freq_scale() that sets the scale
    factor.


The current version is somewhat a combination of (a) and (b):
cpufreq_set_freq_scale would initially be enabled if the proper callbacks
are implemented (a), but later if the weak version of
arch_set_freq_scale() is called, we disabled it (b) (as can be seen below).

[..]
> >  __weak void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
> >                 unsigned long max_freq)
> >  {
> > +       if (cpufreq_sets_freq_scale())
> > +               static_branch_disable_cpuslocked(&cpufreq_set_freq_scale);
> > +
> >  }
> >  EXPORT_SYMBOL_GPL(arch_set_freq_scale);

I suppose a clear (a) or (b) solution might be better here.

IMO, given that (b) cannot actually guarantee that a scale factor is set
through arch_set_freq_scale() given cpufreq information about current and
maximum frequencies, for me (a) is preferred as it conveys the only
information that cpufreq can convey - the fact that it *can* set the scale
factor, not that it *does*.

Can you please confirm whether you still prefer (b), given the details
above?

Thank you,
Ionela.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 1/7] cpufreq: move invariance setter calls in cpufreq core
  2020-07-27 13:48   ` Rafael J. Wysocki
  2020-07-29  9:03     ` Ionela Voinescu
@ 2020-07-30  3:41     ` Viresh Kumar
  2020-08-03 13:26       ` Ionela Voinescu
  1 sibling, 1 reply; 35+ messages in thread
From: Viresh Kumar @ 2020-07-30  3:41 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Catalin Marinas, Linux PM, Peter Zijlstra, Will Deacon,
	Valentin Schneider, Rafael J. Wysocki, Russell King - ARM Linux,
	Linux Kernel Mailing List, Ingo Molnar, Sudeep Holla,
	Ionela Voinescu, Dietmar Eggemann, Linux ARM

On 27-07-20, 15:48, Rafael J. Wysocki wrote:
> On Wed, Jul 22, 2020 at 11:38 AM Ionela Voinescu
> <ionela.voinescu@arm.com> wrote:
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 036f4cc42ede..bac4101546db 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -2058,9 +2058,16 @@ 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);
> > +
> > +       if (freq)
> > +               arch_set_freq_scale(policy->related_cpus, freq,
> > +                                   policy->cpuinfo.max_freq);
> 
> Why can't arch_set_freq_scale() handle freq == 0?

Actually there is no need to. AFAIU the freq returned by fast_switch
can never be 0 (yeah qcom driver does it right now and I am fixing
it). And so we can drop this check altogether.

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 2/7] cpufreq: set invariance scale factor on transition end
  2020-07-22  9:37 ` [PATCH v2 2/7] cpufreq: set invariance scale factor on transition end Ionela Voinescu
  2020-07-27 13:52   ` Rafael J. Wysocki
@ 2020-07-30  4:13   ` Viresh Kumar
  2020-08-03 13:58     ` Ionela Voinescu
  1 sibling, 1 reply; 35+ messages in thread
From: Viresh Kumar @ 2020-07-30  4:13 UTC (permalink / raw)
  To: Ionela Voinescu
  Cc: linux-pm, peterz, catalin.marinas, rjw, linux, linux-kernel,
	mingo, sudeep.holla, will, dietmar.eggemann, linux-arm-kernel

On 22-07-20, 10:37, Ionela Voinescu wrote:
> While the move of the invariance setter calls (arch_set_freq_scale())
> from cpufreq drivers to cpufreq core maintained the previous
> functionality for existing drivers that use target_index() and
> fast_switch() for frequency switching, it also gives the possibility
> of adding support for users of the target() callback, which is exploited
> here.
> 
> To be noted that the target() callback has been flagged as deprecated
> since:
> 
> commit 9c0ebcf78fde ("cpufreq: Implement light weight ->target_index() routine")
> 
> It also doesn't have that many users:
> 
>   cpufreq-nforce2.c:371:2:      .target = nforce2_target,
>   cppc_cpufreq.c:416:2:         .target = cppc_cpufreq_set_target,
>   gx-suspmod.c:439:2:           .target = cpufreq_gx_target,
>   pcc-cpufreq.c:573:2:          .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.
> 
> Two notes are worthwhile here:
>  - In __target_index(), cpufreq_freq_transition_end() is called only for
>    drivers that have synchronous notifications enabled. There is only one
>    driver that disables them,
> 
>    drivers/cpufreq/powernow-k8.c:1142: .flags = CPUFREQ_ASYNC_NOTIFICATION,
> 
>    which is deprecated.

I don't think this is deprecated.

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 3/7] arch_topology: disable frequency invariance for CONFIG_BL_SWITCHER
  2020-07-22  9:37 ` [PATCH v2 3/7] arch_topology: disable frequency invariance for CONFIG_BL_SWITCHER Ionela Voinescu
@ 2020-07-30  4:24   ` Viresh Kumar
  2020-07-30 10:29     ` Dietmar Eggemann
  0 siblings, 1 reply; 35+ messages in thread
From: Viresh Kumar @ 2020-07-30  4:24 UTC (permalink / raw)
  To: Ionela Voinescu
  Cc: linux-pm, peterz, catalin.marinas, rjw, linux, linux-kernel,
	mingo, sudeep.holla, will, dietmar.eggemann, linux-arm-kernel

On 22-07-20, 10:37, Ionela Voinescu wrote:
> +++ b/drivers/base/arch_topology.c
> @@ -27,6 +27,7 @@ __weak bool arch_freq_counters_available(struct cpumask *cpus)
>  }
>  DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE;
>  
> +#ifndef CONFIG_BL_SWITCHER
>  void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
>  			 unsigned long max_freq)
>  {
> @@ -46,6 +47,7 @@ void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
>  	for_each_cpu(i, cpus)
>  		per_cpu(freq_scale, i) = scale;
>  }
> +#endif

I don't really like this change, the ifdef hackery is disgusting and
then we are putting that in a completely different part of the kernel.

There are at least these two ways of solving this, maybe more:

- Fix the bl switcher driver and add the complexity in it (which you
  tried to do earlier).

- Add a cpufreq flag to skip arch-set-freq-scale call.

Rafael ?

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 4/7] cpufreq: report whether cpufreq supports Frequency Invariance (FI)
  2020-07-22  9:37 ` [PATCH v2 4/7] cpufreq: report whether cpufreq supports Frequency Invariance (FI) Ionela Voinescu
  2020-07-27 14:02   ` Rafael J. Wysocki
@ 2020-07-30  4:43   ` Viresh Kumar
  2020-08-03 15:24     ` Ionela Voinescu
  1 sibling, 1 reply; 35+ messages in thread
From: Viresh Kumar @ 2020-07-30  4:43 UTC (permalink / raw)
  To: Ionela Voinescu
  Cc: linux-pm, peterz, catalin.marinas, rjw, linux, linux-kernel,
	mingo, sudeep.holla, will, dietmar.eggemann, linux-arm-kernel

On 22-07-20, 10:37, Ionela Voinescu wrote:
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 3497c1cd6818..1d0b046fe8e9 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -61,6 +61,9 @@ static struct cpufreq_driver *cpufreq_driver;
>  static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
>  static DEFINE_RWLOCK(cpufreq_driver_lock);
>  
> +/* Mark support for the scheduler's frequency invariance engine */
> +static DEFINE_STATIC_KEY_FALSE(cpufreq_set_freq_scale);
> +
>  /* Flag to suspend/resume CPUFreq governors */
>  static bool cpufreq_suspended;
>  
> @@ -69,6 +72,25 @@ static inline bool has_target(void)
>  	return cpufreq_driver->target_index || cpufreq_driver->target;
>  }
>  
> +static inline
> +void enable_cpufreq_freq_invariance(struct cpufreq_driver *driver)
> +{
> +	if ((driver->target || driver->target_index || driver->fast_switch) &&
> +	    !driver->setpolicy) {

Just checking for !driver->setpolicy should be enough here.

> +		static_branch_enable_cpuslocked(&cpufreq_set_freq_scale);
> +		pr_debug("%s: Driver %s can provide frequency invariance.",
> +			 __func__, driver->name);

I think a simpler print will work well too.

                pr_debug("Freq invariance enabled");

__func__ isn't really required as this is the only print with that
kind of info in cpufreq.c.

> +	} else
> +		pr_err("%s: Driver %s cannot provide frequency invariance.",
> +		__func__, driver->name);

Why not supporting freq-invariance an error ? I will just drop this
message completely.

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 7/7] cpufreq: make schedutil the default for arm and arm64
  2020-07-22  9:37 ` [PATCH v2 7/7] cpufreq: make schedutil the default for arm and arm64 Ionela Voinescu
@ 2020-07-30  4:54   ` Viresh Kumar
  0 siblings, 0 replies; 35+ messages in thread
From: Viresh Kumar @ 2020-07-30  4:54 UTC (permalink / raw)
  To: Ionela Voinescu
  Cc: linux-pm, peterz, catalin.marinas, Valentin Schneider, rjw,
	linux, linux-kernel, mingo, sudeep.holla, will, dietmar.eggemann,
	linux-arm-kernel

On 22-07-20, 10:37, Ionela Voinescu wrote:
> From: Valentin Schneider <valentin.schneider@arm.com>
> 
> schedutil is already a hard-requirement for EAS, which has lead to making
> it default on arm (when CONFIG_BIG_LITTLE), see:
> 
>   commit 8fdcca8e254a ("cpufreq: Select schedutil when using big.LITTLE")
> 
> One thing worth pointing out is that schedutil isn't only relevant for
> asymmetric CPU capacity systems; for instance, schedutil is the only
> governor that honours util-clamp performance requests. Another good example
> of this is x86 switching to using it by default in:
> 
>   commit a00ec3874e7d ("cpufreq: intel_pstate: Select schedutil as the default governor")
> 
> Arguably it should be made the default for all architectures, but it seems
> better to wait for them to also gain frequency invariance powers. Make it
> the default for arm && arm64 for now.
> 
> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> index e91750132552..2c7171e0b001 100644
> --- a/drivers/cpufreq/Kconfig
> +++ b/drivers/cpufreq/Kconfig
> @@ -37,7 +37,7 @@ config CPU_FREQ_STAT
>  choice
>  	prompt "Default CPUFreq governor"
>  	default CPU_FREQ_DEFAULT_GOV_USERSPACE if ARM_SA1100_CPUFREQ || ARM_SA1110_CPUFREQ
> -	default CPU_FREQ_DEFAULT_GOV_SCHEDUTIL if BIG_LITTLE
> +	default CPU_FREQ_DEFAULT_GOV_SCHEDUTIL if ARM64 || ARM
>  	default CPU_FREQ_DEFAULT_GOV_SCHEDUTIL if X86_INTEL_PSTATE && SMP
>  	default CPU_FREQ_DEFAULT_GOV_PERFORMANCE
>  	help

Applied. Thanks.

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 3/7] arch_topology: disable frequency invariance for CONFIG_BL_SWITCHER
  2020-07-30  4:24   ` Viresh Kumar
@ 2020-07-30 10:29     ` Dietmar Eggemann
  2020-07-31 15:48       ` Sudeep Holla
  2020-08-04  6:30       ` Viresh Kumar
  0 siblings, 2 replies; 35+ messages in thread
From: Dietmar Eggemann @ 2020-07-30 10:29 UTC (permalink / raw)
  To: Viresh Kumar, Ionela Voinescu
  Cc: linux-pm, peterz, catalin.marinas, rjw, linux, linux-kernel,
	mingo, sudeep.holla, will, linux-arm-kernel

On 30/07/2020 06:24, Viresh Kumar wrote:
> On 22-07-20, 10:37, Ionela Voinescu wrote:
>> +++ b/drivers/base/arch_topology.c
>> @@ -27,6 +27,7 @@ __weak bool arch_freq_counters_available(struct cpumask *cpus)
>>  }
>>  DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE;
>>  
>> +#ifndef CONFIG_BL_SWITCHER
>>  void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
>>  			 unsigned long max_freq)
>>  {
>> @@ -46,6 +47,7 @@ void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
>>  	for_each_cpu(i, cpus)
>>  		per_cpu(freq_scale, i) = scale;
>>  }
>> +#endif
> 
> I don't really like this change, the ifdef hackery is disgusting and
> then we are putting that in a completely different part of the kernel.
> 
> There are at least these two ways of solving this, maybe more:
> 
> - Fix the bl switcher driver and add the complexity in it (which you
>   tried to do earlier).
> 
> - Add a cpufreq flag to skip arch-set-freq-scale call.

I agree it's not nice but IMHO the cpufreq flag is worse since we would
introduce new infrastructure only for a deprecated feature. I'm assuming
that BL SWITCHER is the only feature needing this CPUfreq flag extension.

#ifdef CONFIG_BL_SWITCHER is already in drivers/irqchip/irq-gic.c so
it's ugly already.

Runtime detecting (via bL_switching_enabled) of BL SWITCHER is right now
also only handled inside vexpress-spc-cpufreq.c via a
bL_switcher_notifier. A mechanism which also sits behind a #ifdef
CONFIG_BL_SWITCHER.


So IMHO, the additional #ifdef CONFIG_BL_SWITCHER in
drivers/base/arch_topology.c it's a small price to pay.

Are there still any users of CONFIG_BL_SWITCHER? I guess it's only
limited to A15/A7 systems w/ vexpress-spc-cpufreq.c ... so probably only
TC2?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 5/7] arch_topology, cpufreq, sched/core: constify arch_* cpumasks
  2020-07-22  9:37 ` [PATCH v2 5/7] arch_topology, cpufreq, sched/core: constify arch_* cpumasks Ionela Voinescu
@ 2020-07-30 11:43   ` Catalin Marinas
  0 siblings, 0 replies; 35+ messages in thread
From: Catalin Marinas @ 2020-07-30 11:43 UTC (permalink / raw)
  To: Ionela Voinescu
  Cc: linux-pm, peterz, viresh.kumar, Valentin Schneider, rjw, linux,
	linux-kernel, mingo, sudeep.holla, will, dietmar.eggemann,
	linux-arm-kernel

On Wed, Jul 22, 2020 at 10:37:30AM +0100, Ionela Voinescu wrote:
> From: Valentin Schneider <valentin.schneider@arm.com>
> 
> The passed cpumask arguments to:
>  - arch_set_freq_scale(),
>  - arch_set_thermal_pressure(), 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>
> 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>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 6/7] arch_topology,arm,arm64: define arch_scale_freq_invariant()
  2020-07-22  9:37 ` [PATCH v2 6/7] arch_topology, arm, arm64: define arch_scale_freq_invariant() Ionela Voinescu
@ 2020-07-30 11:44   ` Catalin Marinas
  0 siblings, 0 replies; 35+ messages in thread
From: Catalin Marinas @ 2020-07-30 11:44 UTC (permalink / raw)
  To: Ionela Voinescu
  Cc: linux-pm, peterz, viresh.kumar, Valentin Schneider, rjw, linux,
	linux-kernel, mingo, sudeep.holla, will, dietmar.eggemann,
	linux-arm-kernel

On Wed, Jul 22, 2020 at 10:37:31AM +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>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 3/7] arch_topology: disable frequency invariance for CONFIG_BL_SWITCHER
  2020-07-30 10:29     ` Dietmar Eggemann
@ 2020-07-31 15:48       ` Sudeep Holla
  2020-08-03 14:39         ` Ionela Voinescu
  2020-08-04  6:30       ` Viresh Kumar
  1 sibling, 1 reply; 35+ messages in thread
From: Sudeep Holla @ 2020-07-31 15:48 UTC (permalink / raw)
  To: Dietmar Eggemann
  Cc: will, linux-pm, peterz, Viresh Kumar, rjw, linux, linux-kernel,
	mingo, catalin.marinas, Ionela Voinescu, linux-arm-kernel

On Thu, Jul 30, 2020 at 12:29:52PM +0200, Dietmar Eggemann wrote:

[...]

>
> Are there still any users of CONFIG_BL_SWITCHER? I guess it's only
> limited to A15/A7 systems w/ vexpress-spc-cpufreq.c ... so probably only
> TC2?

I think so as no one shouted when I merged bL switcher driver into
vexpress-spc-cpufreq.c

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 1/7] cpufreq: move invariance setter calls in cpufreq core
  2020-07-30  3:41     ` Viresh Kumar
@ 2020-08-03 13:26       ` Ionela Voinescu
  2020-08-03 13:46         ` Rafael J. Wysocki
  0 siblings, 1 reply; 35+ messages in thread
From: Ionela Voinescu @ 2020-08-03 13:26 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael J. Wysocki, Peter Zijlstra, Catalin Marinas, Linux PM,
	Rafael J. Wysocki, Russell King - ARM Linux,
	Linux Kernel Mailing List, Valentin Schneider, Ingo Molnar,
	Sudeep Holla, Will Deacon, Dietmar Eggemann, Linux ARM

Hi guys,

On Thursday 30 Jul 2020 at 09:11:28 (+0530), Viresh Kumar wrote:
> On 27-07-20, 15:48, Rafael J. Wysocki wrote:
> > On Wed, Jul 22, 2020 at 11:38 AM Ionela Voinescu
> > <ionela.voinescu@arm.com> wrote:
> > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > index 036f4cc42ede..bac4101546db 100644
> > > --- a/drivers/cpufreq/cpufreq.c
> > > +++ b/drivers/cpufreq/cpufreq.c
> > > @@ -2058,9 +2058,16 @@ 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);
> > > +
> > > +       if (freq)
> > > +               arch_set_freq_scale(policy->related_cpus, freq,
> > > +                                   policy->cpuinfo.max_freq);
> > 
> > Why can't arch_set_freq_scale() handle freq == 0?
> 

Sorry, I seem to have missed this question the first time around.

arch_set_freq_scale() could handle freq == 0, but given that freq == 0
is signaling an error here, I do believe this check is well placed, to
prevent a useless call to arch_set_freq_scale(). Also [1]:

"""
 * If 0 is returned by the driver's ->fast_switch() callback to indicate an
 * error condition, the hardware configuration must be preserved.
 */
"""

> Actually there is no need to. AFAIU the freq returned by fast_switch
> can never be 0 (yeah qcom driver does it right now and I am fixing
> it). And so we can drop this check altogether.
> 

It's not only the qcom driver, it's also the scmi driver that could
return 0 [2]. But I don't think "fixing" these drivers is the solution,
given that 0 is indicated as a valid return value of .fast_switch() to
signal an error condition [1], while schedutil (the caller), also does
validation that the returned frequency is !0 before setting it as
current frequency [3].

Therefore, it is know and (somewhat) documented that 0 indicates an
error condition and it should be allowed as a return value for
.fast_switch(). Also, I believe is a good idea to leave the option for
drivers to return 0 (signaling error) from their implementation of
.fast_switch().

[1] https://elixir.bootlin.com/linux/v5.8-rc4/source/drivers/cpufreq/cpufreq.c#L2043
[2] https://elixir.bootlin.com/linux/v5.8-rc4/source/drivers/cpufreq/scmi-cpufreq.c#L76
[3] https://elixir.bootlin.com/linux/v5.8-rc4/source/kernel/sched/cpufreq_schedutil.c#L124

Thanks,
Ionela.

> -- 
> viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 1/7] cpufreq: move invariance setter calls in cpufreq core
  2020-08-03 13:26       ` Ionela Voinescu
@ 2020-08-03 13:46         ` Rafael J. Wysocki
  2020-08-03 14:16           ` Ionela Voinescu
  0 siblings, 1 reply; 35+ messages in thread
From: Rafael J. Wysocki @ 2020-08-03 13:46 UTC (permalink / raw)
  To: Ionela Voinescu
  Cc: Catalin Marinas, Rafael J. Wysocki, Peter Zijlstra, Viresh Kumar,
	Linux PM, Rafael J. Wysocki, Russell King - ARM Linux,
	Linux Kernel Mailing List, Valentin Schneider, Ingo Molnar,
	Sudeep Holla, Will Deacon, Dietmar Eggemann, Linux ARM

On Mon, Aug 3, 2020 at 3:26 PM Ionela Voinescu <ionela.voinescu@arm.com> wrote:
>
> Hi guys,
>
> On Thursday 30 Jul 2020 at 09:11:28 (+0530), Viresh Kumar wrote:
> > On 27-07-20, 15:48, Rafael J. Wysocki wrote:
> > > On Wed, Jul 22, 2020 at 11:38 AM Ionela Voinescu
> > > <ionela.voinescu@arm.com> wrote:
> > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > > index 036f4cc42ede..bac4101546db 100644
> > > > --- a/drivers/cpufreq/cpufreq.c
> > > > +++ b/drivers/cpufreq/cpufreq.c
> > > > @@ -2058,9 +2058,16 @@ 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);
> > > > +
> > > > +       if (freq)
> > > > +               arch_set_freq_scale(policy->related_cpus, freq,
> > > > +                                   policy->cpuinfo.max_freq);
> > >
> > > Why can't arch_set_freq_scale() handle freq == 0?
> >
>
> Sorry, I seem to have missed this question the first time around.
>
> arch_set_freq_scale() could handle freq == 0, but given that freq == 0
> is signaling an error here, I do believe this check is well placed, to
> prevent a useless call to arch_set_freq_scale(). Also [1]:

So let me rephrase:

Doesn't this check add overhead in the empty arch_set_freq_scale() case?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 2/7] cpufreq: set invariance scale factor on transition end
  2020-07-30  4:13   ` Viresh Kumar
@ 2020-08-03 13:58     ` Ionela Voinescu
  2020-08-04  6:26       ` Viresh Kumar
  0 siblings, 1 reply; 35+ messages in thread
From: Ionela Voinescu @ 2020-08-03 13:58 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: linux-pm, peterz, catalin.marinas, rjw, linux, linux-kernel,
	mingo, sudeep.holla, will, dietmar.eggemann, linux-arm-kernel

Hi Viresh,

On Thursday 30 Jul 2020 at 09:43:34 (+0530), Viresh Kumar wrote:
> On 22-07-20, 10:37, Ionela Voinescu wrote:
> > While the move of the invariance setter calls (arch_set_freq_scale())
> > from cpufreq drivers to cpufreq core maintained the previous
> > functionality for existing drivers that use target_index() and
> > fast_switch() for frequency switching, it also gives the possibility
> > of adding support for users of the target() callback, which is exploited
> > here.
> > 
> > To be noted that the target() callback has been flagged as deprecated
> > since:
> > 
> > commit 9c0ebcf78fde ("cpufreq: Implement light weight ->target_index() routine")
> > 
> > It also doesn't have that many users:
> > 
> >   cpufreq-nforce2.c:371:2:      .target = nforce2_target,
> >   cppc_cpufreq.c:416:2:         .target = cppc_cpufreq_set_target,
> >   gx-suspmod.c:439:2:           .target = cpufreq_gx_target,
> >   pcc-cpufreq.c:573:2:          .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.
> > 
> > Two notes are worthwhile here:
> >  - In __target_index(), cpufreq_freq_transition_end() is called only for
> >    drivers that have synchronous notifications enabled. There is only one
> >    driver that disables them,
> > 
> >    drivers/cpufreq/powernow-k8.c:1142: .flags = CPUFREQ_ASYNC_NOTIFICATION,
> > 
> >    which is deprecated.
> 
> I don't think this is deprecated.

Sorry, possibly 'deprecated' is a strong word.

As far as I knew acpi_cpufreq was recommended more recently for K8/K10
CPUs so that's why I decided not to create a special case for it, also
considering that it was not supporting cpufreq-based frequency
invariance to begin with.

We could support this as well by having a call to arch_set_freq_scale()
on the else path in __target_index(). But given that there was only this
one user of CPUFREQ_ASYNC_NOTIFICATION, I thought I'd propose this simpler
version first.

Let me know if my reasoning is wrong.

Thank you,
Ionela.

> 
> -- 
> viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 1/7] cpufreq: move invariance setter calls in cpufreq core
  2020-08-03 13:46         ` Rafael J. Wysocki
@ 2020-08-03 14:16           ` Ionela Voinescu
  0 siblings, 0 replies; 35+ messages in thread
From: Ionela Voinescu @ 2020-08-03 14:16 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Catalin Marinas, Linux PM, Peter Zijlstra, Viresh Kumar,
	Valentin Schneider, Rafael J. Wysocki, Russell King - ARM Linux,
	Linux Kernel Mailing List, Ingo Molnar, Sudeep Holla,
	Will Deacon, Dietmar Eggemann, Linux ARM

Hi Rafael,

On Monday 03 Aug 2020 at 15:46:59 (+0200), Rafael J. Wysocki wrote:
> On Mon, Aug 3, 2020 at 3:26 PM Ionela Voinescu <ionela.voinescu@arm.com> wrote:
> >
> > Hi guys,
> >
> > On Thursday 30 Jul 2020 at 09:11:28 (+0530), Viresh Kumar wrote:
> > > On 27-07-20, 15:48, Rafael J. Wysocki wrote:
> > > > On Wed, Jul 22, 2020 at 11:38 AM Ionela Voinescu
> > > > <ionela.voinescu@arm.com> wrote:
> > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > > > index 036f4cc42ede..bac4101546db 100644
> > > > > --- a/drivers/cpufreq/cpufreq.c
> > > > > +++ b/drivers/cpufreq/cpufreq.c
> > > > > @@ -2058,9 +2058,16 @@ 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);
> > > > > +
> > > > > +       if (freq)
> > > > > +               arch_set_freq_scale(policy->related_cpus, freq,
> > > > > +                                   policy->cpuinfo.max_freq);
> > > >
> > > > Why can't arch_set_freq_scale() handle freq == 0?
> > >
> >
> > Sorry, I seem to have missed this question the first time around.
> >
> > arch_set_freq_scale() could handle freq == 0, but given that freq == 0
> > is signaling an error here, I do believe this check is well placed, to
> > prevent a useless call to arch_set_freq_scale(). Also [1]:
> 
> So let me rephrase:
> 
> Doesn't this check add overhead in the empty arch_set_freq_scale() case?

Yes, you are right, I did not consider that. I can add a patch for the
arch_topology driver's arch_set_freq_scale() to handle this and we can
remove it from here.

Thank you for pointing this out,
Ionela.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 3/7] arch_topology: disable frequency invariance for CONFIG_BL_SWITCHER
  2020-07-31 15:48       ` Sudeep Holla
@ 2020-08-03 14:39         ` Ionela Voinescu
  0 siblings, 0 replies; 35+ messages in thread
From: Ionela Voinescu @ 2020-08-03 14:39 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: linux-pm, peterz, Viresh Kumar, rjw, linux, linux-kernel, mingo,
	catalin.marinas, will, Dietmar Eggemann, linux-arm-kernel

Hi guys,

On Friday 31 Jul 2020 at 16:48:38 (+0100), Sudeep Holla wrote:
> On Thu, Jul 30, 2020 at 12:29:52PM +0200, Dietmar Eggemann wrote:
> 
> [...]
> 
> >
> > Are there still any users of CONFIG_BL_SWITCHER? I guess it's only
> > limited to A15/A7 systems w/ vexpress-spc-cpufreq.c ... so probably only
> > TC2?
> 
> I think so as no one shouted when I merged bL switcher driver into
> vexpress-spc-cpufreq.c
> 

I think a good indication is also the fact that frequency invariance was
broken for a long time for bL_switcher_enabled systems and nobody
shouted.

A way to make this nicer is to fully remove BL_SWITCHER support. This
support was valuable at its time, but given that now there is proper
asymmetric CPU capacity support, is there any reason to hold on to this?

Thanks,
Ionela.

> -- 
> Regards,
> Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 4/7] cpufreq: report whether cpufreq supports Frequency Invariance (FI)
  2020-07-30  4:43   ` Viresh Kumar
@ 2020-08-03 15:24     ` Ionela Voinescu
  2020-08-04  6:46       ` Viresh Kumar
  0 siblings, 1 reply; 35+ messages in thread
From: Ionela Voinescu @ 2020-08-03 15:24 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: linux-pm, peterz, catalin.marinas, rjw, linux, linux-kernel,
	mingo, sudeep.holla, will, dietmar.eggemann, linux-arm-kernel

Hi Viresh,

On Thursday 30 Jul 2020 at 10:13:46 (+0530), Viresh Kumar wrote:
> On 22-07-20, 10:37, Ionela Voinescu wrote:
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 3497c1cd6818..1d0b046fe8e9 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -61,6 +61,9 @@ static struct cpufreq_driver *cpufreq_driver;
> >  static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
> >  static DEFINE_RWLOCK(cpufreq_driver_lock);
> >  
> > +/* Mark support for the scheduler's frequency invariance engine */
> > +static DEFINE_STATIC_KEY_FALSE(cpufreq_set_freq_scale);
> > +
> >  /* Flag to suspend/resume CPUFreq governors */
> >  static bool cpufreq_suspended;
> >  
> > @@ -69,6 +72,25 @@ static inline bool has_target(void)
> >  	return cpufreq_driver->target_index || cpufreq_driver->target;
> >  }
> >  
> > +static inline
> > +void enable_cpufreq_freq_invariance(struct cpufreq_driver *driver)
> > +{
> > +	if ((driver->target || driver->target_index || driver->fast_switch) &&
> > +	    !driver->setpolicy) {
> 
> Just checking for !driver->setpolicy should be enough here.
> 

Right, cpufreq_register_driver() should check that at least one of them
is present (although currently cpufreq_register_driver() will return
-EINVAL if .fast_switch() alone is present - something to be fixed).

Will do, on both accounts.


> > +		static_branch_enable_cpuslocked(&cpufreq_set_freq_scale);
> > +		pr_debug("%s: Driver %s can provide frequency invariance.",
> > +			 __func__, driver->name);
> 
> I think a simpler print will work well too.
> 
>                 pr_debug("Freq invariance enabled");
> 

I think the right way of reporting this support is important here.

By enabling the static key here, we're not actually enabling frequency
invariance. So printing "Freq invariance enabled" would be very
misleading. Frequency invariance (scheduler concept) being truly enabled
depends on three things:
 - Having a source of information for current frequency and maximum
   frequency (cpufreq, counters)
 - Having arch support for using that information to set the frequency
   scale factor: arch_set_freq_scale(), arch_scale_freq_tick()
 - Having arch support for passing the set frequency scale factor to the
   scheduler and reporting support for frequency invariance:
   arch_scale_freq_capacity(), arch_scale_freq_invariant().

Therefore, cpufreq can only report that the current driver can be a
source of information for frequency invariance "Driver %s can provide
frequency invariance", but it can't guarantee that the other conditions
are accomplished.

So I would recommend to keep this original debug message.

> __func__ isn't really required as this is the only print with that
> kind of info in cpufreq.c.
>

Makes sense!

> > +	} else
> > +		pr_err("%s: Driver %s cannot provide frequency invariance.",
> > +		__func__, driver->name);
> 
> Why not supporting freq-invariance an error ? I will just drop this
> message completely.

Yes, an error does not make sense here. I was thinking to demote it to a
warning instead in my previous reply to Rafael, but removing it
altogether might be better.

Many thanks for the thorough review,
Ionela.

> 
> -- 
> viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 2/7] cpufreq: set invariance scale factor on transition end
  2020-08-03 13:58     ` Ionela Voinescu
@ 2020-08-04  6:26       ` Viresh Kumar
  2020-08-05 10:35         ` Ionela Voinescu
  0 siblings, 1 reply; 35+ messages in thread
From: Viresh Kumar @ 2020-08-04  6:26 UTC (permalink / raw)
  To: Ionela Voinescu
  Cc: linux-pm, peterz, catalin.marinas, rjw, linux, linux-kernel,
	mingo, sudeep.holla, will, dietmar.eggemann, linux-arm-kernel

On 03-08-20, 14:58, Ionela Voinescu wrote:
> Hi Viresh,
> 
> On Thursday 30 Jul 2020 at 09:43:34 (+0530), Viresh Kumar wrote:
> > On 22-07-20, 10:37, Ionela Voinescu wrote:
> > > While the move of the invariance setter calls (arch_set_freq_scale())
> > > from cpufreq drivers to cpufreq core maintained the previous
> > > functionality for existing drivers that use target_index() and
> > > fast_switch() for frequency switching, it also gives the possibility
> > > of adding support for users of the target() callback, which is exploited
> > > here.
> > > 
> > > To be noted that the target() callback has been flagged as deprecated
> > > since:
> > > 
> > > commit 9c0ebcf78fde ("cpufreq: Implement light weight ->target_index() routine")
> > > 
> > > It also doesn't have that many users:
> > > 
> > >   cpufreq-nforce2.c:371:2:      .target = nforce2_target,
> > >   cppc_cpufreq.c:416:2:         .target = cppc_cpufreq_set_target,
> > >   gx-suspmod.c:439:2:           .target = cpufreq_gx_target,
> > >   pcc-cpufreq.c:573:2:          .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.
> > > 
> > > Two notes are worthwhile here:
> > >  - In __target_index(), cpufreq_freq_transition_end() is called only for
> > >    drivers that have synchronous notifications enabled. There is only one
> > >    driver that disables them,
> > > 
> > >    drivers/cpufreq/powernow-k8.c:1142: .flags = CPUFREQ_ASYNC_NOTIFICATION,
> > > 
> > >    which is deprecated.
> > 
> > I don't think this is deprecated.

Heh, maybe I misunderstood. I thought you are talking about the flag,
while you were talking about the driver.

> Sorry, possibly 'deprecated' is a strong word.
> 
> As far as I knew acpi_cpufreq was recommended more recently for K8/K10
> CPUs so that's why I decided not to create a special case for it, also
> considering that it was not supporting cpufreq-based frequency
> invariance to begin with.
> 
> We could support this as well by having a call to arch_set_freq_scale()
> on the else path in __target_index(). But given that there was only this
> one user of CPUFREQ_ASYNC_NOTIFICATION, I thought I'd propose this simpler
> version first.
> 
> Let me know if my reasoning is wrong.

Nevertheless, I don't think you need to mention this detail in
changelog for powernow-k8 as cpufreq_freq_transition_end() does get
called for it as well, by the driver instead of the core.

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 3/7] arch_topology: disable frequency invariance for CONFIG_BL_SWITCHER
  2020-07-30 10:29     ` Dietmar Eggemann
  2020-07-31 15:48       ` Sudeep Holla
@ 2020-08-04  6:30       ` Viresh Kumar
  2020-08-10  9:01         ` Ionela Voinescu
  1 sibling, 1 reply; 35+ messages in thread
From: Viresh Kumar @ 2020-08-04  6:30 UTC (permalink / raw)
  To: Dietmar Eggemann
  Cc: catalin.marinas, linux-pm, peterz, will, rjw, linux,
	linux-kernel, mingo, sudeep.holla, Ionela Voinescu,
	linux-arm-kernel

On 30-07-20, 12:29, Dietmar Eggemann wrote:
> On 30/07/2020 06:24, Viresh Kumar wrote:
> > On 22-07-20, 10:37, Ionela Voinescu wrote:
> >> +++ b/drivers/base/arch_topology.c
> >> @@ -27,6 +27,7 @@ __weak bool arch_freq_counters_available(struct cpumask *cpus)
> >>  }
> >>  DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE;
> >>  
> >> +#ifndef CONFIG_BL_SWITCHER
> >>  void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
> >>  			 unsigned long max_freq)
> >>  {
> >> @@ -46,6 +47,7 @@ void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
> >>  	for_each_cpu(i, cpus)
> >>  		per_cpu(freq_scale, i) = scale;
> >>  }
> >> +#endif
> > 
> > I don't really like this change, the ifdef hackery is disgusting and
> > then we are putting that in a completely different part of the kernel.
> > 
> > There are at least these two ways of solving this, maybe more:
> > 
> > - Fix the bl switcher driver and add the complexity in it (which you
> >   tried to do earlier).
> > 
> > - Add a cpufreq flag to skip arch-set-freq-scale call.
> 
> I agree it's not nice but IMHO the cpufreq flag is worse since we would
> introduce new infrastructure only for a deprecated feature. I'm assuming
> that BL SWITCHER is the only feature needing this CPUfreq flag extension.
> 
> #ifdef CONFIG_BL_SWITCHER is already in drivers/irqchip/irq-gic.c so
> it's ugly already.
> 
> Runtime detecting (via bL_switching_enabled) of BL SWITCHER is right now
> also only handled inside vexpress-spc-cpufreq.c via a
> bL_switcher_notifier. A mechanism which also sits behind a #ifdef
> CONFIG_BL_SWITCHER.

Vexpress one is a driver and so ugliness could be ignored here :)

So here is option number 3 (in continuation of the earlier two
options):
- Don't do anything for bL switcher, just add a TODO/NOTE in the
  driver that FIE is broken for switcher. And I don't think anyone
  will care about FIE for the switcher anyway :)

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 4/7] cpufreq: report whether cpufreq supports Frequency Invariance (FI)
  2020-08-03 15:24     ` Ionela Voinescu
@ 2020-08-04  6:46       ` Viresh Kumar
  2020-08-05 10:35         ` Ionela Voinescu
  0 siblings, 1 reply; 35+ messages in thread
From: Viresh Kumar @ 2020-08-04  6:46 UTC (permalink / raw)
  To: Ionela Voinescu
  Cc: linux-pm, peterz, catalin.marinas, rjw, linux, linux-kernel,
	mingo, sudeep.holla, will, dietmar.eggemann, linux-arm-kernel

On 03-08-20, 16:24, Ionela Voinescu wrote:
> Right, cpufreq_register_driver() should check that at least one of them
> is present

> (although currently cpufreq_register_driver() will return
> -EINVAL if .fast_switch() alone is present - something to be fixed).

I think it is fine as there is no guarantee from cpufreq core if
.fast_switch() will get called and so target/target_index must be
present. We can't do fast-switch today without schedutil (as only that
enables it) and if a notifier gets registered before the driver, then
we are gone again.

> Will do, on both accounts.
> 
> 
> > > +		static_branch_enable_cpuslocked(&cpufreq_set_freq_scale);
> > > +		pr_debug("%s: Driver %s can provide frequency invariance.",
> > > +			 __func__, driver->name);
> > 
> > I think a simpler print will work well too.
> > 
> >                 pr_debug("Freq invariance enabled");
> > 
> 
> I think the right way of reporting this support is important here.

Yeah, we can't say it is enabled as you explained, though I meant
something else here then, i.e. getting rid of driver name and
unimportant stuff. What about this now:

pr_debug("supports frequency invariance");

This shall get printed as this finally:

cpufreq: supports frequency invariance

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 2/7] cpufreq: set invariance scale factor on transition end
  2020-08-04  6:26       ` Viresh Kumar
@ 2020-08-05 10:35         ` Ionela Voinescu
  0 siblings, 0 replies; 35+ messages in thread
From: Ionela Voinescu @ 2020-08-05 10:35 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: linux-pm, peterz, catalin.marinas, rjw, linux, linux-kernel,
	mingo, sudeep.holla, will, dietmar.eggemann, linux-arm-kernel

On Tuesday 04 Aug 2020 at 11:56:11 (+0530), Viresh Kumar wrote:
[..]
> > > >  - In __target_index(), cpufreq_freq_transition_end() is called only for
> > > >    drivers that have synchronous notifications enabled. There is only one
> > > >    driver that disables them,
> > > > 
> > > >    drivers/cpufreq/powernow-k8.c:1142: .flags = CPUFREQ_ASYNC_NOTIFICATION,
> > > > 
> > > >    which is deprecated.
> > > 
> > > I don't think this is deprecated.
> 
> Heh, maybe I misunderstood. I thought you are talking about the flag,
> while you were talking about the driver.
> 
> > Sorry, possibly 'deprecated' is a strong word.
> > 
> > As far as I knew acpi_cpufreq was recommended more recently for K8/K10
> > CPUs so that's why I decided not to create a special case for it, also
> > considering that it was not supporting cpufreq-based frequency
> > invariance to begin with.
> > 
> > We could support this as well by having a call to arch_set_freq_scale()
> > on the else path in __target_index(). But given that there was only this
> > one user of CPUFREQ_ASYNC_NOTIFICATION, I thought I'd propose this simpler
> > version first.
> > 
> > Let me know if my reasoning is wrong.
> 
> Nevertheless, I don't think you need to mention this detail in
> changelog for powernow-k8 as cpufreq_freq_transition_end() does get
> called for it as well, by the driver instead of the core.
> 

Agreed!

Many thanks,
Ionela.

> -- 
> viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 4/7] cpufreq: report whether cpufreq supports Frequency Invariance (FI)
  2020-08-04  6:46       ` Viresh Kumar
@ 2020-08-05 10:35         ` Ionela Voinescu
  0 siblings, 0 replies; 35+ messages in thread
From: Ionela Voinescu @ 2020-08-05 10:35 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: linux-pm, peterz, catalin.marinas, rjw, linux, linux-kernel,
	mingo, sudeep.holla, will, dietmar.eggemann, linux-arm-kernel

On Tuesday 04 Aug 2020 at 12:16:56 (+0530), Viresh Kumar wrote:
> On 03-08-20, 16:24, Ionela Voinescu wrote:
> > Right, cpufreq_register_driver() should check that at least one of them
> > is present
> 
> > (although currently cpufreq_register_driver() will return
> > -EINVAL if .fast_switch() alone is present - something to be fixed).
> 
> I think it is fine as there is no guarantee from cpufreq core if
> .fast_switch() will get called and so target/target_index must be
> present. We can't do fast-switch today without schedutil (as only that
> enables it) and if a notifier gets registered before the driver, then
> we are gone again.
> 
> > Will do, on both accounts.
> > 
> > 
> > > > +		static_branch_enable_cpuslocked(&cpufreq_set_freq_scale);
> > > > +		pr_debug("%s: Driver %s can provide frequency invariance.",
> > > > +			 __func__, driver->name);
> > > 
> > > I think a simpler print will work well too.
> > > 
> > >                 pr_debug("Freq invariance enabled");
> > > 
> > 
> > I think the right way of reporting this support is important here.
> 
> Yeah, we can't say it is enabled as you explained, though I meant
> something else here then, i.e. getting rid of driver name and
> unimportant stuff. What about this now:
> 
> pr_debug("supports frequency invariance");
> 
> This shall get printed as this finally:
> 
> cpufreq: supports frequency invariance
> 

Will do!

Thanks,
Ionela.

> -- 
> viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v2 3/7] arch_topology: disable frequency invariance for CONFIG_BL_SWITCHER
  2020-08-04  6:30       ` Viresh Kumar
@ 2020-08-10  9:01         ` Ionela Voinescu
  0 siblings, 0 replies; 35+ messages in thread
From: Ionela Voinescu @ 2020-08-10  9:01 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: linux-pm, peterz, catalin.marinas, rjw, linux, linux-kernel,
	mingo, sudeep.holla, will, Dietmar Eggemann, linux-arm-kernel

Hi guys,

On Tuesday 04 Aug 2020 at 12:00:46 (+0530), Viresh Kumar wrote:
> On 30-07-20, 12:29, Dietmar Eggemann wrote:
> > On 30/07/2020 06:24, Viresh Kumar wrote:
> > > On 22-07-20, 10:37, Ionela Voinescu wrote:
> > >> +++ b/drivers/base/arch_topology.c
> > >> @@ -27,6 +27,7 @@ __weak bool arch_freq_counters_available(struct cpumask *cpus)
> > >>  }
> > >>  DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE;
> > >>  
> > >> +#ifndef CONFIG_BL_SWITCHER
> > >>  void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
> > >>  			 unsigned long max_freq)
> > >>  {
> > >> @@ -46,6 +47,7 @@ void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
> > >>  	for_each_cpu(i, cpus)
> > >>  		per_cpu(freq_scale, i) = scale;
> > >>  }
> > >> +#endif
> > > 
> > > I don't really like this change, the ifdef hackery is disgusting and
> > > then we are putting that in a completely different part of the kernel.
> > > 
> > > There are at least these two ways of solving this, maybe more:
> > > 
> > > - Fix the bl switcher driver and add the complexity in it (which you
> > >   tried to do earlier).
> > > 
> > > - Add a cpufreq flag to skip arch-set-freq-scale call.
> > 
> > I agree it's not nice but IMHO the cpufreq flag is worse since we would
> > introduce new infrastructure only for a deprecated feature. I'm assuming
> > that BL SWITCHER is the only feature needing this CPUfreq flag extension.
> > 
> > #ifdef CONFIG_BL_SWITCHER is already in drivers/irqchip/irq-gic.c so
> > it's ugly already.
> > 
> > Runtime detecting (via bL_switching_enabled) of BL SWITCHER is right now
> > also only handled inside vexpress-spc-cpufreq.c via a
> > bL_switcher_notifier. A mechanism which also sits behind a #ifdef
> > CONFIG_BL_SWITCHER.
> 
> Vexpress one is a driver and so ugliness could be ignored here :)
> 
> So here is option number 3 (in continuation of the earlier two
> options):
> - Don't do anything for bL switcher, just add a TODO/NOTE in the
>   driver that FIE is broken for switcher. And I don't think anyone
>   will care about FIE for the switcher anyway :)
> 

I gave it a bit of time in case anyone had strong opinions about this,
but given the lack of those, what I can do in this series is the
following: ignore the problem :). This issue was there before these
patches and it will continue to be there after these patches - nothing
changes.

Separately from this series, I can submit a patch with Viresh's
suggestion above and we can spin around a bit discussing this, if there
is interest. My opinion on this is that option 1 is ugly but it does fix
an issue in a relatively non-invasive way. I agree with "I don't think
anyone will care about FIE for the switcher anyway", but for me this
means that nobody will care if it's supported (and therefore option 1
is the proper solution). But if bL switcher is used, I think people might
care if it's broken, as it results in incorrect scheduler signals.
Therefore, I would not like leaving it broken (option 3). If it's not
used, option 2 is obvious.


Many thanks,
Ionela.

> -- 
> viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 35+ messages in thread

end of thread, back to index

Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-22  9:37 [PATCH v2 0/7] cpufreq: improve frequency invariance support Ionela Voinescu
2020-07-22  9:37 ` [PATCH v2 1/7] cpufreq: move invariance setter calls in cpufreq core Ionela Voinescu
2020-07-27 13:48   ` Rafael J. Wysocki
2020-07-29  9:03     ` Ionela Voinescu
2020-07-30  3:41     ` Viresh Kumar
2020-08-03 13:26       ` Ionela Voinescu
2020-08-03 13:46         ` Rafael J. Wysocki
2020-08-03 14:16           ` Ionela Voinescu
2020-07-22  9:37 ` [PATCH v2 2/7] cpufreq: set invariance scale factor on transition end Ionela Voinescu
2020-07-27 13:52   ` Rafael J. Wysocki
2020-07-29  9:14     ` Ionela Voinescu
2020-07-30  4:13   ` Viresh Kumar
2020-08-03 13:58     ` Ionela Voinescu
2020-08-04  6:26       ` Viresh Kumar
2020-08-05 10:35         ` Ionela Voinescu
2020-07-22  9:37 ` [PATCH v2 3/7] arch_topology: disable frequency invariance for CONFIG_BL_SWITCHER Ionela Voinescu
2020-07-30  4:24   ` Viresh Kumar
2020-07-30 10:29     ` Dietmar Eggemann
2020-07-31 15:48       ` Sudeep Holla
2020-08-03 14:39         ` Ionela Voinescu
2020-08-04  6:30       ` Viresh Kumar
2020-08-10  9:01         ` Ionela Voinescu
2020-07-22  9:37 ` [PATCH v2 4/7] cpufreq: report whether cpufreq supports Frequency Invariance (FI) Ionela Voinescu
2020-07-27 14:02   ` Rafael J. Wysocki
2020-07-29 14:39     ` Ionela Voinescu
2020-07-30  4:43   ` Viresh Kumar
2020-08-03 15:24     ` Ionela Voinescu
2020-08-04  6:46       ` Viresh Kumar
2020-08-05 10:35         ` Ionela Voinescu
2020-07-22  9:37 ` [PATCH v2 5/7] arch_topology, cpufreq, sched/core: constify arch_* cpumasks Ionela Voinescu
2020-07-30 11:43   ` Catalin Marinas
2020-07-22  9:37 ` [PATCH v2 6/7] arch_topology, arm, arm64: define arch_scale_freq_invariant() Ionela Voinescu
2020-07-30 11:44   ` [PATCH v2 6/7] arch_topology,arm,arm64: " Catalin Marinas
2020-07-22  9:37 ` [PATCH v2 7/7] cpufreq: make schedutil the default for arm and arm64 Ionela Voinescu
2020-07-30  4:54   ` Viresh Kumar

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git