Re: [PATCH] cpufreq: schedutil: set sg_policy->next_freq to the final cpufreq

[Viresh Kumar]

On 29-10-20, 19:17, zhuguangqing83 wrote:
> I think your patch is ok for tackling this problem.

Great.

> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index 0c5c61a095f6..8991cc31b011 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -121,13 +121,8 @@ static void sugov_fast_switch(struct sugov_policy *sg_policy, u64 time,
> >  static void sugov_deferred_update(struct sugov_policy *sg_policy, u64 time,
> >                                   unsigned int next_freq)
> >  {
> > -       if (!sugov_update_next_freq(sg_policy, time, next_freq))
> > -               return;
> > -
> > -       if (!sg_policy->work_in_progress) {
> > -               sg_policy->work_in_progress = true;
> > +       if (sugov_update_next_freq(sg_policy, time, next_freq))
> >                 irq_work_queue(&sg_policy->irq_work);
> > -       }
> >  }
> > 
> >  /**
> > @@ -159,6 +154,15 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
> >         unsigned int freq = arch_scale_freq_invariant() ?
> >                                 policy->cpuinfo.max_freq : policy->cur;
> > 
> > +       /*
> > +        * The previous frequency update didn't go as we expected it to be. Lets
> > +        * start again to make sure we don't miss any updates.
> > +        */
> > +       if (unlikely(policy->cur != sg_policy->next_freq)) {
> > +               sg_policy->next_freq = 0;
> > +               sg_policy->cached_raw_freq = 0;
> > +       }
> > +
> >         freq = map_util_freq(util, freq, max);
> > 
> >         if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
> > @@ -337,8 +341,14 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
> > 
> >         ignore_dl_rate_limit(sg_cpu, sg_policy);
> > 
> > +       if (!sg_policy->policy->fast_switch_enabled) {
> > +               raw_spin_lock(&sg_policy->update_lock);
> > +               if (sg_policy->work_in_progress)
> > +                       goto unlock;
> > +       }
> > +
> 
> Maybe it's better to bring the following code before the code above.
>          if (!sugov_should_update_freq(sg_policy, time))
>                return;

Maybe not. We want to avoid everything in case a freq-update is on the
way elsewhere as there are other flags we are touching in
sugov_should_update_freq().

I will send a proper patch for this shortly. It would be helpful if
you can give it a go and provide your tested-by.

-- 
viresh