From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from e23smtp03.au.ibm.com (e23smtp03.au.ibm.com [202.81.31.145]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "e23smtp03.au.ibm.com", Issuer "GeoTrust SSL CA" (not verified)) by ozlabs.org (Postfix) with ESMTPS id 722472C027F for ; Wed, 24 Jul 2013 02:22:40 +1000 (EST) Received: from /spool/local by e23smtp03.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 24 Jul 2013 02:12:24 +1000 Received: from d23relay03.au.ibm.com (d23relay03.au.ibm.com [9.190.235.21]) by d23dlp02.au.ibm.com (Postfix) with ESMTP id 636F82BB004F for ; Wed, 24 Jul 2013 02:22:34 +1000 (EST) Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r6NGMO8t4784506 for ; Wed, 24 Jul 2013 02:22:24 +1000 Received: from d23av04.au.ibm.com (loopback [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r6NGMXuG027983 for ; Wed, 24 Jul 2013 02:22:33 +1000 Message-ID: <51EEAD93.8060300@linux.vnet.ibm.com> Date: Tue, 23 Jul 2013 21:51:39 +0530 From: Deepthi Dharwar MIME-Version: 1.0 To: Michael Ellerman Subject: Re: cpuidle/pseries: Fix kernel command line parameter smt-snooze-delay References: <51EE0C65.6020903@linux.vnet.ibm.com> <20130723140256.GH31944@concordia> In-Reply-To: <20130723140256.GH31944@concordia> Content-Type: text/plain; charset=ISO-8859-1 Cc: Preeti Murthy , PowerPC email list , "linux-kernel@vger.kernel.org" , "Srivatsa S. Bhat" List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On 07/23/2013 07:32 PM, Michael Ellerman wrote: > On Tue, Jul 23, 2013 at 10:23:57AM +0530, Deepthi Dharwar wrote: >> smt-snooze-delay is a tun-able provided currently on powerpc to delay the >> entry of an idle cpu to NAP state. By default, the value is 100us, >> which is entry criteria for NAP state i.e only if the idle period is >> above 100us it would enter NAP. Value of -1 disables entry into NAP. >> This value can be set either through sysfs, ppc64_cpu util or by >> passing it via kernel command line. Currently this feature is broken >> when the value is passed via the kernel command line. > > Has it always been broken? Or just since some commit? Yes, it is broken since inclusion of pseries back-end driver, around 3.3 kernel time-frame. >> This patch aims to fix this, by taking the appropriate action >> based on the value after the pseries driver is registered. >> This check is carried on in the back-end driver rather than >> setup_smt_snooze_delay(), as one is not sure if the cpuidle driver >> is even registered when setup routine is executed. >> Also, this fixes re-enabling of NAP states by setting appropriate >> value without having to reboot using smt-snooze-delay parameter. >> >> Also, to note is, smt-snooze-delay is per-cpu variable. >> This can be used to enable/disable NAP on per-cpu >> basis using sysfs but when this variable is passed >> via kernel command line or using the smt-snooze-delay >> it applies to all the cpus. Per-cpu tuning can >> only be done via sysfs. >> >> Signed-off-by: Deepthi Dharwar >> --- >> arch/powerpc/platforms/pseries/processor_idle.c | 34 ++++++++++++++++++----- >> 1 file changed, 27 insertions(+), 7 deletions(-) >> >> diff --git a/arch/powerpc/platforms/pseries/processor_idle.c b/arch/powerpc/platforms/pseries/processor_idle.c >> index 4644efa0..8133f50 100644 >> --- a/arch/powerpc/platforms/pseries/processor_idle.c >> +++ b/arch/powerpc/platforms/pseries/processor_idle.c >> @@ -170,18 +170,36 @@ static struct cpuidle_state shared_states[MAX_IDLE_STATE_COUNT] = { >> void update_smt_snooze_delay(int cpu, int residency) >> { >> struct cpuidle_driver *drv = cpuidle_get_driver(); >> - struct cpuidle_device *dev = per_cpu(cpuidle_devices, cpu); >> + struct cpuidle_device *dev; >> >> if (cpuidle_state_table != dedicated_states) >> return; >> >> - if (residency < 0) { >> - /* Disable the Nap state on that cpu */ >> - if (dev) >> - dev->states_usage[1].disable = 1; >> - } else >> - if (drv) >> + if (!drv) >> + return; >> + >> + if (cpu == -1) { >> + if (residency < 0) { >> + /* Disable NAP on all cpus */ >> + drv->states[1].disabled = true; >> + } else { >> drv->states[1].target_residency = residency; >> + drv->states[1].disabled = false; >> + } >> + return; >> + } >> + >> + dev = per_cpu(cpuidle_devices, cpu); >> + if (!dev) >> + return; >> + >> + if (residency < 0) >> + dev->states_usage[1].disable = 1; >> + else { >> + drv->states[1].target_residency = residency; >> + drv->states[1].disabled = false; >> + dev->states_usage[1].disable = 0; > > Why are we indexing into all these array with '1' ? We are disabling/enabling NAP state, which indexes into pseries cpuidle state table array with index value of 1. smt-snooze-delay of -1, disables NAP state. Else, we need to set the residency of NAP state i.e minimum time CPU should spend in NAP state ( based on which menu governor takes a decision on selection of Idle state) there by delaying the entry to NAP state. Thanks for the review. Regards, Deepthi > cheers >