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

Peter Zijlstra
2026-04-22T13:39:23Z

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;
> > > }
> > >
> > > /*
> > >