From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751033AbdE3LKZ (ORCPT ); Tue, 30 May 2017 07:10:25 -0400 Received: from mail-pf0-f195.google.com ([209.85.192.195]:35989 "EHLO mail-pf0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750890AbdE3LKW (ORCPT ); Tue, 30 May 2017 07:10:22 -0400 Date: Tue, 30 May 2017 21:10:06 +1000 From: Nicholas Piggin To: Gautham R Shenoy Cc: Michael Ellerman , Michael Neuling , Vaidyanathan Srinivasan , Shilpasri G Bhat , Akshay Adiga , Benjamin Herrenschmidt , linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 6/6] cpuidle-powernv: Allow Deep stop states that don't stop time Message-ID: <20170530211006.49ce9eff@roar.ozlabs.ibm.com> In-Reply-To: <20170530105055.GD8563@in.ibm.com> References: <3429547945465851cfcbf81f6e762037c395c8ac.1494585671.git.ego@linux.vnet.ibm.com> <20170530171357.4e0b87a7@roar.ozlabs.ibm.com> <20170530105055.GD8563@in.ibm.com> Organization: IBM X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 30 May 2017 16:20:55 +0530 Gautham R Shenoy wrote: > On Tue, May 30, 2017 at 05:13:57PM +1000, Nicholas Piggin wrote: > > On Tue, 16 May 2017 14:19:48 +0530 > > "Gautham R. Shenoy" wrote: > > > > > From: "Gautham R. Shenoy" > > > > > > The current code in the cpuidle-powernv intialization only allows deep > > > stop states (indicated by OPAL_PM_STOP_INST_DEEP) which lose timebase > > > (indicated by OPAL_PM_TIMEBASE_STOP). This assumption goes back to > > > POWER8 time where deep states used to lose the timebase. However, on > > > POWER9, we do have stop states that are deep (they lose hypervisor > > > state) but retain the timebase. > > > > > > Fix the initialization code in the cpuidle-powernv driver to allow > > > such deep states. > > > > > > Further, there is a bug in cpuidle-powernv driver with > > > CONFIG_TICK_ONESHOT=n where we end up incrementing the nr_idle_states > > > even if a platform idle state which loses time base was not added to > > > the cpuidle table. > > > > > > Fix this by ensuring that the nr_idle_states variable gets incremented > > > only when the platform idle state was added to the cpuidle table. > > > > Should this be a separate patch? Stable? > > Ok. Will send it out separately. Looks like mpe has merged this in next now. I just wonder if this particular bit would be relevant for POWER8 and therefore be a stable candidate? All the POWER9 idle fixes may not be suitable for stable. > > > Signed-off-by: Gautham R. Shenoy > > > --- > > > drivers/cpuidle/cpuidle-powernv.c | 16 ++++++++++------ > > > 1 file changed, 10 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c > > > index 12409a5..45eaf06 100644 > > > --- a/drivers/cpuidle/cpuidle-powernv.c > > > +++ b/drivers/cpuidle/cpuidle-powernv.c > > > @@ -354,6 +354,7 @@ static int powernv_add_idle_states(void) > > > > > > for (i = 0; i < dt_idle_states; i++) { > > > unsigned int exit_latency, target_residency; > > > + bool stops_timebase = false; > > > /* > > > * If an idle state has exit latency beyond > > > * POWERNV_THRESHOLD_LATENCY_NS then don't use it > > > @@ -381,6 +382,9 @@ static int powernv_add_idle_states(void) > > > } > > > } > > > > > > + if (flags[i] & OPAL_PM_TIMEBASE_STOP) > > > + stops_timebase = true; > > > + > > > /* > > > * For nap and fastsleep, use default target_residency > > > * values if f/w does not expose it. > > > @@ -392,8 +396,7 @@ static int powernv_add_idle_states(void) > > > add_powernv_state(nr_idle_states, "Nap", > > > CPUIDLE_FLAG_NONE, nap_loop, > > > target_residency, exit_latency, 0, 0); > > > - } else if ((flags[i] & OPAL_PM_STOP_INST_FAST) && > > > - !(flags[i] & OPAL_PM_TIMEBASE_STOP)) { > > > + } else if (has_stop_states && !stops_timebase) { > > > add_powernv_state(nr_idle_states, names[i], > > > CPUIDLE_FLAG_NONE, stop_loop, > > > target_residency, exit_latency, > > > @@ -405,8 +408,8 @@ static int powernv_add_idle_states(void) > > > * within this config dependency check. > > > */ > > > #ifdef CONFIG_TICK_ONESHOT > > > - if (flags[i] & OPAL_PM_SLEEP_ENABLED || > > > - flags[i] & OPAL_PM_SLEEP_ENABLED_ER1) { > > > + else if (flags[i] & OPAL_PM_SLEEP_ENABLED || > > > + flags[i] & OPAL_PM_SLEEP_ENABLED_ER1) { > > > > Hmm, seems okay but readability is isn't the best with the ifdef and > > mixing power8 and 9 cases IMO. > > > > Particularly with the nice regular POWER9 states, we're not doing much > > logic in this loop besides checking for the timebase stop flag, right? > > Would it be clearer if it was changed to something like this (untested > > quick hack)? > > Yes, this is very much doable. Some comments below. > > > > > --- > > drivers/cpuidle/cpuidle-powernv.c | 76 +++++++++++++++++++-------------------- > > 1 file changed, 37 insertions(+), 39 deletions(-) > > > > diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c > > index 12409a519cc5..77291389f9ac 100644 > > --- a/drivers/cpuidle/cpuidle-powernv.c > > +++ b/drivers/cpuidle/cpuidle-powernv.c > > @@ -353,7 +353,9 @@ static int powernv_add_idle_states(void) > > } > > > > for (i = 0; i < dt_idle_states; i++) { > > + unsigned int cpuidle_flags = CPUIDLE_FLAG_NONE; > > unsigned int exit_latency, target_residency; > > + > > /* > > * If an idle state has exit latency beyond > > * POWERNV_THRESHOLD_LATENCY_NS then don't use it > > @@ -371,6 +373,16 @@ static int powernv_add_idle_states(void) > > else > > target_residency = 0; > > > > + if (flags[i] & OPAL_PM_TIMEBASE_STOP) { > > + /* > > + * All cpuidle states with CPUIDLE_FLAG_TIMER_STOP set > > + * depend on CONFIG_TICK_ONESHOT. > > + */ > > + if (!IS_ENABLED(CONFIG_TICK_ONESHOT)) > > + continue; > > + cpuidle_flags = CPUIDLE_FLAG_TIMER_STOP; > > Yes, this can be done. We just need to extend this condition to > (flags[i] & OPAL_PM_TIMEBASE_STOP || flags[i] & > OPAL_PM_SLEEP_ENABLED). > > Even though all the recent versions of OPAL have OPAL_PM_TIMEBASE_STOP > set for states which have OPAL_PM_SLEEP_ENABLED, I am not sure if > that was always the case and which is why we have the fastsleep case > explicitly coded in the kernel. Okay that makes sense, thanks for explaining. Since your patches are now merged, it's a matter of preference, if you want to send any cleanup patches you can take any of my suggestions. Thanks for the fixes!