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

[Rafael J. Wysocki]

On Wed, Dec 9, 2020 at 6:16 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>
> On 08-12-20, 18:01, Rafael J. Wysocki wrote:
> > On Tue, Dec 8, 2020 at 9:52 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> > >
> > > On 07-12-20, 17:29, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxx>
> > > > ---
> > > >  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.

Ah, so that's in the freq-dependent case.

In the freq-invariant case next_f doesn't depend on the current frequency.

> > > 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.

So I have misunderstood your example.

In the non-invariant case (which is or shortly will be relevant for
everybody interested) cpuinfo.max_freq goes into the calculation
instead of the current frequency and the mapping between util and freq
is linear.  In the freq-dependent case it is not linear, of course.

So I guess the concern is that this changes the behavior in the
freq-dependent case which may not be desirable.

Fair enough, but I'm not sure if that is enough of a reason to avoid
sharing the code between the "perf" and "freq" paths.