On Tue, 12 Jan 2016, Daniel Lezcano wrote:

On 01/12/2016 03:26 PM, Thomas Gleixner wrote:

quoted

You better implement the switching part in the cpuidle core first, i.e.
proper
callbacks when a governor is switched in/out. Then make use of this
switcheroo
right away. Doing it the other way round is just wrong.

The problem is this code is not another governor but a 'predictor' where the
scheduler will use the information to ask the cpuidle to go to a specific idle
state without going through the governor code, so into the governor's
callbacks. It is on top of cpuidle. The scheduler will become the governor.

The current straightforward code, does the switch in the cpu_idle_loop
idle_task's function:

[ ... ]

if (cpu_idle_force_poll || tick_check_broadcast_expired())
	cpu_idle_poll();
else {
	if (sched_idle_enabled()) {
		int latency = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
		s64 duration = sched_idle_next_wakeup();
		sched_idle(duration, latency);
	} else {
		cpuidle_idle_call();
	}
}

Due to the complexity of the code, this first step introduce a mechanism to
predict the next event and re-use it trivially in the idle task.

This looks really wrong. Why on earth don't you implement a proper governor
and just get rid of this extra hackery?

Thanks,

	tglx