On Mon, Feb 8, 2021 at 4:08 AM Yue Hu <zbest...@gmail.com> wrote: > > From: Yue Hu <huy...@yulong.com> > > The limits_changed flag was introduced by commit 600f5badb78c > ("cpufreq: schedutil: Don't skip freq update when limits change") due > to race condition where need_freq_update is cleared in get_next_freq() > which causes reducing the CPU frequency is ineffective while busy. > > But now, the race condition above is gone because get_next_freq() > doesn't clear the flag any more after commit 23a881852f3e ("cpufreq: > schedutil: Don't skip freq update if need_freq_update is set"). > > Moreover, need_freq_update currently will be set to true only in > sugov_should_update_freq() if CPUFREQ_NEED_UPDATE_LIMITS is not set > for the driver. However, limits may have changed at any time.
Yes, they may change at any time. > And subsequent frequence update is depending on need_freq_update. I'm not following, sorry. need_freq_update is set in sugov_should_update_freq() when limits_changed is cleared and it cannot be modified until sugov_update_next_freq() runs on the same CPU. > So, we may skip this update. I'm not sure why? > Hence, let's remove it to avoid above issue and make code more simple. > > Signed-off-by: Yue Hu <huy...@yulong.com> > --- > kernel/sched/cpufreq_schedutil.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/kernel/sched/cpufreq_schedutil.c > b/kernel/sched/cpufreq_schedutil.c > index 41e498b..7dd85fb 100644 > --- a/kernel/sched/cpufreq_schedutil.c > +++ b/kernel/sched/cpufreq_schedutil.c > @@ -40,7 +40,6 @@ struct sugov_policy { > struct task_struct *thread; > bool work_in_progress; > > - bool limits_changed; > bool need_freq_update; > }; > > @@ -89,11 +88,8 @@ static bool sugov_should_update_freq(struct sugov_policy > *sg_policy, u64 time) > if (!cpufreq_this_cpu_can_update(sg_policy->policy)) > return false; > > - if (unlikely(sg_policy->limits_changed)) { > - sg_policy->limits_changed = false; > - sg_policy->need_freq_update = true; > + if (unlikely(sg_policy->need_freq_update)) > return true; > - } > > delta_ns = time - sg_policy->last_freq_update_time; > > @@ -323,7 +319,7 @@ static bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu) > static inline void ignore_dl_rate_limit(struct sugov_cpu *sg_cpu, struct > sugov_policy *sg_policy) > { > if (cpu_bw_dl(cpu_rq(sg_cpu->cpu)) > sg_cpu->bw_dl) > - sg_policy->limits_changed = true; > + sg_policy->need_freq_update = true; > } > > static inline bool sugov_update_single_common(struct sugov_cpu *sg_cpu, > @@ -759,7 +755,6 @@ static int sugov_start(struct cpufreq_policy *policy) > sg_policy->last_freq_update_time = 0; > sg_policy->next_freq = 0; > sg_policy->work_in_progress = false; > - sg_policy->limits_changed = false; > sg_policy->cached_raw_freq = 0; > > sg_policy->need_freq_update = > cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS); > @@ -813,7 +808,7 @@ static void sugov_limits(struct cpufreq_policy *policy) > mutex_unlock(&sg_policy->work_lock); > } > > - sg_policy->limits_changed = true; > + sg_policy->need_freq_update = true; This may be running in parallel with sugov_update_next_freq() on a different CPU, so the latter may clear need_freq_update right after it has been set here unless I'm overlooking something. > } > > struct cpufreq_governor schedutil_gov = { > -- > 1.9.1 >