Messages in this thread Patch in this message | | | From | "Rafael J. Wysocki" <> | Date | Fri, 30 Oct 2020 16:23:23 +0100 | Subject | Re: [PATCH] cpufreq: schedutil: Don't skip freq update if need_freq_update is set |
| |
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.
So I'm going to make this change while applying the patch.
> > + } > > > > 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!
| |