From: Shreyas B. Prabhu > Sent: 24 June 2016 09:24 > > Snooze is a poll idle state in powernv and pseries platforms. Snooze > has a timeout so that if a cpu stays in snooze for more than target > residency of the next available idle state, then it would exit thereby > giving chance to the cpuidle governor to re-evaluate and > promote the cpu to a deeper idle state. Therefore whenever snooze exits > due to this timeout, its last_residency will be target_residency of next > deeper state. > > commit e93e59ce5b85 ("cpuidle: Replace ktime_get() with local_clock()") > changed the math around last_residency calculation. Specifically, while > converting last_residency value from nanoseconds to microseconds it does > right shift by 10. Due to this, in snooze timeout exit scenarios > last_residency calculated is roughly 2.3% less than target_residency of > next available state. This pattern is picked up get_typical_interval() > in the menu governor and therefore expected_interval in menu_select() is > frequently less than the target_residency of any state but snooze. > > Due to this we are entering snooze at a higher rate, thereby affecting > the single thread performance. > > Fix this by replacing right shift by 10 with /1000 while calculating > last_residency. > > Reported-by: Anton Blanchard <an...@samba.org> > Bisected-by: Shilpasri G Bhat <shilpa.b...@linux.vnet.ibm.com> > Signed-off-by: Shreyas B. Prabhu <shre...@linux.vnet.ibm.com> > --- > Changes in v2 > ============= > - Fixing it in the cpuidle core code instead of driver code. > > drivers/cpuidle/cpuidle.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c > index a4d0059..30d67a8 100644 > --- a/drivers/cpuidle/cpuidle.c > +++ b/drivers/cpuidle/cpuidle.c > @@ -218,10 +218,10 @@ int cpuidle_enter_state(struct cpuidle_device *dev, > struct cpuidle_driver *drv, > local_irq_enable(); > > /* > - * local_clock() returns the time in nanosecond, let's shift > - * by 10 (divide by 1024) to have microsecond based time. > + * local_clock() returns the time in nanosecond, let's > + * divide by 1000 to have microsecond based time. > */ > - diff = (time_end - time_start) >> 10; > + diff = (time_end - time_start) / 1000; > if (diff > INT_MAX) > diff = INT_MAX;
The intent of the >> 10 was probably to avoid an expensive 64bit divide. So maybe something like: diff = time_end - time_start; if (diff >= INT_MAX/2) diff_32 = INT_MAX/2/1000; else diff_32 = diff; diff_32 += diff_32 >> 6; diff_32 >>= 10; } Adding an extra 1/32 makes the division by be something slightly below 1000. David _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev