[RFC PATCH V1 0/2] cpuidle: global registration of idle states with per-cpu statistics

This patch series is an early RFC to discuss the feasibility of
avoiding registering of idle states from each cpu.

The core change is to split the cpuidle_device structure into parts
that can be global and parts that has to remain per-cpu. The per-cpu
pieces are mostly generic statistics that can be independent of
current running driver.

Motivation:
* Simplify the cpuidle subsystem framework and have
registration/unregistration done by single cpu.

* Minimise the data structure that needs to be maintained for multiple
cpuidle drivers

* Reference: https://lkml.org/lkml/2011/2/10/37

Advantages:
* Make the cpuidle framework simple for most use cases where C-States
are symmetric. In case there are asymmetric C-States detected,
fallback mechanism should be incorporated to maintain the system
functional
https://lkml.org/lkml/2011/2/10/257
https://lkml.org/lkml/2011/2/10/37

* Non x86 archs that does not have asymmetric C-States like POWER, may
not need the fallback mechanism and hence the framework will be
simple for most use cases.

Disadvantages:
* Asymmetric C-States are part of x86 ACPI specification. Incorrect
handling may functionally affect the system

* Incorporating per-cpu masks for each state to allow/dis-allow global
states on subset of CPUs may result in an implementation that is
not better than current solution of having per-cpu states.

This patch series applies on top of the pm_idle cleanup patch
https://lkml.org/lkml/2011/3/22/150 (cpuidle: Cleanup pm_idle and
include driver/cpuidle.c in-kernel)

This patch series is tested on x86 Nehalem system with multiple ACPI
C-States.

This patch series has limitations of not handling multiple driver
registration and switching between drivers on all CPUs mainly due to
incomplete handling of per-cpu enable/disable and driver_data.

On Tue, 22 Mar 2011, Trinabh Gupta wrote:

> This patch series is an early RFC to discuss the feasibility of
> avoiding registering of idle states from each cpu.
>
> The core change is to split the cpuidle_device structure into parts
> that can be global and parts that has to remain per-cpu. The per-cpu
> pieces are mostly generic statistics that can be independent of
> current running driver.
>
> Motivation:
> * Simplify the cpuidle subsystem framework and have
> registration/unregistration done by single cpu.
>
> * Minimise the data structure that needs to be maintained for multiple
> cpuidle drivers
>
> * Reference: https://lkml.org/lkml/2011/2/10/37
>
> Advantages:
> * Make the cpuidle framework simple for most use cases where C-States
> are symmetric. In case there are asymmetric C-States detected,
> fallback mechanism should be incorporated to maintain the system
> functional
> https://lkml.org/lkml/2011/2/10/257
> https://lkml.org/lkml/2011/2/10/37
>
> * Non x86 archs that does not have asymmetric C-States like POWER, may
> not need the fallback mechanism and hence the framework will be
> simple for most use cases.
>
> Disadvantages:
> * Asymmetric C-States are part of x86 ACPI specification. Incorrect
> handling may functionally affect the system

I think this is a non-issue.

> * Incorporating per-cpu masks for each state to allow/dis-allow global
> states on subset of CPUs may result in an implementation that is
> not better than current solution of having per-cpu states.

I don't think this is needed.

> This patch series applies on top of the pm_idle cleanup patch
> https://lkml.org/lkml/2011/3/22/150 (cpuidle: Cleanup pm_idle and
> include driver/cpuidle.c in-kernel)
>
> This patch series is tested on x86 Nehalem system with multiple ACPI
> C-States.
>
> This patch series has limitations of not handling multiple driver
> registration and switching between drivers on all CPUs mainly due to
> incomplete handling of per-cpu enable/disable and driver_data.

I think this series is more important than the feature of multiple
driver/system support, and thus should come first.

I agree it is silly to allocate a cpuidle_device
for every cpu in the system as we do today.

Yes, splitting the counters out of cpuidle_device
is a necessary part of fixing that.

However, cpuidle_device.cpuidle_state[] is currently not per-driver,
it is per-cpu, and it is writable.

In particular, the cpuidle_device->prepare() mechanism
causes updates to the cpuidle_state[].flags,
setting and clearing CPUIDLE_FLAG_IGNORE to
tell the governor not to chose a state
on a per-cpu basis at run-time.

I don't like that mechanism.
I'd like to see it replaced, and when replaced,
cpuidle_state[] can be per system-wide driver.

I think the real problem that prepare() was trying to solve
is that the driver today does not have the ability to over-rule
the choice made by the governor. The driver may discover
in the course of trying to satisfy the request of the governor
that it needs to demote to a shallower state; or it may
do its best to satisfy the governor's request, and the hardware
may demote its request to a shallower state.

Unfortunately, when this happens, the driver dutifully
returns the time spent in the state to cpuidle_idle_call(),
who then updates the wrong last_residency, time, and usage counters.

Sure is ironic for the driver to allocate the data structures and
then hand the timer to the uppper layer, just to have the upper layer
update the wrong data structures...

Surely the driver enter routine should update the counters
that the driver was obligated to allocate, and it should return
the state actually entered (for tracing), rather than the time spent
there.

The generic cpuidle code should simply handle where the counters live
in the sysfs namespace, not updating the counters.
This needs to be addressed before cpuidle_device.cpuidle_state[]
can be made one/system.

> I think the real problem that prepare() was trying to solve
> is that the driver today does not have the ability to over-rule
> the choice made by the governor. The driver may discover
> in the course of trying to satisfy the request of the governor
> that it needs to demote to a shallower state; or it may
> do its best to satisfy the governor's request, and the hardware
> may demote its request to a shallower state.
>
> Unfortunately, when this happens, the driver dutifully
> returns the time spent in the state to cpuidle_idle_call(),
> who then updates the wrong last_residency, time, and usage counters.

I did not get this scenario. Are you saying

target_state->enter(dev, target_state) can enter a different state
than the one suggested by target_state?

I understand the hardware demotion part, but can we really detect the
target 'demoted' state in that case? I guess not.

> Sure is ironic for the driver to allocate the data structures and
> then hand the timer to the uppper layer, just to have the upper layer
> update the wrong data structures...
>
> Surely the driver enter routine should update the counters
> that the driver was obligated to allocate, and it should return
> the state actually entered (for tracing), rather than the time spent
> there.

Can we do something like this:

last_state = target_state->enter(dev, target_state)

dev->last_state and dev->last_residency are updated inside
target_state->enter()

The returned last_state is just for tracing, actual data is already
updated in the cpuidle_dev structure and used for sysfs display.

> The generic cpuidle code should simply handle where the counters live
> in the sysfs namespace, not updating the counters.
> This needs to be addressed before cpuidle_device.cpuidle_state[]
> can be made one/system.

Agreed.