Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 28 Oct 2020 21:05:16 +0530
From:   Viresh Kumar <viresh.kumar@linaro.org>
To:     zhuguangqing83 <zhuguangqing83@gmail.com>
Cc:     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, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        'zhuguangqing' <zhuguangqing@xiaomi.com>
Subject: Re: [PATCH] cpufreq: schedutil: set sg_policy->next_freq to the
 final cpufreq
Message-ID: <20201028153516.5aimftwtjbqgayqc@vireshk-i7>
References: <083a01d6ad19$fbdfbca0$f39f35e0$@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <083a01d6ad19$fbdfbca0$f39f35e0$@gmail.com>
User-Agent: NeoMutt/20180716-391-311a52
Precedence: bulk

On 28-10-20, 19:03, zhuguangqing83 wrote:
> Thanks for your comments. Maybe my description was not clear before.
> 
> If I understand correctly, when policy->min/max get changed in the time
> Window between get_next_freq() and sugov_fast_switch(), to be more
> precise between cpufreq_driver_resolve_freq() and
> cpufreq_driver_fast_switch(), the issue may happen.
> 
> For example, the first time schedutil callback gets called from the
> scheduler, we reached get_next_freq() and calculate the next_freq,
> suppose next_freq is 1.0 GHz, then sg_policy->next_freq is updated
> to 1.0 GHz in sugov_update_next_freq(). If policy->min/max get
> change right now, suppose policy->min is changed to 1.2 GHz, 
> then the final next_freq is 1.2 GHz for there is another clamp
> between policy->min and policy->max in cpufreq_driver_fast_switch().
> Then sg_policy->next_freq(1.0 GHz) is not the final next_freq(1.2 GHz).
> 
> The second time schedutil callback gets called from the scheduler, there
> are two issues:
> (1) Suppose policy->min is still 1.2 GHz, we reached get_next_freq() and
> calculate the next_freq, because sg_policy->limits_changed gets set to
> true by sugov_limits() and there is a clamp between policy->min and
> policy->max, so this time next_freq will be greater than or equal to 1.2
> GHz, suppose next_freq is also 1.2 GHz. Now next_freq is 1.2 GHz and
> sg_policy->next_freq is 1.0 GHz,  then we find
> "if (sg_policy->next_freq == next_freq)" is not satisfied and we call
> cpufreq driver to change the cpufreq to 1.2 GHz. Actually it's already
> 1.2 GHz, it's not necessary to change this time.

This isn't that bad, but ...

> (2) Suppose policy->min was changed again to 1.0 GHz before, we reached
> get_next_freq() and calculate the next_freq, suppose next_freq is also
> 1.0 GHz. Now next_freq is 1.0 GHz and sg_policy->next_freq is also 1.0 GHz,
> then we find "if (sg_policy->next_freq == next_freq)" is satisfied and we
> don't change the cpufreq. Actually we should change the cpufreq to 1.0 GHz
> this time.

This is a real problem we can get into. What about this diff instead ?

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 0c5c61a095f6..bf7800e853d3 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -105,7 +105,6 @@ static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
        if (sg_policy->next_freq == next_freq)
                return false;
 
-       sg_policy->next_freq = next_freq;
        sg_policy->last_freq_update_time = time;
 
        return true;
@@ -115,7 +114,7 @@ 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);
+               sg_policy->next_freq = cpufreq_driver_fast_switch(sg_policy->policy, next_freq);
 }
 
 static void sugov_deferred_update(struct sugov_policy *sg_policy, u64 time,
@@ -124,6 +123,7 @@ static void sugov_deferred_update(struct sugov_policy *sg_policy, u64 time,
        if (!sugov_update_next_freq(sg_policy, time, next_freq))
                return;
 
+       sg_policy->next_freq = next_freq;
        if (!sg_policy->work_in_progress) {
                sg_policy->work_in_progress = true;
                irq_work_queue(&sg_policy->irq_work);

-- 
viresh