linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Viresh Kumar <viresh.kumar@linaro.org>
To: Rafael Wysocki <rjw@rjwysocki.net>,
	Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>
Cc: Viresh Kumar <viresh.kumar@linaro.org>,
	linaro-kernel@lists.linaro.org, linux-pm@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Juri Lelli <Juri.Lelli@arm.com>,
	patrick.bellasi@arm.com, john.ettedgui@gmail.com,
	Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
	Joel Fernandes <joelaf@google.com>,
	Morten Rasmussen <morten.rasmussen@arm.com>
Subject: [PATCH 1/3] cpufreq: schedutil: Restore cached_raw_freq behavior
Date: Fri,  9 Jun 2017 15:45:54 +0530	[thread overview]
Message-ID: <f979ca208e4582b7db1eed17df5543d4b70d5de7.1497002895.git.viresh.kumar@linaro.org> (raw)
In-Reply-To: <cover.1497002895.git.viresh.kumar@linaro.org>
In-Reply-To: <cover.1497002895.git.viresh.kumar@linaro.org>

The purpose of the "cached_raw_freq" field is to avoid a call to
cpufreq_driver_resolve_freq() when the next selected frequency is same
as the one selected last time and we can use sg_policy->next_freq then.

With the recent changes (reduce frequencies slower), we update the next
frequency at the very last moment from sugov_update_commit() and that
breaks the working of cached_raw_freq somewhat.

Here is an example to illustrate what happens now.
Min freq: 1 GHz
Max and current freq: 4 GHz

- get_next_freq() gets called and it calculates the next frequency to be
  1 GHz and so it updates cached_raw_freq as 1 GHz as well. It then
  calls cpufreq_driver_resolve_freq() and that also returns 1 GHz.

- We then call sugov_update_commit() and it updates the
  sg_policy->next_freq to 2.5 GHz.

- get_next_freq() gets called again and this time it calculates the next
  frequency as 2.5 GHz. Even when the previous next_freq was set to 2.5
  GHz, we end up calling cpufreq_driver_resolve_freq() as
  cached_raw_freq was set to 1 GHz.

Moreover, it is not right to update the target frequency after we have
called cpufreq_driver_resolve_freq() as that was called to map the
target frequency to the driver supported one, so that we can avoid the
update completely if we are already at the driver supported frequency.

Fix this by moving the newly added code to get_next_freq() before the
cached_raw_freq is accessed/updated.

Also add a minor comment above that code to explain why it is done.

We cannot take a simple average anymore as the "freq" here can be well
below policy->min and we may end up reducing the frequency drastically.
Take care of that by making sure "freq" is at least as much as
policy->min.

Fixes: 39b64aa1c007 ("cpufreq: schedutil: Reduce frequencies slower")
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/sched/cpufreq_schedutil.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 622eed1b7658..1852bd73d903 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -101,9 +101,6 @@ static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
 	if (sg_policy->next_freq == next_freq)
 		return;
 
-	if (sg_policy->next_freq > next_freq)
-		next_freq = (sg_policy->next_freq + next_freq) >> 1;
-
 	sg_policy->next_freq = next_freq;
 	sg_policy->last_freq_update_time = time;
 
@@ -151,6 +148,17 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
 
 	freq = (freq + (freq >> 2)) * util / max;
 
+	/*
+	 * Reduce frequency gradually to avoid undesirable performance drops.
+	 * Before that we need to make sure that freq >= policy->min, else we
+	 * may still end up reducing frequency rapidly.
+	 */
+	if (freq < policy->min)
+		freq = policy->min;
+
+	if (sg_policy->next_freq > freq)
+		freq = (sg_policy->next_freq + freq) >> 1;
+
 	if (freq == sg_policy->cached_raw_freq && sg_policy->next_freq != UINT_MAX)
 		return sg_policy->next_freq;
 	sg_policy->cached_raw_freq = freq;
-- 
2.13.0.70.g6367777092d9

  reply	other threads:[~2017-06-09 10:15 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-06-09 10:15 [PATCH 0/3] cpufreq: schedutil: Fix 4.12 regressions Viresh Kumar
2017-06-09 10:15 ` Viresh Kumar [this message]
2017-06-09 10:15 ` [PATCH 2/3] cpufreq: schedutil: Fix selection algorithm while reducing frequency Viresh Kumar
2017-06-10  9:11   ` Joel Fernandes
2017-06-11  6:21     ` Joel Fernandes
2017-06-12  3:44       ` Viresh Kumar
2017-06-12 12:33         ` Rafael J. Wysocki
2017-06-09 10:15 ` [PATCH 3/3] cpufreq: intel_pstate: Provide resolve_freq() to fix regression Viresh Kumar
2017-06-10  9:26   ` Joel Fernandes
2017-06-12  3:41     ` Viresh Kumar
2017-06-09 12:24 ` [PATCH 0/3] cpufreq: schedutil: Fix 4.12 regressions Rafael J. Wysocki
2017-06-09 12:32   ` Viresh Kumar
2017-06-09 13:25     ` Rafael J. Wysocki

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=f979ca208e4582b7db1eed17df5543d4b70d5de7.1497002895.git.viresh.kumar@linaro.org \
    --to=viresh.kumar@linaro.org \
    --cc=Juri.Lelli@arm.com \
    --cc=joelaf@google.com \
    --cc=john.ettedgui@gmail.com \
    --cc=linaro-kernel@lists.linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=morten.rasmussen@arm.com \
    --cc=patrick.bellasi@arm.com \
    --cc=peterz@infradead.org \
    --cc=rjw@rjwysocki.net \
    --cc=srinivas.pandruvada@linux.intel.com \
    --cc=vincent.guittot@linaro.org \
    /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).