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

[Vincent Guittot]

On Fri, 16 Oct 2026 at 16:22, Kayra Cizmeci <kayracizmeci@xxxxxxxxx> wrote:
>
> On Fri, Oct 16, 2026 at 04:06:47PM +0200, Vincent Guittot wrote:
> > 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...
>
> Yeah. There is really no clean way of fixing vprot in all the ways.
> Just a cleaner way than the other.
>
> >
> > 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.
>
> So something like below for !curr path?
>
>         if (!curr) {
>                 reweight_eevdf(cfs_rq, se, weight, false);
>                 place_entity(cfs_rq, se, flags | ENQUEUE_QUEUED);
>                 if (task_current_donor(rq, p))
>                         cancel_protect_slice(se);
>                 __enqueue_entity(cfs_rq, se);
>         }
>
> Does this work for you? And keeping the reweight_eevdf() part as is?

yes, looks good for me

> BTW, I didn't typed a comment right now, so if you say 'yes' I'll add a comment
> above how even task_current_donor() can return true on the !curr path.

Thanks
Vincent

>
> Thanks!
>
> Kayra
>
>
> >
> > >
> > > Thanks!
> > > Kayra