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:24:40 +0530
From:   Viresh Kumar <viresh.kumar@linaro.org>
To:     "Rafael J. Wysocki" <rafael@kernel.org>
Cc:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Linux PM <linux-pm@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        Zhang Rui <rui.zhang@intel.com>
Subject: Re: [PATCH v2.1 4/4] cpufreq: schedutil: Always call driver if
 need_freq_update is set
Message-ID: <20201029105440.gcaizotprhloxiih@vireshk-i7>
References: <2183878.gTFULuzKx9@kreacher>
 <1905098.zDJocX6404@kreacher>
 <12275472.W5IoEtXICo@kreacher>
 <20201028035702.75f6rnbkvfaic4si@vireshk-i7>
 <CAJZ5v0ikw9M4-NOEqtoxqs_948iqaX4P5euiXD+VmpaDHd91vg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAJZ5v0ikw9M4-NOEqtoxqs_948iqaX4P5euiXD+VmpaDHd91vg@mail.gmail.com>
User-Agent: NeoMutt/20180716-391-311a52
Precedence: bulk

On 29-10-20, 11:42, Rafael J. Wysocki wrote:
> On Thu, Oct 29, 2020 at 12:10 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 27-10-20, 16:35, Rafael J. Wysocki wrote:
> > > Index: linux-pm/kernel/sched/cpufreq_schedutil.c
> > > ===================================================================
> > > --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
> > > +++ linux-pm/kernel/sched/cpufreq_schedutil.c
> > > @@ -102,11 +102,12 @@ static bool sugov_should_update_freq(str
> > >  static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
> > >                                  unsigned int next_freq)
> > >  {
> > > -     if (sg_policy->next_freq == next_freq)
> > > +     if (sg_policy->next_freq == next_freq && !sg_policy->need_freq_update)
> > >               return false;
> > >
> > >       sg_policy->next_freq = next_freq;
> > >       sg_policy->last_freq_update_time = time;
> > > +     sg_policy->need_freq_update = false;
> > >
> > >       return true;
> > >  }
> > > @@ -161,10 +162,12 @@ static unsigned int get_next_freq(struct
> > >
> > >       freq = map_util_freq(util, freq, max);
> > >
> > > -     if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
> > > +     if (cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
> > > +             sg_policy->need_freq_update = true;
> > > +     else 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);
> > >  }
> >
> > What about just this instead ?
> >
> >   static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
> >                                    unsigned int next_freq)
> >   {
> >  -      if (sg_policy->next_freq == next_freq)
> >  +      if (sg_policy->next_freq == next_freq &&
> >  +          !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
> >                 return false;
> >
> >         sg_policy->next_freq = next_freq;
> >         sg_policy->last_freq_update_time = time;
> >
> >         return true;
> >   }
> >
> 
> Without any changes in get_next_freq() this is not sufficient, because
> get_next_freq() may skip the update too.
> 
> If the intention is to always let the driver callback run when
> CPUFREQ_NEED_UPDATE_LIMITS is set, then both get_next_freq() and
> sugov_update_next_freq() need to be modified.

Right, my mistake. I was just suggesting that we may not need to touch
need_freq_update at all but just check the flag.

-- 
viresh