From: Qais Yousef <qyousef@layalina.io>
To: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: Ingo Molnar <mingo@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
"Rafael J. Wysocki" <rafael@kernel.org>,
Viresh Kumar <viresh.kumar@linaro.org>,
Vincent Guittot <vincent.guittot@linaro.org>,
linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
Lukasz Luba <lukasz.luba@arm.com>, Wei Wang <wvw@google.com>,
Rick Yiu <rickyiu@google.com>,
Chung-Kai Mei <chungkai@google.com>,
Hongyan Xia <hongyan.xia2@arm.com>
Subject: Re: [PATCH 1/4] sched/fair: Be less aggressive in calling cpufreq_update_util()
Date: Sun, 17 Dec 2023 21:44:55 +0000 [thread overview]
Message-ID: <20231217214455.5rf67ezdrwqdvwwh@airbuntu> (raw)
In-Reply-To: <212396c7-8c36-4850-8871-ea4c757a9324@arm.com>
On 12/18/23 09:51, Dietmar Eggemann wrote:
> On 08/12/2023 02:52, Qais Yousef wrote:
> > Due to the way code is structured, it makes a lot of sense to trigger
> > cpufreq_update_util() from update_load_avg(). But this is too aggressive
> > as in most cases we are iterating through entities in a loop to
> > update_load_avg() in the hierarchy. So we end up sending too many
> > request in an loop as we're updating the hierarchy.
> >
> > Combine this with the rate limit in schedutil, we could end up
> > prematurely send up a wrong frequency update before we have actually
> > updated all entities appropriately.
> >
> > Be smarter about it by limiting the trigger to perform frequency updates
> > after all accounting logic has done. This ended up being in the
>
> What are the boundaries of the 'accounting logic' here? Is this related
> to the update of all sched_entities and cfs_rq's involved when a task is
> attached/detached (or enqueued/dequeued)?
Yes.
>
> I can't see that there are any premature cfs_rq_util_change() in the
> current code when we consider this.
Thanks for checking. I'll revisit the problem as indicated previously. This
patch is still needed; I'll update rationale at least and fix highlighted
issues with decay.
>
> And avoiding updates for a smaller task to make sure updates for a
> bigger task go through is IMHO not feasible.
Where did this line of thought come from? This patch is about consolidating
how scheduler request frequency updates. And later patches requires the single
update at tick to pass the new SCHED_CPUFREQ_PERF_HINTS.
If you're referring to the logic in later patches about ignore_short_tasks();
then we only ignore the performance hints for this task.
Why not feasible? What's the rationale?
>
> I wonder how much influence does this patch has on the test results
> presented the patch header?
The only change of behavior is how we deal with decay. Which I thought wouldn't
introduce a functional change, but as caught to by Christian, it did. No
functional changes are supposed to happen that can affect the test results
AFAICT.
Cheers
--
Qais Yousef
next prev parent reply other threads:[~2023-12-18 18:19 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-08 1:52 [PATCH 0/4] sched: cpufreq: Remove uclamp max-aggregation Qais Yousef
2023-12-08 1:52 ` [PATCH 1/4] sched/fair: Be less aggressive in calling cpufreq_update_util() Qais Yousef
2023-12-08 10:05 ` Lukasz Luba
2023-12-10 20:51 ` Qais Yousef
2023-12-11 7:56 ` Lukasz Luba
2023-12-12 12:10 ` Qais Yousef
2023-12-14 8:19 ` Lukasz Luba
2023-12-11 18:47 ` Christian Loehle
2023-12-12 12:34 ` Qais Yousef
2023-12-12 13:09 ` Christian Loehle
2023-12-12 13:29 ` Qais Yousef
2023-12-12 10:46 ` Dietmar Eggemann
2023-12-12 12:35 ` Qais Yousef
2023-12-12 18:22 ` Hongyan Xia
2023-12-12 10:47 ` Hongyan Xia
2023-12-12 11:06 ` Vincent Guittot
2023-12-12 12:40 ` Qais Yousef
2023-12-29 0:25 ` Qais Yousef
2024-01-03 13:41 ` Vincent Guittot
2024-01-04 19:40 ` Qais Yousef
2023-12-18 8:51 ` Dietmar Eggemann
2023-12-17 21:44 ` Qais Yousef [this message]
2023-12-08 1:52 ` [PATCH 2/4] sched/uclamp: Remove rq max aggregation Qais Yousef
2023-12-11 0:08 ` Qais Yousef
2023-12-08 1:52 ` [PATCH 3/4] sched/schedutil: Ignore update requests for short running tasks Qais Yousef
2023-12-08 10:42 ` Hongyan Xia
2023-12-10 22:22 ` Qais Yousef
2023-12-11 11:15 ` Hongyan Xia
2023-12-12 12:23 ` Qais Yousef
2023-12-08 1:52 ` [PATCH 4/4] sched/documentation: Remove reference to max aggregation Qais Yousef
2023-12-18 8:19 ` [PATCH 0/4] sched: cpufreq: Remove uclamp max-aggregation Dietmar Eggemann
2023-12-17 21:23 ` Qais Yousef
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231217214455.5rf67ezdrwqdvwwh@airbuntu \
--to=qyousef@layalina.io \
--cc=chungkai@google.com \
--cc=dietmar.eggemann@arm.com \
--cc=hongyan.xia2@arm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=lukasz.luba@arm.com \
--cc=mingo@kernel.org \
--cc=peterz@infradead.org \
--cc=rafael@kernel.org \
--cc=rickyiu@google.com \
--cc=vincent.guittot@linaro.org \
--cc=viresh.kumar@linaro.org \
--cc=wvw@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox