Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1653111
| From | Gautham R Shenoy <ego@linux.vnet.ibm.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH 6/6] cpuidle-powernv: Allow Deep stop states that don't stop time |
| Date | 2017-05-30 13:00 +0200 |
| Message-ID | <tMN2x-6he-5@gated-at.bofh.it> (permalink) |
| References | <tHGuJ-3oA-3@gated-at.bofh.it> <tHGuK-3oA-11@gated-at.bofh.it> <tMJBD-49a-15@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
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" <ego@linux.vnet.ibm.com> wrote:
>
> > From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
> >
> > 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.
>
> >
> > Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> > ---
> > 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.
> + }
> +
> if (has_stop_states) {
> int err = validate_psscr_val_mask(&psscr_val[i],
> &psscr_mask[i],
> @@ -379,50 +391,36 @@ static int powernv_add_idle_states(void)
> report_invalid_psscr_val(psscr_val[i], err);
> continue;
> }
> - }
>
> - /*
> - * For nap and fastsleep, use default target_residency
> - * values if f/w does not expose it.
> - */
> - if (flags[i] & OPAL_PM_NAP_ENABLED) {
> - if (!rc)
> - target_residency = 100;
> - /* Add NAP state */
> - 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)) {
> add_powernv_state(nr_idle_states, names[i],
> - CPUIDLE_FLAG_NONE, stop_loop,
> - target_residency, exit_latency,
> - psscr_val[i], psscr_mask[i]);
> - }
> -
> - /*
> - * All cpuidle states with CPUIDLE_FLAG_TIMER_STOP set must come
> - * within this config dependency check.
> - */
> -#ifdef CONFIG_TICK_ONESHOT
> - if (flags[i] & OPAL_PM_SLEEP_ENABLED ||
> - flags[i] & OPAL_PM_SLEEP_ENABLED_ER1) {
> - if (!rc)
> - target_residency = 300000;
> - /* Add FASTSLEEP state */
> - add_powernv_state(nr_idle_states, "FastSleep",
> - CPUIDLE_FLAG_TIMER_STOP,
> - fastsleep_loop,
> - target_residency, exit_latency, 0, 0);
> - } else if ((flags[i] & OPAL_PM_STOP_INST_DEEP) &&
> - (flags[i] & OPAL_PM_TIMEBASE_STOP)) {
> - add_powernv_state(nr_idle_states, names[i],
> - CPUIDLE_FLAG_TIMER_STOP, stop_loop,
> + cpuidle_flags, stop_loop,
> target_residency, exit_latency,
> psscr_val[i], psscr_mask[i]);
> + nr_idle_states++;
> + } else {
> + /*
> + * For nap and fastsleep, use default target_residency
> + * values if f/w does not expose it.
> + */
> + if (flags[i] & OPAL_PM_NAP_ENABLED) {
> + if (!rc)
> + target_residency = 100;
> + /* Add NAP state */
> + add_powernv_state(nr_idle_states, "Nap",
> + cpuidle_flags, nap_loop,
> + target_residency, exit_latency, 0, 0);
> + nr_idle_states++;
> + } else if (flags[i] & (OPAL_PM_SLEEP_ENABLED |
> + OPAL_PM_SLEEP_ENABLED_ER1)) {
> + if (!rc)
> + target_residency = 300000;
> + /* Add FASTSLEEP state */
> + add_powernv_state(nr_idle_states, "FastSleep",
> + cpuidle_flags, fastsleep_loop,
> + target_residency, exit_latency, 0, 0);
> + nr_idle_states++;
> + }
> }
> -#endif
> - nr_idle_states++;
> }
> out:
> return nr_idle_states;
> --
> 2.11.0
>
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
Re: [PATCH 6/6] cpuidle-powernv: Allow Deep stop states that don't stop time Nicholas Piggin <npiggin@gmail.com> - 2017-05-30 09:20 +0200
Re: [PATCH 6/6] cpuidle-powernv: Allow Deep stop states that don't stop time Gautham R Shenoy <ego@linux.vnet.ibm.com> - 2017-05-30 13:00 +0200
Re: [PATCH 6/6] cpuidle-powernv: Allow Deep stop states that don't stop time Nicholas Piggin <npiggin@gmail.com> - 2017-05-30 13:20 +0200
Re: [PATCH 6/6] cpuidle-powernv: Allow Deep stop states that don't stop time Gautham R Shenoy <ego@linux.vnet.ibm.com> - 2017-05-31 10:50 +0200
csiph-web