Re: [PATCH v2] sched: fair: Prevent negative lag increase during delayed dequeue

[Vincent Guittot]

On Wed, 22 Apr 2026 at 15:39, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> On Fri, Apr 17, 2026 at 07:18:48PM +0200, Vincent Guittot wrote:
> > On Thu, 2 Apr 2026 at 15:17, Vincent Guittot <vincent.guittot@xxxxxxxxxx> wrote:
> > > On Thu, 2 Apr 2026 at 15:14, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> > > > On Tue, Mar 31, 2026 at 06:23:52PM +0200, Vincent Guittot wrote:
> > > > > +/*
> > > > > + * Delayed dequeue aims to reduce the negative lag of a dequeued task.
> > > > > + * While updating the lag of an entity, check that negative lag didn't increase
> > > > > + * during the delayed dequeue period which would be unfair.
> > > > > + * Similarly, check that the entity didn't gain positive lag when DELAY_ZERO is
> > > > > + * set.
> > > > > + *
> > > > > + * Return true if the lag has been adjusted.
> > > > > + */
> > > > > +static bool update_entity_lag(struct cfs_rq *cfs_rq, struct sched_entity *se)
> > > > >  {
> > > > > +     s64 vlag;
> > > > > +
> > > > >       WARN_ON_ONCE(!se->on_rq);
> > > > >
> > > > > +     vlag = entity_lag(cfs_rq, se, avg_vruntime(cfs_rq));
> > > > > +
> > > > > +     if (se->sched_delayed)
> > > > > +             /* previous vlag < 0 otherwise se would not be delayed */
> > > > > +             se->vlag = clamp(vlag, se->vlag, sched_feat(DELAY_ZERO) ? 0 : S64_MAX);
> > > > > +     else
> > > > > +             se->vlag = vlag;
> > > > > +
> > > > > +     return (vlag != se->vlag);
> > > > >  }
>
> > > > --- a/kernel/sched/fair.c
> > > > +++ b/kernel/sched/fair.c
> > > > @@ -841,29 +841,32 @@ static s64 entity_lag(struct cfs_rq *cfs
> > > >  }
> > > >
> > > >  /*
> > > > + * Delayed dequeue aims to reduce the negative lag of a dequeued task. While
> > > > + * updating the lag of an entity, check that negative lag didn't increase
> > > >   * during the delayed dequeue period which would be unfair.
> > > > + * Similarly, check that the entity didn't gain positive lag when DELAY_ZERO
> > > > + * is set.
> > > >   *
> > > >   * Return true if the lag has been adjusted.
> > > >   */
> > > > +static __always_inline
> > > > +bool update_entity_lag(struct cfs_rq *cfs_rq, struct sched_entity *se)
> > > >  {
> > > > +       s64 vlag = entity_lag(cfs_rq, se, avg_vruntime(cfs_rq));
> > > > +       bool ret;
> > > >
> > > >         WARN_ON_ONCE(!se->on_rq);
> > > >
> > > > +       if (se->sched_delayed) {
> > > >                 /* previous vlag < 0 otherwise se would not be delayed */
> > > > +               vlag = max(vlag, se->vlag);
> > > > +               if (sched_feat(DELAY_ZERO))
> > > > +                       vlag = min(vlag, 0);
> > > > +       }
> > > > +       ret = (vlag == se->vlag);
> >
> > hmm, I missed that we now return false when the lag has been updated
> > instead of true
> >
> > I sent a fix
> > https://lore.kernel.org/all/20260417171642.3539914-1-vincent.guittot@xxxxxxxxxx/
>
> D'oh.
>
> That said, on IRQ you mentioned that this wasn't quite good enough and
> that your original patch is best.

Yes, it fixes one case but breaks the other one :-(

>
> The trouble is, your original patch can update vlag (!se->sched_delayed)
> and report it hasn't changed; because then vlag == se->clag, obviously.

In the  (!se->sched_delayed),  we don't care because the entity is not
enqueued so we don't need to place it with new vlag

>
> This invalidates the comment on the return value of the function. In
> fact, it makes the function have a very non-obvious return meaning.
>
> So I'm a little confused -- what do we actually want this function to
> do?

I want update_entity_lag() to return true if we have modified the vlag
of an enqueued entity. In this case we need to dequeue, place entity
with new vlag and enqueue it.

we don't need to test se->on_rq because update_entity_lag() is called
for enqueued task only with delayed dequeue entity so
se->sched_delayed implies se->on_rq

In fact we should test :
(vlag != se->vlag) && se->on_rq
but  && se->on_rq is useless

That being said, this probably deserves a comment

>
>
> > > > +       se->vlag = vlag;
> > > >
> > > > +       return ret;
> > > >  }
> > > >
> > > >  /*
> > > >