[RFC PATCH v2 0/4] cpuidle: (POWER) cpuidle driver for pSeries

On Mon, 2011-11-28 at 16:33 +0530, Deepthi Dharwar wrote:

On an LPAR if cpuidle is disabled, ppc_md.power_save is still set to
cpuidle_idle_call by default here. This would result in calling of
cpuidle_idle_call repeatedly, only for the call to return -ENODEV. The
default idle is never executed.
This would be a major design flaw. No fallback idle routine.

We propose to fix this by checking the return value of
ppc_md.power_save() call from void to int.
Right now return value is void, but if we change this to int, this
would solve two problems. One being removing the cast to a function
pointer in the prev patch and this design flaw stated above.

So by checking the return value of ppc_md.power_save(), we can invoke
the default idle on failure. But my only concern is about the effects of
changing the ppc_md.power_save() to return int on other powerpc
architectures. Would it be a good idea to change the return type to int
which would help us flag an error and fallback to default idle?

I would have preferred an approach where the cpuidle module sets
ppc_md.power_save when loaded and restores it when unloaded ... but that
would have to go into the cpuidle core as a powerpc specific tweak and
might not be generally well received.

So go for it, add the return value, but you'll have to update all the
idle functions (grep for power_save in arch/powerpc to find them).

On Mon, 2011-11-28 at 16:32 +0530, Deepthi Dharwar wrote:

Additionally, I'm a bit worried (but maybe we already discussed that a
while back, I don't know) but cpu_idle_wait() has "wait" in the name,
which makes me think it might need to actually -wait- for all cpus to
have come out of the function.

cpu_idle_wait is used to ensure that all the CPUs discard old idle
handler and update to new one. Required while changing idle
handler on SMP systems.

Now your implementation doesn't provide that guarantee. It might be
fine, I don't know, but if it is, you'd better document it well in the
comments surrounding the code, because as it is, all you do is shoot an
interrupt which will cause the target CPU to eventually come out of idle
some time in the future.

I was hoping that sending an explicit reschedule to the cpus would
do the trick but sure we can add some documentation around the code.

Well, the question is what guarantee do you expect. Sending a reschedule
IPI will take the other CPUs out of the actual sleep mode, but it will
be some time from there back to getting out of the handler function
(first back out of hypervisor etc...).

The code as you implemented it doesn't wait for that to happen. It might
be fine ... or not. I don't know what semantics you are after precisely.

Cheers,
Ben.

Yes, this could be problematic as there is small window for the
race condition to occur . Otherwise we need to manually schedule
it by running a kernel thread but this would definitely have a
overhead and would be an overkill.

On Tue, 2011-11-29 at 12:12 +0530, Deepthi Dharwar wrote:

Yes, this could be problematic as there is small window for the
race condition to occur . Otherwise we need to manually schedule
it by running a kernel thread but this would definitely have a
overhead and would be an overkill.

Depends what this "window" is. IE. What are you trying to protect
yourself against ? What's the risk ?

If it's just module unload, then stop_machine is probably your
friend :-)

Cheers,
Ben.

On 11/29/2011 02:09 AM, Benjamin Herrenschmidt wrote:

On Mon, 2011-11-28 at 16:33 +0530, Deepthi Dharwar wrote:

On an LPAR if cpuidle is disabled, ppc_md.power_save is still set to
cpuidle_idle_call by default here. This would result in calling of
cpuidle_idle_call repeatedly, only for the call to return -ENODEV. The
default idle is never executed.
This would be a major design flaw. No fallback idle routine.

We propose to fix this by checking the return value of
ppc_md.power_save() call from void to int.
Right now return value is void, but if we change this to int, this
would solve two problems. One being removing the cast to a function
pointer in the prev patch and this design flaw stated above.

So by checking the return value of ppc_md.power_save(), we can invoke
the default idle on failure. But my only concern is about the effects of
changing the ppc_md.power_save() to return int on other powerpc
architectures. Would it be a good idea to change the return type to int
which would help us flag an error and fallback to default idle?

I would have preferred an approach where the cpuidle module sets
ppc_md.power_save when loaded and restores it when unloaded ... but that
would have to go into the cpuidle core as a powerpc specific tweak and
might not be generally well received.

So go for it, add the return value, but you'll have to update all the
idle functions (grep for power_save in arch/powerpc to find them).

Thanks Ben. Yes, I will update all the idle functions under powerpc.
I will re-work these patches with the discussed changes.

Regards,
Deepthi

_______________________________________________
linux-pm mailing list

https://lists.linuxfoundation.org/m...o/linux-pm