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

[Vincent Guittot]

On Fri, 16 Oct 2026 at 12:51, Kayra Cizmeci <kayracizmeci@xxxxxxxxx> wrote:
>
> On Fri, Oct 16, 2026 at 12:27:16PM +0200, Vincent Guittot wrote:
> > Le vendredi 16 oct. 2026 à 12:45:45 (+0300), Kayra Cizmeci a écrit :
>
>
> > > >  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.
> >
> > okay, but we don't want to spread this all over the code.
> > reweight_eevdf/scale_entity is  doing this already.
> >
> > The below should do the stuff while keeping the computation in reweight_eevdf:
> >
> > ---
> >  kernel/sched/fair.c | 26 ++++++++++++++++++--------
> >  1 file changed, 18 insertions(+), 8 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 0feec172fd37..c212c6aa9691 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -4863,10 +4863,11 @@ rescale_entity(struct sched_entity *se, unsigned long weight, bool rel_vprot)
> >  }
> >
> >  static void reweight_eevdf(struct cfs_rq *cfs_rq, struct sched_entity *se,
> > -                        unsigned long weight, bool on_rq)
> > +                        unsigned long weight, bool update_vprot)
> >  {
> >       bool curr = cfs_rq->curr == se;
> >       bool rel_vprot = false;
> > +     bool on_rq = se->on_rq;
> >       u64 avruntime = 0;
> >
> >       if (se->h_load.weight == weight)
> > @@ -4878,22 +4879,25 @@ static void reweight_eevdf(struct cfs_rq *cfs_rq, struct sched_entity *se,
> >               se->deadline -= avruntime;
> >               se->rel_deadline = 1;
> >               if (curr && protect_slice(se)) {
> > -                     se->vprot -= avruntime;
> >                       rel_vprot = true;
> >               }
> >
> >               cfs_rq->h_nr_queued--;
> >               if (!curr)
> >                       __dequeue_entity(cfs_rq, se);
> > +     } else if (update_vprot) {
> > +             avruntime = se->vruntime;
> > +             rel_vprot = true;
> >       }
> >
> > +     if (rel_vprot)
> > +             se->vprot -= avruntime;
> > +
> >       rescale_entity(se, weight, rel_vprot);
> >
> >       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;
> > @@ -4902,6 +4906,12 @@ static void reweight_eevdf(struct cfs_rq *cfs_rq, struct sched_entity *se,
> >                       __enqueue_entity(cfs_rq, se);
> >               cfs_rq->h_nr_queued++;
> >       }
> > +
> > +     if (rel_vprot)
> > +             se->vprot += avruntime;
> > +     else if (curr || update_vprot)
> > +             se->vprot = se->vruntime;
> > +
> >  }
> >
> >  static void reweight_entity(struct cfs_rq *cfs_rq, struct sched_entity *se,
> > @@ -4965,7 +4975,7 @@ static void reweight_task_fair(struct rq *rq, struct task_struct *p,
> >       for_each_sched_entity(se)
> >               weight = __calc_prop_weight(cfs_rq_of(se), se, weight);
> >
> > -     reweight_eevdf(&rq->cfs, &p->se, weight, p->se.on_rq);
> > +     reweight_eevdf(&rq->cfs, &p->se, weight, false);
> >  }
> >
> >  static inline int throttled_hierarchy(struct cfs_rq *cfs_rq);
> > @@ -8200,7 +8210,7 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
> >       weight = enqueue_hierarchy(p, flags);
> >
> >       if (!curr) {
> > -             reweight_eevdf(cfs_rq, se, weight, false);
> > +             reweight_eevdf(cfs_rq, se, weight, task_current_donor(rq, p));
> >               place_entity(cfs_rq, se, flags | ENQUEUE_QUEUED);
> >               __enqueue_entity(cfs_rq, se);
> >       }
>
> On the old code on_rq was always false. But because enqueue_hierarchy(),
> se->on_rq is always true. So this just goes to the other path in
> reweight_eevdf().

I changed the interface and the last arg is no more on_rq but force
the update of vprot

>
> Also, vruntime can move in place_entity() too.

But still, this is horrible...

Also, after more thinking, I don't think trying to keep vprot correct
in all cases is worth the effort, nor is it achievable without cost.
When one changes the prio and/or the slice of the running task, that
task might no longer be the best one to pick anyway. We should just do
our best effort like other places where we only decrease or cancel
protection to ensure it will not have an extended protection slice.

>
> Thanks!
> Kayra