linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] cpuidle:powernv: Make the snooze timeout dynamic.
@ 2018-05-31 12:15 Gautham R. Shenoy
  2018-05-31 14:51 ` Balbir Singh
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Gautham R. Shenoy @ 2018-05-31 12:15 UTC (permalink / raw)
  To: Rafael J. Wysocki, Daniel Lezcano, Michael Ellerman,
	Stewart Smith, Michael Neuling, Vaidyanathan Srinivasan,
	Shilpasri G Bhat, Akshay Adiga, Nicholas Piggin
  Cc: linuxppc-dev, linux-kernel, linux-pm, Gautham R. Shenoy

From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>

The commit 78eaa10f027c ("cpuidle: powernv/pseries: Auto-promotion of
snooze to deeper idle state") introduced a timeout for the snooze idle
state so that it could be eventually be promoted to a deeper idle
state. The snooze timeout value is static and set to the target
residency of the next idle state, which would train the cpuidle
governor to pick the next idle state eventually.

The unfortunate side-effect of this is that if the next idle state(s)
is disabled, the CPU will forever remain in snooze, despite the fact
that the system is completely idle, and other deeper idle states are
available.

This patch fixes the issue by dynamically setting the snooze timeout
to the target residency of the next enabled state on the device.

Before Patch
==================
POWER8 : Only nap disabled.
 $cpupower monitor sleep 30
sleep took 30.01297 seconds and exited with status 0
              |Idle_Stats
PKG |CORE|CPU | snoo | Nap  | Fast
   0|   8|   0| 96.41|  0.00|  0.00
   0|   8|   1| 96.43|  0.00|  0.00
   0|   8|   2| 96.47|  0.00|  0.00
   0|   8|   3| 96.35|  0.00|  0.00
   0|   8|   4| 96.37|  0.00|  0.00
   0|   8|   5| 96.37|  0.00|  0.00
   0|   8|   6| 96.47|  0.00|  0.00
   0|   8|   7| 96.47|  0.00|  0.00

POWER9: Shallow states (stop0lite, stop1lite, stop2lite, stop0, stop1,
stop2) disabled:
$cpupower monitor sleep 30
sleep took 30.05033 seconds and exited with status 0
              |Idle_Stats
PKG |CORE|CPU | snoo | stop | stop | stop | stop | stop | stop | stop | stop
   0|  16|   0| 89.79|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00
   0|  16|   1| 90.12|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00
   0|  16|   2| 90.21|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00
   0|  16|   3| 90.29|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00

After Patch
======================
POWER8 : Only nap disabled.
$ cpupower monitor sleep 30
sleep took 30.01200 seconds and exited with status 0
              |Idle_Stats
PKG |CORE|CPU | snoo | Nap  | Fast
   0|   8|   0| 16.58|  0.00| 77.21
   0|   8|   1| 18.42|  0.00| 75.38
   0|   8|   2|  4.70|  0.00| 94.09
   0|   8|   3| 17.06|  0.00| 81.73
   0|   8|   4|  3.06|  0.00| 95.73
   0|   8|   5|  7.00|  0.00| 96.80
   0|   8|   6|  1.00|  0.00| 98.79
   0|   8|   7|  5.62|  0.00| 94.17

POWER9: Shallow states (stop0lite, stop1lite, stop2lite, stop0, stop1,
stop2) disabled:

$cpupower monitor sleep 30
sleep took 30.02110 seconds and exited with status 0
              |Idle_Stats
PKG |CORE|CPU | snoo | stop | stop | stop | stop | stop | stop | stop | stop
   0|   0|   0|  0.69|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  9.39| 89.70
   0|   0|   1|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.05| 93.21
   0|   0|   2|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00| 89.93
   0|   0|   3|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00| 93.26

Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
 drivers/cpuidle/cpuidle-powernv.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
index 1a8234e..d29e4f0 100644
--- a/drivers/cpuidle/cpuidle-powernv.c
+++ b/drivers/cpuidle/cpuidle-powernv.c
@@ -43,9 +43,31 @@ struct stop_psscr_table {
 
 static struct stop_psscr_table stop_psscr_table[CPUIDLE_STATE_MAX] __read_mostly;
 
-static u64 snooze_timeout __read_mostly;
+static u64 default_snooze_timeout __read_mostly;
 static bool snooze_timeout_en __read_mostly;
 
+static u64 get_snooze_timeout(struct cpuidle_device *dev,
+			      struct cpuidle_driver *drv,
+			      int index)
+{
+	int i;
+
+	if (unlikely(!snooze_timeout_en))
+		return default_snooze_timeout;
+
+	for (i = index + 1; i < drv->state_count; i++) {
+		struct cpuidle_state *s = &drv->states[i];
+		struct cpuidle_state_usage *su = &dev->states_usage[i];
+
+		if (s->disabled || su->disable)
+			continue;
+
+		return s->target_residency * tb_ticks_per_usec;
+	}
+
+	return default_snooze_timeout;
+}
+
 static int snooze_loop(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv,
 			int index)
@@ -56,7 +78,7 @@ static int snooze_loop(struct cpuidle_device *dev,
 
 	local_irq_enable();
 
-	snooze_exit_time = get_tb() + snooze_timeout;
+	snooze_exit_time = get_tb() + get_snooze_timeout(dev, drv, index);
 	ppc64_runlatch_off();
 	HMT_very_low();
 	while (!need_resched()) {
@@ -465,11 +487,9 @@ static int powernv_idle_probe(void)
 		cpuidle_state_table = powernv_states;
 		/* Device tree can indicate more idle states */
 		max_idle_state = powernv_add_idle_states();
-		if (max_idle_state > 1) {
+		default_snooze_timeout = TICK_USEC * tb_ticks_per_usec;
+		if (max_idle_state > 1)
 			snooze_timeout_en = true;
-			snooze_timeout = powernv_states[1].target_residency *
-					 tb_ticks_per_usec;
-		}
  	} else
  		return -ENODEV;
 
-- 
1.9.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] cpuidle:powernv: Make the snooze timeout dynamic.
  2018-05-31 12:15 [PATCH] cpuidle:powernv: Make the snooze timeout dynamic Gautham R. Shenoy
@ 2018-05-31 14:51 ` Balbir Singh
  2018-06-01  4:54   ` Gautham R Shenoy
  2018-06-04 11:27 ` Michael Ellerman
  2018-06-05 15:19 ` Michael Ellerman
  2 siblings, 1 reply; 9+ messages in thread
From: Balbir Singh @ 2018-05-31 14:51 UTC (permalink / raw)
  To: Gautham R. Shenoy
  Cc: Rafael J. Wysocki, Daniel Lezcano, Michael Ellerman,
	Stewart Smith, Michael Neuling, Vaidyanathan Srinivasan,
	Shilpasri G Bhat, Akshay Adiga, Nicholas Piggin,
	open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
	linux-kernel, linux-pm

On Thu, May 31, 2018 at 10:15 PM, Gautham R. Shenoy
<ego@linux.vnet.ibm.com> wrote:
> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
>
> The commit 78eaa10f027c ("cpuidle: powernv/pseries: Auto-promotion of
> snooze to deeper idle state") introduced a timeout for the snooze idle
> state so that it could be eventually be promoted to a deeper idle
> state. The snooze timeout value is static and set to the target
> residency of the next idle state, which would train the cpuidle
> governor to pick the next idle state eventually.
>
> The unfortunate side-effect of this is that if the next idle state(s)
> is disabled, the CPU will forever remain in snooze, despite the fact
> that the system is completely idle, and other deeper idle states are
> available.
>
> This patch fixes the issue by dynamically setting the snooze timeout
> to the target residency of the next enabled state on the device.
>
> Before Patch
> ==================
> POWER8 : Only nap disabled.
>  $cpupower monitor sleep 30
> sleep took 30.01297 seconds and exited with status 0
>               |Idle_Stats
> PKG |CORE|CPU | snoo | Nap  | Fast
>    0|   8|   0| 96.41|  0.00|  0.00
>    0|   8|   1| 96.43|  0.00|  0.00
>    0|   8|   2| 96.47|  0.00|  0.00
>    0|   8|   3| 96.35|  0.00|  0.00
>    0|   8|   4| 96.37|  0.00|  0.00
>    0|   8|   5| 96.37|  0.00|  0.00
>    0|   8|   6| 96.47|  0.00|  0.00
>    0|   8|   7| 96.47|  0.00|  0.00
>
> POWER9: Shallow states (stop0lite, stop1lite, stop2lite, stop0, stop1,
> stop2) disabled:
> $cpupower monitor sleep 30
> sleep took 30.05033 seconds and exited with status 0
>               |Idle_Stats
> PKG |CORE|CPU | snoo | stop | stop | stop | stop | stop | stop | stop | stop
>    0|  16|   0| 89.79|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00
>    0|  16|   1| 90.12|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00
>    0|  16|   2| 90.21|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00
>    0|  16|   3| 90.29|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00
>
> After Patch
> ======================
> POWER8 : Only nap disabled.
> $ cpupower monitor sleep 30
> sleep took 30.01200 seconds and exited with status 0
>               |Idle_Stats
> PKG |CORE|CPU | snoo | Nap  | Fast
>    0|   8|   0| 16.58|  0.00| 77.21
>    0|   8|   1| 18.42|  0.00| 75.38
>    0|   8|   2|  4.70|  0.00| 94.09
>    0|   8|   3| 17.06|  0.00| 81.73
>    0|   8|   4|  3.06|  0.00| 95.73
>    0|   8|   5|  7.00|  0.00| 96.80
>    0|   8|   6|  1.00|  0.00| 98.79
>    0|   8|   7|  5.62|  0.00| 94.17
>
> POWER9: Shallow states (stop0lite, stop1lite, stop2lite, stop0, stop1,
> stop2) disabled:
>
> $cpupower monitor sleep 30
> sleep took 30.02110 seconds and exited with status 0
>               |Idle_Stats
> PKG |CORE|CPU | snoo | stop | stop | stop | stop | stop | stop | stop | stop
>    0|   0|   0|  0.69|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  9.39| 89.70
>    0|   0|   1|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.05| 93.21
>    0|   0|   2|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00| 89.93
>    0|   0|   3|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00| 93.26
>
> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> ---
>  drivers/cpuidle/cpuidle-powernv.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
> index 1a8234e..d29e4f0 100644
> --- a/drivers/cpuidle/cpuidle-powernv.c
> +++ b/drivers/cpuidle/cpuidle-powernv.c
> @@ -43,9 +43,31 @@ struct stop_psscr_table {
>
>  static struct stop_psscr_table stop_psscr_table[CPUIDLE_STATE_MAX] __read_mostly;
>
> -static u64 snooze_timeout __read_mostly;
> +static u64 default_snooze_timeout __read_mostly;
>  static bool snooze_timeout_en __read_mostly;
>
> +static u64 get_snooze_timeout(struct cpuidle_device *dev,
> +                             struct cpuidle_driver *drv,
> +                             int index)
> +{
> +       int i;
> +
> +       if (unlikely(!snooze_timeout_en))
> +               return default_snooze_timeout;
> +
> +       for (i = index + 1; i < drv->state_count; i++) {
> +               struct cpuidle_state *s = &drv->states[i];
> +               struct cpuidle_state_usage *su = &dev->states_usage[i];
> +
> +               if (s->disabled || su->disable)
> +                       continue;
> +
> +               return s->target_residency * tb_ticks_per_usec;

Can we ensure this is not prone to overflow?

Otherwise looks good

Reviewed-by: Balbir Singh <bsingharora@gmail.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] cpuidle:powernv: Make the snooze timeout dynamic.
  2018-05-31 14:51 ` Balbir Singh
@ 2018-06-01  4:54   ` Gautham R Shenoy
  2018-06-02  0:16     ` Balbir Singh
  0 siblings, 1 reply; 9+ messages in thread
From: Gautham R Shenoy @ 2018-06-01  4:54 UTC (permalink / raw)
  To: Balbir Singh
  Cc: Gautham R. Shenoy, Rafael J. Wysocki, Daniel Lezcano,
	Michael Ellerman, Stewart Smith, Michael Neuling,
	Vaidyanathan Srinivasan, Shilpasri G Bhat, Akshay Adiga,
	Nicholas Piggin, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
	linux-kernel, linux-pm

Hi Balbir,

Thanks for reviewing the patch!

On Fri, Jun 01, 2018 at 12:51:05AM +1000, Balbir Singh wrote:
> On Thu, May 31, 2018 at 10:15 PM, Gautham R. Shenoy

[..snip..]
> >
> > +static u64 get_snooze_timeout(struct cpuidle_device *dev,
> > +                             struct cpuidle_driver *drv,
> > +                             int index)
> > +{
> > +       int i;
> > +
> > +       if (unlikely(!snooze_timeout_en))
> > +               return default_snooze_timeout;
> > +
> > +       for (i = index + 1; i < drv->state_count; i++) {
> > +               struct cpuidle_state *s = &drv->states[i];
> > +               struct cpuidle_state_usage *su = &dev->states_usage[i];
> > +
> > +               if (s->disabled || su->disable)
> > +                       continue;
> > +
> > +               return s->target_residency * tb_ticks_per_usec;
> 
> Can we ensure this is not prone to overflow?

s->target_residency is an "unsigned int" so can take a maximum value
of UINT_MAX. tb_ticks_per_usec is an "unsigned long" with a value in
the range of 100-1000. The return value is a u64. The product of
s->target_residency and tb_ticks_per_usec should be contained in u64.

Is there a potential case of overflow that I am missing ?

> 
> Otherwise looks good
> 
> Reviewed-by: Balbir Singh <bsingharora@gmail.com>

--
Thanks and Regards
gautham.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] cpuidle:powernv: Make the snooze timeout dynamic.
  2018-06-01  4:54   ` Gautham R Shenoy
@ 2018-06-02  0:16     ` Balbir Singh
  0 siblings, 0 replies; 9+ messages in thread
From: Balbir Singh @ 2018-06-02  0:16 UTC (permalink / raw)
  To: Gautham R Shenoy
  Cc: Rafael J. Wysocki, Daniel Lezcano, Michael Ellerman,
	Stewart Smith, Michael Neuling, Vaidyanathan Srinivasan,
	Shilpasri G Bhat, Akshay Adiga, Nicholas Piggin,
	open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
	linux-kernel, linux-pm

On Fri, Jun 1, 2018 at 2:54 PM, Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:
> Hi Balbir,
>
> Thanks for reviewing the patch!
>
> On Fri, Jun 01, 2018 at 12:51:05AM +1000, Balbir Singh wrote:
>> On Thu, May 31, 2018 at 10:15 PM, Gautham R. Shenoy
>
> [..snip..]
>> >
>> > +static u64 get_snooze_timeout(struct cpuidle_device *dev,
>> > +                             struct cpuidle_driver *drv,
>> > +                             int index)
>> > +{
>> > +       int i;
>> > +
>> > +       if (unlikely(!snooze_timeout_en))
>> > +               return default_snooze_timeout;
>> > +
>> > +       for (i = index + 1; i < drv->state_count; i++) {
>> > +               struct cpuidle_state *s = &drv->states[i];
>> > +               struct cpuidle_state_usage *su = &dev->states_usage[i];
>> > +
>> > +               if (s->disabled || su->disable)
>> > +                       continue;
>> > +
>> > +               return s->target_residency * tb_ticks_per_usec;
>>
>> Can we ensure this is not prone to overflow?
>
> s->target_residency is an "unsigned int" so can take a maximum value
> of UINT_MAX. tb_ticks_per_usec is an "unsigned long" with a value in
> the range of 100-1000. The return value is a u64. The product of
> s->target_residency and tb_ticks_per_usec should be contained in u64.
>

Fair enough, looks reasonable to me

Balbir Singh.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] cpuidle:powernv: Make the snooze timeout dynamic.
  2018-05-31 12:15 [PATCH] cpuidle:powernv: Make the snooze timeout dynamic Gautham R. Shenoy
  2018-05-31 14:51 ` Balbir Singh
@ 2018-06-04 11:27 ` Michael Ellerman
  2018-06-05  3:47   ` Stewart Smith
  2018-06-05  8:45   ` Gautham R Shenoy
  2018-06-05 15:19 ` Michael Ellerman
  2 siblings, 2 replies; 9+ messages in thread
From: Michael Ellerman @ 2018-06-04 11:27 UTC (permalink / raw)
  To: Gautham R. Shenoy, Rafael J. Wysocki, Daniel Lezcano,
	Stewart Smith, Michael Neuling, Vaidyanathan Srinivasan,
	Shilpasri G Bhat, Akshay Adiga, Nicholas Piggin
  Cc: linuxppc-dev, linux-kernel, linux-pm, Gautham R. Shenoy

"Gautham R. Shenoy" <ego@linux.vnet.ibm.com> writes:

> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
>
> The commit 78eaa10f027c ("cpuidle: powernv/pseries: Auto-promotion of
> snooze to deeper idle state") introduced a timeout for the snooze idle
> state so that it could be eventually be promoted to a deeper idle
> state. The snooze timeout value is static and set to the target
> residency of the next idle state, which would train the cpuidle
> governor to pick the next idle state eventually.
>
> The unfortunate side-effect of this is that if the next idle state(s)
> is disabled, the CPU will forever remain in snooze, despite the fact
> that the system is completely idle, and other deeper idle states are
> available.

That sounds like a bug, I'll add?

Fixes: 78eaa10f027c ("cpuidle: powernv/pseries: Auto-promotion of snooze to deeper idle state")
Cc: stable@vger.kernel.org # v4.2+

cheers

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] cpuidle:powernv: Make the snooze timeout dynamic.
  2018-06-04 11:27 ` Michael Ellerman
@ 2018-06-05  3:47   ` Stewart Smith
  2018-06-05  8:45   ` Gautham R Shenoy
  1 sibling, 0 replies; 9+ messages in thread
From: Stewart Smith @ 2018-06-05  3:47 UTC (permalink / raw)
  To: Michael Ellerman, Gautham R. Shenoy, Rafael J. Wysocki,
	Daniel Lezcano, Michael Neuling, Vaidyanathan Srinivasan,
	Shilpasri G Bhat, Akshay Adiga, Nicholas Piggin
  Cc: linuxppc-dev, linux-kernel, linux-pm, Gautham R. Shenoy

Michael Ellerman <mpe@ellerman.id.au> writes:
> "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> writes:
>
>> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
>>
>> The commit 78eaa10f027c ("cpuidle: powernv/pseries: Auto-promotion of
>> snooze to deeper idle state") introduced a timeout for the snooze idle
>> state so that it could be eventually be promoted to a deeper idle
>> state. The snooze timeout value is static and set to the target
>> residency of the next idle state, which would train the cpuidle
>> governor to pick the next idle state eventually.
>>
>> The unfortunate side-effect of this is that if the next idle state(s)
>> is disabled, the CPU will forever remain in snooze, despite the fact
>> that the system is completely idle, and other deeper idle states are
>> available.
>
> That sounds like a bug, I'll add?
>
> Fixes: 78eaa10f027c ("cpuidle: powernv/pseries: Auto-promotion of snooze to deeper idle state")
> Cc: stable@vger.kernel.org # v4.2+

Yes, it's a bug - we had a customer bug because we lacked this that
meant we had to do firmware changes rather than just tweaking what stop
states were used.

-- 
Stewart Smith
OPAL Architect, IBM.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] cpuidle:powernv: Make the snooze timeout dynamic.
  2018-06-04 11:27 ` Michael Ellerman
  2018-06-05  3:47   ` Stewart Smith
@ 2018-06-05  8:45   ` Gautham R Shenoy
  2018-06-05 12:18     ` Michael Ellerman
  1 sibling, 1 reply; 9+ messages in thread
From: Gautham R Shenoy @ 2018-06-05  8:45 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Gautham R. Shenoy, Rafael J. Wysocki, Daniel Lezcano,
	Stewart Smith, Michael Neuling, Vaidyanathan Srinivasan,
	Shilpasri G Bhat, Akshay Adiga, Nicholas Piggin, linuxppc-dev,
	linux-kernel, linux-pm

Hello Michael,

On Mon, Jun 04, 2018 at 09:27:40PM +1000, Michael Ellerman wrote:
> "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> writes:
> 
> > From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
> >
> > The commit 78eaa10f027c ("cpuidle: powernv/pseries: Auto-promotion of
> > snooze to deeper idle state") introduced a timeout for the snooze idle
> > state so that it could be eventually be promoted to a deeper idle
> > state. The snooze timeout value is static and set to the target
> > residency of the next idle state, which would train the cpuidle
> > governor to pick the next idle state eventually.
> >
> > The unfortunate side-effect of this is that if the next idle state(s)
> > is disabled, the CPU will forever remain in snooze, despite the fact
> > that the system is completely idle, and other deeper idle states are
> > available.
> 
> That sounds like a bug, I'll add?
>

Yes, this is a bug-fix for a customer scenario which we encountered
recently.

> Fixes: 78eaa10f027c ("cpuidle: powernv/pseries: Auto-promotion of snooze to deeper idle state")
> Cc: stable@vger.kernel.org # v4.2+

This patch applies cleanly from v4.13 onwards. Prior to that there are
some (minor) conflicts.

Should I spin a version separately for the prior stable versions ?

> 
> cheers
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] cpuidle:powernv: Make the snooze timeout dynamic.
  2018-06-05  8:45   ` Gautham R Shenoy
@ 2018-06-05 12:18     ` Michael Ellerman
  0 siblings, 0 replies; 9+ messages in thread
From: Michael Ellerman @ 2018-06-05 12:18 UTC (permalink / raw)
  To: Gautham R Shenoy
  Cc: Gautham R. Shenoy, Rafael J. Wysocki, Daniel Lezcano,
	Stewart Smith, Michael Neuling, Vaidyanathan Srinivasan,
	Shilpasri G Bhat, Akshay Adiga, Nicholas Piggin, linuxppc-dev,
	linux-kernel, linux-pm

Gautham R Shenoy <ego@linux.vnet.ibm.com> writes:
> Hello Michael,
>
> On Mon, Jun 04, 2018 at 09:27:40PM +1000, Michael Ellerman wrote:
>> "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> writes:
>> 
>> > From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
>> >
>> > The commit 78eaa10f027c ("cpuidle: powernv/pseries: Auto-promotion of
>> > snooze to deeper idle state") introduced a timeout for the snooze idle
>> > state so that it could be eventually be promoted to a deeper idle
>> > state. The snooze timeout value is static and set to the target
>> > residency of the next idle state, which would train the cpuidle
>> > governor to pick the next idle state eventually.
>> >
>> > The unfortunate side-effect of this is that if the next idle state(s)
>> > is disabled, the CPU will forever remain in snooze, despite the fact
>> > that the system is completely idle, and other deeper idle states are
>> > available.
>> 
>> That sounds like a bug, I'll add?
>
> Yes, this is a bug-fix for a customer scenario which we encountered
> recently.

OK, the change log could have used some more scary words to make that
clearer ;)

I changed the subject to:

  cpuidle: powernv: Fix promotion from snooze if next state disabled

Which hopefully makes sense.

>> Fixes: 78eaa10f027c ("cpuidle: powernv/pseries: Auto-promotion of snooze to deeper idle state")
>> Cc: stable@vger.kernel.org # v4.2+
>
> This patch applies cleanly from v4.13 onwards. Prior to that there are
> some (minor) conflicts.
>
> Should I spin a version separately for the prior stable versions ?

Yes please, that would be great.

You might want to avoid "=====" in the change log too, as patchwork and
possibly other tools will think it's part of the diff.

cheers

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: cpuidle:powernv: Make the snooze timeout dynamic.
  2018-05-31 12:15 [PATCH] cpuidle:powernv: Make the snooze timeout dynamic Gautham R. Shenoy
  2018-05-31 14:51 ` Balbir Singh
  2018-06-04 11:27 ` Michael Ellerman
@ 2018-06-05 15:19 ` Michael Ellerman
  2 siblings, 0 replies; 9+ messages in thread
From: Michael Ellerman @ 2018-06-05 15:19 UTC (permalink / raw)
  To: Gautham R. Shenoy, Rafael J. Wysocki, Daniel Lezcano,
	Stewart Smith, Michael Neuling, Vaidyanathan Srinivasan,
	Shilpasri G Bhat, Akshay Adiga, Nicholas Piggin
  Cc: Gautham R. Shenoy, linuxppc-dev, linux-kernel, linux-pm

On Thu, 2018-05-31 at 12:15:09 UTC, "Gautham R. Shenoy" wrote:
> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
> 
> The commit 78eaa10f027c ("cpuidle: powernv/pseries: Auto-promotion of
> snooze to deeper idle state") introduced a timeout for the snooze idle
> state so that it could be eventually be promoted to a deeper idle
> state. The snooze timeout value is static and set to the target
> residency of the next idle state, which would train the cpuidle
> governor to pick the next idle state eventually.
> 
> The unfortunate side-effect of this is that if the next idle state(s)
> is disabled, the CPU will forever remain in snooze, despite the fact
> that the system is completely idle, and other deeper idle states are
> available.
> 
> This patch fixes the issue by dynamically setting the snooze timeout
> to the target residency of the next enabled state on the device.
> 
> Before Patch
> Reviewed-by: Balbir Singh <bsingharora@gmail.com>
> 
> ==================
> POWER8 : Only nap disabled.
>  $cpupower monitor sleep 30
> sleep took 30.01297 seconds and exited with status 0
>               |Idle_Stats
> PKG |CORE|CPU | snoo | Nap  | Fast
>    0|   8|   0| 96.41|  0.00|  0.00
>    0|   8|   1| 96.43|  0.00|  0.00
>    0|   8|   2| 96.47|  0.00|  0.00
>    0|   8|   3| 96.35|  0.00|  0.00
>    0|   8|   4| 96.37|  0.00|  0.00
>    0|   8|   5| 96.37|  0.00|  0.00
>    0|   8|   6| 96.47|  0.00|  0.00
>    0|   8|   7| 96.47|  0.00|  0.00
> 
> POWER9: Shallow states (stop0lite, stop1lite, stop2lite, stop0, stop1,
> stop2) disabled:
> $cpupower monitor sleep 30
> sleep took 30.05033 seconds and exited with status 0
>               |Idle_Stats
> PKG |CORE|CPU | snoo | stop | stop | stop | stop | stop | stop | stop | stop
>    0|  16|   0| 89.79|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00
>    0|  16|   1| 90.12|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00
>    0|  16|   2| 90.21|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00
>    0|  16|   3| 90.29|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00
> 
> After Patch
> ======================
> POWER8 : Only nap disabled.
> $ cpupower monitor sleep 30
> sleep took 30.01200 seconds and exited with status 0
>               |Idle_Stats
> PKG |CORE|CPU | snoo | Nap  | Fast
>    0|   8|   0| 16.58|  0.00| 77.21
>    0|   8|   1| 18.42|  0.00| 75.38
>    0|   8|   2|  4.70|  0.00| 94.09
>    0|   8|   3| 17.06|  0.00| 81.73
>    0|   8|   4|  3.06|  0.00| 95.73
>    0|   8|   5|  7.00|  0.00| 96.80
>    0|   8|   6|  1.00|  0.00| 98.79
>    0|   8|   7|  5.62|  0.00| 94.17
> 
> POWER9: Shallow states (stop0lite, stop1lite, stop2lite, stop0, stop1,
> stop2) disabled:
> 
> $cpupower monitor sleep 30
> sleep took 30.02110 seconds and exited with status 0
>               |Idle_Stats
> PKG |CORE|CPU | snoo | stop | stop | stop | stop | stop | stop | stop | stop
>    0|   0|   0|  0.69|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  9.39| 89.70
>    0|   0|   1|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.05| 93.21
>    0|   0|   2|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00| 89.93
>    0|   0|   3|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00|  0.00| 93.26
> 
> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/0a4ec6aa035a52c422eceb2ed51ed8

cheers

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-06-05 15:19 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-31 12:15 [PATCH] cpuidle:powernv: Make the snooze timeout dynamic Gautham R. Shenoy
2018-05-31 14:51 ` Balbir Singh
2018-06-01  4:54   ` Gautham R Shenoy
2018-06-02  0:16     ` Balbir Singh
2018-06-04 11:27 ` Michael Ellerman
2018-06-05  3:47   ` Stewart Smith
2018-06-05  8:45   ` Gautham R Shenoy
2018-06-05 12:18     ` Michael Ellerman
2018-06-05 15:19 ` Michael Ellerman

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).