On Fri, Oct 30, 2020 at 8:31 AM Viresh Kumar <viresh.ku...@linaro.org> wrote:
>
> The cpufreq policy's frequency limits (min/max) can get changed at any
> point of time, while schedutil is trying to update the next frequency.
> Though the schedutil governor has necessary locking and support in place
> to make sure we don't miss any of those updates, there is a corner case
> where the governor will find that the CPU is already running at the
> desired frequency and so may skip an update.
>
> For example, consider that the CPU can run at 1 GHz, 1.2 GHz and 1.4 GHz
> and is running at 1 GHz currently. Schedutil tries to update the
> frequency to 1.2 GHz, during this time the policy limits get changed as
> policy->min = 1.4 GHz. As schedutil (and cpufreq core) does clamp the
> frequency at various instances, we will eventually set the frequency to
> 1.4 GHz, while we will save 1.2 GHz in sg_policy->next_freq.
>
> Now lets say the policy limits get changed back at this time with
> policy->min as 1 GHz. The next time schedutil is invoked by the
> scheduler, we will reevaluate the next frequency (because
> need_freq_update will get set due to limits change event) and lets say
> we want to set the frequency to 1.2 GHz again. At this point
> sugov_update_next_freq() will find the next_freq == current_freq and
> will abort the update, while the CPU actually runs at 1.4 GHz.
>
> Until now need_freq_update was used as a flag to indicate that the
> policy's frequency limits have changed, and that we should consider the
> new limits while reevaluating the next frequency.
>
> This patch fixes the above mentioned issue by extending the purpose of
> the need_freq_update flag. If this flag is set now, the schedutil
> governor will not try to abort a frequency change even if next_freq ==
> current_freq.
>
> As similar behavior is required in the case of
> CPUFREQ_NEED_UPDATE_LIMITS flag as well, need_freq_update will never be
> set to false if that flag is set for the driver.
>
> We also don't need to consider the need_freq_update flag in
> sugov_update_single() anymore to handle the special case of busy CPU, as
> we won't abort a frequency update anymore.
>
> Reported-by: zhuguangqing <zhuguangq...@xiaomi.com>
> Suggested-by: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
Thanks for following my suggestion!
> Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
> ---
> kernel/sched/cpufreq_schedutil.c | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c
> b/kernel/sched/cpufreq_schedutil.c
> index c03a5775d019..c6861be02c86 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -102,9 +102,12 @@ static bool sugov_should_update_freq(struct sugov_policy
> *sg_policy, u64 time)
> static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
> unsigned int next_freq)
> {
> - if (sg_policy->next_freq == next_freq &&
> - !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
> - return false;
> + if (!sg_policy->need_freq_update) {
> + if (sg_policy->next_freq == next_freq)
> + return false;
> + } else if (!cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS)) {
> + sg_policy->need_freq_update = false;
> + }
>
> sg_policy->next_freq = next_freq;
> sg_policy->last_freq_update_time = time;
> @@ -162,11 +165,9 @@ static unsigned int get_next_freq(struct sugov_policy
> *sg_policy,
>
> freq = map_util_freq(util, freq, max);
>
> - if (freq == sg_policy->cached_raw_freq &&
> !sg_policy->need_freq_update &&
> - !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
> + if (freq == sg_policy->cached_raw_freq &&
> !sg_policy->need_freq_update)
> return sg_policy->next_freq;
>
> - sg_policy->need_freq_update = false;
> sg_policy->cached_raw_freq = freq;
> return cpufreq_driver_resolve_freq(policy, freq);
> }
> @@ -442,7 +443,6 @@ static void sugov_update_single(struct update_util_data
> *hook, u64 time,
> struct sugov_policy *sg_policy = sg_cpu->sg_policy;
> unsigned long util, max;
> unsigned int next_f;
> - bool busy;
> unsigned int cached_freq = sg_policy->cached_raw_freq;
>
> sugov_iowait_boost(sg_cpu, time, flags);
> @@ -453,9 +453,6 @@ static void sugov_update_single(struct update_util_data
> *hook, u64 time,
> if (!sugov_should_update_freq(sg_policy, time))
> return;
>
> - /* Limits may have changed, don't skip frequency update */
> - busy = !sg_policy->need_freq_update && sugov_cpu_is_busy(sg_cpu);
> -
> util = sugov_get_util(sg_cpu);
> max = sg_cpu->max;
> util = sugov_iowait_apply(sg_cpu, time, util, max);
> @@ -464,7 +461,7 @@ static void sugov_update_single(struct update_util_data
> *hook, u64 time,
> * Do not reduce the frequency if the CPU has not been idle
> * recently, as the reduction is likely to be premature then.
> */
> - if (busy && next_f < sg_policy->next_freq) {
> + if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {
> next_f = sg_policy->next_freq;
>
> /* Restore cached freq as next_freq has changed */
> @@ -829,9 +826,10 @@ static int sugov_start(struct cpufreq_policy *policy)
> sg_policy->next_freq = 0;
> sg_policy->work_in_progress = false;
> sg_policy->limits_changed = false;
> - sg_policy->need_freq_update = false;
> sg_policy->cached_raw_freq = 0;
>
> + sg_policy->need_freq_update =
> cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);
> +
> for_each_cpu(cpu, policy->cpus) {
> struct sugov_cpu *sg_cpu = &per_cpu(sugov_cpu, cpu);
>
> --
I'll queue it up for -rc3 next week, thanks!
