| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <bbb9a1f3b796e8d7bd0010bf0f4862d67368516a.camel@mediatek.com>
Date: Wed, 18 Jun 2025 11:41:11 +0800
From: Kuyo Chang <kuyo.chang@...iatek.com>
To: John Stultz <jstultz@...gle.com>
CC: Ingo Molnar <mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>,
Juri Lelli <juri.lelli@...hat.com>, Vincent Guittot
<vincent.guittot@...aro.org>, Dietmar Eggemann <dietmar.eggemann@....com>,
Steven Rostedt <rostedt@...dmis.org>, Ben Segall <bsegall@...gle.com>, "Mel
Gorman" <mgorman@...e.de>, Valentin Schneider <vschneid@...hat.com>, Matthias
Brugger <matthias.bgg@...il.com>, AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>, <linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>, <linux-mediatek@...ts.infradead.org>
Subject: Re: [PATCH v2 1/1] sched/deadline: Fix dl_server runtime
calculation formula
On Tue, 2025-06-17 at 14:02 -0700, John Stultz wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> On Tue, Jun 17, 2025 at 8:54 AM Kuyo Chang <kuyo.chang@...iatek.com>
> wrote:
> >
> > From: kuyo chang <kuyo.chang@...iatek.com>
> >
> > [Symptom]
> > The calculation formula for dl_server runtime is based on
> > Frequency/capacity scale-invariance.
> > This will cause excessive RT latency (expect absolute time).
> >
> > [Analysis]
> > Consider the following case under a Big.LITTLE architecture:
> >
> > Assume the runtime is: 50,000,000 ns, and Frequency/capacity
> > scale-invariance defined as below:
> >
> > Frequency scale-invariance: 100
> > Capacity scale-invariance: 50
> > First by Frequency scale-invariance,
> > the runtime is scaled to 50,000,000 * 100 >> 10 = 4,882,812
> > Then by capacity scale-invariance,
> > it is further scaled to 4,882,812 * 50 >> 10 = 238,418.
> >
> > So it will scaled to 238,418 ns.
> >
> > [Solution]
> > The runtime for dl_server should be fixed time
> > asis RT bandwidth control.
> > Fix the runtime calculation formula for the dl_server.
>
> Thanks again for iterating on this patch! I've got a few minor nits
> below.
>
> > Signed-off-by: kuyo chang <kuyo.chang@...iatek.com>
> > Acked-by: Juri Lelli <juri.lelli@...hat.com>
> > Suggested-by: Peter Zijlstra <peterz@...radead.org>
> >
> > v1:
> > https://lore.kernel.org/all/20250614020524.631521-1-kuyo.chang@mediatek.com/
> >
> > ---
> > kernel/sched/deadline.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > index ad45a8fea245..f68a158d01e9 100644
> > --- a/kernel/sched/deadline.c
> > +++ b/kernel/sched/deadline.c
> > @@ -1504,7 +1504,9 @@ static void update_curr_dl_se(struct rq *rq,
> > struct sched_dl_entity *dl_se, s64
> > if (dl_entity_is_special(dl_se))
> > return;
> >
> > - scaled_delta_exec = dl_scaled_delta_exec(rq, dl_se,
> > delta_exec);
> > + scaled_delta_exec = delta_exec;
> > + if (!dl_se->dl_server)
> > + scaled_delta_exec = dl_scaled_delta_exec(rq, dl_se,
> > delta_exec);
>
> Just a nit, but would
> if (!dl_server(dl_se))
>
> be a little cleaner/consistent with other readers?
> > dl_se->runtime -= scaled_delta_exec;
> >
> > @@ -1624,7 +1626,9 @@ void dl_server_update_idle_time(struct rq
> > *rq, struct task_struct *p)
> > if (delta_exec < 0)
> > return;
> >
> > - scaled_delta_exec = dl_scaled_delta_exec(rq, &rq-
> > >fair_server, delta_exec);
> > + scaled_delta_exec = delta_exec;
> > + if (!rq->fair_server.dl_server)
> > + scaled_delta_exec = dl_scaled_delta_exec(rq, &rq-
> > >fair_server, delta_exec);
> >
> > rq->fair_server.runtime -= scaled_delta_exec;
>
Thanks for cleaner code suggestion, how about this?
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index f68a158d01e9..3ccffdf4dec6 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -1505,7 +1505,7 @@ static void update_curr_dl_se(struct rq *rq,
struct sched_dl_entity *dl_se, s64
return;
scaled_delta_exec = delta_exec;
- if (!dl_se->dl_server)
+ if (!dl_server(dl_se))
scaled_delta_exec = dl_scaled_delta_exec(rq, dl_se,
delta_exec);
dl_se->runtime -= scaled_delta_exec;
@@ -1614,12 +1614,13 @@ static void update_curr_dl_se(struct rq *rq,
struct sched_dl_entity *dl_se, s64
void dl_server_update_idle_time(struct rq *rq, struct task_struct *p)
{
s64 delta_exec, scaled_delta_exec;
+ struct sched_dl_entity *dl_se = &rq->fair_server;
- if (!rq->fair_server.dl_defer)
+ if (!dl_se->dl_defer)
return;
/* no need to discount more */
- if (rq->fair_server.runtime < 0)
+ if (dl_se->runtime < 0)
return;
delta_exec = rq_clock_task(rq) - p->se.exec_start;
@@ -1627,14 +1628,14 @@ void dl_server_update_idle_time(struct rq *rq,
struct task_struct *p)
return;
scaled_delta_exec = delta_exec;
- if (!rq->fair_server.dl_server)
- scaled_delta_exec = dl_scaled_delta_exec(rq, &rq-
>fair_server, delta_exec);
+ if (!dl_server(dl_se))
+ scaled_delta_exec = dl_scaled_delta_exec(rq, dl_se,
delta_exec);
- rq->fair_server.runtime -= scaled_delta_exec;
+ dl_se->runtime -= scaled_delta_exec;
- if (rq->fair_server.runtime < 0) {
- rq->fair_server.dl_defer_running = 0;
- rq->fair_server.runtime = 0;
+ if (dl_se->runtime < 0) {
+ dl_se->dl_defer_running = 0;
+ dl_se->runtime = 0;
}
p->se.exec_start = rq_clock_task(rq);
If it's ok, should I split it into two separate patches
1.Fix dl_server runtime calculation formula
2.cleaner code patch
or just submit it as a single patch?
> I'm a little confused on the conditional here. Is
> fair_server.dl_server ever not true (after the first call to
> dl_server_start())?
>
For now, it's true.
But based on our previous discussion,
use the dl_server flag to identify and handle a 'fixed time' type of
isolation.
This approach makes it easier to extend and allows multiple servers to
configure it as needed.
> Also, in the discussion on your first version, it seemed there might
> be a need for different servers to have different requirements, but
> it
> seemed like fair_server would always not want to be scaled.
>
> thanks
> -john
Powered by blists - more mailing lists