| Message ID | 1527768909-32637-1-git-send-email-ego@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0a4ec6aa035a52c422eceb2ed51ed88392a3d6c2 |
| Headers | show |
| Series | cpuidle:powernv: Make the snooze timeout dynamic. | expand |
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>
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.
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.
"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
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.
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 >
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
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
==================
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;
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