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

[Vincent Guittot <vincent.guittot@linaro.org>]

Le vendredi 16 oct. 2026 à 12:45:45 (+0300), Kayra Cizmeci a écrit :
> On Fri, Oct 16, 2026 at 11:15:31AM +0200, Vincent Guittot wrote:
> > On Fri, 16 Oct 2026 at 10:46, Kayra Cizmeci <kayracizmeci@gmail.com> 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@linaro.org>
> > > Signed-off-by: Kayra Cizmeci <kayracizmeci@gmail.com>
> > > ---
> > >
> > > 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.

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);
 	}
@@ -15335,7 +15345,7 @@ static void task_tick_fair(struct rq *rq, struct task_struct *curr, int queued)
 		}
 
 		se = &curr->se;
-		reweight_eevdf(cfs_rq, se, weight, se->on_rq);
+		reweight_eevdf(cfs_rq, se, weight, false);
 	}
 
 	if (queued)
@@ -15542,7 +15552,7 @@ static void set_next_task_fair(struct rq *rq, struct task_struct *p, enum snt_e
 	cfs_rq->curr = se;
 
 	if (on_rq) {
-		reweight_eevdf(cfs_rq, se, weight, se->on_rq);
+		reweight_eevdf(cfs_rq, se, weight, false);
 		if (first)
 			set_protect_slice(cfs_rq, se);
 	}
-- 
2.53.0


> 
> > 
> > > +
> > > +               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!