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 13:51:48 +0530
From:   Viresh Kumar <viresh.kumar@linaro.org>
To:     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: <20201028082148.zkvcqau4p6xcihoq@vireshk-i7>
References: <20201027115459.19318-1-zhuguangqing83@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201027115459.19318-1-zhuguangqing83@gmail.com>
User-Agent: NeoMutt/20180716-391-311a52
Precedence: bulk

On 27-10-20, 19:54, zhuguangqing83@gmail.com wrote:
> 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.

Understood until here, but not sure I followed everything after that.

> 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.

FWIW, whenever policy->min/max gets changed, sg_policy->limits_changed
gets set to true by sugov_limits() and the next time schedutil
callback gets called from the scheduler, we will fix the frequency.

And so there shouldn't be any issue here, unless I am missing
something.

-- 
viresh