On Thu, 2012-05-17 at 15:52 +0530, Deepthi Dharwar wrote: > On 05/17/2012 09:58 AM, Benjamin Herrenschmidt wrote: > > > On Thu, 2012-05-17 at 12:01 +0800, Li Zhong wrote: > >> This patch tries to fix following lockdep complaints: > > > > .../... > > > >> pseries_notify_cpu_idle_add_cpu() actually does > >> cpuidle_disable_device(), and then cpuidle_enable_device(), which > >> releases and allocates the resources respectively. ( Also, all the data > >> are cleared and reinitialized after this cycle). The problem here is: > >> something like kzalloc(GFP_KERNEL), wait_for_completion() would have > >> problems running here where irqs are still disabled. > > > This is true when the system is booting up. > > > > > So yes, it looks definitely fishy. I don't have time to study cpuidle > > today to check whether that's correct or not so I'm CCing Deepthi > > Dharwar who did all that cpuidle work for pseries. > > > > Deepthi, can you check whether that patch is correct ? > > > pseries_notify_cpu_idle_add_cpu() is essential to be called for > hotplug event. So by removing this call completely wouldn't > support cpus registering under cpuidle on hotplug and default idle is > executed on those with do not give much powersavings.
Maybe I missed that part.. would you please give some details how removing this would prevent powersaving cpuidle being called after hotplug? After rereading the codes, I think ppc_md.power_save() is the one you mentioned that could give much powersavings? It is registered as pSeries_idle(), which calls cpuidle_idle_call(). It seems to me that it would still be called after hotplug. Or maybe I misunderstood your point? > Ideal way it to > have a notifier in pseries backend driver for hotplug notification and > then remove this function from here. > I am currently working on this patch, will post it out soon. > > > > >> Actually, cpuidle_enable_device() is called for each possible cpu when > >> the driver is registered. So I don't think the resources needed to be > >> released and allocated each time cpu becomes online. Something like > >> cpuidle_reset_device() would be enough to clear and reinitialize the > >> data. > >> > >> However, after some studying of the data to be cleared, I think it's > >> also reasonable to keep the previous data. For example: > >> > >> /sys/devices/system/cpu/cpu#/cpuidle/state#/usage > >> the number of times this idle state has been entered > >> /sys/devices/system/cpu/cpu#/cpuidle/state#/time > >> the amount of time spent in this idle state > >> > >> So I think we could just remove the function call doing the > >> disable/enable cycle: > >> > >> Please correct me if I missed anything. > > > If removed, this would not handle cpu hotplug events for cpuidle. > > > >> > >> Reported-by: Paul E. McKenney <paul...@linux.vnet.ibm.com> > >> Signed-off-by: Li Zhong <zh...@linux.vnet.ibm.com> > >> Tested-by: Paul E. McKenney <paul...@linux.vnet.ibm.com> > >> --- > >> arch/powerpc/platforms/pseries/smp.c | 1 - > >> 1 files changed, 0 insertions(+), 1 deletions(-) > >> > >> diff --git a/arch/powerpc/platforms/pseries/smp.c > >> b/arch/powerpc/platforms/pseries/smp.c > >> index e16bb8d..71706bc 100644 > >> --- a/arch/powerpc/platforms/pseries/smp.c > >> +++ b/arch/powerpc/platforms/pseries/smp.c > >> @@ -147,7 +147,6 @@ static void __devinit smp_xics_setup_cpu(int cpu) > >> set_cpu_current_state(cpu, CPU_STATE_ONLINE); > >> set_default_offline_state(cpu); > >> #endif > >> - pseries_notify_cpuidle_add_cpu(cpu); > >> } > >> > >> static int __devinit smp_pSeries_kick_cpu(int nr) > > > > > > > > Cheers, > Deepthi _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
