Re: [PATCH v3] sched/eevdf: Fix vprot across reweighting

[Kayra Cizmeci]

On Fri, Oct 16, 2026 at 11:15:31AM +0200, Vincent Guittot wrote:
> On Fri, 16 Oct 2026 at 10:46, Kayra Cizmeci <kayracizmeci@xxxxxxxxx> wrote:
> >
> > Currently in reweighting, while a tasks vruntime changes dead protection
> > could go alive, or alive protection could decrease, increase and go dead.
> > Fix it.
> >
> > Fixes: 80390ead2080 ("sched/fair: Separate se->vlag from se->vprot")
> > Fixes: bcd74b2ffdd0 ("sched/fair: Only set slice protection at pick time")
> > Suggested-by: Vincent Guittot <vincent.guittot@xxxxxxxxxx>
> > Signed-off-by: Kayra Cizmeci <kayracizmeci@xxxxxxxxx>
> > ---
> >
> > Changes v2->v3:
> > * Rebase and move the enqueue_task_fair() block to if (!curr) block.
> >
> >
> > Not tested at all :-(. But I think
> > This should'a cover the shced_change_begin() path same,
> > since curr == NULL, so we always enter if (!curr) block.
> >
> >  kernel/sched/fair.c | 28 ++++++++++++++++++++++++----
> >  1 file changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 0feec172fd37..4593d0122d66 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -4892,12 +4892,15 @@ static void reweight_eevdf(struct cfs_rq *cfs_rq, struct sched_entity *se,
> >         update_load_set(&se->h_load, weight);
> >
> >         if (on_rq) {
> > -               if (rel_vprot)
> > -                       se->vprot += avruntime;
> >                 se->deadline += avruntime;
> >                 se->rel_deadline = 0;
> >                 se->vruntime = avruntime - se->vlag;
> >
> > +               if (rel_vprot)
> > +                       se->vprot += avruntime;
> > +               else if (curr)
> > +                       se->vprot = se->vruntime;
> > +
> 
> This part looks good

:-)
> 
> >                 if (!curr)
> >                         __enqueue_entity(cfs_rq, se);
> >                 cfs_rq->h_nr_queued++;
> > @@ -8155,8 +8158,9 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
> >         int task_new = !(flags & ENQUEUE_WAKEUP);
> >         struct sched_entity *se = &p->se;
> >         struct cfs_rq *cfs_rq = &rq->cfs;
> > -       unsigned long weight;
> > -       bool curr;
> > +       unsigned long weight, old_weight;
> > +       bool curr, live, running;
> > +       s64 dist;
> >
> >         if (task_is_throttled(p) && enqueue_throttled_task(p))
> >                 return;
> > @@ -8200,8 +8204,24 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
> >         weight = enqueue_hierarchy(p, flags);
> >
> >         if (!curr) {
> > +               running = task_current_donor(rq, p);
> > +               live = running && protect_slice(se);
> 
> IMO, "protected" is a better name than "live".

OK. Will rename it.

> 
>  se != curr so why is this needed?

sched_change_begin() sets .running as task_current_donor(), then if
.running is true calls put_prev_task(). put_prev_task_fair() sets
cfs_rq->curr to NULL. While the task still is the donor. Then we enter
sched_change_end(), we call enqueue_task() with cfs_rq->curr == NULL but
the donor is still p. There is set_next_task() call afterwards, but it
does not sets protection since its called with SNT_NORMAL.

> 
> > +
> > +               if (live) {
> > +                       old_weight = se->h_load.weight;
> > +                       dist = se->vprot - se->vruntime;
> > +               }
> >                 reweight_eevdf(cfs_rq, se, weight, false);
> >                 place_entity(cfs_rq, se, flags | ENQUEUE_QUEUED);
> > +
> > +               if (live) {
> > +                       if (weight != old_weight)
> > +                               dist = div64_long(dist * old_weight, weight);
> > +                       se->vprot = se->vruntime + dist;
> > +               } else if (running) {
> > +                       cancel_protect_slice(se);
> > +               }
> > +
> >                 __enqueue_entity(cfs_rq, se);
> >         }
> >
> > --
> > 2.53.0
> >

Thanks for the review!