On 09/08/2012 12:17 AM, Rafael J. Wysocki wrote:
On Friday, September 07, 2012, Daniel Lezcano wrote:
Since commit 46bcfad7a819bd17ac4e831b04405152d59784ab, cpuidle: Single/Global registration of idle states
we have a single registration for the cpuidle states which makes sense. But now two new architectures are coming: tegra3 and big.LITTLE.
These architectures have different cpus with different caracteristics for power saving. High load => powerfull processors, idle => small processors.
That implies different cpu latencies.
This patchset keeps the current behavior as introduced by Deepthi without breaking the drivers and add the possibility to specify a per cpu states.
- Tested on intel core 2 duo T9500
- Tested on vexpress by Lorenzo Pieralsi
- Tested on tegra3 by Peter De Schrijver
Daniel Lezcano (6): acpi : move the acpi_idle_driver variable declaration acpi : move cpuidle_device field out of the acpi_processor_power structure acpi : remove pointless cpuidle device state_count init
I've posted comments about patches [1-3/6] already. In short, I don't like [1/6], [2/6] would require some more work IMO and I'm not sure about the validity of the observation that [3/6] is based on.
Yes, I agree that the ACPI processor driver as a whole might be cleaner and it probably would be good to spend some time on cleaning it up, but not necessarily in a hurry.
Unfortunately, I also don't agree with the approach used by the remaining patches, which is to try to use a separate array of states for each individual CPU core. This way we end up with quite some duplicated data if the CPU cores in question actually happen to be identical.
Actually, there is a single array of states which is defined with the cpuidle_driver. A pointer to this array from the cpuidle_device structure is added and used from the cpuidle core.
If the cpu cores are identical, this pointer will refer to the same array.
Maybe I misunderstood you remark but there is no data duplication, that was the purpose of this approach to just add a pointer to point to a single array when the core are identical and to a different array when the cores are different (set by the driver). Furthermore, this patch allows to support multiple cpu latencies without impacting the existing drivers.
What about using a separate cpuidle driver for every kind of different CPUs in the system (e.g. one driver for "big" CPUs and the other for "little" ones)?
Have you considered this approach already?
No, what would be the benefit of this approach ? We will need to switch the driver each time we switch the cluster (assuming all it is the bL switcher in place and not the scheduler). IMHO, that could be suboptimal because we will have to (un)register the driver, register the devices, pull all the sysfs and notifications mechanisms. The cpuidle core is not designed for that.
eg.
int cpuidle_register_driver(struct cpuidle_driver *drv) { if (!drv || !drv->state_count) return -EINVAL;
if (cpuidle_disabled()) return -ENODEV;
spin_lock(&cpuidle_driver_lock);
if (cpuidle_curr_driver) { spin_unlock(&cpuidle_driver_lock); return -EBUSY; }
__cpuidle_register_driver(drv); cpuidle_curr_driver = drv; spin_unlock(&cpuidle_driver_lock);
return 0; }
cpuidle : add a pointer for cpuidle_state in the cpuidle_device cpuidle : use per cpuidle device cpu states cpuidle : add cpuidle_register_states function
drivers/acpi/processor_driver.c | 2 +- drivers/acpi/processor_idle.c | 27 +++++++++++++++------- drivers/cpuidle/cpuidle.c | 42 ++++++++++++++++++++++++++++------- drivers/cpuidle/governors/ladder.c | 22 +++++++++--------- drivers/cpuidle/governors/menu.c | 8 +++--- drivers/cpuidle/sysfs.c | 3 +- include/acpi/processor.h | 3 -- include/linux/cpuidle.h | 11 ++++++-- 8 files changed, 76 insertions(+), 42 deletions(-)
Thanks, Rafael