cpuidle: small improvements & fixes for menu governor
cover cpuidle: small improvements & fixes for menu governor
While working on a paravirt cpuidle driver for KVM guests, I noticed a number of small logic errors in the menu governor code.
These patches should get rid of some artifacts that can break the logic in the menu governor under certain corner cases, and make idle state selection work better on CPUs with long C1 exit latencies.
I have not seen any adverse effects with them in my (quick) tests. As expected, they do not seem to do much on systems with many power states and very low C1 exit latencies and target residencies.
PATCH 1/3 cpuidle,x86: increase forced cut-off for polling to 20us
From: Rik van Riel <[email protected]>
The cpuidle menu governor has a forced cut-off for polling at 5us, in order to deal with firmware that gives the OS bad information on cpuidle states, leading to the system spending way too much time in polling.
However, at least one x86 CPU family (Atom) has chips that have a 20us break-even point for C1. Forcing the polling cut-off to less than that wastes performance and power.
Increase the polling cut-off to 20us.
Systems with a lower C1 latency will be found in the states table by the menu governor, which will pick those states as appropriate.
--- drivers/cpuidle/governors/menu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/cpuidle/governors/menu.c +1 -1
@@ -330,7 +330,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)* We want to default to C1 (hlt), not to busy polling * unless the timer is happening really really soon. */ -if (data->next_timer_us >5 && +if (data->next_timer_us >20 && !drv->states[CPUIDLE_DRIVER_STATE_START ].disabled && dev->states_usage[CPUIDLE_DRIVER_STATE_START ].disable ==0 ) data->last_state_idx =CPUIDLE_DRIVER_STATE_START ;
Signature · 1 line
2.1.0
Re: [PATCH 1/3] cpuidle,x86: increase forced cut-off for polling to 20us
Quoted 6 lines · On 10/28/2015 11:46 PM, [email protected] wrote:
From: Rik van Riel <[email protected]>
The cpuidle menu governor has a forced cut-off for polling at 5us, in order to deal with firmware that gives the OS bad information on cpuidle states, leading to the system spending way too much time in polling.
May be I am misunderstanding your explanation but it is not how I read the code.
The default idle state is C1 (hlt) if no other states suits the constraint. If a timer is happening really soon, then set the default idle state to POLL if no other idle state suits the constraint.
That applies only on x86.
This is not related to break-even but exit latency.
IMO, we should just drop this 5us and the POLL state selection in the menu governor as we have since a while hyper fast C1 exit. Except a few embedded processors where polling is not adequate.
Furthermore, the number of times the poll state is selected vs the other states is negligible.
I already raised this point but Len is opposed to the removal.
Len ? Can you elaborate why you are opposed to this removal ?
Quoted 8 lines
However, at least one x86 CPU family (Atom) has chips that have a 20us break-even point for C1. Forcing the polling cut-off to less than that wastes performance and power.
Increase the polling cut-off to 20us.
Systems with a lower C1 latency will be found in the states table by the menu governor, which will pick those states as appropriate.
With this change, I believe the poll state will be selected more often (not too much certainly), hence implying a bigger energy consumption.
And finally it may improve the situation for specific processor but deteriorate for other processors.
Does anyone have the rational behind this '5' number ? why not '3' or '7' ?
Quoted 19 lines
Signed-off-by Rik van Riel <[email protected]>--- drivers/cpuidle/governors/menu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/cpuidle/governors/menu.c +1
-1@@ -330,7 +330,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)* We want to default to C1 (hlt), not to busy polling * unless the timer is happening really really soon. */ -if (data->next_timer_us >5 && +if (data->next_timer_us >20 && !drv->states[CPUIDLE_DRIVER_STATE_START ].disabled && dev->states_usage[CPUIDLE_DRIVER_STATE_START ].disable ==0 ) data->last_state_idx =CPUIDLE_DRIVER_STATE_START ;
Signature · 5 lines
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
Re: [PATCH 1/3] cpuidle,x86: increase forced cut-off for polling to 20us
Quoted 8 lines · Daniel Lezcano
Quoted 6 lines · On 10/28/2015 11:46 PM, [email protected] wrote:
From: Rik van Riel <[email protected]>
The cpuidle menu governor has a forced cut-off for polling at 5us, in order to deal with firmware that gives the OS bad information on cpuidle states, leading to the system spending way too much time in polling.
Quoted 8 lines
May be I am misunderstanding your explanation but it is not how I read the code.
The default idle state is C1 (hlt) if no other states suits the constraint. If a timer is happening really soon, then set the default idle state to POLL if no other idle state suits the constraint.
That applies only on x86.
With the current code, the default idle state is C1 (hlt) even if C1 does not suit the constraint.
Quoted 1 line
This is not related to break-even but exit latency.
Why would we not care about break-even for C1?
On systems where going into C1 for too-short periods wastes power, why would we waste the power when we expect a very short sleep?
Quoted 3 lines
IMO, we should just drop this 5us and the POLL state selection in the menu governor as we have since a while hyper fast C1 exit. Except a few embedded processors where polling is not adequate.
We have hyper fast C1 exit on Nehalem and newer high performance chips. On those chips, we will pick C1 (or deeper) when we have an expected sleep time of just a few microseconds.
However, on Atom, and for the paravirt cpuidle driver I am working on, C1 exit latency and target residence are higher than the cut-off hardcoded in the menu governor.
Quoted 2 lines
Furthermore, the number of times the poll state is selected vs the other states is negligible.
And it will continue to be with this patch, on CPUs with hyper fast C1 exit.
Which makes me confused about what your are objecting to, since the system should continue to be have the way you want, with the patch applied.
Signature · 1 line
All rights reversed
Re: [PATCH 1/3] cpuidle,x86: increase forced cut-off for polling to 20us
Quoted 44 lines · Rik van Riel
Quoted 8 lines · Daniel Lezcano
Quoted 6 lines · On 10/28/2015 11:46 PM, [email protected] wrote:
From: Rik van Riel <[email protected]>
The cpuidle menu governor has a forced cut-off for polling at 5us, in order to deal with firmware that gives the OS bad information on cpuidle states, leading to the system spending way too much time in polling.
Quoted 8 lines
May be I am misunderstanding your explanation but it is not how I read the code.
The default idle state is C1 (hlt) if no other states suits the constraint. If a timer is happening really soon, then set the default idle state to POLL if no other idle state suits the constraint.
That applies only on x86.
With the current code, the default idle state is C1 (hlt) even if C1 does not suit the constraint.
Quoted 1 line
This is not related to break-even but exit latency.
Why would we not care about break-even for C1?
On systems where going into C1 for too-short periods wastes power, why would we waste the power when we expect a very short sleep?
Quoted 3 lines
IMO, we should just drop this 5us and the POLL state selection in the menu governor as we have since a while hyper fast C1 exit. Except a few embedded processors where polling is not adequate.
We have hyper fast C1 exit on Nehalem and newer high performance chips. On those chips, we will pick C1 (or deeper) when we have an expected sleep time of just a few microseconds.
However, on Atom, and for the paravirt cpuidle driver I am working on, C1 exit latency and target residence are higher than the cut-off hardcoded in the menu governor.
Quoted 2 lines
Furthermore, the number of times the poll state is selected vs the other states is negligible.
Quoted 6 lines
And it will continue to be with this patch, on CPUs with hyper fast C1 exit.
Which makes me confused about what your are objecting to, since the system should continue to be have the way you want, with the patch applied.
Ok, I don't object the correctness of your patch but the reasoning behind this small optimization which bring us a lot of mess in the cpuidle code.
As you are touching this part of the code, I take the opportunity to raise a discussion about it.
From my POV, the poll state is *not* an idle state. It is like a vehicle burnout [1].
But it is inserted into the idle state tables using a trick with a macro CPUIDLE_DRIVER_STATE_START which already led us to some bugs.
So instead of falling back into the poll state under certain circumstances, I propose we extract this state from the idle state table and we let the menu governor to fail choosing a state (or not).
From the caller, we decide what to do (poll or C1) if the idle state selection fails or we choose to poll *before* like what we already have in kernel/sched/idle.c:
in the idle loop:
if (cpu_idle_force_poll || tick_check_broadcast_expired())
cpu_idle_poll(); else cpuidle_idle_call();
By this way, we:
1) factor out the idle state selection with the find_deepest_idle_state
2) remove the CPUIDLE_DRIVER_STATE_START macro
3) concentrate the optimization logic outside of a governor which will benefit to all architectures
Does it make sense ?
-- Daniel
[1] https://en.wikipedia.org/wiki/Burnout_%28vehicle%29
Signature · 5 lines
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
PATCH 2/3 cpuidle,menu: use interactivity_req to disable polling
In reply to [email protected]From: Rik van Riel <[email protected]>
The menu governor carefully figures out how much time we typically sleep for an estimated sleep interval, or whether there is a repeating pattern going on, and corrects that estimate for the CPU load.
Then it proceeds to ignore that information when determining whether or not to consider polling. This is not a big deal on most x86 CPUs, which have very low C1 latencies, and the patch should not have any effect on those CPUs.
However, certain CPUs (eg. Atom) have much higher C1 latencies, and it would be good to not waste performance and power on those CPUs if we are expecting a very low wakeup latency.
Disable polling based on the estimated interactivity requirement, not on the time to the next timer interrupt.
--- drivers/cpuidle/governors/menu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/cpuidle/governors/menu.c +1 -1
@@ -330,7 +330,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)* We want to default to C1 (hlt), not to busy polling * unless the timer is happening really really soon. */ -if (data->next_timer_us >20 && +if (interactivity_req >20 && !drv->states[CPUIDLE_DRIVER_STATE_START ].disabled && dev->states_usage[CPUIDLE_DRIVER_STATE_START ].disable ==0 ) data->last_state_idx =CPUIDLE_DRIVER_STATE_START ;
Signature · 1 line
2.1.0
PATCH 3/3 cpuidle,menu: smooth out measured_us calculation
In reply to [email protected]From: Rik van Riel <[email protected]>
The cpuidle state tables contain the maximum exit latency for each cpuidle state. On x86, that is the exit latency for when the entire package goes into that same idle state.
However, a lot of the time we only go into the core idle state, not the package idle state. This means we see a much smaller exit latency.
We have no way to detect whether we went into the core or package idle state while idle, and that is ok.
However, the current menu_update logic does have the potential to trip up the repeating pattern detection in get_typical_interval. If the system is experiencing an exit latency near the idle state's exit latency, some of the samples will have exit_us subtracted, while others will not. This turns a repeating pattern into mush, potentially breaking get_typical_interval.
Furthermore, for smaller sleep intervals, we know the chance that all the cores in the package went to the same idle state are fairly small. Dividing the measured_us by two, instead of subtracting the full exit latency when hitting a small measured_us, will reduce the error.
--- drivers/cpuidle/governors/menu.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/cpuidle/governors/menu.c +3 -1
@@ -404,8 +404,10 @@ static void menu_update(struct cpuidle_driver *drv, struct cpuidle_device *dev) measured_us =cpuidle_get_last_residency (dev);/* Deduct exit latency */ -if (measured_us > target->exit_latency) +if (measured_us >2 * target->exit_latency) measured_us -= target->exit_latency; +else + measured_us /=2 ;/* Make sure our coefficients do not exceed unity */ if (measured_us > data->next_timer_us)
Signature · 1 line
2.1.0
Quoted 4 lines · On 10/28/2015 11:46 PM, [email protected] wrote:
While working on a paravirt cpuidle driver for KVM guests, I noticed a number of small logic errors in the menu governor code.
Quoted 9 lines
These patches should get rid of some artifacts that can break the logic in the menu governor under certain corner cases, and make idle state selection work better on CPUs with long C1 exit latencies.
I have not seen any adverse effects with them in my (quick) tests. As expected, they do not seem to do much on systems with many power states and very low C1 exit latencies and target residencies.
Sorry for the trouble, but can you please resend the series with CCs to [email protected]? That will make it way easier to handle for me.
Thanks,
Rafael
Quoted 10 lines · Rafael J. Wysocki
Quoted 9 lines · On 10/28/2015 11:46 PM, [email protected] wrote:
While working on a paravirt cpuidle driver for KVM guests, I noticed a number of small logic errors in the menu governor code.
These patches should get rid of some artifacts that can break the logic in the menu governor under certain corner cases, and make idle state selection work better on CPUs with long C1 exit latencies.
Quoted 7 lines
Quoted 4 lines
I have not seen any adverse effects with them in my (quick) tests. As expected, they do not seem to do much on systems with many power states and very low C1 exit latencies and target residencies.
Sorry for the trouble, but can you please resend the series with CCs to [email protected]? That will make it way easier to handle for me.
Not a problem. Done.
What change do I need to send in to ensure that the linux-pm mailing list shows up in get_maintainer.pl output?
$ ./scripts/get_maintainer.pl -f drivers/cpuidle/governors/menu.c "Rafael J. Wysocki" <[email protected]> (commit_signer:4/7=57%,authored:1/7=14%,added_lines:2/19=11%,removed_lines:2/28=7%) Daniel Lezcano <[email protected]> (commit_signer:3/7=43%,authored:1/7=14%,added_lines:1/19=5%) Rik van Riel <[email protected]> (commit_signer:3/7=43%,authored:3/7=43%,added_lines:5/19=26%,removed_lines:3/28=11%) Len Brown <[email protected]> (commit_signer:1/7=14%,authored:1/7=14%,added_lines:10/19=53%,removed_lines:15/28=54%) "Peter Zijlstra (Intel)" <[email protected]> (commit_signer:1/7=14%) Javi Merino <[email protected]> (authored:1/7=14%,added_lines:1/19=5%,removed_lines:7/28=25%) [email protected] (open list)
Quoted 23 lines · Rik van Riel
Quoted 10 lines · Rafael J. Wysocki
Quoted 9 lines · On 10/28/2015 11:46 PM, [email protected] wrote:
While working on a paravirt cpuidle driver for KVM guests, I noticed a number of small logic errors in the menu governor code.
These patches should get rid of some artifacts that can break the logic in the menu governor under certain corner cases, and make idle state selection work better on CPUs with long C1 exit latencies.
Quoted 6 lines
Quoted 4 lines
I have not seen any adverse effects with them in my (quick) tests. As expected, they do not seem to do much on systems with many power states and very low C1 exit latencies and target residencies.
Sorry for the trouble, but can you please resend the series with CCs to [email protected]? That will make it way easier to handle for me.
Not a problem. Done.
What change do I need to send in to ensure that the linux-pm mailing list shows up in get_maintainer.pl output?
Quoted 14 lines
$ ./scripts/get_maintainer.pl -f drivers/cpuidle/governors/menu.c "Rafael J. Wysocki" <[email protected]> (commit_signer:4/7=57%,authored:1/7=14%,added_lines:2/19=11%,removed_lines:2/28=7%) Daniel Lezcano <[email protected]> (commit_signer:3/7=43%,authored:1/7=14%,added_lines:1/19=5%) Rik van Riel <[email protected]> (commit_signer:3/7=43%,authored:3/7=43%,added_lines:5/19=26%,removed_lines:3/28=11%) Len Brown <[email protected]> (commit_signer:1/7=14%,authored:1/7=14%,added_lines:10/19=53%,removed_lines:15/28=54%) "Peter Zijlstra (Intel)" <[email protected]> (commit_signer:1/7=14%) Javi Merino <[email protected]> (authored:1/7=14%,added_lines:1/19=5%,removed_lines:7/28=25%) [email protected] (open list)
I'm not sure why it doesn't show up in there. If you look at MAINTAINERS under CPUIDLE DRIVERS, linux-pm is actually listed there.
Maybe the answer is that get_maintainer.pl needs to be fixed ...
Thanks,
Rafael
Quoted 46 lines · Rafael J. Wysocki
Quoted 35 lines · Rik van Riel
Quoted 15 lines · Rafael J. Wysocki
Quoted 9 lines · On 10/28/2015 11:46 PM, [email protected] wrote:
While working on a paravirt cpuidle driver for KVM guests, I noticed a number of small logic errors in the menu governor code.
These patches should get rid of some artifacts that can break the logic in the menu governor under certain corner cases, and make idle state selection work better on CPUs with long C1 exit latencies.
Quoted 5 lines
I have not seen any adverse effects with them in my (quick) tests. As expected, they do not seem to do much on systems with many power states and very low C1 exit latencies and target residencies.
Quoted 4 lines
Sorry for the trouble, but can you please resend the series with CCs to
[email protected]? That will make it way easier to handle for me.Not a problem. Done.
What change do I need to send in to ensure that the linux-pm mailing list shows up in get_maintainer.pl output?
$ ./scripts/get_maintainer.pl -f drivers/cpuidle/governors/menu.c "Rafael J. Wysocki" <[email protected]> (commit_signer:4/7=57%,authored:1/7=14%,added_lines:2/19=11%,remove d_lines:2/28=7%) Daniel Lezcano <[email protected]> (commit_signer:3/7=43%,authored:1/7=14%,added_lines:1/19=5%) Rik van Riel <[email protected]> (commit_signer:3/7=43%,authored:3/7=43%,added_lines:5/19=26%,remove d_lines:3/28=11%)Quoted 9 lines
Len Brown <[email protected]> (commit_signer:1/7=14%,authored:1/7=14%,added_lines:10/19=53%,remov ed_lines:15/28=54%) "Peter Zijlstra (Intel)" <[email protected]> (commit_signer:1/7=14%) Javi Merino <[email protected]> (authored:1/7=14%,added_lines:1/19=5%,removed_lines:7/28=25%) [email protected] (open list)
Quoted 3 lines
I'm not sure why it doesn't show up in there.
If you look atMAINTAINERSunder CPUIDLE DRIVERS, linux-pm is actually listed there.
Because the pattern is just the files in the top level directory of drivers/cpuidle and not any files in any directory below
Quoted 1 line
From the
MAINTAINERSpattern descriptions:
F: Files and directories with wildcard patterns.
A trailing slash includes all files and subdirectory files.
F: drivers/net/ all files in and belowdrivers/net
F: drivers/net/* all files in drivers/net, but
not below
Quoted 1 line
Maybe the answer is that get_maintainer.pl needs to be fixed ...
Nope, the MAINTAINERS - CPUIDLE DRIVERS entry does though.
--- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
MAINTAINERS +1 -1
@@ -3004,7 +3004,7 @@ M: Daniel Lezcano <[email protected]> L: [email protected] S: Maintained T: git git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git -F: drivers/cpuidle/* +F: drivers/cpuidle/ F: include/linux/cpuidle.h CPUID/MSR DRIVER
Quoted 61 lines · Joe Perches
Quoted 45 lines · Rafael J. Wysocki
Quoted 35 lines · Rik van Riel
Quoted 15 lines · Rafael J. Wysocki
Quoted 9 lines · On 10/28/2015 11:46 PM, [email protected] wrote:
While working on a paravirt cpuidle driver for KVM guests, I noticed a number of small logic errors in the menu governor code.
These patches should get rid of some artifacts that can break the logic in the menu governor under certain corner cases, and make idle state selection work better on CPUs with long C1 exit latencies.
Quoted 5 lines
I have not seen any adverse effects with them in my (quick) tests. As expected, they do not seem to do much on systems with many power states and very low C1 exit latencies and target residencies.
Quoted 4 lines
Sorry for the trouble, but can you please resend the series with CCs to
[email protected]? That will make it way easier to handle for me.Not a problem. Done.
What change do I need to send in to ensure that the linux-pm mailing list shows up in get_maintainer.pl output?
$ ./scripts/get_maintainer.pl -f drivers/cpuidle/governors/menu.c "Rafael J. Wysocki" <[email protected]> (commit_signer:4/7=57%,authored:1/7=14%,added_lines:2/19=11%,remove d_lines:2/28=7%) Daniel Lezcano <[email protected]> (commit_signer:3/7=43%,authored:1/7=14%,added_lines:1/19=5%) Rik van Riel <[email protected]> (commit_signer:3/7=43%,authored:3/7=43%,added_lines:5/19=26%,remove d_lines:3/28=11%)Quoted 9 lines
Len Brown <[email protected]> (commit_signer:1/7=14%,authored:1/7=14%,added_lines:10/19=53%,remov ed_lines:15/28=54%) "Peter Zijlstra (Intel)" <[email protected]> (commit_signer:1/7=14%) Javi Merino <[email protected]> (authored:1/7=14%,added_lines:1/19=5%,removed_lines:7/28=25%) [email protected] (open list)Quoted 3 lines
I'm not sure why it doesn't show up in there.
If you look atMAINTAINERSunder CPUIDLE DRIVERS, linux-pm is actually listed there.Because the pattern is just the files in the top level directory of
drivers/cpuidleand not any files in any directory belowFrom the
MAINTAINERSpattern descriptions:F: Files and directories with wildcard patterns.
A trailing slash includes all files and subdirectory files.
F:drivers/net/all files in and belowdrivers/net
F:drivers/net/*all files indrivers/net, but
not below
Quoted 2 lines
Quoted 1 line
Maybe the answer is that get_maintainer.pl needs to be fixed ...
Nope, the
MAINTAINERS- CPUIDLE DRIVERS entry does though.
OK, I see.
Care to send this patch with a proper changelog/sign-off?
Quoted 18 lines
--- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)MAINTAINERS +1
-1@@ -3004,7 +3004,7 @@ M: Daniel Lezcano <[email protected]> L: [email protected] S: Maintained T: git git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git -F: drivers/cpuidle/* +F: drivers/cpuidle/ F: include/linux/cpuidle.h CPUID/MSR DRIVER
Quoted 5 lines · Rafael J. Wysocki
Quoted 4 lines · Joe Perches
Quoted 3 lines · Rafael J. Wysocki
Quoted 2 lines · Rik van Riel
Quoted 1 line · Rafael J. Wysocki
On 10/28/2015 11:46 PM, [email protected] wrote:
[]
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 6 lines
Sorry for the trouble, but can you please resend the series with
CCs to
[email protected]? That will make it way easier to handle
for me.Not a problem. Done.
What change do I need to send in to ensure that the linux-pm mailing list shows up in get_maintainer.pl output?
[]
Quoted 1 line
Quoted 1 line
Quoted 1 line
I'm not sure why it doesn't show up in there.
[]
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
If you look at
MAINTAINERSunder CPUIDLE DRIVERS, linux-pm is actually listed there.Because the pattern is just the files in the top level directory of
drivers/cpuidleand not any files in any directory belowCare to send this patch with a proper changelog/sign-off?
Sending patches with my current version of the evolution email client (3.16.5) is a bit troublesome.
I'm going to either upgrade to 3.19, downgrade back to 3.12 or change clients altogether.
Can you do it instead please?