Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 29 Oct 2020 16:56:24 +0530
From:   Viresh Kumar <viresh.kumar@linaro.org>
To:     zhuguangqing83 <zhuguangqing83@gmail.com>
Cc:     rjw@rjwysocki.net, mingo@redhat.com, peterz@infradead.org,
        juri.lelli@redhat.com, vincent.guittot@linaro.org,
        dietmar.eggemann@arm.com, rostedt@goodmis.org, bsegall@google.com,
        mgorman@suse.de, bristot@redhat.com, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        'zhuguangqing' <zhuguangqing@xiaomi.com>
Subject: Re: [PATCH] cpufreq: schedutil: set sg_policy->next_freq to the
 final cpufreq
Message-ID: <20201029112624.zvl6yh7fgqautzry@vireshk-i7>
References: <004001d6ade5$0f8dde00$2ea99a00$@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <004001d6ade5$0f8dde00$2ea99a00$@gmail.com>
User-Agent: NeoMutt/20180716-391-311a52
Precedence: bulk

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