| Message ID | 1391696188-14540-1-git-send-email-nicolas.pitre@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
An Thu, 6 Feb 2014, Nicolas Pitre wrote: > The core idle loop now takes care of it. > > Signed-off-by: Nicolas Pitre <nico@linaro.org> Acked-by: Thomas Gleixner <tglx@linutronix.de>
On 02/06/2014 07:46 PM, Nicolas Pitre wrote: > The core idle loop now takes care of it. > > Signed-off-by: Nicolas Pitre <nico@linaro.org> > --- > arch/powerpc/platforms/powernv/setup.c | 13 +------------ > 1 file changed, 1 insertion(+), 12 deletions(-) > > diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c > index 21166f65c9..a932feb290 100644 > --- a/arch/powerpc/platforms/powernv/setup.c > +++ b/arch/powerpc/platforms/powernv/setup.c > @@ -26,7 +26,6 @@ > #include <linux/of_fdt.h> > #include <linux/interrupt.h> > #include <linux/bug.h> > -#include <linux/cpuidle.h> > > #include <asm/machdep.h> > #include <asm/firmware.h> > @@ -217,16 +216,6 @@ static int __init pnv_probe(void) > return 1; > } > > -void powernv_idle(void) > -{ > - /* Hook to cpuidle framework if available, else > - * call on default platform idle code > - */ > - if (cpuidle_idle_call()) { > - power7_idle(); > - } > -} > - > define_machine(powernv) { > .name = "PowerNV", > .probe = pnv_probe, > @@ -236,7 +225,7 @@ define_machine(powernv) { > .show_cpuinfo = pnv_show_cpuinfo, > .progress = pnv_progress, > .machine_shutdown = pnv_shutdown, > - .power_save = powernv_idle, > + .power_save = power7_idle, > .calibrate_decr = generic_calibrate_decr, > #ifdef CONFIG_KEXEC > .kexec_cpu_down = pnv_kexec_cpu_down, > Reviewed-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Hi Nico, On 6 February 2014 14:16, Nicolas Pitre <nicolas.pitre@linaro.org> wrote: > The core idle loop now takes care of it. > > Signed-off-by: Nicolas Pitre <nico@linaro.org> > --- > arch/powerpc/platforms/powernv/setup.c | 13 +------------ > 1 file changed, 1 insertion(+), 12 deletions(-) > > diff --git a/arch/powerpc/platforms/powernv/setup.c > b/arch/powerpc/platforms/powernv/setup.c > index 21166f65c9..a932feb290 100644 > --- a/arch/powerpc/platforms/powernv/setup.c > +++ b/arch/powerpc/platforms/powernv/setup.c > @@ -26,7 +26,6 @@ > #include <linux/of_fdt.h> > #include <linux/interrupt.h> > #include <linux/bug.h> > -#include <linux/cpuidle.h> > > #include <asm/machdep.h> > #include <asm/firmware.h> > @@ -217,16 +216,6 @@ static int __init pnv_probe(void) > return 1; > } > > -void powernv_idle(void) > -{ > - /* Hook to cpuidle framework if available, else > - * call on default platform idle code > - */ > - if (cpuidle_idle_call()) { > - power7_idle(); > - } > The cpuidle_idle_call is called from arch_cpu_idle in arch/powerpc/kernel/idle.c between a ppc64_runlatch_off|on section. Shouldn't the cpuidle-powernv driver call these functions when entering idle ? -- Daniel > -} > - > define_machine(powernv) { > .name = "PowerNV", > .probe = pnv_probe, > @@ -236,7 +225,7 @@ define_machine(powernv) { > .show_cpuinfo = pnv_show_cpuinfo, > .progress = pnv_progress, > .machine_shutdown = pnv_shutdown, > - .power_save = powernv_idle, > + .power_save = power7_idle, > .calibrate_decr = generic_calibrate_decr, > #ifdef CONFIG_KEXEC > .kexec_cpu_down = pnv_kexec_cpu_down, > -- > 1.8.4.108.g55ea5f6 > >
Hi Daniel, On 02/06/2014 09:55 PM, Daniel Lezcano wrote: > Hi Nico, > > > On 6 February 2014 14:16, Nicolas Pitre <nicolas.pitre@linaro.org> wrote: > >> The core idle loop now takes care of it. >> >> Signed-off-by: Nicolas Pitre <nico@linaro.org> >> --- >> arch/powerpc/platforms/powernv/setup.c | 13 +------------ >> 1 file changed, 1 insertion(+), 12 deletions(-) >> >> diff --git a/arch/powerpc/platforms/powernv/setup.c >> b/arch/powerpc/platforms/powernv/setup.c >> index 21166f65c9..a932feb290 100644 >> --- a/arch/powerpc/platforms/powernv/setup.c >> +++ b/arch/powerpc/platforms/powernv/setup.c >> @@ -26,7 +26,6 @@ >> #include <linux/of_fdt.h> >> #include <linux/interrupt.h> >> #include <linux/bug.h> >> -#include <linux/cpuidle.h> >> >> #include <asm/machdep.h> >> #include <asm/firmware.h> >> @@ -217,16 +216,6 @@ static int __init pnv_probe(void) >> return 1; >> } >> >> -void powernv_idle(void) >> -{ >> - /* Hook to cpuidle framework if available, else >> - * call on default platform idle code >> - */ >> - if (cpuidle_idle_call()) { >> - power7_idle(); >> - } >> > > The cpuidle_idle_call is called from arch_cpu_idle in > arch/powerpc/kernel/idle.c between a ppc64_runlatch_off|on section. > Shouldn't the cpuidle-powernv driver call these functions when entering > idle ? Yes they should, I will send out a patch that does that ontop of this. There have been cpuidle driver cleanups for powernv and pseries in this merge window. While no change would be required in the pseries cpuidle driver as a result of Nicolas's cleanup, we would need to add the ppc64_runlatch_on and off functions before and after the entry into the powernv idle states. Thanks Regards Preeti U Murthy > > -- Daniel > > >> -} >> - >> define_machine(powernv) { >> .name = "PowerNV", >> .probe = pnv_probe, >> @@ -236,7 +225,7 @@ define_machine(powernv) { >> .show_cpuinfo = pnv_show_cpuinfo, >> .progress = pnv_progress, >> .machine_shutdown = pnv_shutdown, >> - .power_save = powernv_idle, >> + .power_save = power7_idle, >> .calibrate_decr = generic_calibrate_decr, >> #ifdef CONFIG_KEXEC >> .kexec_cpu_down = pnv_kexec_cpu_down, >> -- >> 1.8.4.108.g55ea5f6 >> >> >
On Thu, 6 Feb 2014, Preeti U Murthy wrote: > Hi Daniel, > > On 02/06/2014 09:55 PM, Daniel Lezcano wrote: > > Hi Nico, > > > > > > On 6 February 2014 14:16, Nicolas Pitre <nicolas.pitre@linaro.org> wrote: > > > >> The core idle loop now takes care of it. > >> > >> Signed-off-by: Nicolas Pitre <nico@linaro.org> > >> --- > >> arch/powerpc/platforms/powernv/setup.c | 13 +------------ > >> 1 file changed, 1 insertion(+), 12 deletions(-) > >> > >> diff --git a/arch/powerpc/platforms/powernv/setup.c > >> b/arch/powerpc/platforms/powernv/setup.c > >> index 21166f65c9..a932feb290 100644 > >> --- a/arch/powerpc/platforms/powernv/setup.c > >> +++ b/arch/powerpc/platforms/powernv/setup.c > >> @@ -26,7 +26,6 @@ > >> #include <linux/of_fdt.h> > >> #include <linux/interrupt.h> > >> #include <linux/bug.h> > >> -#include <linux/cpuidle.h> > >> > >> #include <asm/machdep.h> > >> #include <asm/firmware.h> > >> @@ -217,16 +216,6 @@ static int __init pnv_probe(void) > >> return 1; > >> } > >> > >> -void powernv_idle(void) > >> -{ > >> - /* Hook to cpuidle framework if available, else > >> - * call on default platform idle code > >> - */ > >> - if (cpuidle_idle_call()) { > >> - power7_idle(); > >> - } > >> > > > > The cpuidle_idle_call is called from arch_cpu_idle in > > arch/powerpc/kernel/idle.c between a ppc64_runlatch_off|on section. > > Shouldn't the cpuidle-powernv driver call these functions when entering > > idle ? > > Yes they should, I will send out a patch that does that ontop of this. > There have been cpuidle driver cleanups for powernv and pseries in this > merge window. While no change would be required in the pseries cpuidle > driver as a result of Nicolas's cleanup, we would need to add the > ppc64_runlatch_on and off functions before and after the entry into the > powernv idle states. What about creating arch_cpu_idle_enter() and arch_cpu_idle_exit() in arch/powerpc/kernel/idle.c and calling ppc64_runlatch_off() and ppc64_runlatch_on() respectively from there instead? Would that work? That would make the idle consolidation much easier afterwards. Nicolas
Hi Nicolas, On 02/07/2014 06:47 AM, Nicolas Pitre wrote: > On Thu, 6 Feb 2014, Preeti U Murthy wrote: > >> Hi Daniel, >> >> On 02/06/2014 09:55 PM, Daniel Lezcano wrote: >>> Hi Nico, >>> >>> >>> On 6 February 2014 14:16, Nicolas Pitre <nicolas.pitre@linaro.org> wrote: >>> >>>> The core idle loop now takes care of it. >>>> >>>> Signed-off-by: Nicolas Pitre <nico@linaro.org> >>>> --- >>>> arch/powerpc/platforms/powernv/setup.c | 13 +------------ >>>> 1 file changed, 1 insertion(+), 12 deletions(-) >>>> >>>> diff --git a/arch/powerpc/platforms/powernv/setup.c >>>> b/arch/powerpc/platforms/powernv/setup.c >>>> index 21166f65c9..a932feb290 100644 >>>> --- a/arch/powerpc/platforms/powernv/setup.c >>>> +++ b/arch/powerpc/platforms/powernv/setup.c >>>> @@ -26,7 +26,6 @@ >>>> #include <linux/of_fdt.h> >>>> #include <linux/interrupt.h> >>>> #include <linux/bug.h> >>>> -#include <linux/cpuidle.h> >>>> >>>> #include <asm/machdep.h> >>>> #include <asm/firmware.h> >>>> @@ -217,16 +216,6 @@ static int __init pnv_probe(void) >>>> return 1; >>>> } >>>> >>>> -void powernv_idle(void) >>>> -{ >>>> - /* Hook to cpuidle framework if available, else >>>> - * call on default platform idle code >>>> - */ >>>> - if (cpuidle_idle_call()) { >>>> - power7_idle(); >>>> - } >>>> >>> >>> The cpuidle_idle_call is called from arch_cpu_idle in >>> arch/powerpc/kernel/idle.c between a ppc64_runlatch_off|on section. >>> Shouldn't the cpuidle-powernv driver call these functions when entering >>> idle ? >> >> Yes they should, I will send out a patch that does that ontop of this. >> There have been cpuidle driver cleanups for powernv and pseries in this >> merge window. While no change would be required in the pseries cpuidle >> driver as a result of Nicolas's cleanup, we would need to add the >> ppc64_runlatch_on and off functions before and after the entry into the >> powernv idle states. > > What about creating arch_cpu_idle_enter() and arch_cpu_idle_exit() in > arch/powerpc/kernel/idle.c and calling ppc64_runlatch_off() and > ppc64_runlatch_on() respectively from there instead? Would that work? > That would make the idle consolidation much easier afterwards. I would not suggest doing this. The ppc64_runlatch_*() routines need to be called when we are sure that the cpu is about to enter or has exit an idle state. Moving the ppc64_runlatch_on() routine to arch_cpu_idle_enter() for instance is not a good idea because there are places where the cpu can decide not to enter any idle state before the call to cpuidle_idle_call() itself. In that case communicating prematurely that we are in an idle state would not be a good idea. So its best to add the ppc64_runlatch_* calls in the powernv cpuidle driver IMO. We could however create idle_loop_prologue/epilogue() variants inside it so that in addition to the runlatch routines we could potentially add more such similar routines that are powernv specific. If there are cases where there is work to be done prior to and post an entry into an idle state common to both pseries and powernv, we will probably put them in arch_cpu_idle_enter/exit(). But the runlatch routines are not suitable to be moved there as far as I can see. Thank you Regards Preeti U Murthy > > > Nicolas > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev >
On Fri, 7 Feb 2014, Preeti U Murthy wrote: > Hi Nicolas, > > On 02/07/2014 06:47 AM, Nicolas Pitre wrote: > > > > What about creating arch_cpu_idle_enter() and arch_cpu_idle_exit() in > > arch/powerpc/kernel/idle.c and calling ppc64_runlatch_off() and > > ppc64_runlatch_on() respectively from there instead? Would that work? > > That would make the idle consolidation much easier afterwards. > > I would not suggest doing this. The ppc64_runlatch_*() routines need to > be called when we are sure that the cpu is about to enter or has exit an > idle state. Moving the ppc64_runlatch_on() routine to > arch_cpu_idle_enter() for instance is not a good idea because there are > places where the cpu can decide not to enter any idle state before the > call to cpuidle_idle_call() itself. In that case communicating > prematurely that we are in an idle state would not be a good idea. > > So its best to add the ppc64_runlatch_* calls in the powernv cpuidle > driver IMO. We could however create idle_loop_prologue/epilogue() > variants inside it so that in addition to the runlatch routines we could > potentially add more such similar routines that are powernv specific. > If there are cases where there is work to be done prior to and post an > entry into an idle state common to both pseries and powernv, we will > probably put them in arch_cpu_idle_enter/exit(). But the runlatch > routines are not suitable to be moved there as far as I can see. OK. However, one thing we need to do as much as possible is to remove those loops based on need_resched() from idle backend drivers. A somewhat common pattern is: my_idle() { /* interrupts disabled on entry */ while (!need_resched()) { lowpower_wait_for_interrupts(); local_irq_enable(); /* IRQ serviced from here */ local_irq_disable(); } local_irq_enable(); /* interrupts enabled on exit */ } To be able to keep statistics on the actual idleness of the CPU we'd need for all idle backends to always return to generic code on every interrupt similar to this: my_idle() { /* interrupts disabled on entry */ lowpower_wait_for_interrupts(); local_irq_enable(); /* interrupts enabled on exit */ } The generic code would be responsible for dealing with need_resched() and call back into the backend right away if necessary after updating some stats. Do you see a problem with the runlatch calls happening around each interrrupt from such a simplified idle backend? Nicolas
Hi Nicolas, On 02/07/2014 04:18 PM, Nicolas Pitre wrote: > On Fri, 7 Feb 2014, Preeti U Murthy wrote: > >> Hi Nicolas, >> >> On 02/07/2014 06:47 AM, Nicolas Pitre wrote: >>> >>> What about creating arch_cpu_idle_enter() and arch_cpu_idle_exit() in >>> arch/powerpc/kernel/idle.c and calling ppc64_runlatch_off() and >>> ppc64_runlatch_on() respectively from there instead? Would that work? >>> That would make the idle consolidation much easier afterwards. >> >> I would not suggest doing this. The ppc64_runlatch_*() routines need to >> be called when we are sure that the cpu is about to enter or has exit an >> idle state. Moving the ppc64_runlatch_on() routine to >> arch_cpu_idle_enter() for instance is not a good idea because there are >> places where the cpu can decide not to enter any idle state before the >> call to cpuidle_idle_call() itself. In that case communicating >> prematurely that we are in an idle state would not be a good idea. >> >> So its best to add the ppc64_runlatch_* calls in the powernv cpuidle >> driver IMO. We could however create idle_loop_prologue/epilogue() >> variants inside it so that in addition to the runlatch routines we could >> potentially add more such similar routines that are powernv specific. >> If there are cases where there is work to be done prior to and post an >> entry into an idle state common to both pseries and powernv, we will >> probably put them in arch_cpu_idle_enter/exit(). But the runlatch >> routines are not suitable to be moved there as far as I can see. > > OK. > > However, one thing we need to do as much as possible is to remove those > loops based on need_resched() from idle backend drivers. A somewhat > common pattern is: > > my_idle() > { > /* interrupts disabled on entry */ > while (!need_resched()) { > lowpower_wait_for_interrupts(); > local_irq_enable(); > /* IRQ serviced from here */ > local_irq_disable(); > } > local_irq_enable(); > /* interrupts enabled on exit */ > } > > To be able to keep statistics on the actual idleness of the CPU we'd > need for all idle backends to always return to generic code on every > interrupt similar to this: > > my_idle() > { > /* interrupts disabled on entry */ > lowpower_wait_for_interrupts(); You can do this for the idle states which do not have the polling nature. IOW, these idle states are capable of doing what you describe as "wait_for_interrupts". They do some kind of spinning at the hardware level with interrupts enabled. A reschedule IPI or any other interrupt will wake them up to enter the generic idle loop where they check for the cause of the interrupt. But observe the idle state "snooze" on powerpc. The power that this idle state saves is through the lowering of the thread priority of the CPU. After it lowers the thread priority, it is done. It cannot "wait_for_interrupts". It will exit my_idle(). It is now upto the generic idle loop to increase the thread priority if the need_resched flag is set. Only an interrupt routine can increase the thread priority. Else we will need to do it explicitly. And in such states which have a polling nature, the cpu will not receive a reschedule IPI. That is why in the snooze_loop() we poll on need_resched. If it is set we up the priority of the thread using HMT_MEDIUM() and then exit the my_idle() loop. In case of interrupts, the priority gets automatically increased. This might not be required to be done for similar idle routines on other archs but this is the consequence of applying this idea of simplified cpuidle backend driver on powerpc. I would say you could let the backend cpuidle drivers be in this regard, it could complicate the generic idle loop IMO depending on how the polling states are implemented in each architecture. > The generic code would be responsible for dealing with need_resched() > and call back into the backend right away if necessary after updating > some stats. > > Do you see a problem with the runlatch calls happening around each > interrrupt from such a simplified idle backend? The runlatch calls could be moved outside the loop.They do not need to be called each time. Thanks Regards Preeti U Murthy > > > Nicolas >
On Fri, Feb 07, 2014 at 05:11:26PM +0530, Preeti U Murthy wrote: > But observe the idle state "snooze" on powerpc. The power that this idle > state saves is through the lowering of the thread priority of the CPU. > After it lowers the thread priority, it is done. It cannot > "wait_for_interrupts". It will exit my_idle(). It is now upto the > generic idle loop to increase the thread priority if the need_resched > flag is set. Only an interrupt routine can increase the thread priority. > Else we will need to do it explicitly. And in such states which have a > polling nature, the cpu will not receive a reschedule IPI. > > That is why in the snooze_loop() we poll on need_resched. If it is set > we up the priority of the thread using HMT_MEDIUM() and then exit the > my_idle() loop. In case of interrupts, the priority gets automatically > increased. You can poll without setting TS_POLLING/TIF_POLLING_NRFLAGS just fine and get the IPI if that is what you want. Depending on how horribly unprovisioned the thread gets at the lowest priority, that might actually be faster than polling and raising the prio whenever it does get ran.
Hi Peter, On 02/07/2014 06:11 PM, Peter Zijlstra wrote: > On Fri, Feb 07, 2014 at 05:11:26PM +0530, Preeti U Murthy wrote: >> But observe the idle state "snooze" on powerpc. The power that this idle >> state saves is through the lowering of the thread priority of the CPU. >> After it lowers the thread priority, it is done. It cannot >> "wait_for_interrupts". It will exit my_idle(). It is now upto the >> generic idle loop to increase the thread priority if the need_resched >> flag is set. Only an interrupt routine can increase the thread priority. >> Else we will need to do it explicitly. And in such states which have a >> polling nature, the cpu will not receive a reschedule IPI. >> >> That is why in the snooze_loop() we poll on need_resched. If it is set >> we up the priority of the thread using HMT_MEDIUM() and then exit the >> my_idle() loop. In case of interrupts, the priority gets automatically >> increased. > > You can poll without setting TS_POLLING/TIF_POLLING_NRFLAGS just fine > and get the IPI if that is what you want. > > Depending on how horribly unprovisioned the thread gets at the lowest > priority, that might actually be faster than polling and raising the > prio whenever it does get ran. So I am assuming you mean something like the below: my_idle() { local_irq_enable(); /* Remove the setting of the polling flag */ HMT_low(); return index; } And then exit into the generic idle loop. But the issue I see here is that the TS_POLLING/TIF_POLLING_NRFLAGS gets set immediately. So, if on testing need_resched() immediately after this returns that the TIF_NEED_RESCHED flag is set, the thread will exit at low priority right? We could raise the priority of the thread in arch_cpu_idle_exit() soon after setting the polling flag but that would mean for cases where the TIF_NEED_RESCHED flag is not set we unnecessarily raise the priority of the thread. Thanks Regards Preeti U Murthy >
diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c index 21166f65c9..a932feb290 100644 --- a/arch/powerpc/platforms/powernv/setup.c +++ b/arch/powerpc/platforms/powernv/setup.c @@ -26,7 +26,6 @@ #include <linux/of_fdt.h> #include <linux/interrupt.h> #include <linux/bug.h> -#include <linux/cpuidle.h> #include <asm/machdep.h> #include <asm/firmware.h> @@ -217,16 +216,6 @@ static int __init pnv_probe(void) return 1; } -void powernv_idle(void) -{ - /* Hook to cpuidle framework if available, else - * call on default platform idle code - */ - if (cpuidle_idle_call()) { - power7_idle(); - } -} - define_machine(powernv) { .name = "PowerNV", .probe = pnv_probe, @@ -236,7 +225,7 @@ define_machine(powernv) { .show_cpuinfo = pnv_show_cpuinfo, .progress = pnv_progress, .machine_shutdown = pnv_shutdown, - .power_save = powernv_idle, + .power_save = power7_idle, .calibrate_decr = generic_calibrate_decr, #ifdef CONFIG_KEXEC .kexec_cpu_down = pnv_kexec_cpu_down,
The core idle loop now takes care of it. Signed-off-by: Nicolas Pitre <nico@linaro.org> --- arch/powerpc/platforms/powernv/setup.c | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-)