From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from e28smtp09.in.ibm.com (e28smtp09.in.ibm.com [122.248.162.9]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "e28smtp09.in.ibm.com", Issuer "GeoTrust SSL CA" (not verified)) by ozlabs.org (Postfix) with ESMTPS id B6DC8B6FB9 for ; Thu, 17 May 2012 20:22:49 +1000 (EST) Received: from /spool/local by e28smtp09.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 17 May 2012 15:52:09 +0530 Received: from d28av05.in.ibm.com (d28av05.in.ibm.com [9.184.220.67]) by d28relay04.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q4HAM6fI62128326 for ; Thu, 17 May 2012 15:52:06 +0530 Received: from d28av05.in.ibm.com (loopback [127.0.0.1]) by d28av05.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q4HFqYIg021542 for ; Fri, 18 May 2012 01:52:36 +1000 Message-ID: <4FB4D14A.20903@linux.vnet.ibm.com> Date: Thu, 17 May 2012 15:52:02 +0530 From: Deepthi Dharwar MIME-Version: 1.0 To: Benjamin Herrenschmidt Subject: Re: [PATCH powerpc] fix a lockdep complaint in start_secondary References: <1337227263.24471.23.camel@ThinkPad-T420> <1337228910.30558.46.camel@pasglop> In-Reply-To: <1337228910.30558.46.camel@pasglop> Content-Type: text/plain; charset=UTF-8 Cc: PowerPC email list , "Paul E. McKenney" , Paul Mackerras , LKML , Li Zhong List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 >> Signed-off-by: Li Zhong >> Tested-by: Paul E. McKenney >> --- >> 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