[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 at 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