Re: [PATCH v1 2/4] cpufreq: schedutil: Adjust utilization instead of frequency

[Viresh Kumar <viresh.kumar@linaro.org>]

On 08-12-20, 18:01, Rafael J. Wysocki wrote:
> On Tue, Dec 8, 2020 at 9:52 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 07-12-20, 17:29, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > When avoiding reduction of the frequency after the target CPU has
> > > been busy since the previous frequency update, adjust the utilization
> > > instead of adjusting the frequency, because doing so is more prudent
> > > (it is done to counter a possible utilization deficit after all) and
> > > it will allow some code to be shared after a subsequent change.
> > >
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  kernel/sched/cpufreq_schedutil.c |   11 ++++-------
> > >  1 file changed, 4 insertions(+), 7 deletions(-)
> > >
> > > Index: linux-pm/kernel/sched/cpufreq_schedutil.c
> > > ===================================================================
> > > --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
> > > +++ linux-pm/kernel/sched/cpufreq_schedutil.c
> > > @@ -437,7 +437,7 @@ static void sugov_update_single(struct u
> > >  {
> > >       struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util);
> > >       struct sugov_policy *sg_policy = sg_cpu->sg_policy;
> > > -     unsigned int cached_freq = sg_policy->cached_raw_freq;
> > > +     unsigned long prev_util = sg_cpu->util;
> > >       unsigned int next_f;
> > >
> > >       sugov_iowait_boost(sg_cpu, time, flags);
> > > @@ -451,17 +451,14 @@ static void sugov_update_single(struct u
> > >       sugov_get_util(sg_cpu);
> > >       sugov_iowait_apply(sg_cpu, time);
> > >
> > > -     next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);
> > >       /*
> > >        * Do not reduce the frequency if the CPU has not been idle
> > >        * recently, as the reduction is likely to be premature then.
> > >        */
> > > -     if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {
> > > -             next_f = sg_policy->next_freq;
> > > +     if (sugov_cpu_is_busy(sg_cpu) && sg_cpu->util < prev_util)
> > > +             sg_cpu->util = prev_util;
> > >
> > > -             /* Restore cached freq as next_freq has changed */
> > > -             sg_policy->cached_raw_freq = cached_freq;
> > > -     }
> > > +     next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max);
> >
> > I don't think we can replace freq comparison by util, or at least it will give
> > us a different final frequency and the behavior is changed.
> >
> > Lets take an example, lets say current freq is 1 GHz and max is 1024.
> >
> > Round 1: Lets say util is 1000
> >
> > next_f = 1GHz * 1.25 * 1000/1024 = 1.2 GHz
> >
> > Round 2: Lets say util has come down to 900 here,
> >
> > before the patch:
> >
> > next_f = 1.2 GHz * 1.25 * 900/1024 = 1.31 GHz
> >
> > after the patch:
> >
> > next_f = 1.2 GHz * 1.25 * 1000/1024 = 1.45 GHz
> >
> > Or did I make a mistake here ?
> 
> I think so, if my understanding is correct.
> 
> Without the patch, next_f will be reset to the previous value
> (sq_policy->next_freq) if the CPU has been busy and the (new) next_f
> is less than that value.
> 
> So the "new" next_f before the patch is 1.31 GHz, but because it is
> less than the previous value (1.45 GHz), it will be reset to that
> value, unless I'm missing something.

The prev frequency here was 1.2 GHz (after Round 1). 1.45 GHz is the
value we get after this patch, as we take the earlier utilization
(1000) into account instead of 900.

> Overall, the patch doesn't change the logic AFAICS and because the
> util->freq mapping is linear, all of the inequalities map exactly from
> one to the other (both ways).

-- 
viresh