Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230505152440.142265-1-hongyan.xia2@arm.com> <20230518113000.pvboekaba7vspjyn@airbuntu>
 <8035fedb-820b-2a98-a1af-3a4a8971bcd1@arm.com>
In-Reply-To: <8035fedb-820b-2a98-a1af-3a4a8971bcd1@arm.com>
From:   Vincent Guittot <vincent.guittot@linaro.org>
Date:   Tue, 23 May 2023 11:23:02 +0200
Message-ID: <CAKfTPtAmV-0+2D5d8-CyQcowaFhWCB=C5xEUoEb0g1ObpmdegQ@mail.gmail.com>
Subject: Re: [PATCH] sched/documentation: elaborate on uclamp limitations
To:     Hongyan Xia <hongyan.xia2@arm.com>
Cc:     Qais Yousef <qyousef@layalina.io>,
        Jonathan Corbet <corbet@lwn.net>, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, 18 May 2023 at 14:42, Hongyan Xia <hongyan.xia2@arm.com> wrote:
>
> Hi Qais,
>
> On 2023-05-18 12:30, Qais Yousef wrote:
> > Please CC sched maintainers (Ingo + Peter) next time as they should pick this
> > up ultimately and they won't see it from the list only.
>
> Will do. I was using the get_maintainers script and I thought that gave
> me all the CCs.
>
> > On 05/05/23 16:24, Hongyan Xia wrote:
> >> The story in 5.2 about util_avg abruptly jumping from 300 when
> >> Fmax/Fmin == 3 to 1024 when Fmax/Fmin == 4 hides some details about how
> >> clock_pelt works behind the scenes. Explicitly mention it to make it
> >> easier for readers to follow.
> >>
> >> Signed-off-by: Hongyan Xia <hongyan.xia2@arm.com>
> >> Cc: Qais Yousef <qyousef@layalina.io>
> >> Cc: Vincent Guittot <vincent.guittot@linaro.org>
> >> ---
> >>   Documentation/scheduler/sched-util-clamp.rst | 17 +++++++++++++++++
> >>   1 file changed, 17 insertions(+)
> >>
> >> diff --git a/Documentation/scheduler/sched-util-clamp.rst b/Documentation/scheduler/sched-util-clamp.rst
> >> index 74d5b7c6431d..524df07bceba 100644
> >> --- a/Documentation/scheduler/sched-util-clamp.rst
> >> +++ b/Documentation/scheduler/sched-util-clamp.rst
> >> @@ -669,6 +669,19 @@ but not proportional to Fmax/Fmin.
> >>
> >>           p0->util_avg = 300 + small_error
> >>
> >> +The reason why util_avg is around 300 even though it runs for 900 at Fmin is:

What does it mean running for 900 at Fmin ? util_avg is a ratio in the
range [0:1024] without time unit

> >> +Although running at Fmin reduces the rate of rq_clock_pelt() to 1/3 thus
> >> +accumulates util_sum at 1/3 of the rate at Fmax, the clock period
> >> +(rq_clock_pelt() now minus previous rq_clock_pelt()) in:
> >> +
> >> +::
> >> +
> >> +        util_sum / clock period = util_avg

I don't get the meaning of the formula above ? There is no "clock
period" (although I'm not sure what it means here) involved when
computing util_avg

Also, there is no linear relation between util_avg and Fmin/Fmax
ratio. Fmin/Fmax ratio is meaningful in regards to the ratio between
running time and period time of a periodic task. I understand the
reference of pelt in this document as a quite simplified description
of PELT so I'm not sure that adding a partial explanation will help.
It will probably cause more confusion to people. The only thing that
is sure, is that PELT expects some idle time to stay fully invariant
for periodic task

> >> +
> >> +does not shrink to 1/3, since rq->clock_pelt is periodically synchronized with
> >> +rq->clock_task as long as there's idle time. As a result, we get util_avg of
> >> +about 300, not 900.
> >> +
> >
> > I feel neutral about these changes. It does answer some questions, but poses
> > more questions like what is clock_pelt. So we might end up in recursive
> > regression of explaining the explanation.
> >
> > I don't think we have a doc about clock_pelt. Worth adding one and just add
> > a reference to it from here for those interested in understanding more details
> > on why we need to go to idle to correct util_avg? I think our code has
> > explanation, a reference to update_rq_clock_pelt() might suffice too.
> >
> > Vincent, do you have an opinion here?
>
> Sounds reasonable. I don't mind drafting a doc or just a couple of
> paragraphs for clock_pelt (or all the different clocks like clock,
> clock_task, clock_idle_*), if that's what we can agree on.

I don't have a strong opinion on adding a doc on PELT.

>
> Hongyan