[PATCH v2 1/1] sched/deadline: Fix dl_server runtime calculation formula

[PATCH v2 1/1] sched/deadline: Fix dl_server runtime calculation formula

[Kuyo Chang]

From: kuyo chang <kuyo.chang@mediatek.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.

Signed-off-by: kuyo chang <kuyo.chang@mediatek.com>
Acked-by: Juri Lelli <juri.lelli@redhat.com>
Suggested-by: Peter Zijlstra <peterz@infradead.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);
 
 	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;
 
-- 
2.45.2

Re: [PATCH v2 1/1] sched/deadline: Fix dl_server runtime calculation formula

[John Stultz]

On Tue, Jun 17, 2025 at 8:54 AM Kuyo Chang <kuyo.chang@mediatek.com> wrote:
>
> From: kuyo chang <kuyo.chang@mediatek.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@mediatek.com>
> Acked-by: Juri Lelli <juri.lelli@redhat.com>
> Suggested-by: Peter Zijlstra <peterz@infradead.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;

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())?

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

Re: [PATCH v2 1/1] sched/deadline: Fix dl_server runtime calculation formula

[Kuyo Chang]

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@mediatek.com>
> wrote:
> > 
> > From: kuyo chang <kuyo.chang@mediatek.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@mediatek.com>
> > Acked-by: Juri Lelli <juri.lelli@redhat.com>
> > Suggested-by: Peter Zijlstra <peterz@infradead.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

Re: [PATCH v2 1/1] sched/deadline: Fix dl_server runtime calculation formula

[John Stultz]

On Tue, Jun 17, 2025 at 8:41 PM Kuyo Chang <kuyo.chang@mediatek.com> wrote:
> 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;

This bit above looks good to me.

> @@ -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;
>         }

Apologies if I'm confused, or my feedback added confusion, but I'm not
sure I see this chunk as a readability improvement, right off.

The indirection of rq->fair_server to a stack local dl_se makes it
less obvious what is being acted upon (might be confused for the dl_se
of the task ptr passed in).
Further, the dl_server() check is confusing as I think we're only
considering dl_servers here (the existing fair_server and potentially
future servers added), right?

I think your original logic for this function of just dropping the
scaled_delta_exec for the rq->fair_server.runtime addition makes the
most sense.

When we have additional dl_servers to handle here, I think we can
refactor/abstract out this logic as makes most sense when they arrive.


>         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?

Others can correct me, but I think just a single fix makes sense.


> > 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.

So from my read of the previous thread, it sounded like the
dl_server() check in update_curr_dl_se() is good for now, as there's
only the fair_server, but when other servers arrive we may need to
have a configurable bit on the server to determine if we scale or not.
For dl_server_update_idle_time(), I think each future dl_server will
need to be handled individually (and the code likely refactored to do
that cleanly), but I think its premature to try to handle that now, so
I'd just keep the original simpliciation for that patch.

thanks!
-john

Re: [PATCH v2 1/1] sched/deadline: Fix dl_server runtime calculation formula

[John Stultz]

On Tue, Jun 17, 2025 at 8:54 AM Kuyo Chang <kuyo.chang@mediatek.com> wrote:
>
> From: kuyo chang <kuyo.chang@mediatek.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.
>
> Signed-off-by: kuyo chang <kuyo.chang@mediatek.com>
> Acked-by: Juri Lelli <juri.lelli@redhat.com>
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
>
> v1: https://lore.kernel.org/all/20250614020524.631521-1-kuyo.chang@mediatek.com/
>

Coding nits aside, I put together a quick test that affines to a
single cpu a SCHED_NORMAL and SCHED_FIFO spinner task to illustrate
the issue.

You can quickly see the requested 50ms/sec dl_sever runtime on the big
cpu, ends up being scaled out to 323ms/sec, blocking RT tasks on that
little cpu for quite awhile.
  https://github.com/johnstultz-work/misc/blob/main/images/2025-06-18_illustration-of-problem-dl-server-scaling.png

The wild thing with the example illustration of the issue above is
that since my test uses cpu spinners, the cpufreq quickly maxes out.
So it's only really considering the capacity scaling between the big
(cpu 7) and little (cpu 0) cpus at their top frequency.

When I capped the cpu 0 max frequency to the lowest available, without
the patch the behavior is crazy:
  https://github.com/johnstultz-work/misc/blob/main/images/2025-06-18_dl-server-scaling-with-cpufreq-lowered.png

Though the image alone maybe isn't as clear, in that case we see the
RT task once it runs ~650ms, the dl_server kicks in and blocks it and
any other RT task from running for over *10 minutes*!

And with the fix to avoid scaling the fair_server, the results looks
much more sane:
https://github.com/johnstultz-work/misc/blob/main/images/2025-06-18_with-patch-to-not-scale-dl-server-fixed.png

So I'm very happy to add:
  Tested-by: John Stultz <jstultz@google.com>

And hope this gets upstream (and -stable) in some form quickly.

Thanks so much to Kuyo and others on his team for reporting and
root-causing this issue!

thanks
-john

Re: [PATCH v2 1/1] sched/deadline: Fix dl_server runtime calculation formula

[Kuyo Chang]

On Wed, 2025-06-18 at 14:45 -0700, John Stultz wrote:
> 
> > 
> 
Hi 
> Coding nits aside, I put together a quick test that affines to a
> single cpu a SCHED_NORMAL and SCHED_FIFO spinner task to illustrate
> the issue.
> 
> You can quickly see the requested 50ms/sec dl_sever runtime on the
> big
> cpu, ends up being scaled out to 323ms/sec, blocking RT tasks on that
> little cpu for quite awhile.
>  
> https://urldefense.com/v3/__https://github.com/johnstultz-work/misc/blob/main/images/2025-06-18_illustration-of-problem-dl-server-scaling.png__;!!CTRNKA9wMg0ARbw!lPy4srzoAcjFBktsYnAx92nK9niGz-Im3xUxfnorT3tEv4TGg2uSPBQFEOPu2l6CrGwO-zFiuMyDz-8jcc0$
> 
> The wild thing with the example illustration of the issue above is
> that since my test uses cpu spinners, the cpufreq quickly maxes out.
> So it's only really considering the capacity scaling between the big
> (cpu 7) and little (cpu 0) cpus at their top frequency.
> 
> When I capped the cpu 0 max frequency to the lowest available,
> without
> the patch the behavior is crazy:
>  
> https://urldefense.com/v3/__https://github.com/johnstultz-work/misc/blob/main/images/2025-06-18_dl-server-scaling-with-cpufreq-lowered.png__;!!CTRNKA9wMg0ARbw!lPy4srzoAcjFBktsYnAx92nK9niGz-Im3xUxfnorT3tEv4TGg2uSPBQFEOPu2l6CrGwO-zFiuMyDA9vSf_I$
> 
> Though the image alone maybe isn't as clear, in that case we see the
> RT task once it runs ~650ms, the dl_server kicks in and blocks it and
> any other RT task from running for over *10 minutes*!
> 
> And with the fix to avoid scaling the fair_server, the results looks
> much more sane:
> https://urldefense.com/v3/__https://github.com/johnstultz-work/misc/blob/main/images/2025-06-18_with-patch-to-not-scale-dl-server-fixed.png__;!!CTRNKA9wMg0ARbw!lPy4srzoAcjFBktsYnAx92nK9niGz-Im3xUxfnorT3tEv4TGg2uSPBQFEOPu2l6CrGwO-zFiuMyDpr-NplY$
> 
> So I'm very happy to add:
>   Tested-by: John Stultz <jstultz@google.com>
> 
> And hope this gets upstream (and -stable) in some form quickly.
> 
> Thanks so much to Kuyo and others on his team for reporting and
> root-causing this issue!
> 
update to patch v3 as below
https://lore.kernel.org/all/20250626030746.2245365-1-kuyo.chang@mediatek.com/

Please help to review, and if anyone have any concerns, please let me
know.

> thanks
> -john