On 11/06/2014 05:57 PM, Daniel Lezcano wrote:
On 11/06/2014 05:08 AM, Preeti U Murthy wrote:
On 11/05/2014 07:58 PM, Daniel Lezcano wrote:
On 10/29/2014 03:01 AM, Preeti U Murthy wrote:
On 10/29/2014 12:29 AM, Daniel Lezcano wrote:
On 10/28/2014 04:51 AM, Preeti Murthy wrote:
Hi Daniel,
On Thu, Oct 23, 2014 at 2:31 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote: > When the pmqos latency requirement is set to zero that means > "poll in > all the > cases". > > That is correctly implemented on x86 but not on the other archs. > > As how is written the code, if the latency request is zero, the > governor will > return zero, so corresponding, for x86, to the poll function, but > for > the > others arch the default idle function. For example, on ARM this is > wait-for- > interrupt with a latency of '1', so violating the constraint.
This is not true actually. On PowerPC the idle state 0 has an exit_latency of 0.
> > In order to fix that, do the latency requirement check *before* > calling the > cpuidle framework in order to jump to the poll function without > entering > cpuidle. That has several benefits:
Doing so actually hurts on PowerPC. Because the idle loop defined for idle state 0 is different from what cpu_relax() does in cpu_idle_loop(). The spinning is more power efficient in the former case. Moreover we also set certain register values which indicate an idle cpu. The ppc_runlatch bits do precisely this. These register values are being read by some user space tools. So we will end up breaking them with this patch
My suggestion is very well keep the latency requirement check in kernel/sched/idle.c like your doing in this patch. But before jumping to cpu_idle_loop verify if the idle state 0 has an exit_latency > 0 in addition to your check on the latency_req == 0. If not, you can fall through to the regular path of calling into the cpuidle driver. The scheduler can query the cpuidle_driver structure anyway.
What do you think?
Thanks for reviewing the patch and spotting this.
Wouldn't make sense to create:
void __weak_cpu_idle_poll(void) ?
and override it with your specific poll function ?
No this would become ugly as far as I can see. A weak function has to be defined under arch/* code. We will either need to duplicate the idle loop that we already have in the drivers or point the weak function to the first idle state defined by our driver. Both of which is not desirable (calling into the driver from arch code is ugly). Another reason why I don't like the idea of a weak function is that if you have missed looking at a specific driver and they have an idle loop with features similar to on powerpc, you will have to spot it yourself and include the arch specific cpu_idle_poll() for them.
Yes, I agree this is a fair point. But actually I don't see the interest of having the poll loop in the cpuidle driver. These cleanups are
We can't do that simply because the idle poll loop has arch specific bits on powerpc.
I am not sure.
Could you describe what is the difference between the arch_cpu_idle function in arch/arm/powerpc/kernel/idle.c and the 0th power PC idle state ?
arch_cpu_idle() is the arch specific idle routine. It goes into deeper idle state. I am guessing you meant to ask the difference between power pc 0th idle state and the polling logic in cpu_idle_poll().
The 0th idle state is also a polling loop. Additionally it sets a couple of registers to indicate idleness.
Is it kind of duplicate ?
And for polling, do you really want to use while (...); cpu_relax(); as it is x86 specific ? instead of the powerpc's arch_idle ?
Today, if latency_req == 0, it returns the 0th idle state, so polling.
If we jump to the arch_cpu_idle_poll, the result will be the same for all architecture.
So you propose creating a weak arch_cpu_idle_poll()? Ok if it is going to make the cleanup easier, go ahead. I can add arch_cpu_idle_poll() in the core code on powerpc.
preparing the removal of the CPUIDLE_DRIVER_STATE_START macro which leads to a lot of mess in the cpuidle code.
How is the suggestion to check the exit_latency of idle state 0 when latency_req == 0 going to hinder this removal?
It sounds a bit hackish. I prefer to sort out the current situation.
And by the way, what is the reasoning behind having a target_residency / exit_latency equal to zero for an idle state ?
Its a polling idle state, hence the exit_latency is 0.
Regards Preeti U Murthy