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

[Kuyo Chang]

On Tue, 2025-07-01 at 11:49 +0100, Juri Lelli wrote:
Hi John/Juri,
> 
> > 
> > > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > > index ad45a8fea245..96a21f38fcc3 100644
> > > --- a/kernel/sched/deadline.c
> > > +++ b/kernel/sched/deadline.c
> > > @@ -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;
> > 
> > As I mentioned earlier, I still don't see this conditional as
> > making a
> > lot of sense, as I don't believe there is time when this function
> > would be called and (!rq->fair_server.dl_server) would be true.
> > And even if there were, I'm not sure it makes sense to scale the
> > time
> > interval based on the fair_server.dl_server flag.
> > 
> > From a separate discussion, you highlighted that it might be useful
> > once we have multiple dl_server types, which may want scaled
> > accounting, but I think in that case we should use an explicit flag
> > instead of the dl_server bit to denote if the accounting should be
> > scaled or not.
> > 
> > So, since your patch is a fix for a pretty bad bug, I think it
> > should
> > be focused on fixing the issue in the simplest and clearest way for
> > the existing code, and not be too worried about integrating with
> > future changes that haven't landed.
> > 
> > Then, as those future changes land, we can see how best to
> > generalize
> > the decision to scale or not scale the accounting on a dl_server.
> > 
> > That said, the conditional is a bit of a moot point, since I don't
> > think we'll actually hit it, and I'm motivated to get the bug you
> > are
> > fixing resolved, so I wouldn't object if this went in as-is, but it
> > seems like it would be much cleaner to just drop that conditional
> > as
> > you did in the original version of this patch.
> 
> I agree. It would be better to drop the conditional.
> 

Thanks for your feedback and suggestion.
So the original version of this patch for fair_server is much better &
cleaner.
Updated to v5 as below link
https://lore.kernel.org/all/20250702021440.2594736-1-kuyo.chang@xxxxxxxxxxxx/

> Thanks!
> Juri
>