Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Date:   Tue, 21 Nov 2023 22:31:34 +0000
From:   Qais Yousef <qyousef@layalina.io>
To:     Vincent Guittot <vincent.guittot@linaro.org>
Cc:     mingo@redhat.com, peterz@infradead.org, juri.lelli@redhat.com,
        dietmar.eggemann@arm.com, rostedt@goodmis.org, bsegall@google.com,
        mgorman@suse.de, bristot@redhat.com, vschneid@redhat.com,
        rafael@kernel.org, viresh.kumar@linaro.org,
        linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
        lukasz.luba@arm.com, wyes.karny@amd.com, beata.michalska@arm.com
Subject: Re: [PATCH v3 1/2] sched/schedutil: Rework performance estimation
Message-ID: <20231121223134.iewtzozmz6bz5jr5@airbuntu>
References: <20231103131821.1176294-1-vincent.guittot@linaro.org>
 <20231103131821.1176294-2-vincent.guittot@linaro.org>
 <20231114205422.k5m6y4m5vnw7dvzj@airbuntu>
 <CAKfTPtDMEes6V2xRHavAwWrVuiZBdFAsaaxv9=-psAZCTPQWKg@mail.gmail.com>
 <20231121211725.gaekv6svnqdiq5l4@airbuntu>
 <CAKfTPtDzAZMcuWOYYOOAjCyvrOQiqyHZJBFVbACAvTqo+pU1gQ@mail.gmail.com>
 <20231121220955.uxk2zanxfemwyfz6@airbuntu>
 <CAKfTPtAHbYCyqA6jLqkoWgQ2X625tann8Mpy0QttgQo5OPvS9w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAKfTPtAHbYCyqA6jLqkoWgQ2X625tann8Mpy0QttgQo5OPvS9w@mail.gmail.com>
Precedence: bulk

On 11/23/23 14:32, Vincent Guittot wrote:
> On Thu, 23 Nov 2023 at 14:15, Qais Yousef <qyousef@layalina.io> wrote:
> >
> > On 11/23/23 08:47, Vincent Guittot wrote:
> >
> > > > > > And is it right to mix irq and uclamp_min with bw_min which is for DL? We might
> > > > >
> > > > > cpu_bw_dl() is not the actual utilization by DL task but the computed
> > > > > bandwidth which can be seen as min performance level
> > > >
> > > > Yep. That's why I am not in favour of a dvfs headroom for DL.
> > > >
> > > > But what I meant here is that in effective_cpu_util(), where we populate min
> > > > and max we have
> > > >
> > > >         if (min) {
> > > >                 /*
> > > >                  * The minimum utilization returns the highest level between:
> > > >                  * - the computed DL bandwidth needed with the irq pressure which
> > > >                  *   steals time to the deadline task.
> > > >                  * - The minimum performance requirement for CFS and/or RT.
> > > >                  */
> > > >                 *min = max(irq + cpu_bw_dl(rq), uclamp_rq_get(rq, UCLAMP_MIN));
> > > >
> > > > So if there was an RT/CFS task requesting a UCLAMP_MIN of 1024 for example,
> > > > bw_min will end up being too high, no?
> > >
> > > But at the end, we want at least uclamp_min for cfs or rt just like we
> > > want at least DL bandwidth for DL tasks
> >
> > The issue I see is that we do
> >
> > static void sugov_get_util()
> > {
> > ..
> >         util = effective_cpu_util(.., &min, ..); // min = max(irq + cpu_bw_dl(), rq_uclamp_min)
> >         ..
> >         sg_cpu->bw_min = min; // bw_min can pick the rq_uclamp_min. Shouldn't it be irq + cpu_bw_dl() only?
> >         ..
> > }
> >
> > If yes, why the comparison in ignore_dl_rate_limit() is still correct then?
> >
> >         if (cpu_bw_dl(cpu_rq(sg_cpu->cpu)) > sg_cpu->bw_min)
> 
> yes, this is to ensure enough performance for the deadline task when
> the dl bandwidth increases without waiting the rate limit period which
> would prevent the system from providing enough bandwidth to the
> deadline scheduler. This remains true because it's still at least
> above cpu_bw_dl()

Okay I think I get it now. I think renaming bw_min to perf_min or something
along those lines in the next opportunity would be a good thing.

> 
> >
> > And does cpufreq_driver_adjust_perf() still need the sg_cpu->bw_min arg
> > actually? sg_cpu->util already calculated based on sugov_effective_cpu_perf()
> > which takes all constraints (including bw_min) into account.
> 
> cpufreq_driver_adjust_perf() is used for systems on which you can't
> actually set an operating frequency but only a min and a desired
> performance level and let the hw move freely in this range.

I see. Thanks for the explanation.


Cheers

--
Qais Yousef