On 2019/11/15 18:45, Rafael J. Wysocki wrote:
On Wednesday, November 6, 2019 12:55:01 PM CET Zhenzhong Duan wrote:
As user can adjust guest_halt_poll_grow_start and guest_halt_poll_ns
which leads to cpu_halt_poll_us beyond the two boundaries. This patch
ensures cpu_halt_poll_us in that scope.
If guest_halt_poll_shrink is 0, shrink the cpu_halt_poll_us to
guest_halt_poll_grow_start instead of 0. To disable poll we can set
guest_halt_poll_ns to 0.
If user wrongly set guest_halt_poll_grow_start > guest_halt_poll_ns > 0,
guest_halt_poll_ns take precedency and poll time is a fixed value of
guest_halt_poll_ns.
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@xxxxxxxxxx>
---
drivers/cpuidle/governors/haltpoll.c | 28 +++++++++++++---------------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/drivers/cpuidle/governors/haltpoll.c b/drivers/cpuidle/governors/haltpoll.c
index 660859d..4a39df4 100644
--- a/drivers/cpuidle/governors/haltpoll.c
+++ b/drivers/cpuidle/governors/haltpoll.c
@@ -97,32 +97,30 @@ static int haltpoll_select(struct cpuidle_driver *drv,
static void adjust_poll_limit(struct cpuidle_device *dev, unsigned int block_us)
{
- unsigned int val;
+ unsigned int val = dev->poll_limit_ns;
Not necessary to initialize it here.
Then an random val may bypass all the check and get assigned to dev->poll_limit_ns
if guest_halt_poll_grow_start< block_ns< uninitialized val< guest_halt_poll_ns
With my change, dev->poll_limit_ns will not be changed in that case, logic same as original code.
u64 block_ns = block_us*NSEC_PER_USEC;
/* Grow cpu_halt_poll_us if
- * cpu_halt_poll_us < block_ns < guest_halt_poll_us
+ * cpu_halt_poll_us < block_ns <= guest_halt_poll_us
You could update the comment to say "dev->poll_limit_ns" instead of
"cpu_halt_poll_us" while at it.
Will do, also guest_halt_poll_us to guest_halt_poll_ns
*/
- if (block_ns > dev->poll_limit_ns && block_ns <= guest_halt_poll_ns) {
+ if (block_ns > dev->poll_limit_ns && block_ns <= guest_halt_poll_ns &&
+ guest_halt_poll_grow)
The "{" brace is still needed as per the coding style and I'm not sure why
to avoid guest_halt_poll_grow equal to zero here?
Will add "{}" and remove guest_halt_poll_grow check. My inital thought was to prevent
dev->poll_limit_ns get shrinked with guest_halt_poll_grow=0.
val = dev->poll_limit_ns * guest_halt_poll_grow;
-
- if (val < guest_halt_poll_grow_start)
- val = guest_halt_poll_grow_start;
- if (val > guest_halt_poll_ns)
- val = guest_halt_poll_ns;
-
- dev->poll_limit_ns = val;
- } else if (block_ns > guest_halt_poll_ns &&
- guest_halt_poll_allow_shrink) {
+ else if (block_ns > guest_halt_poll_ns &&
+ guest_halt_poll_allow_shrink) {
unsigned int shrink = guest_halt_poll_shrink;
- val = dev->poll_limit_ns;
if (shrink == 0)
- val = 0;
+ val = guest_halt_poll_grow_start;
That's going to be corrected below, so the original code would be fine.
val was assigned twice using 'val = 0' while it's once with my change, optimal a bit?
else
val /= shrink;
Here you can do
val = dev->poll_limit_ns / shrink;
Any special reason？Looks no difference for me.
- dev->poll_limit_ns = val;
}
+ if (val < guest_halt_poll_grow_start)
+ val = guest_halt_poll_grow_start;
Note that guest_halt_poll_grow_start is in us (as per the comment next to its
definition and the initial value). That is a bug in the original code too,
but anyway.
Good catch! will fix the comment. The default 50000ns vs 50000us, looks author means ns.
guest_halt_poll_ns defaults to 200000, also hints ns for guest_halt_poll_grow_start.
Thanks
Zhenzhong
