* [PATCH powerpc] fix a lockdep complaint in start_secondary @ 2012-05-17 4:01 Li Zhong 2012-05-17 4:28 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 6+ messages in thread From: Li Zhong @ 2012-05-17 4:01 UTC (permalink / raw) To: LKML Cc: Benjamin Herrenschmidt, Paul Mackerras, PowerPC email list, Paul E. McKenney This patch tries to fix following lockdep complaints: [ 81.882506] ================================= [ 81.882508] [ INFO: inconsistent lock state ] [ 81.882511] 3.4.0-rc4-autokern1 #1 Not tainted [ 81.882513] --------------------------------- [ 81.882516] inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage. [ 81.882519] swapper/5/0 [HC0[0]:SC0[0]:HE1:SE1] takes: [ 81.882522] (call_function.lock){?.....}, at: [<c0000000000fdaa0>] .ipi_call_lock+0x20/0x40 [ 81.882536] {IN-HARDIRQ-W} state was registered at: [ 81.882538] [<c0000000000f5a9c>] .__lock_acquire+0x44c/0x9e0 [ 81.882543] [<c0000000000f60f4>] .lock_acquire+0xc4/0x260 [ 81.882548] [<c00000000063f648>] ._raw_spin_lock+0x48/0x70 [ 81.882554] [<c0000000000fede4>] .generic_smp_call_function_interrupt+0x1d4/0x320 [ 81.882559] [<c000000000037020>] .smp_ipi_demux+0x90/0x100 [ 81.882565] [<c00000000004f98c>] .icp_hv_ipi_action+0x5c/0xc0 [ 81.882571] [<c00000000013420c>] .handle_irq_event_percpu +0xec/0x570 [ 81.882577] [<c000000000138ab4>] .handle_percpu_irq+0x84/0xd0 [ 81.882582] [<c0000000000221b4>] .call_handle_irq+0x1c/0x2c [ 81.882588] [<c0000000000103fc>] .do_IRQ+0x16c/0x500 [ 81.882593] [<c0000000000038d0>] hardware_interrupt_common +0x150/0x180 [ 81.882598] [<c000000000010a38>] .arch_local_irq_restore+0x38/0x90 [ 81.882603] [<c000000000017450>] .cpu_idle+0x250/0x2d0 [ 81.882607] [<c000000000651ce0>] .start_secondary+0x378/0x384 [ 81.882613] [<c00000000000936c>] .start_secondary_prolog+0x10/0x14 [ 81.882618] irq event stamp: 332475 [ 81.882620] hardirqs last enabled at (332475): [<c000000000640414>] ._raw_spin_unlock_irqrestore+0x94/0xc0 [ 81.882625] hardirqs last disabled at (332474): [<c00000000063f7c0>] ._raw_spin_lock_irqsave+0x30/0x90 [ 81.882631] softirqs last enabled at (332288): [<c0000000000873c4>] .irq_enter+0x94/0xd0 [ 81.882636] softirqs last disabled at (332287): [<c0000000000873b4>] .irq_enter+0x84/0xd0 [ 81.882640] [ 81.882641] other info that might help us debug this: [ 81.882644] Possible unsafe locking scenario: [ 81.882645] [ 81.882647] CPU0 [ 81.882649] ---- [ 81.882650] lock(call_function.lock); [ 81.882654] <Interrupt> [ 81.882656] lock(call_function.lock); [ 81.882660] [ 81.882661] *** DEADLOCK *** [ 81.882662] [ 81.882664] no locks held by swapper/5/0. [ 81.882666] [ 81.882667] stack backtrace: [ 81.882669] Call Trace: [ 81.882672] [c0000003c07bf860] [c0000000000146f4] .show_stack +0x74/0x1c0 (unreliable) [ 81.882678] [c0000003c07bf910] [c0000000000f1304] .print_usage_bug +0x1e4/0x230 [ 81.882683] [c0000003c07bf9d0] [c0000000000f150c] .mark_lock_irq +0x1bc/0x3c0 [ 81.882688] [c0000003c07bfa90] [c0000000000f18a0] .mark_lock +0x190/0x4b0 [ 81.882693] [c0000003c07bfb40] [c0000000000f1d10] .mark_irqflags +0x150/0x240 [ 81.882697] [c0000003c07bfbd0] [c0000000000f5a9c] .__lock_acquire +0x44c/0x9e0 [ 81.882702] [c0000003c07bfce0] [c0000000000f60f4] .lock_acquire +0xc4/0x260 [ 81.882707] [c0000003c07bfdc0] [c00000000063f648] ._raw_spin_lock +0x48/0x70 [ 81.882712] [c0000003c07bfe50] [c0000000000fdaa0] .ipi_call_lock +0x20/0x40 [ 81.882717] [c0000003c07bfed0] [c000000000651aa0] .start_secondary +0x138/0x384 [ 81.882722] [c0000003c07bff90] [c00000000000936c] .start_secondary_prolog+0x10/0x14 >From the log, ipi_call_lock() is called in start_secondary() with irqs enabled. The irqs are enabled by smp_ops->setup_cpu(), in following call chain: start_secondary --> smp_ops->setup_cpu --> smp_xics_setup_cpu --> pseries_notify_cpu_idle_add_cpu --> cpuidle_disable_device --> cpuidle_remove_state_sysfs --> cpuidle_free_state_kobj --> wait_for_completion --> wait_for_common >From my understanding of the codes, I think it's not necessary to call pseries_notify_cpu_idle_add_cpu() in the early start_secondary() function before irqs could be enabled. 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. 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. Reported-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> Tested-by: Paul E. McKenney <paulmck@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) -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH powerpc] fix a lockdep complaint in start_secondary 2012-05-17 4:01 [PATCH powerpc] fix a lockdep complaint in start_secondary Li Zhong @ 2012-05-17 4:28 ` Benjamin Herrenschmidt 2012-05-17 10:22 ` Deepthi Dharwar 0 siblings, 1 reply; 6+ messages in thread From: Benjamin Herrenschmidt @ 2012-05-17 4:28 UTC (permalink / raw) To: Li Zhong Cc: LKML, Paul Mackerras, PowerPC email list, Paul E. McKenney, deepthi 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. 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 ? > 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. > > Reported-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> > Tested-by: Paul E. McKenney <paulmck@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) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH powerpc] fix a lockdep complaint in start_secondary 2012-05-17 4:28 ` Benjamin Herrenschmidt @ 2012-05-17 10:22 ` Deepthi Dharwar 2012-05-18 2:54 ` Li Zhong 0 siblings, 1 reply; 6+ messages in thread From: Deepthi Dharwar @ 2012-05-17 10:22 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: Li Zhong, LKML, Paul Mackerras, PowerPC email list, Paul E. McKenney 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. 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 <paulmck@linux.vnet.ibm.com> >> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> >> Tested-by: Paul E. McKenney <paulmck@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH powerpc] fix a lockdep complaint in start_secondary 2012-05-17 10:22 ` Deepthi Dharwar @ 2012-05-18 2:54 ` Li Zhong 2012-05-18 11:24 ` Deepthi Dharwar 0 siblings, 1 reply; 6+ messages in thread From: Li Zhong @ 2012-05-18 2:54 UTC (permalink / raw) To: Deepthi Dharwar Cc: Benjamin Herrenschmidt, LKML, Paul Mackerras, PowerPC email list, Paul E. McKenney 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 <paulmck@linux.vnet.ibm.com> > >> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> > >> Tested-by: Paul E. McKenney <paulmck@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH powerpc] fix a lockdep complaint in start_secondary 2012-05-18 2:54 ` Li Zhong @ 2012-05-18 11:24 ` Deepthi Dharwar 2012-05-18 12:17 ` Li Zhong 0 siblings, 1 reply; 6+ messages in thread From: Deepthi Dharwar @ 2012-05-18 11:24 UTC (permalink / raw) To: Li Zhong Cc: Benjamin Herrenschmidt, LKML, Paul Mackerras, PowerPC email list, Paul E. McKenney 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. 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. 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 <paulmck@linux.vnet.ibm.com> >>>> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> >>>> Tested-by: Paul E. McKenney <paulmck@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 > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH powerpc] fix a lockdep complaint in start_secondary 2012-05-18 11:24 ` Deepthi Dharwar @ 2012-05-18 12:17 ` Li Zhong 0 siblings, 0 replies; 6+ messages in thread From: Li Zhong @ 2012-05-18 12:17 UTC (permalink / raw) To: Deepthi Dharwar Cc: Benjamin Herrenschmidt, LKML, Paul Mackerras, PowerPC email list, Paul E. McKenney 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 <paulmck@linux.vnet.ibm.com> > >>>> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> > >>>> Tested-by: Paul E. McKenney <paulmck@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 > > > > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-05-18 12:18 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-05-17 4:01 [PATCH powerpc] fix a lockdep complaint in start_secondary Li Zhong 2012-05-17 4:28 ` Benjamin Herrenschmidt 2012-05-17 10:22 ` Deepthi Dharwar 2012-05-18 2:54 ` Li Zhong 2012-05-18 11:24 ` Deepthi Dharwar 2012-05-18 12:17 ` Li Zhong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).