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

[Peter Zijlstra <peterz@infradead.org>]

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@linaro.org> wrote:
> > On Thu, 2 Apr 2026 at 15:14, Peter Zijlstra <peterz@infradead.org> 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@linaro.org/

D'oh.

That said, on IRQ you mentioned that this wasn't quite good enough and
that your original patch is best.

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

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?


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