From: zhuguangqing83@gmail.com
To: viresh.kumar@linaro.org, rjw@rjwysocki.net, mingo@redhat.com,
peterz@infradead.org, juri.lelli@redhat.com,
vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de,
bristot@redhat.com
Cc: linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
zhuguangqing <zhuguangqing@xiaomi.com>
Subject: [PATCH] cpufreq: schedutil: set sg_policy->next_freq to the final cpufreq
Date: Tue, 27 Oct 2020 19:54:59 +0800 [thread overview]
Message-ID: <20201027115459.19318-1-zhuguangqing83@gmail.com> (raw)
From: zhuguangqing <zhuguangqing@xiaomi.com>
In the following code path, next_freq is clamped between policy->min
and policy->max twice in functions cpufreq_driver_resolve_freq() and
cpufreq_driver_fast_switch(). For there is no update_lock in the code
path, policy->min and policy->max may be modified (one or more times),
so sg_policy->next_freq updated in function sugov_update_next_freq()
may be not the final cpufreq. Next time when we use
"if (sg_policy->next_freq == next_freq)" to judge whether to update
next_freq, we may get a wrong result.
-> sugov_update_single()
-> get_next_freq()
-> cpufreq_driver_resolve_freq()
-> sugov_fast_switch()
-> sugov_update_next_freq()
-> cpufreq_driver_fast_switch()
For example, at first sg_policy->next_freq is 1 GHz, but the final
cpufreq is 1.2 GHz because policy->min is modified to 1.2 GHz when
we reached cpufreq_driver_fast_switch(). Then next time, policy->min
is changed before we reached cpufreq_driver_resolve_freq() and (assume)
next_freq is 1 GHz, we find "if (sg_policy->next_freq == next_freq)" is
satisfied so we don't change the cpufreq. Actually we should change
the cpufreq to 1.0 GHz this time.
Signed-off-by: zhuguangqing <zhuguangqing@xiaomi.com>
---
drivers/cpufreq/cpufreq.c | 6 +++---
include/linux/cpufreq.h | 2 +-
kernel/sched/cpufreq_schedutil.c | 21 ++++++++++-----------
3 files changed, 14 insertions(+), 15 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index f4b60663efe6..7e8e03c7506b 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2057,13 +2057,13 @@ EXPORT_SYMBOL(cpufreq_unregister_notifier);
* error condition, the hardware configuration must be preserved.
*/
unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
- unsigned int target_freq)
+ unsigned int *target_freq)
{
unsigned int freq;
int cpu;
- target_freq = clamp_val(target_freq, policy->min, policy->max);
- freq = cpufreq_driver->fast_switch(policy, target_freq);
+ *target_freq = clamp_val(*target_freq, policy->min, policy->max);
+ freq = cpufreq_driver->fast_switch(policy, *target_freq);
if (!freq)
return 0;
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index fa37b1c66443..790df38d48de 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -569,7 +569,7 @@ struct cpufreq_governor {
/* Pass a target to the cpufreq driver */
unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
- unsigned int target_freq);
+ unsigned int *target_freq);
int cpufreq_driver_target(struct cpufreq_policy *policy,
unsigned int target_freq,
unsigned int relation);
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index e254745a82cb..38d2dc55dd95 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -99,31 +99,30 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
return delta_ns >= sg_policy->freq_update_delay_ns;
}
-static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
+static inline bool sugov_update_next_freq(struct sugov_policy *sg_policy,
unsigned int next_freq)
{
- if (sg_policy->next_freq == next_freq)
- return false;
-
- sg_policy->next_freq = next_freq;
- sg_policy->last_freq_update_time = time;
-
- return true;
+ return sg_policy->next_freq == next_freq ? false : true;
}
static void sugov_fast_switch(struct sugov_policy *sg_policy, u64 time,
unsigned int next_freq)
{
- if (sugov_update_next_freq(sg_policy, time, next_freq))
- cpufreq_driver_fast_switch(sg_policy->policy, next_freq);
+ if (sugov_update_next_freq(sg_policy, next_freq)) {
+ cpufreq_driver_fast_switch(sg_policy->policy, &next_freq);
+ sg_policy->next_freq = next_freq;
+ sg_policy->last_freq_update_time = time;
+ }
}
static void sugov_deferred_update(struct sugov_policy *sg_policy, u64 time,
unsigned int next_freq)
{
- if (!sugov_update_next_freq(sg_policy, time, next_freq))
+ if (!sugov_update_next_freq(sg_policy, next_freq))
return;
+ sg_policy->next_freq = next_freq;
+ sg_policy->last_freq_update_time = time;
if (!sg_policy->work_in_progress) {
sg_policy->work_in_progress = true;
irq_work_queue(&sg_policy->irq_work);
--
2.17.1
next reply other threads:[~2020-10-27 11:55 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-27 11:54 zhuguangqing83 [this message]
2020-10-28 8:21 ` [PATCH] cpufreq: schedutil: set sg_policy->next_freq to the final cpufreq Viresh Kumar
2020-10-28 11:03 zhuguangqing83
2020-10-28 15:35 ` Viresh Kumar
2020-10-29 1:43 zhuguangqing83
2020-10-29 7:19 ` Viresh Kumar
2020-10-29 12:52 ` Rafael J. Wysocki
2020-10-29 11:17 zhuguangqing83
2020-10-29 11:26 ` Viresh Kumar
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=20201027115459.19318-1-zhuguangqing83@gmail.com \
--to=zhuguangqing83@gmail.com \
--cc=bristot@redhat.com \
--cc=bsegall@google.com \
--cc=dietmar.eggemann@arm.com \
--cc=juri.lelli@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=mgorman@suse.de \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=rjw@rjwysocki.net \
--cc=rostedt@goodmis.org \
--cc=vincent.guittot@linaro.org \
--cc=viresh.kumar@linaro.org \
--cc=zhuguangqing@xiaomi.com \
/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).