Re: [PATCH] cpufreq: schedutil: Don't skip freq update if need_freq_update is set

[Viresh Kumar]

On 30-10-20, 16:23, Rafael J. Wysocki wrote:
> On Fri, Oct 30, 2020 at 4:07 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> >
> > On Fri, Oct 30, 2020 at 8:31 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >
> > Thanks for following my suggestion!
> >
> > > Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> > > ---
> > >  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