[RFC PATCH v3 00/10] Energy Aware Scheduling

[RFC PATCH v3 00/10] Energy Aware Scheduling

[Quentin Perret]

1. Overview

The Energy Aware Scheduler (EAS) based on Morten Rasmussen's posting on
LKML [1] is currently part of the AOSP Common Kernel and runs on today's
smartphones with Arm's big.LITTLE CPUs. This series implements a new and
largely simplified version of EAS based on an Energy Model (EM) of the
platform with only costs for the active states of the CPUs.

Previous versions of this patch-set (i.e. [2]) relied on the PM_OPP
framework to provide the scheduler with an Energy Model. As agreed during
the 2nd OSPM sumit, this new revision removes this dependency by
implementing a new independent EM framework. This framework aggregates
the power values provided by drivers into a table for each frequency
domain in the system. Those tables are then available to interested
clients (e.g. the task scheduler or the thermal subsystem) via
platform-agnostic APIs. The topology code of the scheduler is modified
accordingly to take a reference on the online frequency domains, hence
guaranteeing a fast access to the shared EM data structures in
latency-sensitive code paths. The modifications required to make the
thermal subsystem use the new Energy Model framework are not covered by
this patch-set.

The v2 of this patch-set used a per-scheduling domain overutilization
flag, which has been abandoned in v3 in favour of a simpler but equally
efficient system-wide implementation attached to the root domain (like
the existing overload flag). Consequently, the integration of EAS in the
wake-up path has been reworked to accommodate this change using a
scheduling domain shortcut.

The patch-set is now arranged as follows:
 - Patches 1-2 refactor code from schedutil and the scheduler to
   simplify the implementation of the EM framework;
 - Patches 3-4 introduce the centralized EM framework;
 - Patch 5 changes the scheduler topology code to make it aware of the EM;
 - Patch 6 implements the overutilization mechanism;
 - Patches 7-9 introduce the energy-aware wake-up path in the CFS class;
 - Patch 10 starts EAS for arm/arm64 from the arch_topology driver.

2. Test results

Two fundamentally different tests were executed. Firstly the energy
test case shows the impact on energy consumption this patch-set has
using a synthetic set of tasks. Secondly the performance test case
provides the conventional hackbench metric numbers.

The tests run on two arm64 big.LITTLE platforms: Hikey960 (4xA73 +
4xA53) and Juno r0 (2xA57 + 4xA53).

Base kernel is tip/sched/core (4.17-rc3), with some Hikey960 and
Juno specific patches, the SD_ASYM_CPUCAPACITY flag set at DIE sched
domain level for arm64 and schedutil as cpufreq governor [4].

2.1 Energy test case

10 iterations of between 10 and 50 periodic rt-app tasks (16ms period,
5% duty-cycle) for 30 seconds with energy measurement. Unit is Joules.
The goal is to save energy, so lower is better.

2.1.1 Hikey960

Energy is measured with an ACME Cape on an instrumented board. Numbers
include consumption of big and little CPUs, LPDDR memory, GPU and most
of the other small components on the board. They do not include
consumption of the radio chip (turned-off anyway) and external
connectors.

+----------+-----------------+------------------------+
|          | Without patches | With patches           |
+----------+--------+--------+-----------------+------+
| Tasks nb |  Mean  | RSD*   | Mean            | RSD* |
+----------+--------+--------+-----------------+------+
|       10 |  33.45 |   1.2% | 28.97 (-13.39%) | 2.0% |
|       20 |  45.45 |   1.7% | 42.76 (-5.92%)  | 0.8% |
|       30 |  65.06 |   0.2% | 64.85 (-0.32%)  | 4.7% |
|       40 |  85.67 |   0.7% | 77.98 (-8.98%)  | 2.8% |
|       50 | 110.14 |   0.9% | 99.34 (-9.81%)  | 2.0% |
+----------+--------+--------+-----------------+------+

2.1.2 Juno r0

Energy is measured with the onboard energy meter. Numbers include
consumption of big and little CPUs.

+----------+-----------------+------------------------+
|          | Without patches | With patches           |
+----------+--------+--------+-----------------+------+
| Tasks nb |  Mean  | RSD*   | Mean            | RSD* |
+----------+--------+--------+-----------------+------+
|       10 |  10.40 |   3.0% |  7.00 (-32.69%) | 2.5% |
|       20 |  18.47 |   1.1% | 12.88 (-30.27%) | 2.4% |
|       30 |  27.97 |   2.2% | 21.26 (-23.99%) | 0.2% |
|       40 |  36.86 |   1.2% | 30.63 (-16.90%) | 0.4% |
|       50 |  46.79 |   0.5% | 45.85 ( -0.02%) | 0.7% |
+----------+--------+--------+------------------+------+

2.2 Performance test case

30 iterations of perf bench sched messaging --pipe --thread --group G
--loop L with G=[1 2 4 8] and L=50000 (Hikey960)/16000 (Juno r0).

2.2.1 Hikey960

The impact of thermal capping was mitigated thanks to a heatsink, a
fan, and a 10 sec delay between two successive executions.

+----------------+-----------------+------------------------+
|                | Without patches | With patches           |
+--------+-------+---------+-------+----------------+-------+
| Groups | Tasks | Mean    | RSD*  | Mean           | RSD*  |
+--------+-------+---------+-------+----------------+-------+
|      1 |    40 |    8.75 | 0.99% |  9.46 (+8.11%) | 3.34% |
|      2 |    80 |   15.64 | 0.68% | 15.96 (+2.05%) | 0.71% |
|      4 |   160 |   31.58 | 0.65% | 32.22 (+2.03%) | 0.61% |
|      8 |   320 |   65.53 | 0.37% | 66.43 (+1.37%) | 0.36% |
+--------+-------+---------+-------+----------------+-------+

2.2.2 Juno r0

+----------------+-----------------+------------------------+
|                | Without patches | With patches           |
+--------+-------+---------+-------+----------------+-------+
| Groups | Tasks | Mean    | RSD*  | Mean           | RSD*  |
+--------+-------+---------+-------+----------------+-------+
|      1 |    40 |    8.25 | 0.11% |  8.21 ( 0.00%) | 0.10% |
|      2 |    80 |   14.40 | 0.14% | 14.37 ( 0.00%) | 0.12% |
|      4 |   160 |   26.72 | 0.24% | 26.73 ( 0.00%) | 0.14% |
|      8 |   320 |   52.89 | 0.10% | 52.87 ( 0.00%) | 0.23% |
+--------+-------+---------+-------+----------------+-------+

*RSD: Relative Standard Deviation (std dev / mean)

3. Changes between versions

Changes v2[2]->v3:
- Removed the PM_OPP dependency by implementing a new EM framework
- Modified the scheduler topology code to take references on the EM data
  structures
- Simplified the overutilization mechanism into a system-wide flag
- Reworked the integration in the wake-up path using the sd_ea shortcut
- Rebased on tip/sched/core (247f2f6f3c70 "sched/core: Don't schedule
  threads on pre-empted vCPUs")

Changes v1[3]->v2:
- Reworked interface between fair.c and energy.[ch] (Remove #ifdef
  CONFIG_PM_OPP from energy.c) (Greg KH)
- Fixed licence & header issue in energy.[ch] (Greg KH)
- Reordered EAS path in select_task_rq_fair() (Joel)
- Avoid prev_cpu if not allowed in select_task_rq_fair() (Morten/Joel)
- Refactored compute_energy() (Patrick)
- Account for RT/IRQ pressure in task_fits() (Patrick)
- Use UTIL_EST and DL utilization during OPP estimation (Patrick/Juri)
- Optimize selection of CPU candidates in the energy-aware wake-up path
- Rebased on top of tip/sched/core (commit b720342849fe “sched/core:
  Update Preempt_notifier_key to modern API”)

[1] https://lkml.org/lkml/2015/7/7/754
[2] https://marc.info/?l=linux-kernel&m=152302902427143&w=2
[3] https://marc.info/?l=linux-kernel&m=152153905805048&w=2
[4] http://linux-arm.org/git?p=linux-qp.git;a=shortlog;h=refs/heads/upstream/eas_v3

Morten Rasmussen (1):
  sched: Add over-utilization/tipping point indicator

Quentin Perret (9):
  sched: Relocate arch_scale_cpu_capacity
  sched/cpufreq: Factor out utilization to frequency mapping
  PM: Introduce an Energy Model management framework
  PM / EM: Expose the Energy Model in sysfs
  sched/topology: Reference the Energy Model of CPUs when available
  sched/fair: Introduce an energy estimation helper function
  sched: Lowest energy aware balancing sched_domain level pointer
  sched/fair: Select an energy-efficient CPU on task wake-up
  arch_topology: Start Energy Aware Scheduling

 drivers/base/arch_topology.c     |  19 ++
 include/linux/energy_model.h     | 123 +++++++++++
 include/linux/sched/cpufreq.h    |   6 +
 include/linux/sched/topology.h   |  19 ++
 kernel/power/Kconfig             |  15 ++
 kernel/power/Makefile            |   2 +
 kernel/power/energy_model.c      | 343 +++++++++++++++++++++++++++++++
 kernel/sched/cpufreq_schedutil.c |   3 +-
 kernel/sched/fair.c              | 186 ++++++++++++++++-
 kernel/sched/sched.h             |  51 +++--
 kernel/sched/topology.c          | 117 +++++++++++
 11 files changed, 860 insertions(+), 24 deletions(-)
 create mode 100644 include/linux/energy_model.h
 create mode 100644 kernel/power/energy_model.c

-- 
2.17.0

[RFC PATCH v3 01/10] sched: Relocate arch_scale_cpu_capacity

[Quentin Perret]

By default, arch_scale_cpu_capacity() is only visible from within the
kernel/sched folder. Relocate it to include/linux/sched/topology.h to
make it visible to other clients needing to know about the capacity of
CPUs, such as the Energy Model framework.

Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Quentin Perret <quentin.perret@arm.com>
---
 include/linux/sched/topology.h | 19 +++++++++++++++++++
 kernel/sched/sched.h           | 18 ------------------
 2 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
index 26347741ba50..1e24e88bee6d 100644
--- a/include/linux/sched/topology.h
+++ b/include/linux/sched/topology.h
@@ -202,6 +202,17 @@ extern void set_sched_topology(struct sched_domain_topology_level *tl);
 # define SD_INIT_NAME(type)
 #endif
 
+#ifndef arch_scale_cpu_capacity
+static __always_inline
+unsigned long arch_scale_cpu_capacity(struct sched_domain *sd, int cpu)
+{
+	if (sd && (sd->flags & SD_SHARE_CPUCAPACITY) && (sd->span_weight > 1))
+		return sd->smt_gain / sd->span_weight;
+
+	return SCHED_CAPACITY_SCALE;
+}
+#endif
+
 #else /* CONFIG_SMP */
 
 struct sched_domain_attr;
@@ -217,6 +228,14 @@ static inline bool cpus_share_cache(int this_cpu, int that_cpu)
 	return true;
 }
 
+#ifndef arch_scale_cpu_capacity
+static __always_inline
+unsigned long arch_scale_cpu_capacity(void __always_unused *sd, int cpu)
+{
+	return SCHED_CAPACITY_SCALE;
+}
+#endif
+
 #endif	/* !CONFIG_SMP */
 
 static inline int task_node(const struct task_struct *p)
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 15750c222ca2..ce562d3b7526 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1724,30 +1724,12 @@ unsigned long arch_scale_freq_capacity(int cpu)
 #ifdef CONFIG_SMP
 extern void sched_avg_update(struct rq *rq);
 
-#ifndef arch_scale_cpu_capacity
-static __always_inline
-unsigned long arch_scale_cpu_capacity(struct sched_domain *sd, int cpu)
-{
-	if (sd && (sd->flags & SD_SHARE_CPUCAPACITY) && (sd->span_weight > 1))
-		return sd->smt_gain / sd->span_weight;
-
-	return SCHED_CAPACITY_SCALE;
-}
-#endif
-
 static inline void sched_rt_avg_update(struct rq *rq, u64 rt_delta)
 {
 	rq->rt_avg += rt_delta * arch_scale_freq_capacity(cpu_of(rq));
 	sched_avg_update(rq);
 }
 #else
-#ifndef arch_scale_cpu_capacity
-static __always_inline
-unsigned long arch_scale_cpu_capacity(void __always_unused *sd, int cpu)
-{
-	return SCHED_CAPACITY_SCALE;
-}
-#endif
 static inline void sched_rt_avg_update(struct rq *rq, u64 rt_delta) { }
 static inline void sched_avg_update(struct rq *rq) { }
 #endif
-- 
2.17.0

[RFC PATCH v3 02/10] sched/cpufreq: Factor out utilization to frequency mapping

[Quentin Perret]

The schedutil governor maps utilization values to frequencies by applying
a 25% margin. Since this sort of mapping mechanism can be needed by other
users (i.e. EAS), factor the utilization-to-frequency mapping code out
of schedutil and move it to include/linux/sched/cpufreq.h to avoid code
duplication. The new map_util_freq() function is inlined to avoid
overheads.

Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Quentin Perret <quentin.perret@arm.com>
---
 include/linux/sched/cpufreq.h    | 6 ++++++
 kernel/sched/cpufreq_schedutil.c | 3 ++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/linux/sched/cpufreq.h b/include/linux/sched/cpufreq.h
index 59667444669f..afa940cd50dc 100644
--- a/include/linux/sched/cpufreq.h
+++ b/include/linux/sched/cpufreq.h
@@ -20,6 +20,12 @@ void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data,
                        void (*func)(struct update_util_data *data, u64 time,
 				    unsigned int flags));
 void cpufreq_remove_update_util_hook(int cpu);
+
+static inline unsigned long map_util_freq(unsigned long util,
+					unsigned long freq, unsigned long cap)
+{
+	return (freq + (freq >> 2)) * util / cap;
+}
 #endif /* CONFIG_CPU_FREQ */
 
 #endif /* _LINUX_SCHED_CPUFREQ_H */
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 63014cff76a5..4fdc6eb7c88b 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -13,6 +13,7 @@
 
 #include "sched.h"
 
+#include <linux/sched/cpufreq.h>
 #include <trace/events/power.h>
 
 struct sugov_tunables {
@@ -163,7 +164,7 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
 	unsigned int freq = arch_scale_freq_invariant() ?
 				policy->cpuinfo.max_freq : policy->cur;
 
-	freq = (freq + (freq >> 2)) * util / max;
+	freq = map_util_freq(util, freq, max);
 
 	if (freq == sg_policy->cached_raw_freq && sg_policy->next_freq != UINT_MAX)
 		return sg_policy->next_freq;
-- 
2.17.0

[RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Quentin Perret]

Several subsystems in the kernel (scheduler and/or thermal at the time
of writing) can benefit from knowing about the energy consumed by CPUs.
Yet, this information can come from different sources (DT or firmware for
example), in different formats, hence making it hard to exploit without
a standard API.

This patch attempts to solve this issue by introducing a centralized
Energy Model (EM) framework which can be used to interface the data
providers with the client subsystems. This framework standardizes the
API to expose power costs, and to access them from multiple locations.

The current design assumes that all CPUs in a frequency domain share the
same micro-architecture. As such, the EM data is structured in a
per-frequency-domain fashion. Drivers aware of frequency domains
(typically, but not limited to, CPUFreq drivers) are expected to register
data in the EM framework using the em_register_freq_domain() API. To do
so, the drivers must provide a callback function that will be called by
the EM framework to populate the tables. As of today, only the active
power of the CPUs is considered. For each frequency domain, the EM
includes a list of <frequency, power, capacity> tuples for the capacity
states of the domain alongside a cpumask covering the involved CPUs.

The EM framework also provides an API to re-scale the capacity values
of the model asynchronously, after it has been created. This is required
for architectures where the capacity scale factor of CPUs can change at
run-time. This is the case for Arm/Arm64 for example where the
arch_topology driver recomputes the capacity scale factors of the CPUs
after the maximum frequency of all CPUs has been discovered. Although
complex, the process of creating and re-scaling the EM has to be kept in
two separate steps to fulfill the needs of the different users. The thermal
subsystem doesn't use the capacity values and shouldn't have dependencies
on subsystems providing them. On the other hand, the task scheduler needs
the capacity values, and it will benefit from seeing them up-to-date when
applicable.

Because of this need for asynchronous update, the capacity state table
of each frequency domain is protected by RCU, hence guaranteeing a safe
modification of the table and a fast access to readers in latency-sensitive
code paths.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Signed-off-by: Quentin Perret <quentin.perret@arm.com>
---
 include/linux/energy_model.h | 122 +++++++++++++++++
 kernel/power/Kconfig         |  15 +++
 kernel/power/Makefile        |   2 +
 kernel/power/energy_model.c  | 249 +++++++++++++++++++++++++++++++++++
 4 files changed, 388 insertions(+)
 create mode 100644 include/linux/energy_model.h
 create mode 100644 kernel/power/energy_model.c

diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
new file mode 100644
index 000000000000..edde888852ba
--- /dev/null
+++ b/include/linux/energy_model.h
@@ -0,0 +1,122 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_ENERGY_MODEL_H
+#define _LINUX_ENERGY_MODEL_H
+#include <linux/types.h>
+#include <linux/cpumask.h>
+#include <linux/jump_label.h>
+#include <linux/rcupdate.h>
+#include <linux/kobject.h>
+#include <linux/sched/cpufreq.h>
+
+#ifdef CONFIG_ENERGY_MODEL
+struct em_cap_state {
+	unsigned long capacity;
+	unsigned long frequency;
+	unsigned long power;
+};
+
+struct em_cs_table {
+	struct em_cap_state *state;
+	int nr_cap_states;
+	struct rcu_head rcu;
+};
+
+struct em_freq_domain {
+	struct em_cs_table *cs_table;
+	cpumask_t cpus;
+};
+
+struct em_data_callback {
+	/**
+	 * active_power() - Provide power at the next capacity state of a CPU
+	 * @power	: Active power at the capacity state (modified)
+	 * @freq	: Frequency at the capacity state (modified)
+	 * @cpu		: CPU for which we do this operation
+	 *
+	 * active_power() must find the lowest capacity state of 'cpu' above
+	 * 'freq' and update 'power' and 'freq' to the matching active power
+	 * and frequency.
+	 *
+	 * Return 0 on success.
+	 */
+	int (*active_power) (unsigned long *power, unsigned long *freq, int cpu);
+};
+
+int em_register_freq_domain(cpumask_t *span, unsigned int nr_states,
+						struct em_data_callback *cb);
+void em_rescale_cpu_capacity(void);
+struct em_freq_domain *em_cpu_get(int cpu);
+
+/**
+ * em_fd_energy() - Estimates the energy consumed by the CPUs of a freq. domain
+ * @fd		: frequency domain for which energy has to be estimated
+ * @max_util	: highest utilization among CPUs of the domain
+ * @sum_util	: sum of the utilization of all CPUs in the domain
+ *
+ * Return: the sum of the energy consumed by the CPUs of the domain assuming
+ * a capacity state satisfying the max utilization of the domain.
+ */
+static inline unsigned long em_fd_energy(struct em_freq_domain *fd,
+				unsigned long max_util, unsigned long sum_util)
+{
+	struct em_cs_table *cs_table;
+	struct em_cap_state *cs;
+	unsigned long freq;
+	int i;
+
+	cs_table = rcu_dereference(fd->cs_table);
+	if (!cs_table)
+		return 0;
+
+	/* Map the utilization value to a frequency */
+	cs = &cs_table->state[cs_table->nr_cap_states - 1];
+	freq = map_util_freq(max_util, cs->frequency, cs->capacity);
+
+	/* Find the lowest capacity state above this frequency */
+	for (i = 0; i < cs_table->nr_cap_states; i++) {
+		cs = &cs_table->state[i];
+		if (cs->frequency >= freq)
+			break;
+	}
+
+	return cs->power * sum_util / cs->capacity;
+}
+
+/**
+ * em_fd_nr_cap_states() - Get the number of capacity states of a freq. domain
+ * @fd		: frequency domain for which want to do this
+ *
+ * Return: the number of capacity state in the frequency domain table
+ */
+static inline int em_fd_nr_cap_states(struct em_freq_domain *fd)
+{
+	struct em_cs_table *table = rcu_dereference(fd->cs_table);
+
+	return table->nr_cap_states;
+}
+
+#else
+struct em_freq_domain;
+struct em_data_callback;
+static inline int em_register_freq_domain(cpumask_t *span,
+			unsigned int nr_states, struct em_data_callback *cb)
+{
+	return -ENOTSUPP;
+}
+static inline struct em_freq_domain *em_cpu_get(int cpu)
+{
+	return NULL;
+}
+static inline unsigned long em_fd_energy(struct em_freq_domain *fd,
+			unsigned long max_util, unsigned long sum_util)
+{
+	return 0;
+}
+static inline int em_fd_nr_cap_states(struct em_freq_domain *fd)
+{
+	return 0;
+}
+static inline void em_rescale_cpu_capacity(void) { }
+#endif
+
+#endif
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index e880ca22c5a5..b9e2b92e3be1 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -297,3 +297,18 @@ config PM_GENERIC_DOMAINS_OF
 
 config CPU_PM
 	bool
+
+config ENERGY_MODEL
+	bool "Energy Model for CPUs"
+	depends on SMP
+	depends on CPU_FREQ
+	default n
+	help
+	  Several subsystems (thermal and/or the task scheduler for example)
+	  can leverage information about the energy consumed by CPUs to make
+	  smarter decisions. This config option enables the framework from
+	  which a user can access the energy models.
+
+	  The exact usage of the energy model is subsystem-dependent.
+
+	  If in doubt, say N.
diff --git a/kernel/power/Makefile b/kernel/power/Makefile
index a3f79f0eef36..e7e47d9be1e5 100644
--- a/kernel/power/Makefile
+++ b/kernel/power/Makefile
@@ -15,3 +15,5 @@ obj-$(CONFIG_PM_AUTOSLEEP)	+= autosleep.o
 obj-$(CONFIG_PM_WAKELOCKS)	+= wakelock.o
 
 obj-$(CONFIG_MAGIC_SYSRQ)	+= poweroff.o
+
+obj-$(CONFIG_ENERGY_MODEL)	+= energy_model.o
diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
new file mode 100644
index 000000000000..a2eece7007a8
--- /dev/null
+++ b/kernel/power/energy_model.c
@@ -0,0 +1,249 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Energy Model of CPUs
+ *
+ * Copyright (c) 2018, Arm ltd.
+ * Written by: Quentin Perret, Arm ltd.
+ */
+
+#define pr_fmt(fmt) "energy_model: " fmt
+
+#include <linux/cpu.h>
+#include <linux/slab.h>
+#include <linux/cpumask.h>
+#include <linux/energy_model.h>
+#include <linux/sched/topology.h>
+
+/* Mapping of each CPU to the frequency domain to which it belongs. */
+static DEFINE_PER_CPU(struct em_freq_domain *, em_data);
+
+/*
+ * Protects the access to em_data. Readers of em_data can be in RCU-critical
+ * sections, and can't afford to sleep.
+ */
+static DEFINE_RWLOCK(em_data_lock);
+
+/*
+ * Mutex serializing the registrations of frequency domains. It allows the
+ * callbacks defined by drivers to sleep.
+ */
+static DEFINE_MUTEX(em_fd_mutex);
+
+static struct em_cs_table *alloc_cs_table(int nr_states)
+{
+	struct em_cs_table *cs_table;
+
+	cs_table = kzalloc(sizeof(*cs_table), GFP_NOWAIT);
+	if (!cs_table)
+		return NULL;
+
+	cs_table->state = kcalloc(nr_states, sizeof(*cs_table->state),
+								GFP_NOWAIT);
+	if (!cs_table->state) {
+		kfree(cs_table);
+		return NULL;
+	}
+
+	cs_table->nr_cap_states = nr_states;
+
+	return cs_table;
+}
+
+static void free_cs_table(struct em_cs_table *table)
+{
+	if (table) {
+		kfree(table->state);
+		kfree(table);
+	}
+}
+
+static void fd_update_cs_table(struct em_cs_table *cs_table, int cpu)
+{
+	unsigned long cmax = arch_scale_cpu_capacity(NULL, cpu);
+	int max_cap_state = cs_table->nr_cap_states - 1;
+	unsigned long fmax = cs_table->state[max_cap_state].frequency;
+	int i;
+
+	for (i = 0; i < cs_table->nr_cap_states; i++)
+		cs_table->state[i].capacity = cmax *
+					cs_table->state[i].frequency / fmax;
+}
+
+static struct em_freq_domain *em_create_fd(cpumask_t *span, int nr_states,
+						struct em_data_callback *cb)
+{
+	unsigned long opp_eff, prev_opp_eff = ULONG_MAX;
+	int i, ret, cpu = cpumask_first(span);
+	struct em_freq_domain *fd;
+	unsigned long power, freq;
+
+	if (!cb->active_power)
+		return NULL;
+
+	fd = kzalloc(sizeof(*fd), GFP_KERNEL);
+	if (!fd)
+		return NULL;
+
+	fd->cs_table = alloc_cs_table(nr_states);
+	if (!fd->cs_table)
+		goto free_fd;
+
+	/* Copy the span of the frequency domain */
+	cpumask_copy(&fd->cpus, span);
+
+	/* Build the list of capacity states for this freq domain */
+	for (i = 0, freq = 0; i < nr_states; i++, freq++) {
+		ret = cb->active_power(&power, &freq, cpu);
+		if (ret)
+			goto free_cs_table;
+
+		fd->cs_table->state[i].power = power;
+		fd->cs_table->state[i].frequency = freq;
+
+		/*
+		 * The hertz/watts efficiency ratio should decrease as the
+		 * frequency grows on sane platforms. If not, warn the user
+		 * that some high OPPs are more power efficient than some
+		 * of the lower ones.
+		 */
+		opp_eff = freq / power;
+		if (opp_eff >= prev_opp_eff)
+			pr_warn("%*pbl: hz/watt efficiency: OPP %d >= OPP%d\n",
+					cpumask_pr_args(span), i, i - 1);
+		prev_opp_eff = opp_eff;
+	}
+	fd_update_cs_table(fd->cs_table, cpu);
+
+	return fd;
+
+free_cs_table:
+	free_cs_table(fd->cs_table);
+free_fd:
+	kfree(fd);
+
+	return NULL;
+}
+
+static void rcu_free_cs_table(struct rcu_head *rp)
+{
+	struct em_cs_table *table;
+
+	table = container_of(rp, struct em_cs_table, rcu);
+	free_cs_table(table);
+}
+
+/**
+ * em_rescale_cpu_capacity() - Re-scale capacity values of the Energy Model
+ *
+ * This re-scales the capacity values for all capacity states of all frequency
+ * domains of the Energy Model. This should be used when the capacity values
+ * of the CPUs are updated at run-time, after the EM was registered.
+ */
+void em_rescale_cpu_capacity(void)
+{
+	struct em_cs_table *old_table, *new_table;
+	struct em_freq_domain *fd;
+	unsigned long flags;
+	int nr_states, cpu;
+
+	read_lock_irqsave(&em_data_lock, flags);
+	for_each_cpu(cpu, cpu_possible_mask) {
+		fd = per_cpu(em_data, cpu);
+		if (!fd || cpu != cpumask_first(&fd->cpus))
+			continue;
+
+		/* Copy the existing table. */
+		old_table = rcu_dereference(fd->cs_table);
+		nr_states = old_table->nr_cap_states;
+		new_table = alloc_cs_table(nr_states);
+		if (!new_table) {
+			read_unlock_irqrestore(&em_data_lock, flags);
+			return;
+		}
+		memcpy(new_table->state, old_table->state,
+					nr_states * sizeof(*new_table->state));
+
+		/* Re-scale the capacity values on the copy. */
+		fd_update_cs_table(new_table, cpumask_first(&fd->cpus));
+
+		/* Replace the table with the rescaled version. */
+		rcu_assign_pointer(fd->cs_table, new_table);
+		call_rcu(&old_table->rcu, rcu_free_cs_table);
+	}
+	read_unlock_irqrestore(&em_data_lock, flags);
+	pr_debug("Re-scaled CPU capacities\n");
+}
+EXPORT_SYMBOL_GPL(em_rescale_cpu_capacity);
+
+/**
+ * em_cpu_get() - Return the frequency domain for a CPU
+ * @cpu : CPU to find the frequency domain for
+ *
+ * Return: the frequency domain to which 'cpu' belongs, or NULL if it doesn't
+ * exist.
+ */
+struct em_freq_domain *em_cpu_get(int cpu)
+{
+	struct em_freq_domain *fd;
+	unsigned long flags;
+
+	read_lock_irqsave(&em_data_lock, flags);
+	fd = per_cpu(em_data, cpu);
+	read_unlock_irqrestore(&em_data_lock, flags);
+
+	return fd;
+}
+EXPORT_SYMBOL_GPL(em_cpu_get);
+
+/**
+ * em_register_freq_domain() - Register the Energy Model of a frequency domain
+ * @span	: Mask of CPUs in the frequency domain
+ * @nr_states	: Number of capacity states to register
+ * @cb		: Callback functions providing the data of the Energy Model
+ *
+ * Create Energy Model tables for a frequency domain using the callbacks
+ * defined in cb.
+ *
+ * If multiple clients register the same frequency domain, all but the first
+ * registration will be ignored.
+ *
+ * Return 0 on success
+ */
+int em_register_freq_domain(cpumask_t *span, unsigned int nr_states,
+						struct em_data_callback *cb)
+{
+	struct em_freq_domain *fd;
+	unsigned long flags;
+	int cpu, ret = 0;
+
+	if (!span || !nr_states || !cb)
+		return -EINVAL;
+
+	mutex_lock(&em_fd_mutex);
+
+	/* Make sure we don't register again an existing domain. */
+	for_each_cpu(cpu, span) {
+		if (per_cpu(em_data, cpu)) {
+			ret = -EEXIST;
+			goto unlock;
+		}
+	}
+
+	/* Create the frequency domain and add it to the Energy Model. */
+	fd = em_create_fd(span, nr_states, cb);
+	if (!fd) {
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	write_lock_irqsave(&em_data_lock, flags);
+	for_each_cpu(cpu, span)
+		per_cpu(em_data, cpu) = fd;
+	write_unlock_irqrestore(&em_data_lock, flags);
+
+	pr_debug("Created freq domain %*pbl\n", cpumask_pr_args(span));
+unlock:
+	mutex_unlock(&em_fd_mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(em_register_freq_domain);
-- 
2.17.0

Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Dietmar Eggemann]

On 05/21/2018 04:24 PM, Quentin Perret wrote:
> Several subsystems in the kernel (scheduler and/or thermal at the time
> of writing) can benefit from knowing about the energy consumed by CPUs.
> Yet, this information can come from different sources (DT or firmware for
> example), in different formats, hence making it hard to exploit without
> a standard API.
> 
> This patch attempts to solve this issue by introducing a centralized
> Energy Model (EM) framework which can be used to interface the data
> providers with the client subsystems. This framework standardizes the
> API to expose power costs, and to access them from multiple locations.
> 
> The current design assumes that all CPUs in a frequency domain share the
> same micro-architecture. As such, the EM data is structured in a
> per-frequency-domain fashion. Drivers aware of frequency domains
> (typically, but not limited to, CPUFreq drivers) are expected to register
> data in the EM framework using the em_register_freq_domain() API. To do
> so, the drivers must provide a callback function that will be called by
> the EM framework to populate the tables. As of today, only the active
> power of the CPUs is considered. For each frequency domain, the EM
> includes a list of <frequency, power, capacity> tuples for the capacity
> states of the domain alongside a cpumask covering the involved CPUs.
> 
> The EM framework also provides an API to re-scale the capacity values
> of the model asynchronously, after it has been created. This is required
> for architectures where the capacity scale factor of CPUs can change at
> run-time. This is the case for Arm/Arm64 for example where the
> arch_topology driver recomputes the capacity scale factors of the CPUs
> after the maximum frequency of all CPUs has been discovered. Although
> complex, the process of creating and re-scaling the EM has to be kept in
> two separate steps to fulfill the needs of the different users. The thermal
> subsystem doesn't use the capacity values and shouldn't have dependencies
> on subsystems providing them. On the other hand, the task scheduler needs
> the capacity values, and it will benefit from seeing them up-to-date when
> applicable.
> 
> Because of this need for asynchronous update, the capacity state table
> of each frequency domain is protected by RCU, hence guaranteeing a safe
> modification of the table and a fast access to readers in latency-sensitive
> code paths.
> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Quentin Perret <quentin.perret@arm.com>

[...]

> +static void fd_update_cs_table(struct em_cs_table *cs_table, int cpu)
> +{
> +	unsigned long cmax = arch_scale_cpu_capacity(NULL, cpu);
> +	int max_cap_state = cs_table->nr_cap_states - 1;
> +	unsigned long fmax = cs_table->state[max_cap_state].frequency;
> +	int i;
> +
> +	for (i = 0; i < cs_table->nr_cap_states; i++)
> +		cs_table->state[i].capacity = cmax *
> +					cs_table->state[i].frequency / fmax;
> +}

This has issues on a 32bit system. cs_table->state[i].capacity (unsigned
long) overflows with the frequency values stored in Hz.

Maybe something like this to cure it:

diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
index 6ad53f1cf7e6..c13b3eb8bf35 100644
--- a/kernel/power/energy_model.c
+++ b/kernel/power/energy_model.c
@@ -144,9 +144,11 @@ static void fd_update_cs_table(struct em_cs_table *cs_table, int cpu)
 	unsigned long fmax = cs_table->state[max_cap_state].frequency;
 	int i;
 
-	for (i = 0; i < cs_table->nr_cap_states; i++)
-		cs_table->state[i].capacity = cmax *
-					cs_table->state[i].frequency / fmax;
+	for (i = 0; i < cs_table->nr_cap_states; i++) {
+		u64 val = (u64)cmax * cs_table->state[i].frequency;
+		do_div(val, fmax);
+		cs_table->state[i].capacity = (unsigned long)val;
+	}
 }

This brings me to another question. Let's say there are multiple users of
the Energy Model in the system. Shouldn't the units of frequency and power
not standardized, maybe Mhz and mW?
The task scheduler doesn't care since it is only interested in power diffs
but other user might do.

[...]

Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Quentin Perret]

Hi Dietmar,

On Wednesday 06 Jun 2018 at 15:12:15 (+0200), Dietmar Eggemann wrote:
> > +static void fd_update_cs_table(struct em_cs_table *cs_table, int cpu)
> > +{
> > +	unsigned long cmax = arch_scale_cpu_capacity(NULL, cpu);
> > +	int max_cap_state = cs_table->nr_cap_states - 1;
> > +	unsigned long fmax = cs_table->state[max_cap_state].frequency;
> > +	int i;
> > +
> > +	for (i = 0; i < cs_table->nr_cap_states; i++)
> > +		cs_table->state[i].capacity = cmax *
> > +					cs_table->state[i].frequency / fmax;
> > +}
> 
> This has issues on a 32bit system. cs_table->state[i].capacity (unsigned
> long) overflows with the frequency values stored in Hz.

Ah, thank you very much for pointing this out ! I haven't tried on a 32bit
machine yet, my bad. I'll fix that for v4.

> 
> Maybe something like this to cure it:
> 
> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
> index 6ad53f1cf7e6..c13b3eb8bf35 100644
> --- a/kernel/power/energy_model.c
> +++ b/kernel/power/energy_model.c
> @@ -144,9 +144,11 @@ static void fd_update_cs_table(struct em_cs_table *cs_table, int cpu)
>  	unsigned long fmax = cs_table->state[max_cap_state].frequency;
>  	int i;
>  
> -	for (i = 0; i < cs_table->nr_cap_states; i++)
> -		cs_table->state[i].capacity = cmax *
> -					cs_table->state[i].frequency / fmax;
> +	for (i = 0; i < cs_table->nr_cap_states; i++) {
> +		u64 val = (u64)cmax * cs_table->state[i].frequency;
> +		do_div(val, fmax);
> +		cs_table->state[i].capacity = (unsigned long)val;
> +	}
>  }

Hmmm yes, that should work.

> 
> This brings me to another question. Let's say there are multiple users of
> the Energy Model in the system. Shouldn't the units of frequency and power
> not standardized, maybe Mhz and mW?
> The task scheduler doesn't care since it is only interested in power diffs
> but other user might do.

So the good thing about specifying units is that we can probably assume
ranges on the values. If the power is in mW, assuming that we're talking
about a single CPU, it'll probably fit in 16 bits. 65W/core should be
a reasonable upper-bound ?
But there are also vendors who might not be happy with disclosing absolute
values ... These are sometimes considered sensitive and only relative
numbers are discussed publicly. Now, you can also argue that we already
have units specified in IPA for ex, and that it doesn't really matter if
a driver "lies" about the real value, as long as the ratios are correct.
And I guess that anyone can do measurement on the hardware and get those
values anyway. So specifying a unit (mW) for the power is probably a
good idea.

For the frequency, I would rather keep it consistent with what CPUFreq
manages. But that should be specified somewhere, I agree.

What do you think ?

Thanks !
Quentin

Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Juri Lelli]

On 06/06/18 15:37, Quentin Perret wrote:
> Hi Dietmar,
> 
> On Wednesday 06 Jun 2018 at 15:12:15 (+0200), Dietmar Eggemann wrote:
> > > +static void fd_update_cs_table(struct em_cs_table *cs_table, int cpu)
> > > +{
> > > +	unsigned long cmax = arch_scale_cpu_capacity(NULL, cpu);
> > > +	int max_cap_state = cs_table->nr_cap_states - 1;
> > > +	unsigned long fmax = cs_table->state[max_cap_state].frequency;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < cs_table->nr_cap_states; i++)
> > > +		cs_table->state[i].capacity = cmax *
> > > +					cs_table->state[i].frequency / fmax;
> > > +}
> > 
> > This has issues on a 32bit system. cs_table->state[i].capacity (unsigned
> > long) overflows with the frequency values stored in Hz.
> 
> Ah, thank you very much for pointing this out ! I haven't tried on a 32bit
> machine yet, my bad. I'll fix that for v4.
> 
> > 
> > Maybe something like this to cure it:
> > 
> > diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
> > index 6ad53f1cf7e6..c13b3eb8bf35 100644
> > --- a/kernel/power/energy_model.c
> > +++ b/kernel/power/energy_model.c
> > @@ -144,9 +144,11 @@ static void fd_update_cs_table(struct em_cs_table *cs_table, int cpu)
> >  	unsigned long fmax = cs_table->state[max_cap_state].frequency;
> >  	int i;
> >  
> > -	for (i = 0; i < cs_table->nr_cap_states; i++)
> > -		cs_table->state[i].capacity = cmax *
> > -					cs_table->state[i].frequency / fmax;
> > +	for (i = 0; i < cs_table->nr_cap_states; i++) {
> > +		u64 val = (u64)cmax * cs_table->state[i].frequency;
> > +		do_div(val, fmax);
> > +		cs_table->state[i].capacity = (unsigned long)val;
> > +	}
> >  }
> 
> Hmmm yes, that should work.
> 
> > 
> > This brings me to another question. Let's say there are multiple users of
> > the Energy Model in the system. Shouldn't the units of frequency and power
> > not standardized, maybe Mhz and mW?
> > The task scheduler doesn't care since it is only interested in power diffs
> > but other user might do.
> 
> So the good thing about specifying units is that we can probably assume
> ranges on the values. If the power is in mW, assuming that we're talking
> about a single CPU, it'll probably fit in 16 bits. 65W/core should be
> a reasonable upper-bound ?
> But there are also vendors who might not be happy with disclosing absolute
> values ... These are sometimes considered sensitive and only relative
> numbers are discussed publicly. Now, you can also argue that we already
> have units specified in IPA for ex, and that it doesn't really matter if
> a driver "lies" about the real value, as long as the ratios are correct.
> And I guess that anyone can do measurement on the hardware and get those
> values anyway. So specifying a unit (mW) for the power is probably a
> good idea.

Mmm, I remember we fought quite a bit while getting capacity-dmpis-mhz
binding accepted, and one of the musts was that the values were going to
be normalized. So, normalized power values again maybe?

Best,

- Juri

Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Quentin Perret]

On Wednesday 06 Jun 2018 at 17:20:00 (+0200), Juri Lelli wrote:
> > > This brings me to another question. Let's say there are multiple users of
> > > the Energy Model in the system. Shouldn't the units of frequency and power
> > > not standardized, maybe Mhz and mW?
> > > The task scheduler doesn't care since it is only interested in power diffs
> > > but other user might do.
> > 
> > So the good thing about specifying units is that we can probably assume
> > ranges on the values. If the power is in mW, assuming that we're talking
> > about a single CPU, it'll probably fit in 16 bits. 65W/core should be
> > a reasonable upper-bound ?
> > But there are also vendors who might not be happy with disclosing absolute
> > values ... These are sometimes considered sensitive and only relative
> > numbers are discussed publicly. Now, you can also argue that we already
> > have units specified in IPA for ex, and that it doesn't really matter if
> > a driver "lies" about the real value, as long as the ratios are correct.
> > And I guess that anyone can do measurement on the hardware and get those
> > values anyway. So specifying a unit (mW) for the power is probably a
> > good idea.
> 
> Mmm, I remember we fought quite a bit while getting capacity-dmpis-mhz
> binding accepted, and one of the musts was that the values were going to
> be normalized. So, normalized power values again maybe?

Hmmm, that's a very good point ... There should be no problems on the
scheduler side -- we're only interested in correct ratios. But I'm not
sure on the thermal side ... I will double check that.

Javi, Viresh, Eduardo: any thoughts about this ?

Thanks !
Quentin

Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Quentin Perret]

On Wednesday 06 Jun 2018 at 16:29:50 (+0100), Quentin Perret wrote:
> On Wednesday 06 Jun 2018 at 17:20:00 (+0200), Juri Lelli wrote:
> > > > This brings me to another question. Let's say there are multiple users of
> > > > the Energy Model in the system. Shouldn't the units of frequency and power
> > > > not standardized, maybe Mhz and mW?
> > > > The task scheduler doesn't care since it is only interested in power diffs
> > > > but other user might do.
> > > 
> > > So the good thing about specifying units is that we can probably assume
> > > ranges on the values. If the power is in mW, assuming that we're talking
> > > about a single CPU, it'll probably fit in 16 bits. 65W/core should be
> > > a reasonable upper-bound ?
> > > But there are also vendors who might not be happy with disclosing absolute
> > > values ... These are sometimes considered sensitive and only relative
> > > numbers are discussed publicly. Now, you can also argue that we already
> > > have units specified in IPA for ex, and that it doesn't really matter if
> > > a driver "lies" about the real value, as long as the ratios are correct.
> > > And I guess that anyone can do measurement on the hardware and get those
> > > values anyway. So specifying a unit (mW) for the power is probably a
> > > good idea.
> > 
> > Mmm, I remember we fought quite a bit while getting capacity-dmpis-mhz
> > binding accepted, and one of the musts was that the values were going to
> > be normalized. So, normalized power values again maybe?
> 
> Hmmm, that's a very good point ... There should be no problems on the
> scheduler side -- we're only interested in correct ratios. But I'm not
> sure on the thermal side ... I will double check that.

So, IPA needs to compare the power of the CPUs with the power of other
things (e.g. GPUs). So we can't normalize the power of the CPUs without
normalizing in the same scale the power of the other devices. I see two
possibilities:

1) we don't normalize the CPU power values, we specify them in mW, and
   we document (and maybe throw a warning if we see an issue at runtime)
   the max range of values. The max expected power for a single core
   could be 65K for ex (16bits). And based on that we can verify
   overflow and precision issues in the algorithms, and we keep it easy
   to compare the CPU power numbers with other devices.

2) we normalize the power values, but that means that the EM framework
   has to manage not only CPUs, but also other types of devices, and
   normalized their power values as well. That's required to keep the
   scale consistent across all of them, and keep comparisons doable.
   But if we do this, we still have to keep a normalized and a "raw"
   version of the power for all devices. And the "raw" power must still
   be in the same unit across all devices, otherwise the re-scaling is
   broken. The main benefit of doing this is that the range of
   acceptable "raw" power values can be larger, probably 32bits, and
   that the precision of the normalized range is arbitrary.

I feel like 2) involves a lot of complexity, and not so many benefits,
so I'd be happy to go with 1). Unless I forgot something ?

Thanks,
Quentin

Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Dietmar Eggemann]

On 06/06/2018 06:26 PM, Quentin Perret wrote:
> On Wednesday 06 Jun 2018 at 16:29:50 (+0100), Quentin Perret wrote:
>> On Wednesday 06 Jun 2018 at 17:20:00 (+0200), Juri Lelli wrote:
>>>>> This brings me to another question. Let's say there are multiple users of
>>>>> the Energy Model in the system. Shouldn't the units of frequency and power
>>>>> not standardized, maybe Mhz and mW?
>>>>> The task scheduler doesn't care since it is only interested in power diffs
>>>>> but other user might do.
>>>>
>>>> So the good thing about specifying units is that we can probably assume
>>>> ranges on the values. If the power is in mW, assuming that we're talking
>>>> about a single CPU, it'll probably fit in 16 bits. 65W/core should be
>>>> a reasonable upper-bound ?
>>>> But there are also vendors who might not be happy with disclosing absolute
>>>> values ... These are sometimes considered sensitive and only relative
>>>> numbers are discussed publicly. Now, you can also argue that we already
>>>> have units specified in IPA for ex, and that it doesn't really matter if
>>>> a driver "lies" about the real value, as long as the ratios are correct.
>>>> And I guess that anyone can do measurement on the hardware and get those
>>>> values anyway. So specifying a unit (mW) for the power is probably a
>>>> good idea.
>>>
>>> Mmm, I remember we fought quite a bit while getting capacity-dmpis-mhz
>>> binding accepted, and one of the musts was that the values were going to
>>> be normalized. So, normalized power values again maybe?
>>
>> Hmmm, that's a very good point ... There should be no problems on the
>> scheduler side -- we're only interested in correct ratios. But I'm not
>> sure on the thermal side ... I will double check that.
> 
> So, IPA needs to compare the power of the CPUs with the power of other
> things (e.g. GPUs). So we can't normalize the power of the CPUs without
> normalizing in the same scale the power of the other devices. I see two
> possibilities:
> 
> 1) we don't normalize the CPU power values, we specify them in mW, and
>     we document (and maybe throw a warning if we see an issue at runtime)
>     the max range of values. The max expected power for a single core
>     could be 65K for ex (16bits). And based on that we can verify
>     overflow and precision issues in the algorithms, and we keep it easy
>     to compare the CPU power numbers with other devices.

I would say we need 1). 32bit values with units and proper documentation 
of the possible ranges.

[...]

Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Javi Merino]

On Wed, Jun 06, 2018 at 05:26:47PM +0100, Quentin Perret wrote:
> On Wednesday 06 Jun 2018 at 16:29:50 (+0100), Quentin Perret wrote:
> > On Wednesday 06 Jun 2018 at 17:20:00 (+0200), Juri Lelli wrote:
> > > > > This brings me to another question. Let's say there are multiple users of
> > > > > the Energy Model in the system. Shouldn't the units of frequency and power
> > > > > not standardized, maybe Mhz and mW?
> > > > > The task scheduler doesn't care since it is only interested in power diffs
> > > > > but other user might do.
> > > > 
> > > > So the good thing about specifying units is that we can probably assume
> > > > ranges on the values. If the power is in mW, assuming that we're talking
> > > > about a single CPU, it'll probably fit in 16 bits. 65W/core should be
> > > > a reasonable upper-bound ?
> > > > But there are also vendors who might not be happy with disclosing absolute
> > > > values ... These are sometimes considered sensitive and only relative
> > > > numbers are discussed publicly. Now, you can also argue that we already
> > > > have units specified in IPA for ex, and that it doesn't really matter if
> > > > a driver "lies" about the real value, as long as the ratios are correct.
> > > > And I guess that anyone can do measurement on the hardware and get those
> > > > values anyway. So specifying a unit (mW) for the power is probably a
> > > > good idea.
> > > 
> > > Mmm, I remember we fought quite a bit while getting capacity-dmpis-mhz
> > > binding accepted, and one of the musts was that the values were going to
> > > be normalized. So, normalized power values again maybe?
> > 
> > Hmmm, that's a very good point ... There should be no problems on the
> > scheduler side -- we're only interested in correct ratios. But I'm not
> > sure on the thermal side ... I will double check that.
> 
> So, IPA needs to compare the power of the CPUs with the power of other
> things (e.g. GPUs). So we can't normalize the power of the CPUs without
> normalizing in the same scale the power of the other devices. I see two
> possibilities:
> 
> 1) we don't normalize the CPU power values, we specify them in mW, and
>    we document (and maybe throw a warning if we see an issue at runtime)
>    the max range of values. The max expected power for a single core
>    could be 65K for ex (16bits). And based on that we can verify
>    overflow and precision issues in the algorithms, and we keep it easy
>    to compare the CPU power numbers with other devices.
> 
> 2) we normalize the power values, but that means that the EM framework
>    has to manage not only CPUs, but also other types of devices, and
>    normalized their power values as well. That's required to keep the
>    scale consistent across all of them, and keep comparisons doable.
>    But if we do this, we still have to keep a normalized and a "raw"
>    version of the power for all devices. And the "raw" power must still
>    be in the same unit across all devices, otherwise the re-scaling is
>    broken. The main benefit of doing this is that the range of
>    acceptable "raw" power values can be larger, probably 32bits, and
>    that the precision of the normalized range is arbitrary.
> 
> I feel like 2) involves a lot of complexity, and not so many benefits,
> so I'd be happy to go with 1). Unless I forgot something ?

>From the thermal point of view, the power values don't need to have
any given unit, as long as the values are comparable to each other.
Do we need to normalize anything in the kernel though?  Can't we just
assume that whatever the platform is telling us is correct?  Quentin
mentioned it earlier: sometimes absolute values are considered
sensitive and we only get ones that are correct relative to the rest
of the system.

(This reminds me that the units in
Documentation/thermal/cpu-cooling-api.txt are wrong and I need to fix
it :-X )

Cheers,
Javi

Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Quentin Perret]

Hi Javi,

On Friday 08 Jun 2018 at 14:39:42 (+0100), Javi Merino wrote:
> On Wed, Jun 06, 2018 at 05:26:47PM +0100, Quentin Perret wrote:
> > On Wednesday 06 Jun 2018 at 16:29:50 (+0100), Quentin Perret wrote:
> > > On Wednesday 06 Jun 2018 at 17:20:00 (+0200), Juri Lelli wrote:
> > > > > > This brings me to another question. Let's say there are multiple users of
> > > > > > the Energy Model in the system. Shouldn't the units of frequency and power
> > > > > > not standardized, maybe Mhz and mW?
> > > > > > The task scheduler doesn't care since it is only interested in power diffs
> > > > > > but other user might do.
> > > > > 
> > > > > So the good thing about specifying units is that we can probably assume
> > > > > ranges on the values. If the power is in mW, assuming that we're talking
> > > > > about a single CPU, it'll probably fit in 16 bits. 65W/core should be
> > > > > a reasonable upper-bound ?
> > > > > But there are also vendors who might not be happy with disclosing absolute
> > > > > values ... These are sometimes considered sensitive and only relative
> > > > > numbers are discussed publicly. Now, you can also argue that we already
> > > > > have units specified in IPA for ex, and that it doesn't really matter if
> > > > > a driver "lies" about the real value, as long as the ratios are correct.
> > > > > And I guess that anyone can do measurement on the hardware and get those
> > > > > values anyway. So specifying a unit (mW) for the power is probably a
> > > > > good idea.
> > > > 
> > > > Mmm, I remember we fought quite a bit while getting capacity-dmpis-mhz
> > > > binding accepted, and one of the musts was that the values were going to
> > > > be normalized. So, normalized power values again maybe?
> > > 
> > > Hmmm, that's a very good point ... There should be no problems on the
> > > scheduler side -- we're only interested in correct ratios. But I'm not
> > > sure on the thermal side ... I will double check that.
> > 
> > So, IPA needs to compare the power of the CPUs with the power of other
> > things (e.g. GPUs). So we can't normalize the power of the CPUs without
> > normalizing in the same scale the power of the other devices. I see two
> > possibilities:
> > 
> > 1) we don't normalize the CPU power values, we specify them in mW, and
> >    we document (and maybe throw a warning if we see an issue at runtime)
> >    the max range of values. The max expected power for a single core
> >    could be 65K for ex (16bits). And based on that we can verify
> >    overflow and precision issues in the algorithms, and we keep it easy
> >    to compare the CPU power numbers with other devices.
> > 
> > 2) we normalize the power values, but that means that the EM framework
> >    has to manage not only CPUs, but also other types of devices, and
> >    normalized their power values as well. That's required to keep the
> >    scale consistent across all of them, and keep comparisons doable.
> >    But if we do this, we still have to keep a normalized and a "raw"
> >    version of the power for all devices. And the "raw" power must still
> >    be in the same unit across all devices, otherwise the re-scaling is
> >    broken. The main benefit of doing this is that the range of
> >    acceptable "raw" power values can be larger, probably 32bits, and
> >    that the precision of the normalized range is arbitrary.
> > 
> > I feel like 2) involves a lot of complexity, and not so many benefits,
> > so I'd be happy to go with 1). Unless I forgot something ?
> 
> From the thermal point of view, the power values don't need to have
> any given unit, as long as the values are comparable to each other.

OK, thanks for confirming that :-)

> Do we need to normalize anything in the kernel though?  Can't we just
> assume that whatever the platform is telling us is correct?  Quentin
> mentioned it earlier: sometimes absolute values are considered
> sensitive and we only get ones that are correct relative to the rest
> of the system.

I'm happy to specify the units as mW and let the drivers lie about the
true values. At least that helps them lie coherently if another
subsystem requires power in uW for example.

> 
> (This reminds me that the units in
> Documentation/thermal/cpu-cooling-api.txt are wrong and I need to fix
> it :-X )

;-)

Thanks,
Quentin

Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Javi Merino]

On Fri, Jun 08, 2018 at 04:47:39PM +0100, Quentin Perret wrote:
> Hi Javi,
> 
> On Friday 08 Jun 2018 at 14:39:42 (+0100), Javi Merino wrote:
> > On Wed, Jun 06, 2018 at 05:26:47PM +0100, Quentin Perret wrote:
> > > On Wednesday 06 Jun 2018 at 16:29:50 (+0100), Quentin Perret wrote:
> > > > On Wednesday 06 Jun 2018 at 17:20:00 (+0200), Juri Lelli wrote:
> > > > > > > This brings me to another question. Let's say there are multiple users of
> > > > > > > the Energy Model in the system. Shouldn't the units of frequency and power
> > > > > > > not standardized, maybe Mhz and mW?
> > > > > > > The task scheduler doesn't care since it is only interested in power diffs
> > > > > > > but other user might do.
> > > > > > 
> > > > > > So the good thing about specifying units is that we can probably assume
> > > > > > ranges on the values. If the power is in mW, assuming that we're talking
> > > > > > about a single CPU, it'll probably fit in 16 bits. 65W/core should be
> > > > > > a reasonable upper-bound ?
> > > > > > But there are also vendors who might not be happy with disclosing absolute
> > > > > > values ... These are sometimes considered sensitive and only relative
> > > > > > numbers are discussed publicly. Now, you can also argue that we already
> > > > > > have units specified in IPA for ex, and that it doesn't really matter if
> > > > > > a driver "lies" about the real value, as long as the ratios are correct.
> > > > > > And I guess that anyone can do measurement on the hardware and get those
> > > > > > values anyway. So specifying a unit (mW) for the power is probably a
> > > > > > good idea.
> > > > > 
> > > > > Mmm, I remember we fought quite a bit while getting capacity-dmpis-mhz
> > > > > binding accepted, and one of the musts was that the values were going to
> > > > > be normalized. So, normalized power values again maybe?
> > > > 
> > > > Hmmm, that's a very good point ... There should be no problems on the
> > > > scheduler side -- we're only interested in correct ratios. But I'm not
> > > > sure on the thermal side ... I will double check that.
> > > 
> > > So, IPA needs to compare the power of the CPUs with the power of other
> > > things (e.g. GPUs). So we can't normalize the power of the CPUs without
> > > normalizing in the same scale the power of the other devices. I see two
> > > possibilities:
> > > 
> > > 1) we don't normalize the CPU power values, we specify them in mW, and
> > >    we document (and maybe throw a warning if we see an issue at runtime)
> > >    the max range of values. The max expected power for a single core
> > >    could be 65K for ex (16bits). And based on that we can verify
> > >    overflow and precision issues in the algorithms, and we keep it easy
> > >    to compare the CPU power numbers with other devices.
> > > 
> > > 2) we normalize the power values, but that means that the EM framework
> > >    has to manage not only CPUs, but also other types of devices, and
> > >    normalized their power values as well. That's required to keep the
> > >    scale consistent across all of them, and keep comparisons doable.
> > >    But if we do this, we still have to keep a normalized and a "raw"
> > >    version of the power for all devices. And the "raw" power must still
> > >    be in the same unit across all devices, otherwise the re-scaling is
> > >    broken. The main benefit of doing this is that the range of
> > >    acceptable "raw" power values can be larger, probably 32bits, and
> > >    that the precision of the normalized range is arbitrary.
> > > 
> > > I feel like 2) involves a lot of complexity, and not so many benefits,
> > > so I'd be happy to go with 1). Unless I forgot something ?
> > 
> > From the thermal point of view, the power values don't need to have
> > any given unit, as long as the values are comparable to each other.
> 
> OK, thanks for confirming that :-)
> 
> > Do we need to normalize anything in the kernel though?  Can't we just
> > assume that whatever the platform is telling us is correct?  Quentin
> > mentioned it earlier: sometimes absolute values are considered
> > sensitive and we only get ones that are correct relative to the rest
> > of the system.
> 
> I'm happy to specify the units as mW and let the drivers lie about the
> true values. At least that helps them lie coherently if another
> subsystem requires power in uW for example.

I think this is a good option.

Cheers,
Javi

Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Juri Lelli]

Hi Quentin,

On 21/05/18 15:24, Quentin Perret wrote:

[...]

> +#ifdef CONFIG_ENERGY_MODEL

[...]

> +struct em_data_callback {
> +	/**
> +	 * active_power() - Provide power at the next capacity state of a CPU
> +	 * @power	: Active power at the capacity state (modified)
> +	 * @freq	: Frequency at the capacity state (modified)
> +	 * @cpu		: CPU for which we do this operation
> +	 *
> +	 * active_power() must find the lowest capacity state of 'cpu' above
> +	 * 'freq' and update 'power' and 'freq' to the matching active power
> +	 * and frequency.
> +	 *
> +	 * Return 0 on success.
> +	 */
> +	int (*active_power) (unsigned long *power, unsigned long *freq, int cpu);
> +};

[...]

> +#else
> +struct em_freq_domain;
> +struct em_data_callback;

This doesn't compile for CONFIG_ENERGY_MODEL=n:

drivers/cpufreq/cpufreq-dt.c:164:9: error: variable 'em_cb' has initializer but incomplete type                                                          
  struct em_data_callback em_cb = { &dev_pm_opp_of_estimate_power };                       
           ^~~~~~~~~~~~~~~~

Guess you need some sort of macro to init the struct (w/o introducing
ifdeffery into drivers code)?

Best,

- Juri

Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Quentin Perret]

On Wednesday 06 Jun 2018 at 18:47:34 (+0200), Juri Lelli wrote:
> Hi Quentin,
> 
> On 21/05/18 15:24, Quentin Perret wrote:
> 
> [...]
> 
> > +#ifdef CONFIG_ENERGY_MODEL
> 
> [...]
> 
> > +struct em_data_callback {
> > +	/**
> > +	 * active_power() - Provide power at the next capacity state of a CPU
> > +	 * @power	: Active power at the capacity state (modified)
> > +	 * @freq	: Frequency at the capacity state (modified)
> > +	 * @cpu		: CPU for which we do this operation
> > +	 *
> > +	 * active_power() must find the lowest capacity state of 'cpu' above
> > +	 * 'freq' and update 'power' and 'freq' to the matching active power
> > +	 * and frequency.
> > +	 *
> > +	 * Return 0 on success.
> > +	 */
> > +	int (*active_power) (unsigned long *power, unsigned long *freq, int cpu);
> > +};
> 
> [...]
> 
> > +#else
> > +struct em_freq_domain;
> > +struct em_data_callback;
> 
> This doesn't compile for CONFIG_ENERGY_MODEL=n:
> 
> drivers/cpufreq/cpufreq-dt.c:164:9: error: variable 'em_cb' has initializer but incomplete type                                                          
>   struct em_data_callback em_cb = { &dev_pm_opp_of_estimate_power };                       
>            ^~~~~~~~~~~~~~~~

Ah, this is an issue with a patch which isn't part of this patch-set ...
But yeah, that's a valid point :-)

> Guess you need some sort of macro to init the struct (w/o introducing
> ifdeffery into drivers code)?

Yes, the EM framework should provide a generic way of doing this
initialization with nice stubs for CONFIG_ENERGY_MODEL=n, otherwise we
risk wild fixes in the drivers I suppose. I'll fix that in v4.

Thanks for pointing that out !
Quentin

Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Juri Lelli]

On 21/05/18 15:24, Quentin Perret wrote:
> Several subsystems in the kernel (scheduler and/or thermal at the time
> of writing) can benefit from knowing about the energy consumed by CPUs.
> Yet, this information can come from different sources (DT or firmware for
> example), in different formats, hence making it hard to exploit without
> a standard API.
> 
> This patch attempts to solve this issue by introducing a centralized
> Energy Model (EM) framework which can be used to interface the data
> providers with the client subsystems. This framework standardizes the
> API to expose power costs, and to access them from multiple locations.
> 
> The current design assumes that all CPUs in a frequency domain share the
> same micro-architecture. As such, the EM data is structured in a
> per-frequency-domain fashion. Drivers aware of frequency domains
> (typically, but not limited to, CPUFreq drivers) are expected to register
> data in the EM framework using the em_register_freq_domain() API. To do
> so, the drivers must provide a callback function that will be called by
> the EM framework to populate the tables. As of today, only the active
> power of the CPUs is considered. For each frequency domain, the EM
> includes a list of <frequency, power, capacity> tuples for the capacity
> states of the domain alongside a cpumask covering the involved CPUs.
> 
> The EM framework also provides an API to re-scale the capacity values
> of the model asynchronously, after it has been created. This is required
> for architectures where the capacity scale factor of CPUs can change at
> run-time. This is the case for Arm/Arm64 for example where the
> arch_topology driver recomputes the capacity scale factors of the CPUs
> after the maximum frequency of all CPUs has been discovered. Although
> complex, the process of creating and re-scaling the EM has to be kept in
> two separate steps to fulfill the needs of the different users. The thermal
> subsystem doesn't use the capacity values and shouldn't have dependencies
> on subsystems providing them. On the other hand, the task scheduler needs
> the capacity values, and it will benefit from seeing them up-to-date when
> applicable.
> 
> Because of this need for asynchronous update, the capacity state table
> of each frequency domain is protected by RCU, hence guaranteeing a safe
> modification of the table and a fast access to readers in latency-sensitive
> code paths.
> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Quentin Perret <quentin.perret@arm.com>
> ---

OK, I think I'll start with a few comments while I get more into
understanding the set. :)

> +static void fd_update_cs_table(struct em_cs_table *cs_table, int cpu)
> +{
> +	unsigned long cmax = arch_scale_cpu_capacity(NULL, cpu);
> +	int max_cap_state = cs_table->nr_cap_states - 1;
                 ^
You don't need this on the stack, right?

> +	unsigned long fmax = cs_table->state[max_cap_state].frequency;
> +	int i;
> +
> +	for (i = 0; i < cs_table->nr_cap_states; i++)
> +		cs_table->state[i].capacity = cmax *
> +					cs_table->state[i].frequency / fmax;
> +}
> +
> +static struct em_freq_domain *em_create_fd(cpumask_t *span, int nr_states,
> +						struct em_data_callback *cb)
> +{
> +	unsigned long opp_eff, prev_opp_eff = ULONG_MAX;
> +	int i, ret, cpu = cpumask_first(span);
> +	struct em_freq_domain *fd;
> +	unsigned long power, freq;
> +
> +	if (!cb->active_power)
> +		return NULL;
> +
> +	fd = kzalloc(sizeof(*fd), GFP_KERNEL);
> +	if (!fd)
> +		return NULL;
> +
> +	fd->cs_table = alloc_cs_table(nr_states);

Mmm, don't you need to rcu_assign_pointer this first one as well?

> +	if (!fd->cs_table)
> +		goto free_fd;
> +
> +	/* Copy the span of the frequency domain */
> +	cpumask_copy(&fd->cpus, span);
> +
> +	/* Build the list of capacity states for this freq domain */
> +	for (i = 0, freq = 0; i < nr_states; i++, freq++) {
                     ^                              ^
The fact that this relies on active_power() to use ceil OPP for a given
freq might deserve a comment. Also, is this behaviour of active_power()
standardized?

> +		ret = cb->active_power(&power, &freq, cpu);
> +		if (ret)
> +			goto free_cs_table;
> +
> +		fd->cs_table->state[i].power = power;
> +		fd->cs_table->state[i].frequency = freq;
> +
> +		/*
> +		 * The hertz/watts efficiency ratio should decrease as the
> +		 * frequency grows on sane platforms. If not, warn the user
> +		 * that some high OPPs are more power efficient than some
> +		 * of the lower ones.
> +		 */
> +		opp_eff = freq / power;
> +		if (opp_eff >= prev_opp_eff)
> +			pr_warn("%*pbl: hz/watt efficiency: OPP %d >= OPP%d\n",
> +					cpumask_pr_args(span), i, i - 1);
> +		prev_opp_eff = opp_eff;
> +	}
> +	fd_update_cs_table(fd->cs_table, cpu);
> +
> +	return fd;
> +
> +free_cs_table:
> +	free_cs_table(fd->cs_table);
> +free_fd:
> +	kfree(fd);
> +
> +	return NULL;
> +}
> +
> +static void rcu_free_cs_table(struct rcu_head *rp)
> +{
> +	struct em_cs_table *table;
> +
> +	table = container_of(rp, struct em_cs_table, rcu);
> +	free_cs_table(table);
> +}
> +
> +/**
> + * em_rescale_cpu_capacity() - Re-scale capacity values of the Energy Model
> + *
> + * This re-scales the capacity values for all capacity states of all frequency
> + * domains of the Energy Model. This should be used when the capacity values
> + * of the CPUs are updated at run-time, after the EM was registered.
> + */
> +void em_rescale_cpu_capacity(void)

So, is this thought to be called eventually also after thermal capping
events and such?

> +{
> +	struct em_cs_table *old_table, *new_table;
> +	struct em_freq_domain *fd;
> +	unsigned long flags;
> +	int nr_states, cpu;
> +
> +	read_lock_irqsave(&em_data_lock, flags);

Don't you need write_lock_ here, since you are going to exchange the
em tables?

> +	for_each_cpu(cpu, cpu_possible_mask) {
> +		fd = per_cpu(em_data, cpu);
> +		if (!fd || cpu != cpumask_first(&fd->cpus))
> +			continue;
> +
> +		/* Copy the existing table. */
> +		old_table = rcu_dereference(fd->cs_table);
> +		nr_states = old_table->nr_cap_states;
> +		new_table = alloc_cs_table(nr_states);
> +		if (!new_table) {
> +			read_unlock_irqrestore(&em_data_lock, flags);
> +			return;
> +		}
> +		memcpy(new_table->state, old_table->state,
> +					nr_states * sizeof(*new_table->state));
> +
> +		/* Re-scale the capacity values on the copy. */
> +		fd_update_cs_table(new_table, cpumask_first(&fd->cpus));
> +
> +		/* Replace the table with the rescaled version. */
> +		rcu_assign_pointer(fd->cs_table, new_table);
> +		call_rcu(&old_table->rcu, rcu_free_cs_table);
> +	}
> +	read_unlock_irqrestore(&em_data_lock, flags);
> +	pr_debug("Re-scaled CPU capacities\n");
> +}
> +EXPORT_SYMBOL_GPL(em_rescale_cpu_capacity);
> +
> +/**
> + * em_cpu_get() - Return the frequency domain for a CPU
> + * @cpu : CPU to find the frequency domain for
> + *
> + * Return: the frequency domain to which 'cpu' belongs, or NULL if it doesn't
> + * exist.
> + */
> +struct em_freq_domain *em_cpu_get(int cpu)
> +{
> +	struct em_freq_domain *fd;
> +	unsigned long flags;
> +
> +	read_lock_irqsave(&em_data_lock, flags);
> +	fd = per_cpu(em_data, cpu);
> +	read_unlock_irqrestore(&em_data_lock, flags);
> +
> +	return fd;
> +}
> +EXPORT_SYMBOL_GPL(em_cpu_get);

Mmm, this gets complicated pretty fast eh? :)

I had to go back and forth between patches to start understanding the
different data structures and how they are use, and I'm not sure yet
I've got the full picture. I guess some nice diagram (cover letter or
documentation patch) would help a lot.

Locking of such data structures is pretty involved as well, adding
comments/docs shouldn't harm. :)

Best,

- Juri

Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Quentin Perret]

Hi Juri,

On Thursday 07 Jun 2018 at 16:44:09 (+0200), Juri Lelli wrote:
> On 21/05/18 15:24, Quentin Perret wrote:
> > Several subsystems in the kernel (scheduler and/or thermal at the time
> > of writing) can benefit from knowing about the energy consumed by CPUs.
> > Yet, this information can come from different sources (DT or firmware for
> > example), in different formats, hence making it hard to exploit without
> > a standard API.
> > 
> > This patch attempts to solve this issue by introducing a centralized
> > Energy Model (EM) framework which can be used to interface the data
> > providers with the client subsystems. This framework standardizes the
> > API to expose power costs, and to access them from multiple locations.
> > 
> > The current design assumes that all CPUs in a frequency domain share the
> > same micro-architecture. As such, the EM data is structured in a
> > per-frequency-domain fashion. Drivers aware of frequency domains
> > (typically, but not limited to, CPUFreq drivers) are expected to register
> > data in the EM framework using the em_register_freq_domain() API. To do
> > so, the drivers must provide a callback function that will be called by
> > the EM framework to populate the tables. As of today, only the active
> > power of the CPUs is considered. For each frequency domain, the EM
> > includes a list of <frequency, power, capacity> tuples for the capacity
> > states of the domain alongside a cpumask covering the involved CPUs.
> > 
> > The EM framework also provides an API to re-scale the capacity values
> > of the model asynchronously, after it has been created. This is required
> > for architectures where the capacity scale factor of CPUs can change at
> > run-time. This is the case for Arm/Arm64 for example where the
> > arch_topology driver recomputes the capacity scale factors of the CPUs
> > after the maximum frequency of all CPUs has been discovered. Although
> > complex, the process of creating and re-scaling the EM has to be kept in
> > two separate steps to fulfill the needs of the different users. The thermal
> > subsystem doesn't use the capacity values and shouldn't have dependencies
> > on subsystems providing them. On the other hand, the task scheduler needs
> > the capacity values, and it will benefit from seeing them up-to-date when
> > applicable.
> > 
> > Because of this need for asynchronous update, the capacity state table
> > of each frequency domain is protected by RCU, hence guaranteeing a safe
> > modification of the table and a fast access to readers in latency-sensitive
> > code paths.
> > 
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Signed-off-by: Quentin Perret <quentin.perret@arm.com>
> > ---
> 
> OK, I think I'll start with a few comments while I get more into
> understanding the set. :)

:-)

> 
> > +static void fd_update_cs_table(struct em_cs_table *cs_table, int cpu)
> > +{
> > +	unsigned long cmax = arch_scale_cpu_capacity(NULL, cpu);
> > +	int max_cap_state = cs_table->nr_cap_states - 1;
>                  ^
> You don't need this on the stack, right?

Oh, why not ?

> 
> > +	unsigned long fmax = cs_table->state[max_cap_state].frequency;
> > +	int i;
> > +
> > +	for (i = 0; i < cs_table->nr_cap_states; i++)
> > +		cs_table->state[i].capacity = cmax *
> > +					cs_table->state[i].frequency / fmax;
> > +}
> > +
> > +static struct em_freq_domain *em_create_fd(cpumask_t *span, int nr_states,
> > +						struct em_data_callback *cb)
> > +{
> > +	unsigned long opp_eff, prev_opp_eff = ULONG_MAX;
> > +	int i, ret, cpu = cpumask_first(span);
> > +	struct em_freq_domain *fd;
> > +	unsigned long power, freq;
> > +
> > +	if (!cb->active_power)
> > +		return NULL;
> > +
> > +	fd = kzalloc(sizeof(*fd), GFP_KERNEL);
> > +	if (!fd)
> > +		return NULL;
> > +
> > +	fd->cs_table = alloc_cs_table(nr_states);
> 
> Mmm, don't you need to rcu_assign_pointer this first one as well?

Hmmmm, nobody can be using this at this point, but yes, it'd be better
to keep that consistent I suppose ...

> 
> > +	if (!fd->cs_table)
> > +		goto free_fd;
> > +
> > +	/* Copy the span of the frequency domain */
> > +	cpumask_copy(&fd->cpus, span);
> > +
> > +	/* Build the list of capacity states for this freq domain */
> > +	for (i = 0, freq = 0; i < nr_states; i++, freq++) {
>                      ^                              ^
> The fact that this relies on active_power() to use ceil OPP for a given
> freq might deserve a comment. Also, is this behaviour of active_power()
> standardized?

Right, this can get confusing pretty quickly. There is a comment in
include/linux/energy_model.h where the expected behaviour of
active_power is explained, but a reminder above this function shouldn't
hurt.

> 
> > +		ret = cb->active_power(&power, &freq, cpu);
> > +		if (ret)
> > +			goto free_cs_table;
> > +
> > +		fd->cs_table->state[i].power = power;
> > +		fd->cs_table->state[i].frequency = freq;
> > +
> > +		/*
> > +		 * The hertz/watts efficiency ratio should decrease as the
> > +		 * frequency grows on sane platforms. If not, warn the user
> > +		 * that some high OPPs are more power efficient than some
> > +		 * of the lower ones.
> > +		 */
> > +		opp_eff = freq / power;
> > +		if (opp_eff >= prev_opp_eff)
> > +			pr_warn("%*pbl: hz/watt efficiency: OPP %d >= OPP%d\n",
> > +					cpumask_pr_args(span), i, i - 1);
> > +		prev_opp_eff = opp_eff;
> > +	}
> > +	fd_update_cs_table(fd->cs_table, cpu);
> > +
> > +	return fd;
> > +
> > +free_cs_table:
> > +	free_cs_table(fd->cs_table);
> > +free_fd:
> > +	kfree(fd);
> > +
> > +	return NULL;
> > +}
> > +
> > +static void rcu_free_cs_table(struct rcu_head *rp)
> > +{
> > +	struct em_cs_table *table;
> > +
> > +	table = container_of(rp, struct em_cs_table, rcu);
> > +	free_cs_table(table);
> > +}
> > +
> > +/**
> > + * em_rescale_cpu_capacity() - Re-scale capacity values of the Energy Model
> > + *
> > + * This re-scales the capacity values for all capacity states of all frequency
> > + * domains of the Energy Model. This should be used when the capacity values
> > + * of the CPUs are updated at run-time, after the EM was registered.
> > + */
> > +void em_rescale_cpu_capacity(void)
> 
> So, is this thought to be called eventually also after thermal capping
> events and such?

The true reason is that the frequency domains will typically be
registered in the EM framework _before_ the arch_topology driver kicks
in on arm/arm64. That means that the EM tables are created, and only
after, the cpu capacities are updated. So we basically need to update
those capacities to be up-to-date.

The reason we need to keep those two steps separate (registering the
freq domains and re-scaling the capacities) in the EM framework is
because thermal doesn't care about the cpu capacities. It is a perfectly
acceptable configuration to use IPA without having dmips-capacity-mhz
values in the DT for ex.

Now, since we have a RCU protection on the EM tables, we might decide in
the future to use the opportunity to modify the tables at run-time for
other reasons. Thermal capping could be one I guess.

> 
> > +{
> > +	struct em_cs_table *old_table, *new_table;
> > +	struct em_freq_domain *fd;
> > +	unsigned long flags;
> > +	int nr_states, cpu;
> > +
> > +	read_lock_irqsave(&em_data_lock, flags);
> 
> Don't you need write_lock_ here, since you are going to exchange the
> em tables?

This lock protects the per_cpu() variable itself. Here we only read
pointers from that per_cpu variable, and we modify one attribute in
the pointed structure. We don't modify the per_cpu table itself. Does
that make sense ?

> 
> > +	for_each_cpu(cpu, cpu_possible_mask) {
> > +		fd = per_cpu(em_data, cpu);
> > +		if (!fd || cpu != cpumask_first(&fd->cpus))
> > +			continue;
> > +
> > +		/* Copy the existing table. */
> > +		old_table = rcu_dereference(fd->cs_table);
> > +		nr_states = old_table->nr_cap_states;
> > +		new_table = alloc_cs_table(nr_states);
> > +		if (!new_table) {
> > +			read_unlock_irqrestore(&em_data_lock, flags);
> > +			return;
> > +		}
> > +		memcpy(new_table->state, old_table->state,
> > +					nr_states * sizeof(*new_table->state));
> > +
> > +		/* Re-scale the capacity values on the copy. */
> > +		fd_update_cs_table(new_table, cpumask_first(&fd->cpus));
> > +
> > +		/* Replace the table with the rescaled version. */
> > +		rcu_assign_pointer(fd->cs_table, new_table);
> > +		call_rcu(&old_table->rcu, rcu_free_cs_table);
> > +	}
> > +	read_unlock_irqrestore(&em_data_lock, flags);
> > +	pr_debug("Re-scaled CPU capacities\n");
> > +}
> > +EXPORT_SYMBOL_GPL(em_rescale_cpu_capacity);
> > +
> > +/**
> > + * em_cpu_get() - Return the frequency domain for a CPU
> > + * @cpu : CPU to find the frequency domain for
> > + *
> > + * Return: the frequency domain to which 'cpu' belongs, or NULL if it doesn't
> > + * exist.
> > + */
> > +struct em_freq_domain *em_cpu_get(int cpu)
> > +{
> > +	struct em_freq_domain *fd;
> > +	unsigned long flags;
> > +
> > +	read_lock_irqsave(&em_data_lock, flags);
> > +	fd = per_cpu(em_data, cpu);
> > +	read_unlock_irqrestore(&em_data_lock, flags);
> > +
> > +	return fd;
> > +}
> > +EXPORT_SYMBOL_GPL(em_cpu_get);
> 
> Mmm, this gets complicated pretty fast eh? :)

Yeah, hopefully I'll be able to explain/clarify that :-).

> 
> I had to go back and forth between patches to start understanding the
> different data structures and how they are use, and I'm not sure yet
> I've got the full picture. I guess some nice diagram (cover letter or
> documentation patch) would help a lot.

Right, so I'd like very much to write a nice documentation patch once we
are more or less OK with the overall design of this framework, but I
felt like it was a little bit early for that. If we finally decide that
what I did is totally stupid and that it'd be better to do things
completely differently, my nice documentation patch would be a lot of
efforts for nothing.

But I agree that at the same time all this complex code has to be
explained. Hopefully the existing comments can help with that.
Otherwise, I'm more than happy to answer all questions :-)

> 
> Locking of such data structures is pretty involved as well, adding
> comments/docs shouldn't harm. :)

Message received. If I do need to come-up with a brand new
design/implementation for v4, I'll make sure to add more comments.

> Best,
> 
> - Juri

Thanks !
Quentin

Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Dietmar Eggemann]

On 06/07/2018 05:19 PM, Quentin Perret wrote:
> Hi Juri,
> 
> On Thursday 07 Jun 2018 at 16:44:09 (+0200), Juri Lelli wrote:
>> On 21/05/18 15:24, Quentin Perret wrote:

[...]

>> Mmm, this gets complicated pretty fast eh? :)
> 
> Yeah, hopefully I'll be able to explain/clarify that :-).
> 
>>
>> I had to go back and forth between patches to start understanding the
>> different data structures and how they are use, and I'm not sure yet
>> I've got the full picture. I guess some nice diagram (cover letter or
>> documentation patch) would help a lot.

+1 on the diagram.

> Right, so I'd like very much to write a nice documentation patch once we
> are more or less OK with the overall design of this framework, but I
> felt like it was a little bit early for that. If we finally decide that
> what I did is totally stupid and that it'd be better to do things
> completely differently, my nice documentation patch would be a lot of
> efforts for nothing.
> 
> But I agree that at the same time all this complex code has to be
> explained. Hopefully the existing comments can help with that.
> Otherwise, I'm more than happy to answer all questions :-)

I'm not sure that the current API is the final one. Not sure that 
em_rescale_cpu_capacity() is really needed.

We should first clarify the provider - consumer relation. Are multiple 
providers allowed, if yes, are they allowed to provide partial EM data? 
Do we really want to allow this overwriting of old EM data 
(em_rescale_cpu_capacity()). In case multiple provider are allowed, is 
there some kind of priority involved?

The re-scaling thing comes from the requirement that the final cpu 
capacity values are only known after the arch_topology driver was able 
to scale the dmipz-capacity-values with the policy->cpuinfo.max_freq but 
why can't we create the EM on arm/arm64 after this? Even though we would 
be forced to get cpufreq's related cpumask from somewhere.

I guess the easiest model will be that the Energy Model (EM) is fully 
initialized with one init call (from the arch) and fixed after that.

In case the EM should not be tight to cpufreq, the interface 
em_create_fd(cpumask_t *span, int nr_states, struct em_data_callback 
*cb) seems ok.

IMHO, part of the problem why this might be harder to understand is the 
fact that the patches show the use of the 2. init call 
'em_rescale_cpu_capacity()' but not the 1. one 
'em_register_freq_domain()'. I guess that Quentin wanted to keep the set 
as small as possible.

[...]

Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

[Quentin Perret]