[PATCH/RFC] PM / Domains: Minimize latencies by not delaying save/restore

On Mon, May 11, 2015 at 03:24:33PM +0200, Ulf Hansson wrote:

This patch actually address two issues which exists due to the current
policy of how genpd deals with devices' ->runtime_suspend|resume()
callbacks.

The two issues are described below.

Issue 1) A thundering herd problem with latencies issues:

Currently genpd delays to walk the hierarchy of ->runtime_suspend()
callbacks for a device, until all devices in a PM domain becomes
inactive.

Support for fine grained PM is instead handled via the gpd_dev_ops'
->stop|start() callbacks. The ->stop() callback is invoked immediately
when genpd's ->runtime_suspend() callback is called at the PM domain
level.

These two paragraphs seem to be contradictory.  I think the first is
telling me that genpd waits until all devices have become inactive
before walking the hierarchy of ->runtime_suspend callbacks[*] for a
device.  As this waits for all devices to become inactivate, this is
not fine grained.  The second paragraph tells me that genpd's ->stop
method is called from the genpd's ->runtime_suspend(), but this is
fine grained... that's confusing.  As I'm not sure what you're trying
to say here, maybe it'd help if you replaced "->runtime_suspend()
callback" with the actual function name so the two ->runtime_suspend()
references don't get confused.  IOW, pm_genpd_runtime_suspend.

Second point here is that "->runtime_suspend callbacks for a device".
Why plural?  Can a single device have more than one runtime_suspend
callback?  My understanding is that a struct device can have up to one
struct device_driver, and there will be one dev_pm_ops available for
use, which may come from one of several sources (device driver, bus
type, device class, etc.)

When genpd realizes that all devices have become inactive within the PM
domain, it starts the power off sequence.

Those devices that were re-activated since the last powered off
sequence, needs to be given the opportunity to have their register
context saved. That means walking the hierarchy of the devices'
->runtime_suspend() callbacks.

There's, no, need, for, a, comma, there.  It's just confusing and makes
it harder to read.

Though, genpd first need to invoke the ->start() callbacks, else
drivers/subsystems won't be able to save their devices' register
context.

Can we please have the above worded a little clearer?  How does this
relate to the previous paragraphs?  This description makes it unclear
at what point this action is required.  Is this talking about the point
where a device is being re-activated, or are we still talking about
the powering down sequence mentioned three paragraphs above?

After invoking the ->start() and then ->runtime_suspend() callback,
genpd again needs to invoke the ->stop() callback. A sequence which
is repeated for each of the re-activated devices within the PM domain,
during the power off sequence.

Hang on, if the ->stop() callback was already called because of a
genpd's ->runtime_suspend() method being called, but which
->runtime_suspend are we talking about here.  Confused.

This leads to a suboptimal power off sequence, both from the
perspective that the ->start|stop() callback needs to be invoked but
also because all operations are serialized. It simply causes a
thundering herd problem and gives latency issues.

To elaborate on the latency issues, let's follow the below scenario.

	* Device [A] and [B] lives in the same generic PM domain.

	* Device [A] has been re-activated and has recently also become
	inactive, which means genpd has invoked the ->stop() callback
	for it and started the power off sequence.

I'm not sure what "also" has to do with anything here.  What action is
"also" referring to?  Maybe this is sufficient:

	* Device [A] has recently become inactive, causing genpd to
	invoke the PM domain's stop() method, which starts the power
	off sequence via pm_genpd_poweroff().

(That's another thing... providing actual function names in the
description allow people to refer to the code and follow your
problem description.)

	* Then someone calls pm_runtime_get_sync() for the inactive
	device [B]. The ongoing power off sequence should thus be
	aborted as soon as possible.

	* In this scenario the actual PM domain hasn't yet been powered
	off, but instead the power off sequence calls
	__pm_genpd_save_device() for device [A].

	* In parallel genpd's ->runtime_resume() callback at the PM
	domain level is invoked for device [B]. It notifies the power
	off sequence to abort, which needs confirmation to be able to
	continue.

	* Unfortunate, aborting the power off sequence won't be
	confirmed until device [A] has been powered off through
	__pm_genpd_save_device().

In this scenario, device [B] will be waiting for device [A] to complete
->start(), ->runtime_suspend() and ->stop(), which are invoked from
__pm_genpd_save_device(). In other words, device [B]'s
->runtime_resume() latency will be extended with device [A]'s time to
complete power off.

To move this reasoning a step further, it means all devices within the
same generic PM domain suffers from an increased ->runtime_resume()
latency. Moreover, that extended latency comes from the device with the
slowest power off sequence.

This behavior in genpd, is a consequence from that the power off
sequence needs to protect itself by using locks and states. Therefore
can an abort only be confirmed in between calls to
__pm_genpd_save_device().

Really, this English needs to be improved... I'm not sure I follow what
you're trying to say in this mail thus far, and the above paragraph makes
very little sense, and like previous ones, is gramatically wrong - there
should be no comma between "genpd" and "is", but it is generally accepted
that a comma always follows "Therefore".  "can an abort" makes zero sense,
maybe you meant "Therefore, an abort can only be..."

I'm giving up at this point... a description which can't be understood is
useless, sorry.

Please try again.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.