From: Viresh Kumar <viresh.kumar@linaro.org>
To: Doug Smythies <dsmythies@telus.net>
Cc: "'Rafael J. Wysocki'" <rafael@kernel.org>,
'Rafael Wysocki' <rjw@rjwysocki.net>,
'Ingo Molnar' <mingo@redhat.com>,
'Peter Zijlstra' <peterz@infradead.org>,
'Linux PM' <linux-pm@vger.kernel.org>,
'Vincent Guittot' <vincent.guittot@linaro.org>,
'Joel Fernandes' <joel@joelfernandes.org>,
"'v4 . 18+'" <stable@vger.kernel.org>,
'Linux Kernel Mailing List' <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] cpufreq: schedutil: Don't skip freq update when limits change
Date: Fri, 26 Jul 2019 12:27:39 +0530 [thread overview]
Message-ID: <20190726065739.xjvyvqpkb3o6m4ty@vireshk-i7> (raw)
In-Reply-To: <000c01d542fc$703ff850$50bfe8f0$@net>
On 25-07-19, 08:20, Doug Smythies wrote:
> I tried the patch ("patch2"). It did not fix the issue.
>
> To summarize, all kernel 5.2 based, all intel_cpufreq driver and schedutil governor:
>
> Test: Does a busy system respond to maximum CPU clock frequency reduction?
>
> stock, unaltered: No.
> revert ecd2884291261e3fddbc7651ee11a20d596bb514: Yes
> viresh patch: No.
> fast_switch edit: No.
> viresh patch2: No.
Hmm, so I tried to reproduce your setup on my ARM board.
- booted only with CPU0 so I hit the sugov_update_single() routine
- And applied below diff to make CPU look permanently busy:
-------------------------8<-------------------------
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 2f382b0959e5..afb47490e5dc 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -121,6 +121,7 @@ static void sugov_fast_switch(struct sugov_policy *sg_policy, u64 time,
if (!sugov_update_next_freq(sg_policy, time, next_freq))
return;
+ pr_info("%s: %d: %u\n", __func__, __LINE__, freq);
next_freq = cpufreq_driver_fast_switch(policy, next_freq);
if (!next_freq)
return;
@@ -424,14 +425,10 @@ static unsigned long sugov_iowait_apply(struct sugov_cpu *sg_cpu, u64 time,
#ifdef CONFIG_NO_HZ_COMMON
static bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu)
{
- unsigned long idle_calls = tick_nohz_get_idle_calls_cpu(sg_cpu->cpu);
- bool ret = idle_calls == sg_cpu->saved_idle_calls;
-
- sg_cpu->saved_idle_calls = idle_calls;
- return ret;
+ return true;
}
#else
-static inline bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu) { return false; }
+static inline bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu) { return true; }
#endif /* CONFIG_NO_HZ_COMMON */
/*
@@ -565,6 +562,7 @@ static void sugov_work(struct kthread_work *work)
sg_policy->work_in_progress = false;
raw_spin_unlock_irqrestore(&sg_policy->update_lock, flags);
+ pr_info("%s: %d: %u\n", __func__, __LINE__, freq);
mutex_lock(&sg_policy->work_lock);
__cpufreq_driver_target(sg_policy->policy, freq, CPUFREQ_RELATION_L);
mutex_unlock(&sg_policy->work_lock);
-------------------------8<-------------------------
Now, the frequency never gets down and so gets set to the maximum
possible after a bit.
- Then I did:
echo <any-low-freq-value> > /sys/devices/system/cpu/cpufreq/policy0/scaling_max_freq
Without my patch applied:
The print never gets printed and so frequency doesn't go down.
With my patch applied:
The print gets printed immediately from sugov_work() and so
the frequency reduces.
Can you try with this diff along with my Patch2 ? I suspect there may
be something wrong with the intel_cpufreq driver as the patch fixes
the only path we have in the schedutil governor which takes busyness
of a CPU into account.
--
viresh
next prev parent reply other threads:[~2019-07-26 6:57 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-18 6:26 [PATCH] Revert "cpufreq: schedutil: Don't set next_freq to UINT_MAX" Doug Smythies
2019-07-18 10:28 ` Viresh Kumar
2019-07-18 15:46 ` Doug Smythies
2019-07-22 6:49 ` Viresh Kumar
2019-07-22 6:51 ` [PATCH] cpufreq: schedutil: Don't skip freq update when limits change Viresh Kumar
2019-07-23 7:10 ` Doug Smythies
2019-07-23 9:13 ` Rafael J. Wysocki
2019-07-23 9:15 ` Viresh Kumar
2019-07-23 10:27 ` Rafael J. Wysocki
2019-07-24 11:43 ` Viresh Kumar
2019-07-25 15:20 ` Doug Smythies
2019-07-26 3:26 ` Viresh Kumar
2019-07-26 6:57 ` Viresh Kumar [this message]
2019-07-29 7:55 ` Doug Smythies
2019-07-29 8:32 ` Viresh Kumar
2019-07-29 8:37 ` Rafael J. Wysocki
2019-08-01 0:20 ` Doug Smythies
2019-08-01 6:17 ` Viresh Kumar
2019-08-01 7:47 ` Rafael J. Wysocki
2019-08-01 7:55 ` Viresh Kumar
2019-08-01 17:57 ` Doug Smythies
2019-08-02 3:48 ` Viresh Kumar
2019-08-02 9:11 ` Rafael J. Wysocki
2019-08-02 9:19 ` Rafael J. Wysocki
2019-08-06 4:00 ` Viresh Kumar
2019-07-31 2:58 Viresh Kumar
2019-07-31 23:19 ` Doug Smythies
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=20190726065739.xjvyvqpkb3o6m4ty@vireshk-i7 \
--to=viresh.kumar@linaro.org \
--cc=dsmythies@telus.net \
--cc=joel@joelfernandes.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=rafael@kernel.org \
--cc=rjw@rjwysocki.net \
--cc=stable@vger.kernel.org \
--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).