From: leo.yan@linaro.org
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Linux PM <linux-pm@vger.kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
LKML <linux-kernel@vger.kernel.org>,
Frederic Weisbecker <frederic@kernel.org>
Subject: Re: [PATCH v2] cpuidle: menu: Handle stopped tick more aggressively
Date: Fri, 10 Aug 2018 17:20:34 +0800 [thread overview]
Message-ID: <20180810092034.GF11817@leoy-ThinkPad-X240s> (raw)
In-Reply-To: <3174357.2tBMdxG3bF@aspire.rjw.lan>
On Fri, Aug 10, 2018 at 09:57:18AM +0200, Rafael J . Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: [PATCH] cpuidle: menu: Handle stopped tick more aggressively
>
> Commit 87c9fe6ee495 (cpuidle: menu: Avoid selecting shallow states
> with stopped tick) missed the case when the target residencies of
> deep idle states of CPUs are above the tick boundary which may cause
> the CPU to get stuck in a shallow idle state for a long time.
>
> Say there are two CPU idle states available: one shallow, with the
> target residency much below the tick boundary and one deep, with
> the target residency significantly above the tick boundary. In
> that case, if the tick has been stopped already and the expected
> next timer event is relatively far in the future, the governor will
> assume the idle duration to be equal to TICK_USEC and it will select
> the idle state for the CPU accordingly. However, that will cause the
> shallow state to be selected even though it would have been more
> energy-efficient to select the deep one.
>
> To address this issue, modify the governor to always assume idle
> duration to be equal to the time till the closest timer event if
> the tick is not running which will cause the selected idle states
> to always match the known CPU wakeup time.
>
> Also make it always indicate that the tick should be stopped in
> that case for consistency.
>
> Fixes: 87c9fe6ee495 (cpuidle: menu: Avoid selecting shallow states with stopped tick)
> Reported-by: Leo Yan <leo.yan@linaro.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> -> v2: Initialize first_idx properly in the stopped tick case.
>
> ---
> drivers/cpuidle/governors/menu.c | 55 +++++++++++++++++----------------------
> 1 file changed, 25 insertions(+), 30 deletions(-)
>
> Index: linux-pm/drivers/cpuidle/governors/menu.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/governors/menu.c
> +++ linux-pm/drivers/cpuidle/governors/menu.c
> @@ -285,9 +285,8 @@ static int menu_select(struct cpuidle_dr
> {
> struct menu_device *data = this_cpu_ptr(&menu_devices);
> int latency_req = cpuidle_governor_latency_req(dev->cpu);
> - int i;
> - int first_idx;
> - int idx;
> + int first_idx = 0;
> + int idx, i;
> unsigned int interactivity_req;
> unsigned int expected_interval;
> unsigned long nr_iowaiters, cpu_load;
> @@ -307,6 +306,18 @@ static int menu_select(struct cpuidle_dr
> /* determine the expected residency time, round up */
> data->next_timer_us = ktime_to_us(tick_nohz_get_sleep_length(&delta_next));
>
> + /*
> + * If the tick is already stopped, the cost of possible short idle
> + * duration misprediction is much higher, because the CPU may be stuck
> + * in a shallow idle state for a long time as a result of it. In that
> + * case say we might mispredict and use the known time till the closest
> + * timer event for the idle state selection.
> + */
> + if (tick_nohz_tick_stopped()) {
> + data->predicted_us = ktime_to_us(delta_next);
> + goto select;
> + }
> +
This introduce two potential issues:
- This will totally ignore the typical pattern in idle loop; I
observed on the mmc driver can trigger multiple times (> 10 times)
with consistent interval; but I have no strong opinion to not
use next timer event for this case.
- Will this break correction factors when the CPU exit from idle?
data->bucket is stale value ....
> get_iowait_load(&nr_iowaiters, &cpu_load);
> data->bucket = which_bucket(data->next_timer_us, nr_iowaiters);
>
> @@ -322,7 +333,6 @@ static int menu_select(struct cpuidle_dr
> expected_interval = get_typical_interval(data);
> expected_interval = min(expected_interval, data->next_timer_us);
>
> - first_idx = 0;
> if (drv->states[0].flags & CPUIDLE_FLAG_POLLING) {
> struct cpuidle_state *s = &drv->states[1];
> unsigned int polling_threshold;
> @@ -344,29 +354,15 @@ static int menu_select(struct cpuidle_dr
> */
> data->predicted_us = min(data->predicted_us, expected_interval);
>
> - if (tick_nohz_tick_stopped()) {
> - /*
> - * If the tick is already stopped, the cost of possible short
> - * idle duration misprediction is much higher, because the CPU
> - * may be stuck in a shallow idle state for a long time as a
> - * result of it. In that case say we might mispredict and try
> - * to force the CPU into a state for which we would have stopped
> - * the tick, unless a timer is going to expire really soon
> - * anyway.
> - */
> - if (data->predicted_us < TICK_USEC)
> - data->predicted_us = min_t(unsigned int, TICK_USEC,
> - ktime_to_us(delta_next));
> - } else {
> - /*
> - * Use the performance multiplier and the user-configurable
> - * latency_req to determine the maximum exit latency.
> - */
> - interactivity_req = data->predicted_us / performance_multiplier(nr_iowaiters, cpu_load);
> - if (latency_req > interactivity_req)
> - latency_req = interactivity_req;
> - }
> + /*
> + * Use the performance multiplier and the user-configurable latency_req
> + * to determine the maximum exit latency.
> + */
> + interactivity_req = data->predicted_us / performance_multiplier(nr_iowaiters, cpu_load);
> + if (latency_req > interactivity_req)
> + latency_req = interactivity_req;
>
> +select:
> expected_interval = data->predicted_us;
> /*
> * Find the idle state with the lowest power while satisfying
> @@ -403,14 +399,13 @@ static int menu_select(struct cpuidle_dr
> * Don't stop the tick if the selected state is a polling one or if the
> * expected idle duration is shorter than the tick period length.
> */
> - if ((drv->states[idx].flags & CPUIDLE_FLAG_POLLING) ||
> - expected_interval < TICK_USEC) {
> + if (((drv->states[idx].flags & CPUIDLE_FLAG_POLLING) ||
> + expected_interval < TICK_USEC) && !tick_nohz_tick_stopped()) {
I am not sure this logic is right... Why not use below checking, so
for POLLING state we will never ask to stop the tick?
if (drv->states[idx].flags & CPUIDLE_FLAG_POLLING ||
(expected_interval < TICK_USEC && !tick_nohz_tick_stopped())) {
> unsigned int delta_next_us = ktime_to_us(delta_next);
>
> *stop_tick = false;
>
> - if (!tick_nohz_tick_stopped() && idx > 0 &&
> - drv->states[idx].target_residency > delta_next_us) {
> + if (idx > 0 && drv->states[idx].target_residency > delta_next_us) {
> /*
> * The tick is not going to be stopped and the target
> * residency of the state to be returned is not within
>
next prev parent reply other threads:[~2018-08-10 9:20 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-10 7:34 [PATCH] cpuidle: menu: Handle stopped tick more aggressively Rafael J. Wysocki
2018-08-10 7:57 ` [PATCH v2] " Rafael J. Wysocki
2018-08-10 9:20 ` leo.yan [this message]
2018-08-10 11:04 ` Rafael J. Wysocki
2018-08-10 12:31 ` leo.yan
2018-08-12 10:07 ` Rafael J. Wysocki
2018-08-12 13:44 ` leo.yan
2018-08-13 7:58 ` Rafael J. Wysocki
2018-08-10 11:15 ` [PATCH v3] " Rafael J. Wysocki
2018-08-12 14:55 ` leo.yan
2018-08-13 8:11 ` Rafael J. Wysocki
2018-08-20 10:15 ` Peter Zijlstra
2018-08-13 11:26 ` [PATCH v4] " Rafael J. Wysocki
2018-08-14 10:34 ` [PATCH v5] " Rafael J. Wysocki
2018-08-14 15:44 ` leo.yan
2018-08-14 17:26 ` Rafael J. Wysocki
2018-08-20 11:04 ` Peter Zijlstra
2018-08-20 11:02 ` Peter Zijlstra
2018-08-11 16:32 ` [PATCH] " kbuild test robot
2018-08-12 22:13 ` Dan Carpenter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180810092034.GF11817@leoy-ThinkPad-X240s \
--to=leo.yan@linaro.org \
--cc=frederic@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=peterz@infradead.org \
--cc=rjw@rjwysocki.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).