| Message ID | 207ae817a778d79a99c30cb48f2ea1f527416519.1604042421.git.viresh.kumar@linaro.org |
|---|---|
| State | Accepted |
| Commit | 23a881852f3eff6a7ba8d240b57de076763fdef9 |
| Headers | show |
| Series | cpufreq: schedutil: Don't skip freq update if need_freq_update is set | expand |
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; > + } > > 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!
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!
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
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);
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> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- kernel/sched/cpufreq_schedutil.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)