On Mon, Feb 22, 2021 at 2:57 PM Yue Hu [off-list ref] wrote:

On Mon, 22 Feb 2021 11:00:14 +0530
Viresh Kumar [off-list ref] wrote:

quoted

On 19-02-21, 19:45, Yue Hu wrote:

quoted

We will set next_f to next_freq(previous freq) if next_f is
reduced for busy CPU. Then the next sugov_update_next_freq() will check
if next_freq matches next_f if need_freq_update is not set.
Obviously, we will do nothing for the case. And The related check to
fast_switch_enabled and raw_spin_{lock,unlock} operations are
unnecessary.

Right, but we will still need sugov_update_next_freq() to have the
same implementation regardless and so I am not sure if we should add

Yes, sugov_update_next_freq() should be keeping current logic for corner case.

quoted

this change:

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 41e498b0008a..7289e1adab73 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c

@@ -362,6 +362,9 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time,
         * recently, as the reduction is likely to be premature then.
         */
        if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {
+               if (!sg_policy->need_freq_update)

The initial purpose about code of `next_f = sg_policy->next_freq` here (for special CPU busy
case) should be skipping the freq update.

Since commit 600f5badb78c ("cpufreq: schedutil: Don't skip freq update when limits change"),
we add the check to busy CPU for not skipping the update, we need to update the freq using
computed one because limits change.

After commit 23a881852f3e ("cpufreq: schedutil: Don't skip freq update if need_freq_update
is set"), we removed the need_freq_update check(no issue of commit 600f5badb78c anymore?)
and introduce to always do an update in sugov_update_next_freq() if need_freq_update is set
even though current freq == sg_policy->next_freq because of corner case issue. But that is
conflict with original purpose of the freq skip code (next_f = sg_policy->next_freq) of
busy CPU.

That's because we realized that it was not always a good idea to skip
the update even if next_f == sg_policy->next_freq.

That's why CPUFREQ_NEED_UPDATE_LIMITS has been introduced and the
current flow is a result of subsequent code rearrangements.