Received-SPF: pass (google.com: domain of linux-kernel+bounces-25498-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <ZTz9RpZxfxysYCmt@gmail.com> <ZZwBi/YmnMqm7zrO@gmail.com>
 <CAHk-=wgWcYX2oXKtgvNN2LLDXP7kXkbo-xTfumEjmPbjSer2RQ@mail.gmail.com>
 <CAHk-=wiXpsxMcQb7MhL-AxOityTajK0G8eWeBOzX-qBJ9X2DSw@mail.gmail.com>
 <CAHk-=wjK28MUqBZzBSMEM8vdJhDOuXGSWPmmp04GEt9CXtW6Pw@mail.gmail.com>
 <20240114091240.xzdvqk75ifgfj5yx@wyes-pc> <ZaPC7o44lEswxOXp@vingu-book>
 <20240114123759.pjs7ctexcpc6pshl@wyes-pc> <CAKfTPtCz+95dR38c_u6_7JbkVt=czj5N2dKYVV-zk9dgbk16hw@mail.gmail.com>
 <20240114151250.5wfexq44o3mdm3nh@airbuntu>
In-Reply-To: <20240114151250.5wfexq44o3mdm3nh@airbuntu>
From: Vincent Guittot <vincent.guittot@linaro.org>
Date: Sun, 14 Jan 2024 16:20:58 +0100
Message-ID: <CAKfTPtAMxiTbvAYav1JQw+MhjzDPCZDrMLL2JOfsc0GWp+FnOA@mail.gmail.com>
Subject: Re: [GIT PULL] Scheduler changes for v6.8
To: Qais Yousef <qyousef@layalina.io>
Cc: Wyes Karny <wkarny@gmail.com>, Linus Torvalds <torvalds@linux-foundation.org>, 
	Ingo Molnar <mingo@kernel.org>, linux-kernel@vger.kernel.org, 
	Peter Zijlstra <peterz@infradead.org>, Thomas Gleixner <tglx@linutronix.de>, 
	Juri Lelli <juri.lelli@redhat.com>, Dietmar Eggemann <dietmar.eggemann@arm.com>, 
	Steven Rostedt <rostedt@goodmis.org>, Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>, 
	Daniel Bristot de Oliveira <bristot@redhat.com>, Valentin Schneider <vschneid@redhat.com>
Content-Type: text/plain; charset="UTF-8"

On Sun, 14 Jan 2024 at 16:12, Qais Yousef <qyousef@layalina.io> wrote:
>
> On 01/14/24 14:03, Vincent Guittot wrote:
>
> > Thanks for the trace. It was really helpful and I think that I got the
> > root cause.
> >
> > The problem comes from get_capacity_ref_freq() which returns current
> > freq when arch_scale_freq_invariant() is not enable, and the fact that
> > we apply map_util_perf() earlier in the path now which is then capped
> > by max capacity.
> >
> > Could you try the below ?
> >
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index e420e2ee1a10..611c621543f4 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -133,7 +133,7 @@ unsigned long get_capacity_ref_freq(struct
> > cpufreq_policy *policy)
> >         if (arch_scale_freq_invariant())
> >                 return policy->cpuinfo.max_freq;
> >
> > -       return policy->cur;
> > +       return policy->cur + policy->cur >> 2;
> >  }
> >
> >  /**
>
> Is this a test patch or a proper fix? I can't see it being the latter. It seems

It's a proper fix. It's the same mechanism that is used already :
 - Either you add margin on the utilization to go above current freq
before it is fully used. This si what was done previously
 - or you add margin on the freq range to select a higher freq than
current one before it become fully used

> the current logic fails when util is already 1024, and I think we're trying to
> fix the invariance issue too late.
>
> Is the problem that we can't read policy->cur in the scheduler to fix the util
> while it's being updated that's why it's done here in this case?
>
> If this is the problem, shouldn't the logic be if util is max then always go to
> max frequency? I don't think we have enough info to correct the invariance here
> IIUC. All we can see the system is saturated at this frequency and whether
> a small jump or a big jump is required is hard to tell.
>
> Something like this
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 95c3c097083e..473d0352030b 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -164,8 +164,12 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
>         struct cpufreq_policy *policy = sg_policy->policy;
>         unsigned int freq;
>
> -       freq = get_capacity_ref_freq(policy);
> -       freq = map_util_freq(util, freq, max);
> +       if (util != max) {
> +               freq = get_capacity_ref_freq(policy);
> +               freq = map_util_freq(util, freq, max);
> +       } else {
> +               freq = policy->cpuinfo.max_freq;
> +       }

This is not correct because you will have to wait to reach full
utilization at the current OPP possibly the lowest OPP before moving
directly to max OPP

>
>         if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
>                 return sg_policy->next_freq;