Re: [PATCH 1/4] PM: EM: convert power field to micro-Watts precision and align drivers
From: Lukasz Luba
Date: Wed Jul 06 2022 - 05:09:29 EST
On 7/5/22 10:09, Daniel Lezcano wrote:
On 22/06/2022 16:57, Lukasz Luba wrote:
The milli-Watts precision causes rounding errors while calculating
efficiency cost for each OPP. This is especially visible in the 'simple'
Energy Model (EM), where the power for each OPP is provided from OPP
framework. This can cause some OPPs to be marked inefficient, while
using micro-Watts precision that might not happen.
Update all EM users which access 'power' field and assume the value is
in milli-Watts.
Solve also an issue with potential overflow in calculation of energy
estimation on 32bit machine. It's needed now since the power value
(thus the 'cost' as well) are higher.
Example calculation which shows the rounding error and impact:
power = 'dyn-power-coeff' * volt_mV * volt_mV * freq_MHz
power_a_uW = (100 * 600mW * 600mW * 500MHz) / 10^6 = 18000
power_a_mW = (100 * 600mW * 600mW * 500MHz) / 10^9 = 18
power_b_uW = (100 * 605mW * 605mW * 600MHz) / 10^6 = 21961
power_b_mW = (100 * 605mW * 605mW * 600MHz) / 10^9 = 21
max_freq = 2000MHz
cost_a_mW = 18 * 2000MHz/500MHz = 72
cost_a_uW = 18000 * 2000MHz/500MHz = 72000
cost_b_mW = 21 * 2000MHz/600MHz = 70 // <- artificially better
cost_b_uW = 21961 * 2000MHz/600MHz = 73203
The 'cost_b_mW' (which is based on old milli-Watts) is misleadingly
better that the 'cost_b_uW' (this patch uses micro-Watts) and such
would have impact on the 'inefficient OPPs' information in the Cpufreq
framework. This patch set removes the rounding issue.
Thanks for this detailed description, it really helps to understand why
this change is needed.
Perhaps it would make sense to add a power_uw in the EM structure and
keeping the old one with the milli-watts in order to reduce the impact
of the change.
It is a suggestion if you find it more convenient. Otherwise I'm fine
with this approach too.
I see your point, it could go with 2 patches instead of one. If there
will be a need of v2 I will consider this split.
A few comments below.
Signed-off-by: Lukasz Luba <lukasz.luba@xxxxxxx>
---
drivers/cpufreq/mediatek-cpufreq-hw.c | 7 +--
drivers/cpufreq/scmi-cpufreq.c | 6 +++
drivers/opp/of.c | 15 ++++---
drivers/powercap/dtpm_cpu.c | 5 +--
drivers/thermal/cpufreq_cooling.c | 13 +++++-
drivers/thermal/devfreq_cooling.c | 19 ++++++--
include/linux/energy_model.h | 63 ++++++++++++++++++++-------
kernel/power/energy_model.c | 31 ++++++++-----
8 files changed, 114 insertions(+), 45 deletions(-)
diff --git a/drivers/cpufreq/mediatek-cpufreq-hw.c
b/drivers/cpufreq/mediatek-cpufreq-hw.c
index 813cccbfe934..f0e0a35c7f21 100644
--- a/drivers/cpufreq/mediatek-cpufreq-hw.c
+++ b/drivers/cpufreq/mediatek-cpufreq-hw.c
@@ -51,7 +51,7 @@ static const u16 cpufreq_mtk_offsets[REG_ARRAY_SIZE]
= {
};
[ ... ]
diff --git a/drivers/thermal/cpufreq_cooling.c
b/drivers/thermal/cpufreq_cooling.c
index b8151d95a806..dc19e7c80751 100644
--- a/drivers/thermal/cpufreq_cooling.c
+++ b/drivers/thermal/cpufreq_cooling.c
@@ -21,6 +21,7 @@
#include <linux/pm_qos.h>
#include <linux/slab.h>
#include <linux/thermal.h>
+#include <linux/units.h>
#include <trace/events/thermal.h>
@@ -101,6 +102,7 @@ static unsigned long get_level(struct
cpufreq_cooling_device *cpufreq_cdev,
static u32 cpu_freq_to_power(struct cpufreq_cooling_device
*cpufreq_cdev,
u32 freq)
{
+ unsigned long power_mw;
int i;
for (i = cpufreq_cdev->max_level - 1; i >= 0; i--) {
@@ -108,16 +110,23 @@ static u32 cpu_freq_to_power(struct
cpufreq_cooling_device *cpufreq_cdev,
break;
}
- return cpufreq_cdev->em->table[i + 1].power;
+ power_mw = cpufreq_cdev->em->table[i + 1].power;
+ power_mw /= MICROWATT_PER_MILLIWATT;
Won't this fail with an unresolved symbols on some archs ? I mean may be
do_div should be used instead ?
I've run that code in internal CI for all archs and didn't crash.
We already have a division in IPA or in devfreq_cooling where
the variables are 32bit and works fine.
+
+ return power_mw;
}
[ ... ]
The em_validate_cost() is in this cut section.
#ifdef CONFIG_64BIT
-#define em_scale_power(p) ((p) * 1000)
+#define em_estimate_energy(cost, sum_util, scale_cpu) \
+ (((cost) * (sum_util)) / (scale_cpu))
#else
-#define em_scale_power(p) (p)
+#define em_estimate_energy(cost, sum_util, scale_cpu) \
+ (((cost) / (scale_cpu)) * (sum_util))
#endif
struct em_data_callback {
@@ -112,7 +143,7 @@ struct em_data_callback {
* and frequency.
*
* In case of CPUs, the power is the one of a single CPU in the
domain,
- * expressed in milli-Watts or an abstract scale. It is expected to
+ * expressed in micro-Watts or an abstract scale. It is expected to
* fit in the [0, EM_MAX_POWER] range.
*
* Return 0 on success.
@@ -148,7 +179,7 @@ struct em_perf_domain *em_cpu_get(int cpu);
struct em_perf_domain *em_pd_get(struct device *dev);
int em_dev_register_perf_domain(struct device *dev, unsigned int
nr_states,
struct em_data_callback *cb, cpumask_t *span,
- bool milliwatts);
+ bool microwatts);
void em_dev_unregister_perf_domain(struct device *dev);
/**
@@ -273,7 +304,7 @@ static inline unsigned long em_cpu_energy(struct
em_perf_domain *pd,
* pd_nrg = ------------------------ (4)
* scale_cpu
*/
- return ps->cost * sum_util / scale_cpu;
+ return em_estimate_energy(ps->cost, sum_util, scale_cpu);
}
/**
@@ -297,7 +328,7 @@ struct em_data_callback {};
static inline
int em_dev_register_perf_domain(struct device *dev, unsigned int
nr_states,
struct em_data_callback *cb, cpumask_t *span,
- bool milliwatts)
+ bool microwatts)
{
return -EINVAL;
}
diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
index 6c373f2960e7..910668ec8838 100644
--- a/kernel/power/energy_model.c
+++ b/kernel/power/energy_model.c
@@ -108,10 +108,11 @@ static void em_debug_remove_pd(struct device
*dev) {}
static int em_create_perf_table(struct device *dev, struct
em_perf_domain *pd,
int nr_states, struct em_data_callback *cb,
- unsigned long flags)
+ unsigned long flags, int num_devs)
{
unsigned long power, freq, prev_freq = 0, prev_cost = ULONG_MAX;
struct em_perf_state *table;
+ unsigned long max_cost = 0;
int i, ret;
u64 fmax;
@@ -145,7 +146,7 @@ static int em_create_perf_table(struct device
*dev, struct em_perf_domain *pd,
/*
* The power returned by active_state() is expected to be
- * positive and to fit into 16 bits.
+ * positive and be in range.
*/
if (!power || power > EM_MAX_POWER) {
dev_err(dev, "EM: invalid power: %lu\n",
@@ -170,7 +171,7 @@ static int em_create_perf_table(struct device
*dev, struct em_perf_domain *pd,
goto free_ps_table;
}
} else {
- power_res = em_scale_power(table[i].power);
+ power_res = table[i].power;
cost = div64_u64(fmax * power_res, table[i].frequency);
}
@@ -183,6 +184,15 @@ static int em_create_perf_table(struct device
*dev, struct em_perf_domain *pd,
} else {
prev_cost = table[i].cost;
}
+
+ if (max_cost < table[i].cost)
+ max_cost = table[i].cost;
+ }
+
+ /* Check if it won't overflow during energy estimation. */
+ if (em_validate_cost(max_cost, num_devs)) {
I'm not finding the em_validate_cost() function
It's in the energy_model.h
+ dev_err(dev, "EM: too big 'cost' value: %lu\n", max_cost);
+ goto free_ps_table;
}
pd->table = table;
@@ -199,9 +209,9 @@ static int em_create_pd(struct device *dev, int
nr_states,
struct em_data_callback *cb, cpumask_t *cpus,
unsigned long flags)
{
+ int cpu, ret, num_devs = 1;
struct em_perf_domain *pd;
struct device *cpu_dev;
- int cpu, ret;
if (_is_cpu_device(dev)) {
pd = kzalloc(sizeof(*pd) + cpumask_size(), GFP_KERNEL);
@@ -209,13 +219,14 @@ static int em_create_pd(struct device *dev, int
nr_states,
return -ENOMEM;
cpumask_copy(em_span_cpus(pd), cpus);
+ num_devs = cpumask_weight(cpus);
Why is this change needed ? What is the connection with the uW unit
change ?
We support 32bit arch still with the 'unsigned long power' variable,
but we would store e.g. 1.2 Watts there as:
power = 1200000 // 0x124f80
not
power = 1200 // 0x4b0
This would use > 20bits as you can see. We then calculate:
cost_i = power_i * fmax / freq_i
which is used by EAS.
The value from the 'cost' is used for calculating energy in EAS:
unsigned long energy = (cost * sum_utilization) / cpu_arch_capacity
OR on 32bit machines:
unsigned long energy = (cost / cpu_arch_capacity) * sum_utilization
We cannot overflow in any use case. The 'num_devs' is part of this
mechanism. as you can see in this example for 32bit:
max_possible_cost_for_fmax = 64000000 //64Watts
energy = (64000000 / cpu_arch_capacity) * (num_cpus *
max_cpu_utilization) =>
// assume: cpu_arch_capacity == max_cpu_utilization is true
unsigned long energy = 64000000 * num_cpus
Then question:
Q: how many cpus you can have to not overflow?
A: depends on your max_power and then 'cost'
In the above example:
num_cpus must be < 68
I can simplify this to just put a new define for 32bit
machines like num_cpus=16 for safety:
#ifdef CONFIG_64BIT
#define EM_MAX_NUM_CPUS UINT_MAX
#else
#define EM_MAX_NUM_CPUS 16 /*we don't expect more than that */
Then there is no need to modify that calculation function
em_create_perf_table()
The more I look at this the more I'm convinced to do that...
In the old code, the power value had limit to 16bits, the num_cpus
also had limit IIRC to 16bit, thus multiplication wasn't a problem.