[PATCH v8 09/26] kernel/cpu_pm: Manage runtime PM in the idle path for CPUs

On Wednesday, July 18, 2018 12:11:06 PM CEST Rafael J. Wysocki wrote:

On Wednesday, June 20, 2018 7:22:09 PM CEST Ulf Hansson wrote:

quoted

To allow CPUs being power managed by PM domains, let's deploy support for
runtime PM for the CPU's corresponding struct device.

More precisely, at the point when the CPU is about to enter an idle state,
decrease the runtime PM usage count for its corresponding struct device,
via calling pm_runtime_put_sync_suspend(). Then, at the point when the CPU
resumes from idle, let's increase the runtime PM usage count, via calling
pm_runtime_get_sync().

Cc: Lina Iyer <redacted>
Co-developed-by: Lina Iyer <redacted>
Signed-off-by: Ulf Hansson <redacted>

I finally got to this one, sorry for the huge delay.

Let me confirm that I understand the code flow correctly.

quoted

---
 kernel/cpu_pm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/kernel/cpu_pm.c b/kernel/cpu_pm.c
index 67b02e138a47..492d4a83dca0 100644
--- a/kernel/cpu_pm.c
+++ b/kernel/cpu_pm.c

@@ -16,9 +16,11 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/cpu.h>
 #include <linux/cpu_pm.h>
 #include <linux/module.h>
 #include <linux/notifier.h>
+#include <linux/pm_runtime.h>
 #include <linux/spinlock.h>
 #include <linux/syscore_ops.h>
 

@@ -91,6 +93,7 @@ int cpu_pm_enter(void)

This is called from a cpuidle driver's ->enter callback for the target state
selected by the idle governor ->

quoted

 {
 	int nr_calls;
 	int ret = 0;
+	struct device *dev = get_cpu_device(smp_processor_id());
 
 	ret = cpu_pm_notify(CPU_PM_ENTER, -1, &nr_calls);
 	if (ret)

@@ -100,6 +103,9 @@ int cpu_pm_enter(void)
 		 */
 		cpu_pm_notify(CPU_PM_ENTER_FAILED, nr_calls - 1, NULL);
 
+	if (!ret && dev && dev->pm_domain)
+		pm_runtime_put_sync_suspend(dev);

-> so this is going to invoke genpd_runtime_suspend() if the usage
counter of dev is 0.

That will cause cpu_power_down_ok() to be called (because this is
a CPU domain) and that will walk the domain cpumask and compute the
estimated idle duration as the minimum of tick_nohz_get_next_wakeup()
values over the CPUs in that cpumask.  [Note that the weight of the
cpumask must be seriously limited for that to actually work, as this
happens in the idle path.]  Next, it will return "true" if it can
find a domain state with residency within the estimated idle
duration.  [Note that this sort of overlaps with the idle governor's
job.]

Next, __genpd_runtime_suspend() will be invoked to run the device-specific
callback if any [Note that this has to be suitable for the idle path if
present.] and genpd_stop_dev() runs (which, again, may invoke a callback)
and genpd_power_off() runs under the domain lock (which must be a spinlock
then).

quoted

+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cpu_pm_enter);

@@ -118,6 +124,11 @@ EXPORT_SYMBOL_GPL(cpu_pm_enter);
  */
 int cpu_pm_exit(void)
 {
+	struct device *dev = get_cpu_device(smp_processor_id());
+
+	if (dev && dev->pm_domain)
+		pm_runtime_get_sync(dev);
+
 	return cpu_pm_notify(CPU_PM_EXIT, -1, NULL);
 }
 EXPORT_SYMBOL_GPL(cpu_pm_exit);

And this is called on wakeup when the cpuidle driver's ->enter callback
is about to return and it reverses the suspend flow (except that the
governor doesn't need to be called now).

Have I got that right?

Assuming that I have got that right, there are concerns, mostly regarding
patch [07/26], but I will reply to that directly.

The $subject patch is fine by me by itself, but it obviously depends on the
previous ones.  Patches [01-02/26] are fine too, but they don't seem to be
particularly useful without the rest of the series.

As far as patches [10-26/26] go, I'd like to see some review comments and/or
tags from the people with vested interest in there, in particular from Daniel
on patch [12/26] and from Sudeep on the PSCI ones.

Thanks,
Rafael