From: Shreyas B. Prabhu <hidden> Date: 2016-06-24 08:24:11
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 <redacted>
Bisected-by: Shilpasri G Bhat [off-list ref]
Signed-off-by: Shreyas B. Prabhu <redacted>
---
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(-)
From: David Laight <hidden> Date: 2016-06-24 09:00:48
From: Shreyas B. Prabhu
quoted hunk
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 <redacted>
Bisected-by: Shilpasri G Bhat [off-list ref]
Signed-off-by: Shreyas B. Prabhu <redacted>
---
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(-)
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
From: Daniel Lezcano <hidden> Date: 2016-06-24 10:05:38
On 06/24/2016 11:00 AM, David Laight wrote:
From: Shreyas B. Prabhu
quoted
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 <redacted>
Bisected-by: Shilpasri G Bhat [off-list ref]
Signed-off-by: Shreyas B. Prabhu <redacted>
---
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(-)
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.
On Friday, June 24, 2016 9:00:48 AM CEST David Laight wrote:
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.
Why not change the definition of the time to nanoseconds and update
the users accordingly?
I see that cpuidle_enter_state() writes to the last_residency value,
and it is read in three places: ladder_select_state(), menu_update()
and show_state_time() (for sysfs).
If those functions are called less often than cpuidle_enter_state(),
we could just move the division there. Since the divisor is constant,
do_div() can convert it into a multiply and shift, or we could use
your the code you suggest above, or use a 32-bit division most of
the time:
if (diff <= UINT_MAX)
diff_32 = (u32)diff / NSECS_PER_USEC;
else
diff_32 = div_u64(diff, NSECS_PER_USEC;
which gcc itself will turn into a multiplication or series of
shifts on CPUs on which that is faster.
Arnd
drivers/cpuidle/cpuidle.c:242: undefined reference to `__udivdi3'
vim +242 drivers/cpuidle/cpuidle.c
56cfbf74a Colin Cross 2012-05-07 236 dev->states_usage[entered_state].usage++;
56cfbf74a Colin Cross 2012-05-07 237 } else {
56cfbf74a Colin Cross 2012-05-07 238 dev->last_residency = 0;
56cfbf74a Colin Cross 2012-05-07 239 }
56cfbf74a Colin Cross 2012-05-07 240
56cfbf74a Colin Cross 2012-05-07 241 return entered_state;
56cfbf74a Colin Cross 2012-05-07 @242 }
56cfbf74a Colin Cross 2012-05-07 243
56cfbf74a Colin Cross 2012-05-07 244 /**
907e30f1b Daniel Lezcano 2014-03-03 245 * cpuidle_select - ask the cpuidle framework to choose an idle state
:::::: The code at line 242 was first introduced by commit
:::::: 56cfbf74a17c40f3a741398103c9f5d5a6806715 cpuidle: refactor out cpuidle_enter_state
:::::: TO: Colin Cross [off-list ref]
:::::: CC: Len Brown [off-list ref]
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
drivers/cpuidle/cpuidle.c:224: undefined reference to `__aeabi_uldivmod'
vim +224 drivers/cpuidle/cpuidle.c
218 local_irq_enable();
219
220 /*
221 * local_clock() returns the time in nanosecond, let's
222 * divide by 1000 to have microsecond based time.
223 */
> 224 diff = (time_end - time_start) / 1000;
225 if (diff > INT_MAX)
226 diff = INT_MAX;
227
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
From: Shreyas B Prabhu <hidden> Date: 2016-06-24 16:01:55
On 06/24/2016 03:41 PM, Arnd Bergmann wrote:
On Friday, June 24, 2016 9:00:48 AM CEST David Laight wrote:
quoted
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.
Why not change the definition of the time to nanoseconds and update
the users accordingly?
I see that cpuidle_enter_state() writes to the last_residency value,
and it is read in three places: ladder_select_state(), menu_update()
and show_state_time() (for sysfs).
Updating show_state_time() to convert ns to us gets bit messy since
show_state_##_name which is used for disable, usage and time simply
returns state_usage->_name. And state_usage->time updation happens in
cpuidle_enter_state() itself.
If those functions are called less often than cpuidle_enter_state(),
we could just move the division there. Since the divisor is constant,
do_div() can convert it into a multiply and shift, or we could use
your the code you suggest above, or use a 32-bit division most of
the time:
if (diff <= UINT_MAX)
diff_32 = (u32)diff / NSECS_PER_USEC;
else
diff_32 = div_u64(diff, NSECS_PER_USEC;
which gcc itself will turn into a multiplication or series of
shifts on CPUs on which that is faster.
I'm not sure which division method of the three suggested here to use.
Does anyone have a strong preference?
--Shreyas
On Friday, June 24, 2016 9:31:35 PM CEST Shreyas B Prabhu wrote:
quoted
If those functions are called less often than cpuidle_enter_state(),
we could just move the division there. Since the divisor is constant,
do_div() can convert it into a multiply and shift, or we could use
your the code you suggest above, or use a 32-bit division most of
the time:
if (diff <= UINT_MAX)
diff_32 = (u32)diff / NSECS_PER_USEC;
else
diff_32 = div_u64(diff, NSECS_PER_USEC;
which gcc itself will turn into a multiplication or series of
shifts on CPUs on which that is faster.
I'm not sure which division method of the three suggested here to use.
Does anyone have a strong preference?
It depends on how accurate we want it and how long we expect
the times to be. The optimization for the 4.2 second cutoff
for doing a 32-bit division only makes sense if the majority
of the sleep times are below that.
Arnd
From: David Laight <hidden> Date: 2016-06-27 08:59:10
From: Arnd Bergmann
Sent: 24 June 2016 20:43
On Friday, June 24, 2016 9:31:35 PM CEST Shreyas B Prabhu wrote:
quoted
quoted
If those functions are called less often than cpuidle_enter_state(),
we could just move the division there. Since the divisor is constant,
do_div() can convert it into a multiply and shift, or we could use
your the code you suggest above, or use a 32-bit division most of
the time:
if (diff <= UINT_MAX)
diff_32 = (u32)diff / NSECS_PER_USEC;
else
diff_32 = div_u64(diff, NSECS_PER_USEC;
which gcc itself will turn into a multiplication or series of
shifts on CPUs on which that is faster.
I'm not sure which division method of the three suggested here to use.
Does anyone have a strong preference?
It depends on how accurate we want it and how long we expect
the times to be. The optimization for the 4.2 second cutoff
for doing a 32-bit division only makes sense if the majority
of the sleep times are below that.
It also depends if the code actually cares about the length of 'long' sleeps.
I'd guess that for cpu idle 4.2 seconds is 'a long time', so the div_u64()
result could be treated as 4.2 seconds without causing grief.
Actually the cost of a 64bit divide after a 4 second sleep will be noise.
OTOH a 64bit divide after a sleep that lasted a few ns will be significant.
David
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
From: Shreyas B Prabhu <hidden> Date: 2016-06-29 07:00:45
On 06/27/2016 02:29 PM, David Laight wrote:
From: Arnd Bergmann
quoted
Sent: 24 June 2016 20:43
On Friday, June 24, 2016 9:31:35 PM CEST Shreyas B Prabhu wrote:
quoted
quoted
If those functions are called less often than cpuidle_enter_state(),
we could just move the division there. Since the divisor is constant,
do_div() can convert it into a multiply and shift, or we could use
your the code you suggest above, or use a 32-bit division most of
the time:
if (diff <= UINT_MAX)
diff_32 = (u32)diff / NSECS_PER_USEC;
else
diff_32 = div_u64(diff, NSECS_PER_USEC;
which gcc itself will turn into a multiplication or series of
shifts on CPUs on which that is faster.
I'm not sure which division method of the three suggested here to use.
Does anyone have a strong preference?
It depends on how accurate we want it and how long we expect
the times to be. The optimization for the 4.2 second cutoff
for doing a 32-bit division only makes sense if the majority
of the sleep times are below that.
It also depends if the code actually cares about the length of 'long' sleeps.
I'd guess that for cpu idle 4.2 seconds is 'a long time', so the div_u64()
result could be treated as 4.2 seconds without causing grief.
Actually the cost of a 64bit divide after a 4 second sleep will be noise.
OTOH a 64bit divide after a sleep that lasted a few ns will be significant.
Agreed. I'll use the code you suggested, with a small change-
Using diff_32 += diff_32 >> 5 instead of diff_32 += diff_32 >> 6
since I want to err on the side of last_residency being more than actual.
And for long sleep cases, I'll use div_u64().
Thanks,
Shreyas