Messages in this thread | | | Date | Mon, 2 Nov 2020 10:13:12 +0530 | From | Viresh Kumar <> | Subject | Re: [PATCH] cpufreq: schedutil: Don't skip freq update if need_freq_update is set |
| |
On 30-10-20, 16:23, Rafael J. Wysocki wrote: > On Fri, Oct 30, 2020 at 4:07 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Fri, Oct 30, 2020 at 8:31 AM Viresh Kumar <viresh.kumar@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 <zhuguangqing@xiaomi.com> > > > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Thanks for following my suggestion! > > > > > Signed-off-by: Viresh Kumar <viresh.kumar@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; > > One nit, though. > > This can be changed into > > } else { > sg_policy->need_freq_update = > cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS); > } > > to save a branch and because need_freq_update is there in the cache > already, this should be a fast update.
Nice.
-- viresh
| |