On Fri, 2012-05-18 at 16:54 +0530, Deepthi Dharwar wrote: > On 05/18/2012 08:24 AM, Li Zhong wrote: > > > 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? > > > If cpuidle_idle_call() fails, in case device is not present, off , > not initialized and not ready to use, default idle is called. > Coming out of a hotplug event, it is good to cleanly exit out > and reallocate all the resources when needed, rather than using the > stale one to make sure this call succeeds always. > > Default idle executed in pSeries_idle() : > HMT_low(); > HMT_very_low(); > This would not have much powersavings.
>From my testing, cpuidle_idle_call didn't fail after hotplug, so it didn't fall back to the default idling. I still don't see any big problems if we don't reallocate the resources. > > CPUIDLE subsystem needs to be informed when a hot plug event occurs > and not a good practice to mask this subsystem from this system wide > event. Ok, I agree that the CPUIDLE subsystem should be notified about the hot plug events. Thank you. I think this would be included in your coming patch, and I could just stop here, hehe > > I agree that putting it in xics setup is not a good thing. > Notifier would be a cleaner way of doing it. > That way, duplication of resources allocated and released at boot > time is not done. > > > > > >> 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