Hi all,
This patch set v4 aims to refactor the thermal pressure update
code. There are already two clients which do similar thing:
convert the capped frequency value into the capacity of
affected CPU and call the 'set' function to store the
reduced capacity into the per-cpu variable.
There might be more than two of these users. In near future
it will be scmi-cpufreq driver, which receives notification
from FW about reduced frequency due to thermal. Other vendors
might follow. Let's avoid code duplication and potential
conversion bugs. Move the conversion code into the arch_topology.c
where the capacity calculation setup code and thermal pressure sit.
Apart from that $subject patches, there is one patch (3/5) which fixes
issue in qcom-cpufreq-hw.c when the thermal pressure is not
updated for offline CPUs. It's similar fix that has been merged
recently for cpufreq_cooling.c:
2ad8ccc17d1e4270cf65a3f2
The patch 4/5 fixes also qcom-cpufreq-hw.c driver code which did
the translation from frequency to capacity wrongly when there
was a boost frequency available and stored in 'policy->cpuinfo.max_freq'.
Changes:
v4:
- remove the warning when boost frequency is passed and set thermal
pressure to 0 in that case, which means the capping is totally removed
(issue reported by Steev)
- remove the check from patch 4/5 with
'throttled_freq > policy->cpuinfo.max_freq' since it doesn't have
effect; instead relay on new arch_update_thermal_pressure() handling
correctly such use case; this would also fix an issue in that original
driver code, where the reduced capacity was calculated wrongly due
to 'policy->cpuinfo.max_freq' used as a divider
- adjusted comments stressing the fact that the boost frequencies are
supported
v3 [3]:
- added warning and check if provided capped frequency is lower than
max (Viresh)
- removed check for empty cpu mask (Viresh)
- replaced tabs with spaces in the doxygen comment (Viresh)
- renamed {arch|topology}_thermal_pressure_update() to
{arch|topology}_update_thermal_pressure() so it's align with scheme (Dietmar)
- added info about MHz in freq_factor into patch description (Dietmar)
v2 [2]:
- added Reviewed-by from Thara for patch 3/5
- changed the doxygen comment and used mult_frac()
according to Thara's suggestion in patch 1/5
v1 -> [1]
Regards,
Lukasz Luba
[1] https://lore.kernel.org/linux-pm/[email protected]/
[2] https://lore.kernel.org/linux-pm/[email protected]/
[3] https://lore.kernel.org/linux-pm/[email protected]/
Lukasz Luba (5):
arch_topology: Introduce thermal pressure update function
thermal: cpufreq_cooling: Use new thermal pressure update function
cpufreq: qcom-cpufreq-hw: Update offline CPUs per-cpu thermal pressure
cpufreq: qcom-cpufreq-hw: Use new thermal pressure update function
arch_topology: Remove unused topology_set_thermal_pressure() and
related
arch/arm/include/asm/topology.h | 2 +-
arch/arm64/include/asm/topology.h | 2 +-
drivers/base/arch_topology.c | 42 ++++++++++++++++++++++++++++---
drivers/cpufreq/qcom-cpufreq-hw.c | 14 +++--------
drivers/thermal/cpufreq_cooling.c | 6 +----
include/linux/arch_topology.h | 4 +--
include/linux/sched/topology.h | 6 ++---
init/Kconfig | 2 +-
8 files changed, 50 insertions(+), 28 deletions(-)
--
2.17.1
The thermal pressure signal gives information to the scheduler about
reduced CPU capacity due to thermal. It is based on a value stored in
a per-cpu 'thermal_pressure' variable. The online CPUs will get the
new value there, while the offline won't. Unfortunately, when the CPU
is back online, the value read from per-cpu variable might be wrong
(stale data). This might affect the scheduler decisions, since it
sees the CPU capacity differently than what is actually available.
Fix it by making sure that all online+offline CPUs would get the
proper value in their per-cpu variable when there is throttling
or throttling is removed.
Fixes: 275157b367f479 ("cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support")
Reviewed-by: Thara Gopinath <[email protected]>
Signed-off-by: Lukasz Luba <[email protected]>
---
drivers/cpufreq/qcom-cpufreq-hw.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index a2be0df7e174..0138b2ec406d 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -304,7 +304,8 @@ static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data)
if (capacity > max_capacity)
capacity = max_capacity;
- arch_set_thermal_pressure(policy->cpus, max_capacity - capacity);
+ arch_set_thermal_pressure(policy->related_cpus,
+ max_capacity - capacity);
/*
* In the unlikely case policy is unregistered do not enable
--
2.17.1
There is no need of this function (and related) since code has been
converted to use the new arch_update_thermal_pressure() API. The old
code can be removed.
Signed-off-by: Lukasz Luba <[email protected]>
---
arch/arm/include/asm/topology.h | 1 -
arch/arm64/include/asm/topology.h | 1 -
drivers/base/arch_topology.c | 17 +++++------------
include/linux/arch_topology.h | 3 ---
include/linux/sched/topology.h | 7 -------
init/Kconfig | 2 +-
6 files changed, 6 insertions(+), 25 deletions(-)
diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
index f1eafacc9a30..c7d2510e5a78 100644
--- a/arch/arm/include/asm/topology.h
+++ b/arch/arm/include/asm/topology.h
@@ -23,7 +23,6 @@
/* Replace task scheduler's default thermal pressure API */
#define arch_scale_thermal_pressure topology_get_thermal_pressure
-#define arch_set_thermal_pressure topology_set_thermal_pressure
#define arch_update_thermal_pressure topology_update_thermal_pressure
#else
diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
index 7a421cbc99ed..f386b90a79c8 100644
--- a/arch/arm64/include/asm/topology.h
+++ b/arch/arm64/include/asm/topology.h
@@ -32,7 +32,6 @@ void update_freq_counters_refs(void);
/* Replace task scheduler's default thermal pressure API */
#define arch_scale_thermal_pressure topology_get_thermal_pressure
-#define arch_set_thermal_pressure topology_set_thermal_pressure
#define arch_update_thermal_pressure topology_update_thermal_pressure
#include <asm-generic/topology.h>
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index bed7d502c9a1..c5d866c5c344 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -159,16 +159,6 @@ void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity)
DEFINE_PER_CPU(unsigned long, thermal_pressure);
-void topology_set_thermal_pressure(const struct cpumask *cpus,
- unsigned long th_pressure)
-{
- int cpu;
-
- for_each_cpu(cpu, cpus)
- WRITE_ONCE(per_cpu(thermal_pressure, cpu), th_pressure);
-}
-EXPORT_SYMBOL_GPL(topology_set_thermal_pressure);
-
/**
* topology_update_thermal_pressure() - Update thermal pressure for CPUs
* @cpus : The related CPUs for which capacity has been reduced
@@ -186,7 +176,7 @@ EXPORT_SYMBOL_GPL(topology_set_thermal_pressure);
void topology_update_thermal_pressure(const struct cpumask *cpus,
unsigned long capped_freq)
{
- unsigned long max_capacity, capacity;
+ unsigned long max_capacity, capacity, th_pressure;
u32 max_freq;
int cpu;
@@ -206,7 +196,10 @@ void topology_update_thermal_pressure(const struct cpumask *cpus,
else
capacity = mult_frac(max_capacity, capped_freq, max_freq);
- arch_set_thermal_pressure(cpus, max_capacity - capacity);
+ th_pressure = max_capacity - capacity;
+
+ for_each_cpu(cpu, cpus)
+ WRITE_ONCE(per_cpu(thermal_pressure, cpu), th_pressure);
}
EXPORT_SYMBOL_GPL(topology_update_thermal_pressure);
diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
index ace1e5dcf773..cce6136b300a 100644
--- a/include/linux/arch_topology.h
+++ b/include/linux/arch_topology.h
@@ -56,9 +56,6 @@ static inline unsigned long topology_get_thermal_pressure(int cpu)
return per_cpu(thermal_pressure, cpu);
}
-void topology_set_thermal_pressure(const struct cpumask *cpus,
- unsigned long th_pressure);
-
void topology_update_thermal_pressure(const struct cpumask *cpus,
unsigned long capped_freq);
diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
index 6e89a8e43aa7..8054641c0a7b 100644
--- a/include/linux/sched/topology.h
+++ b/include/linux/sched/topology.h
@@ -266,13 +266,6 @@ unsigned long arch_scale_thermal_pressure(int cpu)
}
#endif
-#ifndef arch_set_thermal_pressure
-static __always_inline
-void arch_set_thermal_pressure(const struct cpumask *cpus,
- unsigned long th_pressure)
-{ }
-#endif
-
#ifndef arch_update_thermal_pressure
static __always_inline
void arch_update_thermal_pressure(const struct cpumask *cpus,
diff --git a/init/Kconfig b/init/Kconfig
index 11f8a845f259..b5d6eeb86275 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -550,7 +550,7 @@ config SCHED_THERMAL_PRESSURE
i.e. put less load on throttled CPUs than on non/less throttled ones.
This requires the architecture to implement
- arch_set_thermal_pressure() and arch_scale_thermal_pressure().
+ arch_update_thermal_pressure() and arch_scale_thermal_pressure().
config BSD_PROCESS_ACCT
bool "BSD Process Accounting"
--
2.17.1
The thermal pressure is a mechanism which is used for providing
information about reduced CPU performance to the scheduler. Usually code
has to convert the value from frequency units into capacity units,
which are understandable by the scheduler. Create a common conversion code
which can be just used via a handy API.
Internally, the topology_update_thermal_pressure() operates on frequency
in MHz and max CPU frequency is taken from 'freq_factor' (per-cpu).
Signed-off-by: Lukasz Luba <[email protected]>
---
arch/arm/include/asm/topology.h | 1 +
arch/arm64/include/asm/topology.h | 1 +
drivers/base/arch_topology.c | 43 ++++++++++++++++++++++++++++++-
include/linux/arch_topology.h | 3 +++
include/linux/sched/topology.h | 7 +++++
5 files changed, 54 insertions(+), 1 deletion(-)
diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
index 470299ee2fba..f1eafacc9a30 100644
--- a/arch/arm/include/asm/topology.h
+++ b/arch/arm/include/asm/topology.h
@@ -24,6 +24,7 @@
/* Replace task scheduler's default thermal pressure API */
#define arch_scale_thermal_pressure topology_get_thermal_pressure
#define arch_set_thermal_pressure topology_set_thermal_pressure
+#define arch_update_thermal_pressure topology_update_thermal_pressure
#else
diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
index ec2db3419c41..7a421cbc99ed 100644
--- a/arch/arm64/include/asm/topology.h
+++ b/arch/arm64/include/asm/topology.h
@@ -33,6 +33,7 @@ void update_freq_counters_refs(void);
/* Replace task scheduler's default thermal pressure API */
#define arch_scale_thermal_pressure topology_get_thermal_pressure
#define arch_set_thermal_pressure topology_set_thermal_pressure
+#define arch_update_thermal_pressure topology_update_thermal_pressure
#include <asm-generic/topology.h>
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index fc0836f460fb..bed7d502c9a1 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -25,6 +25,7 @@
static DEFINE_PER_CPU(struct scale_freq_data __rcu *, sft_data);
static struct cpumask scale_freq_counters_mask;
static bool scale_freq_invariant;
+static DEFINE_PER_CPU(u32, freq_factor) = 1;
static bool supports_scale_freq_counters(const struct cpumask *cpus)
{
@@ -168,6 +169,47 @@ void topology_set_thermal_pressure(const struct cpumask *cpus,
}
EXPORT_SYMBOL_GPL(topology_set_thermal_pressure);
+/**
+ * topology_update_thermal_pressure() - Update thermal pressure for CPUs
+ * @cpus : The related CPUs for which capacity has been reduced
+ * @capped_freq : The maximum allowed frequency that CPUs can run at
+ *
+ * Update the value of thermal pressure for all @cpus in the mask. The
+ * cpumask should include all (online+offline) affected CPUs, to avoid
+ * operating on stale data when hot-plug is used for some CPUs. The
+ * @capped_freq reflects the currently allowed max CPUs frequency due to
+ * thermal capping. It might be also a boost frequency value, which is bigger
+ * than the internal 'freq_factor' max frequency. In such case the pressure
+ * value should simply be removed, since this is an indication that there is
+ * no thermal throttling. The @capped_freq must be provided in kHz.
+ */
+void topology_update_thermal_pressure(const struct cpumask *cpus,
+ unsigned long capped_freq)
+{
+ unsigned long max_capacity, capacity;
+ u32 max_freq;
+ int cpu;
+
+ cpu = cpumask_first(cpus);
+ max_capacity = arch_scale_cpu_capacity(cpu);
+ max_freq = per_cpu(freq_factor, cpu);
+
+ /* Convert to MHz scale which is used in 'freq_factor' */
+ capped_freq /= 1000;
+
+ /*
+ * Handle properly the boost frequencies, which should simply clean
+ * the thermal pressure value.
+ */
+ if (max_freq <= capped_freq)
+ capacity = max_capacity;
+ else
+ capacity = mult_frac(max_capacity, capped_freq, max_freq);
+
+ arch_set_thermal_pressure(cpus, max_capacity - capacity);
+}
+EXPORT_SYMBOL_GPL(topology_update_thermal_pressure);
+
static ssize_t cpu_capacity_show(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -220,7 +262,6 @@ static void update_topology_flags_workfn(struct work_struct *work)
update_topology = 0;
}
-static DEFINE_PER_CPU(u32, freq_factor) = 1;
static u32 *raw_capacity;
static int free_raw_capacity(void)
diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
index b97cea83b25e..ace1e5dcf773 100644
--- a/include/linux/arch_topology.h
+++ b/include/linux/arch_topology.h
@@ -59,6 +59,9 @@ static inline unsigned long topology_get_thermal_pressure(int cpu)
void topology_set_thermal_pressure(const struct cpumask *cpus,
unsigned long th_pressure);
+void topology_update_thermal_pressure(const struct cpumask *cpus,
+ unsigned long capped_freq);
+
struct cpu_topology {
int thread_id;
int core_id;
diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
index c07bfa2d80f2..6e89a8e43aa7 100644
--- a/include/linux/sched/topology.h
+++ b/include/linux/sched/topology.h
@@ -273,6 +273,13 @@ void arch_set_thermal_pressure(const struct cpumask *cpus,
{ }
#endif
+#ifndef arch_update_thermal_pressure
+static __always_inline
+void arch_update_thermal_pressure(const struct cpumask *cpus,
+ unsigned long capped_frequency)
+{ }
+#endif
+
static inline int task_node(const struct task_struct *p)
{
return cpu_to_node(task_cpu(p));
--
2.17.1
Thermal pressure provides a new API, which allows to use CPU frequency
as an argument. That removes the need of local conversion to capacity.
Use this new function and remove old conversion code.
Signed-off-by: Lukasz Luba <[email protected]>
---
drivers/thermal/cpufreq_cooling.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/thermal/cpufreq_cooling.c b/drivers/thermal/cpufreq_cooling.c
index 43b1ae8a7789..0bfb8eebd126 100644
--- a/drivers/thermal/cpufreq_cooling.c
+++ b/drivers/thermal/cpufreq_cooling.c
@@ -462,7 +462,6 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
struct cpufreq_cooling_device *cpufreq_cdev = cdev->devdata;
struct cpumask *cpus;
unsigned int frequency;
- unsigned long max_capacity, capacity;
int ret;
/* Request state should be less than max_level */
@@ -479,10 +478,7 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
if (ret >= 0) {
cpufreq_cdev->cpufreq_state = state;
cpus = cpufreq_cdev->policy->related_cpus;
- max_capacity = arch_scale_cpu_capacity(cpumask_first(cpus));
- capacity = frequency * max_capacity;
- capacity /= cpufreq_cdev->policy->cpuinfo.max_freq;
- arch_set_thermal_pressure(cpus, max_capacity - capacity);
+ arch_update_thermal_pressure(cpus, frequency);
ret = 0;
}
--
2.17.1
Thermal pressure provides a new API, which allows to use CPU frequency
as an argument. That removes the need of local conversion to capacity.
Use this new API and remove old local conversion code.
The new arch_update_thermal_pressure() also accepts boost frequencies,
which solves issue in the driver code with wrong reduced capacity
calculation. The reduced capacity was calculated wrongly due to
'policy->cpuinfo.max_freq' used as a divider. The value present there was
actually the boost frequency. Thus, even a normal maximum frequency value
which corresponds to max CPU capacity (arch_scale_cpu_capacity(cpu_id))
is not able to remove the capping.
The second side effect which is solved is that the reduced frequency wasn't
properly translated into the right reduced capacity,
e.g.
boost frequency = 3000MHz (stored in policy->cpuinfo.max_freq)
max normal frequency = 2500MHz (which is 1024 capacity)
2nd highest frequency = 2000MHz (which translates to 819 capacity)
Then in a scenario when the 'throttled_freq' max allowed frequency was
2000MHz the driver translated it into 682 capacity:
capacity = 1024 * 2000 / 3000 = 682
Then set the pressure value bigger than actually applied by the HW:
max_capacity - capacity => 1024 - 682 = 342 (<- thermal pressure)
Which was causing higher throttling and misleading task scheduler
about available CPU capacity.
A proper calculation in such case should be:
capacity = 1024 * 2000 / 2500 = 819
1024 - 819 = 205 (<- thermal pressure)
This patch relies on the new arch_update_thermal_pressure() handling
correctly such use case (with boost frequencies).
Signed-off-by: Lukasz Luba <[email protected]>
---
drivers/cpufreq/qcom-cpufreq-hw.c | 15 +++------------
1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 0138b2ec406d..248135e5087e 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -275,10 +275,10 @@ static unsigned int qcom_lmh_get_throttle_freq(struct qcom_cpufreq_data *data)
static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data)
{
- unsigned long max_capacity, capacity, freq_hz, throttled_freq;
struct cpufreq_policy *policy = data->policy;
int cpu = cpumask_first(policy->cpus);
struct device *dev = get_cpu_device(cpu);
+ unsigned long freq_hz, throttled_freq;
struct dev_pm_opp *opp;
unsigned int freq;
@@ -295,17 +295,8 @@ static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data)
throttled_freq = freq_hz / HZ_PER_KHZ;
- /* Update thermal pressure */
-
- max_capacity = arch_scale_cpu_capacity(cpu);
- capacity = mult_frac(max_capacity, throttled_freq, policy->cpuinfo.max_freq);
-
- /* Don't pass boost capacity to scheduler */
- if (capacity > max_capacity)
- capacity = max_capacity;
-
- arch_set_thermal_pressure(policy->related_cpus,
- max_capacity - capacity);
+ /* Update thermal pressure (the boost frequencies are accepted) */
+ arch_update_thermal_pressure(policy->related_cpus, throttled_freq);
/*
* In the unlikely case policy is unregistered do not enable
--
2.17.1
On 09-11-21, 19:57, Lukasz Luba wrote:
> Hi all,
>
> This patch set v4 aims to refactor the thermal pressure update
> code. There are already two clients which do similar thing:
> convert the capped frequency value into the capacity of
> affected CPU and call the 'set' function to store the
> reduced capacity into the per-cpu variable.
> There might be more than two of these users. In near future
> it will be scmi-cpufreq driver, which receives notification
> from FW about reduced frequency due to thermal. Other vendors
> might follow. Let's avoid code duplication and potential
> conversion bugs. Move the conversion code into the arch_topology.c
> where the capacity calculation setup code and thermal pressure sit.
>
> Apart from that $subject patches, there is one patch (3/5) which fixes
> issue in qcom-cpufreq-hw.c when the thermal pressure is not
> updated for offline CPUs. It's similar fix that has been merged
> recently for cpufreq_cooling.c:
> 2ad8ccc17d1e4270cf65a3f2
>
> The patch 4/5 fixes also qcom-cpufreq-hw.c driver code which did
> the translation from frequency to capacity wrongly when there
> was a boost frequency available and stored in 'policy->cpuinfo.max_freq'.
LGTM. I will apply this in a few days so people get time to Ack/Review
the patches.
--
viresh
On 11/9/21 2:57 PM, Lukasz Luba wrote:
> Thermal pressure provides a new API, which allows to use CPU frequency
> as an argument. That removes the need of local conversion to capacity.
> Use this new API and remove old local conversion code.
>
> The new arch_update_thermal_pressure() also accepts boost frequencies,
> which solves issue in the driver code with wrong reduced capacity
> calculation. The reduced capacity was calculated wrongly due to
> 'policy->cpuinfo.max_freq' used as a divider. The value present there was
> actually the boost frequency. Thus, even a normal maximum frequency value
> which corresponds to max CPU capacity (arch_scale_cpu_capacity(cpu_id))
> is not able to remove the capping.
Yes, although cpuinfo.max_freq does not reflect the boost frequency
unless boost is enabled atleast once. I have sent a patch to fix this.
But I agree that using cpuinfo.max_freq has issues you have mentioned in
this patch if boost is enabled once.
So, for this patch
Reviewed-by: Thara Gopinath <[email protected]>
Warm Regards
Thara (She/Her/Hers)
>
> The second side effect which is solved is that the reduced frequency wasn't
> properly translated into the right reduced capacity,
> e.g.
> boost frequency = 3000MHz (stored in policy->cpuinfo.max_freq)
> max normal frequency = 2500MHz (which is 1024 capacity)
> 2nd highest frequency = 2000MHz (which translates to 819 capacity)
>
> Then in a scenario when the 'throttled_freq' max allowed frequency was
> 2000MHz the driver translated it into 682 capacity:
> capacity = 1024 * 2000 / 3000 = 682
> Then set the pressure value bigger than actually applied by the HW:
> max_capacity - capacity => 1024 - 682 = 342 (<- thermal pressure)
> Which was causing higher throttling and misleading task scheduler
> about available CPU capacity.
> A proper calculation in such case should be:
> capacity = 1024 * 2000 / 2500 = 819
> 1024 - 819 = 205 (<- thermal pressure)
>
> This patch relies on the new arch_update_thermal_pressure() handling
> correctly such use case (with boost frequencies).
>
> Signed-off-by: Lukasz Luba <[email protected]>
> ---
> drivers/cpufreq/qcom-cpufreq-hw.c | 15 +++------------
> 1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index 0138b2ec406d..248135e5087e 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -275,10 +275,10 @@ static unsigned int qcom_lmh_get_throttle_freq(struct qcom_cpufreq_data *data)
>
> static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data)
> {
> - unsigned long max_capacity, capacity, freq_hz, throttled_freq;
> struct cpufreq_policy *policy = data->policy;
> int cpu = cpumask_first(policy->cpus);
> struct device *dev = get_cpu_device(cpu);
> + unsigned long freq_hz, throttled_freq;
> struct dev_pm_opp *opp;
> unsigned int freq;
>
> @@ -295,17 +295,8 @@ static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data)
>
> throttled_freq = freq_hz / HZ_PER_KHZ;
>
> - /* Update thermal pressure */
> -
> - max_capacity = arch_scale_cpu_capacity(cpu);
> - capacity = mult_frac(max_capacity, throttled_freq, policy->cpuinfo.max_freq);
> -
> - /* Don't pass boost capacity to scheduler */
> - if (capacity > max_capacity)
> - capacity = max_capacity;
> -
> - arch_set_thermal_pressure(policy->related_cpus,
> - max_capacity - capacity);
> + /* Update thermal pressure (the boost frequencies are accepted) */
> + arch_update_thermal_pressure(policy->related_cpus, throttled_freq);
>
> /*
> * In the unlikely case policy is unregistered do not enable
>
--
On 11/15/21 3:57 PM, Thara Gopinath wrote:
>
>
> On 11/9/21 2:57 PM, Lukasz Luba wrote:
>> Thermal pressure provides a new API, which allows to use CPU frequency
>> as an argument. That removes the need of local conversion to capacity.
>> Use this new API and remove old local conversion code.
>>
>> The new arch_update_thermal_pressure() also accepts boost frequencies,
>> which solves issue in the driver code with wrong reduced capacity
>> calculation. The reduced capacity was calculated wrongly due to
>> 'policy->cpuinfo.max_freq' used as a divider. The value present there was
>> actually the boost frequency. Thus, even a normal maximum frequency value
>> which corresponds to max CPU capacity (arch_scale_cpu_capacity(cpu_id))
>> is not able to remove the capping.
Also I failed to mention that, currently freq_factor is initialized as
cpuinfo.max_freq / 1000 which means again all the issues you mentioned
below can be hit, if some cpufreq driver decides to set boost at init.
I have sent a patch earlier today to fix this.
https://lore.kernel.org/linux-arm-msm/[email protected]/T/#u
--
Warm Regards
Thara (She/Her/Hers)
>
> Yes, although cpuinfo.max_freq does not reflect the boost frequency
> unless boost is enabled atleast once. I have sent a patch to fix this.
> But I agree that using cpuinfo.max_freq has issues you have mentioned in
> this patch if boost is enabled once.
>
> So, for this patch
>
> Reviewed-by: Thara Gopinath <[email protected]>
>
> Warm Regards
> Thara (She/Her/Hers)
>>
>> The second side effect which is solved is that the reduced frequency
>> wasn't
>> properly translated into the right reduced capacity,
>> e.g.
>> boost frequency = 3000MHz (stored in policy->cpuinfo.max_freq)
>> max normal frequency = 2500MHz (which is 1024 capacity)
>> 2nd highest frequency = 2000MHz (which translates to 819 capacity)
>>
>> Then in a scenario when the 'throttled_freq' max allowed frequency was
>> 2000MHz the driver translated it into 682 capacity:
>> capacity = 1024 * 2000 / 3000 = 682
>> Then set the pressure value bigger than actually applied by the HW:
>> max_capacity - capacity => 1024 - 682 = 342 (<- thermal pressure)
>> Which was causing higher throttling and misleading task scheduler
>> about available CPU capacity.
>> A proper calculation in such case should be:
>> capacity = 1024 * 2000 / 2500 = 819
>> 1024 - 819 = 205 (<- thermal pressure)
>>
>> This patch relies on the new arch_update_thermal_pressure() handling
>> correctly such use case (with boost frequencies).
>>
>> Signed-off-by: Lukasz Luba <[email protected]>
>> ---
>> drivers/cpufreq/qcom-cpufreq-hw.c | 15 +++------------
>> 1 file changed, 3 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c
>> b/drivers/cpufreq/qcom-cpufreq-hw.c
>> index 0138b2ec406d..248135e5087e 100644
>> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>> @@ -275,10 +275,10 @@ static unsigned int
>> qcom_lmh_get_throttle_freq(struct qcom_cpufreq_data *data)
>> static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data)
>> {
>> - unsigned long max_capacity, capacity, freq_hz, throttled_freq;
>> struct cpufreq_policy *policy = data->policy;
>> int cpu = cpumask_first(policy->cpus);
>> struct device *dev = get_cpu_device(cpu);
>> + unsigned long freq_hz, throttled_freq;
>> struct dev_pm_opp *opp;
>> unsigned int freq;
>> @@ -295,17 +295,8 @@ static void qcom_lmh_dcvs_notify(struct
>> qcom_cpufreq_data *data)
>> throttled_freq = freq_hz / HZ_PER_KHZ;
>> - /* Update thermal pressure */
>> -
>> - max_capacity = arch_scale_cpu_capacity(cpu);
>> - capacity = mult_frac(max_capacity, throttled_freq,
>> policy->cpuinfo.max_freq);
>> -
>> - /* Don't pass boost capacity to scheduler */
>> - if (capacity > max_capacity)
>> - capacity = max_capacity;
>> -
>> - arch_set_thermal_pressure(policy->related_cpus,
>> - max_capacity - capacity);
>> + /* Update thermal pressure (the boost frequencies are accepted) */
>> + arch_update_thermal_pressure(policy->related_cpus, throttled_freq);
>> /*
>> * In the unlikely case policy is unregistered do not enable
>>
>
Hi Thara,
On 11/15/21 11:39 PM, Thara Gopinath wrote:
>
>
> On 11/15/21 3:57 PM, Thara Gopinath wrote:
>>
>>
>> On 11/9/21 2:57 PM, Lukasz Luba wrote:
>>> Thermal pressure provides a new API, which allows to use CPU frequency
>>> as an argument. That removes the need of local conversion to capacity.
>>> Use this new API and remove old local conversion code.
>>>
>>> The new arch_update_thermal_pressure() also accepts boost frequencies,
>>> which solves issue in the driver code with wrong reduced capacity
>>> calculation. The reduced capacity was calculated wrongly due to
>>> 'policy->cpuinfo.max_freq' used as a divider. The value present there
>>> was
>>> actually the boost frequency. Thus, even a normal maximum frequency
>>> value
>>> which corresponds to max CPU capacity (arch_scale_cpu_capacity(cpu_id))
>>> is not able to remove the capping.
>
> Also I failed to mention that, currently freq_factor is initialized as
> cpuinfo.max_freq / 1000 which means again all the issues you mentioned
> below can be hit, if some cpufreq driver decides to set boost at init.
> I have sent a patch earlier today to fix this.
Yes, you are right.
>
> https://lore.kernel.org/linux-arm-msm/[email protected]/T/#u
>
>
Looking at the change, it makes sense. I'll try to respond to that
patch.
Thank you for looking into this issue.
Regards,
Lukasz
On 11/15/21 8:57 PM, Thara Gopinath wrote:
>
>
> On 11/9/21 2:57 PM, Lukasz Luba wrote:
>> Thermal pressure provides a new API, which allows to use CPU frequency
>> as an argument. That removes the need of local conversion to capacity.
>> Use this new API and remove old local conversion code.
>>
>> The new arch_update_thermal_pressure() also accepts boost frequencies,
>> which solves issue in the driver code with wrong reduced capacity
>> calculation. The reduced capacity was calculated wrongly due to
>> 'policy->cpuinfo.max_freq' used as a divider. The value present there was
>> actually the boost frequency. Thus, even a normal maximum frequency value
>> which corresponds to max CPU capacity (arch_scale_cpu_capacity(cpu_id))
>> is not able to remove the capping.
>
> Yes, although cpuinfo.max_freq does not reflect the boost frequency
> unless boost is enabled atleast once. I have sent a patch to fix this.
> But I agree that using cpuinfo.max_freq has issues you have mentioned in
> this patch if boost is enabled once.
>
> So, for this patch
>
> Reviewed-by: Thara Gopinath <[email protected]>
Thank you for the review!
>
> Warm Regards
> Thara (She/Her/Hers)
On 11/9/21 2:57 PM, Lukasz Luba wrote:
> The thermal pressure is a mechanism which is used for providing
> information about reduced CPU performance to the scheduler. Usually code
> has to convert the value from frequency units into capacity units,
> which are understandable by the scheduler. Create a common conversion code
> which can be just used via a handy API.
>
> Internally, the topology_update_thermal_pressure() operates on frequency
> in MHz and max CPU frequency is taken from 'freq_factor' (per-cpu).
>
> Signed-off-by: Lukasz Luba <[email protected]>
Reviewed-by: Thara Gopinath <[email protected]>
--
Warm Regards
Thara (She/Her/Hers)
> ---
> arch/arm/include/asm/topology.h | 1 +
> arch/arm64/include/asm/topology.h | 1 +
> drivers/base/arch_topology.c | 43 ++++++++++++++++++++++++++++++-
> include/linux/arch_topology.h | 3 +++
> include/linux/sched/topology.h | 7 +++++
> 5 files changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
> index 470299ee2fba..f1eafacc9a30 100644
> --- a/arch/arm/include/asm/topology.h
> +++ b/arch/arm/include/asm/topology.h
> @@ -24,6 +24,7 @@
> /* Replace task scheduler's default thermal pressure API */
> #define arch_scale_thermal_pressure topology_get_thermal_pressure
> #define arch_set_thermal_pressure topology_set_thermal_pressure
> +#define arch_update_thermal_pressure topology_update_thermal_pressure
>
> #else
>
> diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
> index ec2db3419c41..7a421cbc99ed 100644
> --- a/arch/arm64/include/asm/topology.h
> +++ b/arch/arm64/include/asm/topology.h
> @@ -33,6 +33,7 @@ void update_freq_counters_refs(void);
> /* Replace task scheduler's default thermal pressure API */
> #define arch_scale_thermal_pressure topology_get_thermal_pressure
> #define arch_set_thermal_pressure topology_set_thermal_pressure
> +#define arch_update_thermal_pressure topology_update_thermal_pressure
>
> #include <asm-generic/topology.h>
>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index fc0836f460fb..bed7d502c9a1 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -25,6 +25,7 @@
> static DEFINE_PER_CPU(struct scale_freq_data __rcu *, sft_data);
> static struct cpumask scale_freq_counters_mask;
> static bool scale_freq_invariant;
> +static DEFINE_PER_CPU(u32, freq_factor) = 1;
>
> static bool supports_scale_freq_counters(const struct cpumask *cpus)
> {
> @@ -168,6 +169,47 @@ void topology_set_thermal_pressure(const struct cpumask *cpus,
> }
> EXPORT_SYMBOL_GPL(topology_set_thermal_pressure);
>
> +/**
> + * topology_update_thermal_pressure() - Update thermal pressure for CPUs
> + * @cpus : The related CPUs for which capacity has been reduced
> + * @capped_freq : The maximum allowed frequency that CPUs can run at
> + *
> + * Update the value of thermal pressure for all @cpus in the mask. The
> + * cpumask should include all (online+offline) affected CPUs, to avoid
> + * operating on stale data when hot-plug is used for some CPUs. The
> + * @capped_freq reflects the currently allowed max CPUs frequency due to
> + * thermal capping. It might be also a boost frequency value, which is bigger
> + * than the internal 'freq_factor' max frequency. In such case the pressure
> + * value should simply be removed, since this is an indication that there is
> + * no thermal throttling. The @capped_freq must be provided in kHz.
> + */
> +void topology_update_thermal_pressure(const struct cpumask *cpus,
> + unsigned long capped_freq)
> +{
> + unsigned long max_capacity, capacity;
> + u32 max_freq;
> + int cpu;
> +
> + cpu = cpumask_first(cpus);
> + max_capacity = arch_scale_cpu_capacity(cpu);
> + max_freq = per_cpu(freq_factor, cpu);
> +
> + /* Convert to MHz scale which is used in 'freq_factor' */
> + capped_freq /= 1000;
> +
> + /*
> + * Handle properly the boost frequencies, which should simply clean
> + * the thermal pressure value.
> + */
> + if (max_freq <= capped_freq)
> + capacity = max_capacity;
> + else
> + capacity = mult_frac(max_capacity, capped_freq, max_freq);
> +
> + arch_set_thermal_pressure(cpus, max_capacity - capacity);
> +}
> +EXPORT_SYMBOL_GPL(topology_update_thermal_pressure);
> +
> static ssize_t cpu_capacity_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -220,7 +262,6 @@ static void update_topology_flags_workfn(struct work_struct *work)
> update_topology = 0;
> }
>
> -static DEFINE_PER_CPU(u32, freq_factor) = 1;
> static u32 *raw_capacity;
>
> static int free_raw_capacity(void)
> diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
> index b97cea83b25e..ace1e5dcf773 100644
> --- a/include/linux/arch_topology.h
> +++ b/include/linux/arch_topology.h
> @@ -59,6 +59,9 @@ static inline unsigned long topology_get_thermal_pressure(int cpu)
> void topology_set_thermal_pressure(const struct cpumask *cpus,
> unsigned long th_pressure);
>
> +void topology_update_thermal_pressure(const struct cpumask *cpus,
> + unsigned long capped_freq);
> +
> struct cpu_topology {
> int thread_id;
> int core_id;
> diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
> index c07bfa2d80f2..6e89a8e43aa7 100644
> --- a/include/linux/sched/topology.h
> +++ b/include/linux/sched/topology.h
> @@ -273,6 +273,13 @@ void arch_set_thermal_pressure(const struct cpumask *cpus,
> { }
> #endif
>
> +#ifndef arch_update_thermal_pressure
> +static __always_inline
> +void arch_update_thermal_pressure(const struct cpumask *cpus,
> + unsigned long capped_frequency)
> +{ }
> +#endif
> +
> static inline int task_node(const struct task_struct *p)
> {
> return cpu_to_node(task_cpu(p));
>
On 11/16/21 11:39 PM, Thara Gopinath wrote:
>
>
> On 11/9/21 2:57 PM, Lukasz Luba wrote:
>> The thermal pressure is a mechanism which is used for providing
>> information about reduced CPU performance to the scheduler. Usually code
>> has to convert the value from frequency units into capacity units,
>> which are understandable by the scheduler. Create a common conversion
>> code
>> which can be just used via a handy API.
>>
>> Internally, the topology_update_thermal_pressure() operates on frequency
>> in MHz and max CPU frequency is taken from 'freq_factor' (per-cpu).
>>
>> Signed-off-by: Lukasz Luba <[email protected]>
>
> Reviewed-by: Thara Gopinath <[email protected]>
>
Thank you Thara for the review!
Hi Viresh,
On 11/11/21 3:15 AM, Viresh Kumar wrote:
> On 09-11-21, 19:57, Lukasz Luba wrote:
>> Hi all,
>>
>> This patch set v4 aims to refactor the thermal pressure update
>> code. There are already two clients which do similar thing:
>> convert the capped frequency value into the capacity of
>> affected CPU and call the 'set' function to store the
>> reduced capacity into the per-cpu variable.
>> There might be more than two of these users. In near future
>> it will be scmi-cpufreq driver, which receives notification
>> from FW about reduced frequency due to thermal. Other vendors
>> might follow. Let's avoid code duplication and potential
>> conversion bugs. Move the conversion code into the arch_topology.c
>> where the capacity calculation setup code and thermal pressure sit.
>>
>> Apart from that $subject patches, there is one patch (3/5) which fixes
>> issue in qcom-cpufreq-hw.c when the thermal pressure is not
>> updated for offline CPUs. It's similar fix that has been merged
>> recently for cpufreq_cooling.c:
>> 2ad8ccc17d1e4270cf65a3f2
>>
>> The patch 4/5 fixes also qcom-cpufreq-hw.c driver code which did
>> the translation from frequency to capacity wrongly when there
>> was a boost frequency available and stored in 'policy->cpuinfo.max_freq'.
>
> LGTM. I will apply this in a few days so people get time to Ack/Review
> the patches.
>
Thara has reviewed the patches. Could you take the patch set into
your tree, please?
On 09-11-21, 19:57, Lukasz Luba wrote:
> Hi all,
>
> This patch set v4 aims to refactor the thermal pressure update
> code. There are already two clients which do similar thing:
> convert the capped frequency value into the capacity of
> affected CPU and call the 'set' function to store the
> reduced capacity into the per-cpu variable.
> There might be more than two of these users. In near future
> it will be scmi-cpufreq driver, which receives notification
> from FW about reduced frequency due to thermal. Other vendors
> might follow. Let's avoid code duplication and potential
> conversion bugs. Move the conversion code into the arch_topology.c
> where the capacity calculation setup code and thermal pressure sit.
>
> Apart from that $subject patches, there is one patch (3/5) which fixes
> issue in qcom-cpufreq-hw.c when the thermal pressure is not
> updated for offline CPUs. It's similar fix that has been merged
> recently for cpufreq_cooling.c:
> 2ad8ccc17d1e4270cf65a3f2
>
> The patch 4/5 fixes also qcom-cpufreq-hw.c driver code which did
> the translation from frequency to capacity wrongly when there
> was a boost frequency available and stored in 'policy->cpuinfo.max_freq'.
>
> Changes:
> v4:
> - remove the warning when boost frequency is passed and set thermal
> pressure to 0 in that case, which means the capping is totally removed
> (issue reported by Steev)
> - remove the check from patch 4/5 with
> 'throttled_freq > policy->cpuinfo.max_freq' since it doesn't have
> effect; instead relay on new arch_update_thermal_pressure() handling
> correctly such use case; this would also fix an issue in that original
> driver code, where the reduced capacity was calculated wrongly due
> to 'policy->cpuinfo.max_freq' used as a divider
> - adjusted comments stressing the fact that the boost frequencies are
> supported
> Lukasz Luba (5):
> arch_topology: Introduce thermal pressure update function
> thermal: cpufreq_cooling: Use new thermal pressure update function
> cpufreq: qcom-cpufreq-hw: Update offline CPUs per-cpu thermal pressure
> cpufreq: qcom-cpufreq-hw: Use new thermal pressure update function
> arch_topology: Remove unused topology_set_thermal_pressure() and
> related
>
> arch/arm/include/asm/topology.h | 2 +-
> arch/arm64/include/asm/topology.h | 2 +-
> drivers/base/arch_topology.c | 42 ++++++++++++++++++++++++++++---
> drivers/cpufreq/qcom-cpufreq-hw.c | 14 +++--------
> drivers/thermal/cpufreq_cooling.c | 6 +----
> include/linux/arch_topology.h | 4 +--
> include/linux/sched/topology.h | 6 ++---
> init/Kconfig | 2 +-
> 8 files changed, 50 insertions(+), 28 deletions(-)
Applied. Thanks.
--
viresh
On 11/23/21 9:45 AM, Viresh Kumar wrote:
> On 09-11-21, 19:57, Lukasz Luba wrote:
>> Hi all,
>>
>> This patch set v4 aims to refactor the thermal pressure update
>> code. There are already two clients which do similar thing:
>> convert the capped frequency value into the capacity of
>> affected CPU and call the 'set' function to store the
>> reduced capacity into the per-cpu variable.
>> There might be more than two of these users. In near future
>> it will be scmi-cpufreq driver, which receives notification
>> from FW about reduced frequency due to thermal. Other vendors
>> might follow. Let's avoid code duplication and potential
>> conversion bugs. Move the conversion code into the arch_topology.c
>> where the capacity calculation setup code and thermal pressure sit.
>>
>> Apart from that $subject patches, there is one patch (3/5) which fixes
>> issue in qcom-cpufreq-hw.c when the thermal pressure is not
>> updated for offline CPUs. It's similar fix that has been merged
>> recently for cpufreq_cooling.c:
>> 2ad8ccc17d1e4270cf65a3f2
>>
>> The patch 4/5 fixes also qcom-cpufreq-hw.c driver code which did
>> the translation from frequency to capacity wrongly when there
>> was a boost frequency available and stored in 'policy->cpuinfo.max_freq'.
>>
>> Changes:
>> v4:
>> - remove the warning when boost frequency is passed and set thermal
>> pressure to 0 in that case, which means the capping is totally removed
>> (issue reported by Steev)
>> - remove the check from patch 4/5 with
>> 'throttled_freq > policy->cpuinfo.max_freq' since it doesn't have
>> effect; instead relay on new arch_update_thermal_pressure() handling
>> correctly such use case; this would also fix an issue in that original
>> driver code, where the reduced capacity was calculated wrongly due
>> to 'policy->cpuinfo.max_freq' used as a divider
>> - adjusted comments stressing the fact that the boost frequencies are
>> supported
>
>> Lukasz Luba (5):
>> arch_topology: Introduce thermal pressure update function
>> thermal: cpufreq_cooling: Use new thermal pressure update function
>> cpufreq: qcom-cpufreq-hw: Update offline CPUs per-cpu thermal pressure
>> cpufreq: qcom-cpufreq-hw: Use new thermal pressure update function
>> arch_topology: Remove unused topology_set_thermal_pressure() and
>> related
>>
>> arch/arm/include/asm/topology.h | 2 +-
>> arch/arm64/include/asm/topology.h | 2 +-
>> drivers/base/arch_topology.c | 42 ++++++++++++++++++++++++++++---
>> drivers/cpufreq/qcom-cpufreq-hw.c | 14 +++--------
>> drivers/thermal/cpufreq_cooling.c | 6 +----
>> include/linux/arch_topology.h | 4 +--
>> include/linux/sched/topology.h | 6 ++---
>> init/Kconfig | 2 +-
>> 8 files changed, 50 insertions(+), 28 deletions(-)
>
> Applied. Thanks.
>
Thank you!