[PATCH 0/4] sched/fair: Rework tash_h_load()

[PATCH 0/4] sched/fair: Rework tash_h_load()

[Peter Zijlstra]

Hi!

So task_h_load() has been known buggered for a while [1], and while looking at
[2] I ran into it again.

These few patches rework the thing and should hopefully result in less broken
behaviour.

Lightly tested...


[1] email that unfortunately ended up private with Vincent
[2] https://lore.kernel.org/all/aoxah90s0bQ4tcUW@three-body/

[PATCH 1/4] sched: Rename/clarify sched_class::task_tick(.queued) argument

[Peter Zijlstra]

For some reason the sched_class::task_tick() argument that indicates it being
an hrtick, is called @queued. I'm sure naming is hard and all, but lets fix
this.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/sched/deadline.c |    4 ++--
 kernel/sched/ext/ext.c  |    2 +-
 kernel/sched/fair.c     |   12 ++++++------
 kernel/sched/idle.c     |    2 +-
 kernel/sched/rt.c       |    2 +-
 kernel/sched/sched.h    |    2 +-
 6 files changed, 12 insertions(+), 12 deletions(-)

--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -2876,7 +2876,7 @@ static void put_prev_task_dl(struct rq *
  * and everything must be accessed through the @rq and @curr passed in
  * parameters.
  */
-static void task_tick_dl(struct rq *rq, struct task_struct *p, int queued)
+static void task_tick_dl(struct rq *rq, struct task_struct *p, int hrtick)
 {
 	update_curr_dl(rq);
 
@@ -2886,7 +2886,7 @@ static void task_tick_dl(struct rq *rq,
 	 * not being the leftmost task anymore. In that case NEED_RESCHED will
 	 * be set and schedule() will start a new hrtick for the next task.
 	 */
-	if (hrtick_enabled_dl(rq) && queued && p->dl.runtime > 0 &&
+	if (hrtick_enabled_dl(rq) && hrtick && p->dl.runtime > 0 &&
 	    is_leftmost(&p->dl, &rq->dl))
 		start_hrtick_dl(rq, &p->dl);
 }
--- a/kernel/sched/ext/ext.c
+++ b/kernel/sched/ext/ext.c
@@ -3787,7 +3787,7 @@ void scx_tick(struct rq *rq)
 	update_other_load_avgs(rq);
 }
 
-static void task_tick_scx(struct rq *rq, struct task_struct *curr, int queued)
+static void task_tick_scx(struct rq *rq, struct task_struct *curr, int hrtick)
 {
 	struct scx_sched *sch = scx_task_sched(curr);
 
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6542,7 +6542,7 @@ static void put_prev_entity(struct cfs_r
 }
 
 static void
-entity_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr, int queued)
+entity_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr, int hrtick)
 {
 	/*
 	 * Update run-time statistics of the 'current'.
@@ -6557,10 +6557,10 @@ entity_tick(struct cfs_rq *cfs_rq, struc
 
 #ifdef CONFIG_SCHED_HRTICK
 	/*
-	 * queued ticks are scheduled to match the slice, so don't bother
+	 * hrticks are scheduled to match the slice, so don't bother
 	 * validating it and just reschedule.
 	 */
-	if (queued) {
+	if (hrtick) {
 		resched_curr(rq_of(cfs_rq));
 		return;
 	}
@@ -15020,7 +15020,7 @@ static inline void task_tick_core(struct
  * and everything must be accessed through the @rq and @curr passed in
  * parameters.
  */
-static void task_tick_fair(struct rq *rq, struct task_struct *curr, int queued)
+static void task_tick_fair(struct rq *rq, struct task_struct *curr, int hrtick)
 {
 	struct sched_entity *se = &curr->se;
 
@@ -15030,7 +15030,7 @@ static void task_tick_fair(struct rq *rq
 
 		for_each_sched_entity(se) {
 			cfs_rq = cfs_rq_of(se);
-			entity_tick(cfs_rq, se, queued);
+			entity_tick(cfs_rq, se, hrtick);
 
 			weight = __calc_prop_weight(cfs_rq, se, weight);
 		}
@@ -15039,7 +15039,7 @@ static void task_tick_fair(struct rq *rq
 		reweight_eevdf(cfs_rq, se, weight, se->on_rq);
 	}
 
-	if (queued)
+	if (hrtick)
 		return;
 
 	if (static_branch_unlikely(&sched_numa_balancing))
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -535,7 +535,7 @@ dequeue_task_idle(struct rq *rq, struct
  * and everything must be accessed through the @rq and @curr passed in
  * parameters.
  */
-static void task_tick_idle(struct rq *rq, struct task_struct *curr, int queued)
+static void task_tick_idle(struct rq *rq, struct task_struct *curr, int hrtick)
 {
 	update_curr_idle(rq);
 }
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -2538,7 +2538,7 @@ static inline void watchdog(struct rq *r
  * and everything must be accessed through the @rq and @curr passed in
  * parameters.
  */
-static void task_tick_rt(struct rq *rq, struct task_struct *p, int queued)
+static void task_tick_rt(struct rq *rq, struct task_struct *p, int hrtick)
 {
 	struct sched_rt_entity *rt_se = &p->rt;
 
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -2718,7 +2718,7 @@ struct sched_class {
 	 * sched_tick: rq->lock
 	 * sched_tick_remote: rq->lock
 	 */
-	void (*task_tick)(struct rq *rq, struct task_struct *p, int queued);
+	void (*task_tick)(struct rq *rq, struct task_struct *p, int hrtick);
 	/*
 	 * sched_cgroup_fork: p->pi_lock
 	 */

[PATCH 2/4] sched/fair: Fold cfs_rq_of(se) into for_each_sched_entity()

[Peter Zijlstra]

Pretty much every for_each_sched_entity() loop does: cfs_rq = cfs_rq_of(se) as
the very first thing. Fold it into the for_each_sched_entity() macro.

This paves the way to have the macro track a backlink transparantly.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/sched/fair.c |   81 +++++++++++++++++++++++-----------------------------
 1 file changed, 37 insertions(+), 44 deletions(-)

--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -317,8 +317,8 @@ const struct sched_class fair_sched_clas
 #ifdef CONFIG_FAIR_GROUP_SCHED
 
 /* Walk up scheduling entities hierarchy */
-#define for_each_sched_entity(se) \
-		for (; se; se = se->parent)
+#define for_each_sched_entity(se, cfs_rq)				\
+	for (; (se) && ((cfs_rq) = cfs_rq_of(se)); (se) = (se)->parent)
 
 static inline bool list_add_leaf_cfs_rq(struct cfs_rq *cfs_rq)
 {
@@ -452,8 +452,8 @@ static int se_is_idle(struct sched_entit
 
 #else /* !CONFIG_FAIR_GROUP_SCHED: */
 
-#define for_each_sched_entity(se) \
-		for (; se; se = NULL)
+#define for_each_sched_entity(se, cfs_rq) \
+	for (; (se) && ((cfs_rq) = cfs_of_of(se)); (se) = NULL)
 
 static inline bool list_add_leaf_cfs_rq(struct cfs_rq *cfs_rq)
 {
@@ -2075,9 +2075,10 @@ static void update_curr(struct cfs_rq *c
 static void update_curr_fair(struct rq *rq)
 {
 	struct sched_entity *se = &rq->donor->se;
+	struct cfs_rq *cfs_rq;
 
-	for_each_sched_entity(se)
-		update_curr(cfs_rq_of(se));
+	for_each_sched_entity(se, cfs_rq)
+		update_curr(cfs_rq);
 }
 
 static inline void
@@ -4800,18 +4801,19 @@ static void reweight_task_fair(struct rq
 {
 	struct sched_entity *se = &p->se;
 	unsigned long weight = NICE_0_LOAD;
+	struct cfs_rq *cfs_rq = cfs_rq_of(se);
 
 	if (se->on_rq)
 		update_curr_fair(rq);
 
-	reweight_entity(cfs_rq_of(se), se, lw->weight);
+	reweight_entity(cfs_rq, se, lw->weight);
 	se->load.inv_weight = lw->inv_weight;
 
 	if (!se->on_rq)
 		return;
 
-	for_each_sched_entity(se)
-		weight = __calc_prop_weight(cfs_rq_of(se), se, weight);
+	for_each_sched_entity(se, cfs_rq)
+		weight = __calc_prop_weight(cfs_rq, se, weight);
 
 	reweight_eevdf(&rq->cfs, &p->se, weight, p->se.on_rq);
 }
@@ -6382,6 +6384,8 @@ static __always_inline void return_cfs_r
 
 static void set_delayed(struct sched_entity *se)
 {
+	struct cfs_rq *cfs_rq;
+
 	se->sched_delayed = 1;
 
 	/*
@@ -6392,15 +6396,14 @@ static void set_delayed(struct sched_ent
 	if (!entity_is_task(se))
 		return;
 
-	for_each_sched_entity(se) {
-		struct cfs_rq *cfs_rq = cfs_rq_of(se);
-
+	for_each_sched_entity(se, cfs_rq)
 		cfs_rq->h_nr_runnable--;
-	}
 }
 
 static void clear_delayed(struct sched_entity *se)
 {
+	struct cfs_rq *cfs_rq;
+
 	se->sched_delayed = 0;
 
 	/*
@@ -6412,11 +6415,8 @@ static void clear_delayed(struct sched_e
 	if (!entity_is_task(se))
 		return;
 
-	for_each_sched_entity(se) {
-		struct cfs_rq *cfs_rq = cfs_rq_of(se);
-
+	for_each_sched_entity(se, cfs_rq)
 		cfs_rq->h_nr_runnable++;
-	}
 }
 
 static void
@@ -7078,14 +7078,16 @@ void unthrottle_cfs_rq(struct cfs_rq *cf
 	walk_tg_tree_from(cfs_rq->tg, tg_nop, tg_unthrottle_up, (void *)rq);
 
 	if (!cfs_rq->load.weight) {
+		struct cfs_rq *cfs_rq_se;
+
 		if (!cfs_rq->on_list)
 			return;
 		/*
 		 * Nothing to run but something to decay (on_list)?
 		 * Complete the branch.
 		 */
-		for_each_sched_entity(se) {
-			if (list_add_leaf_cfs_rq(cfs_rq_of(se)))
+		for_each_sched_entity(se, cfs_rq_se) {
+			if (list_add_leaf_cfs_rq(cfs_rq_se))
 				break;
 		}
 	}
@@ -7931,13 +7933,12 @@ static unsigned long enqueue_hierarchy(s
 	struct sched_entity *se = &p->se;
 	int h_nr_idle = task_has_idle_policy(p);
 	int h_nr_runnable = 1;
+	struct cfs_rq *cfs_rq;
 
 	if (task_new && se->sched_delayed)
 		h_nr_runnable = 0;
 
-	for_each_sched_entity(se) {
-		struct cfs_rq *cfs_rq = cfs_rq_of(se);
-
+	for_each_sched_entity(se, cfs_rq) {
 		update_curr(cfs_rq);
 
 		if (!se->on_rq) {
@@ -8066,16 +8067,15 @@ static void dequeue_hierarchy(struct tas
 	bool task_sleep = flags & DEQUEUE_SLEEP;
 	bool task_delayed = flags & DEQUEUE_DELAYED;
 	bool task_throttled = flags & DEQUEUE_THROTTLE;
-	int h_nr_runnable = 0;
 	int h_nr_idle = task_has_idle_policy(p);
+	int h_nr_runnable = 0;
+	struct cfs_rq *cfs_rq;
 	bool dequeue = true;
 
 	if (task_sleep || task_delayed || !se->sched_delayed)
 		h_nr_runnable = 1;
 
-	for_each_sched_entity(se) {
-		struct cfs_rq *cfs_rq = cfs_rq_of(se);
-
+	for_each_sched_entity(se, cfs_rq) {
 		update_curr(cfs_rq);
 
 		if (dequeue) {
@@ -11301,8 +11301,7 @@ static void update_cfs_rq_h_load(struct
 		return;
 
 	WRITE_ONCE(cfs_rq->h_load_next, NULL);
-	for_each_sched_entity(se) {
-		cfs_rq = cfs_rq_of(se);
+	for_each_sched_entity(se, cfs_rq) {
 		WRITE_ONCE(cfs_rq->h_load_next, se);
 		if (cfs_rq->last_h_load_update == now)
 			break;
@@ -14948,9 +14947,9 @@ static inline void task_tick_core(struct
 static void se_fi_update(const struct sched_entity *se, unsigned int fi_seq,
 			 bool forceidle)
 {
-	for_each_sched_entity(se) {
-		struct cfs_rq *cfs_rq = cfs_rq_of(se);
+	struct cfs_rq *cfs_rq;
 
+	for_each_sched_entity(se, cfs_rq) {
 		if (forceidle) {
 			if (cfs_rq->forceidle_seq == fi_seq)
 				break;
@@ -15028,10 +15027,8 @@ static void task_tick_fair(struct rq *rq
 		unsigned long weight = NICE_0_LOAD;
 		struct cfs_rq *cfs_rq;
 
-		for_each_sched_entity(se) {
-			cfs_rq = cfs_rq_of(se);
+		for_each_sched_entity(se, cfs_rq) {
 			entity_tick(cfs_rq, se, hrtick);
-
 			weight = __calc_prop_weight(cfs_rq, se, weight);
 		}
 
@@ -15113,9 +15110,7 @@ static void propagate_entity_cfs_rq(stru
 	/* Start to propagate at parent */
 	se = se->parent;
 
-	for_each_sched_entity(se) {
-		cfs_rq = cfs_rq_of(se);
-
+	for_each_sched_entity(se, cfs_rq) {
 		update_load_avg(cfs_rq, se, UPDATE_TG);
 
 		if (!cfs_rq_pelt_clock_throttled(cfs_rq))
@@ -15218,9 +15213,7 @@ static void set_next_task_fair(struct rq
 	if (on_rq)
 		__dequeue_entity(cfs_rq, se);
 
-	for_each_sched_entity(se) {
-		cfs_rq = cfs_rq_of(se);
-
+	for_each_sched_entity(se, cfs_rq) {
 		if (!IS_ENABLED(CONFIG_FAIR_GROUP_SCHED) ||
 		    !first || !cfs_rq->h_curr)
 			set_next_entity(cfs_rq, se);
@@ -15420,13 +15413,14 @@ static int __sched_group_set_shares(stru
 	for_each_possible_cpu(i) {
 		struct rq *rq = cpu_rq(i);
 		struct sched_entity *se = tg_se(tg, i);
+		struct cfs_rq *cfs_rq;
 		struct rq_flags rf;
 
 		/* Propagate contribution to hierarchy */
 		rq_lock_irqsave(rq, &rf);
 		update_rq_clock(rq);
-		for_each_sched_entity(se) {
-			update_load_avg(cfs_rq_of(se), se, UPDATE_TG);
+		for_each_sched_entity(se, cfs_rq) {
+			update_load_avg(cfs_rq, se, UPDATE_TG);
 			update_cfs_group(se);
 		}
 		rq_unlock_irqrestore(rq, &rf);
@@ -15473,6 +15467,7 @@ int sched_group_set_idle(struct task_gro
 		struct sched_entity *se = tg_se(tg, i);
 		struct cfs_rq *grp_cfs_rq = tg_cfs_rq(tg, i);
 		bool was_idle = cfs_rq_is_idle(grp_cfs_rq);
+		struct cfs_rq *cfs_rq;
 		long idle_task_delta;
 		struct rq_flags rf;
 
@@ -15487,9 +15482,7 @@ int sched_group_set_idle(struct task_gro
 		if (!cfs_rq_is_idle(grp_cfs_rq))
 			idle_task_delta *= -1;
 
-		for_each_sched_entity(se) {
-			struct cfs_rq *cfs_rq = cfs_rq_of(se);
-
+		for_each_sched_entity(se, cfs_rq) {
 			if (!se->on_rq)
 				break;
 

[PATCH 3/4] sched/fair: Extend for_each_sched_entity() with a back-link

[Peter Zijlstra]

Leave a trail of bread crumbs, such that we can find out way back down the
hierarchy. No actual users yet, but split out because its a bit tricky.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/sched/fair.c  |    9 +++++++--
 kernel/sched/sched.h |    1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -317,8 +317,13 @@ const struct sched_class fair_sched_clas
 #ifdef CONFIG_FAIR_GROUP_SCHED
 
 /* Walk up scheduling entities hierarchy */
-#define for_each_sched_entity(se, cfs_rq)				\
-	for (; (se) && ((cfs_rq) = cfs_rq_of(se)); (se) = (se)->parent)
+#define for_each_sched_entity(se, cfs_rq)					\
+	for (struct sched_entity *_BL = NULL;					\
+	     (se) && ((cfs_rq) = cfs_rq_of(se), (cfs_rq)->backlink = _BL, true);\
+	     (se) = (se)->parent, _BL = (se))
+
+#define for_each_sched_entity_bl(se, cfs_rq) \
+	for (; ((se) = (cfs_rq)->backlink); (cfs_rq) = group_cfs_rq(se))
 
 static inline bool list_add_leaf_cfs_rq(struct cfs_rq *cfs_rq)
 {
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -726,6 +726,7 @@ struct cfs_rq {
 	unsigned long		tg_runnable_avg_contrib;
 	long			propagate;
 	long			prop_runnable_sum;
+	struct sched_entity	*backlink;
 
 	/*
 	 *   h_load = weight * f(tg)

[PATCH 4/4] sched/fair: Rework/fix task_h_load()

[Peter Zijlstra]

There are a number of issues with task_h_load():

 - its hierarchy traversal is racy to the point of being broken; where
   originally it was meant to be used under rq->lock, but lacking an assertion
   for that fact, its use spread and violated this. The result is that the
   back-link state is prone to races.

 - it is rate-limited on jiffies, which is HZ, not the underlying PELT decay.

 - since its update is tied to task_h_load() usage, the cfs_rq->h_load numbers are
   not often 'up-to-date', rendering their output in sched/debug near useless.

Rework the whole thing to keep a more up-to-date and less broken cfs_rq->h_load
number.

Move the back-link tracking into for_each_sched_entity(), such that any such
loop sets up a path back. Use this to (optionally) re-compute cfs_rq->h_load on
enqueue, dequeue, set_next and tick, all sites that hold rq->lock.

This ensures that 'active' cgroups have reasonably up-to-date cfs_rq->h_load.
Additionally, have __update_blocked_fair() update cfs_rq->h_load for all
cgroups.

Finally, replace the jiffy rate-limit with one that is tied to the PELT decay.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/sched/fair.c  |  108 ++++++++++++++++++++++++++++++++-------------------
 kernel/sched/pelt.h  |    7 +++
 kernel/sched/sched.h |    1 
 3 files changed, 76 insertions(+), 40 deletions(-)

--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5541,6 +5541,44 @@ static inline bool skip_blocked_update(s
 	return true;
 }
 
+static inline void __update_cfs_rq_h_load(struct cfs_rq *cfs_rq,
+					  struct sched_entity *se,
+					  struct cfs_rq *p_cfs_rq)
+{
+	unsigned long load = cfs_rq->avg.load_avg;
+
+	if (cfs_rq != &cfs_rq->rq->cfs) {
+		if (!se)
+			se = &container_of(cfs_rq, struct cfs_tg_state, cfs_rq)->se;
+		if (!p_cfs_rq)
+			p_cfs_rq = cfs_rq_of(se);
+
+		load = p_cfs_rq->h_load;
+		load = div64_ul(load * se->avg.load_avg,
+				p_cfs_rq->avg.load_avg + 1);
+	}
+
+	WRITE_ONCE(cfs_rq->h_load, load);
+}
+
+static inline bool update_cfs_rq_h_load(struct cfs_rq *cfs_rq,
+					struct sched_entity *se,
+					struct cfs_rq *p_cfs_rq)
+{
+	/*
+	 * Mask out the segment bits, if the remaining bits match, then there
+	 * hasn't been a decay since the last time.
+	 */
+	if ((cfs_rq->last_h_load_update & ~PELT_SEGMENT_MASK) ==
+	    (cfs_rq->avg.last_update_time & ~PELT_SEGMENT_MASK))
+		return false;
+
+	__update_cfs_rq_h_load(cfs_rq, se, p_cfs_rq);
+
+	cfs_rq->last_h_load_update = cfs_rq->avg.last_update_time;
+	return true;
+}
+
 #else /* !CONFIG_FAIR_GROUP_SCHED: */
 
 static inline void update_tg_load_avg(struct cfs_rq *cfs_rq) {}
@@ -5554,6 +5592,10 @@ static inline int propagate_entity_load_
 
 static inline void add_tg_cfs_propagate(struct cfs_rq *cfs_rq, long runnable_sum) {}
 
+static inline bool update_cfs_rq_h_load(struct cfs_rq *cfs_rq,
+					struct sched_entity *se,
+					struct cfs_rq *p_cfs_rq) { return false; }
+
 #endif /* !CONFIG_FAIR_GROUP_SCHED */
 
 #ifdef CONFIG_NO_HZ_COMMON
@@ -7966,6 +8008,9 @@ static unsigned long enqueue_hierarchy(s
 		flags = ENQUEUE_WAKEUP;
 	}
 
+	for_each_sched_entity_bl(se, cfs_rq)
+		update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
+
 	return weight;
 }
 
@@ -8107,6 +8152,9 @@ static void dequeue_hierarchy(struct tas
 		flags |= DEQUEUE_SLEEP;
 		flags &= ~(DEQUEUE_DELAYED | DEQUEUE_SPECIAL);
 	}
+
+	for_each_sched_entity_bl(se, cfs_rq)
+		update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
 }
 
 /*
@@ -11288,51 +11336,23 @@ static bool __update_blocked_fair(struct
 			*done = false;
 	}
 
-	return decayed;
-}
-
-/*
- * Compute the hierarchical load factor for cfs_rq and all its ascendants.
- * This needs to be done in a top-down fashion because the load of a child
- * group is a fraction of its parents load.
- */
-static void update_cfs_rq_h_load(struct cfs_rq *cfs_rq)
-{
-	struct sched_entity *se = cfs_rq_se(cfs_rq);
-	unsigned long now = jiffies;
-	unsigned long load;
-
-	if (cfs_rq->last_h_load_update == now)
-		return;
-
-	WRITE_ONCE(cfs_rq->h_load_next, NULL);
-	for_each_sched_entity(se, cfs_rq) {
-		WRITE_ONCE(cfs_rq->h_load_next, se);
-		if (cfs_rq->last_h_load_update == now)
-			break;
-	}
-
-	if (!se) {
-		cfs_rq->h_load = cfs_rq_load_avg(cfs_rq);
-		cfs_rq->last_h_load_update = now;
-	}
+	/*
+	 * The above (forward) leaf_cfs_rq_list traversal will have done
+	 * update_cfs_rq_load_avg() in a bottom-up fashion. Now iterate the
+	 * list backwards, such that we're ensured to have visited every
+	 * parent of the current group to update h_load in a top-down fashion.
+	 */
+	list_for_each_entry_reverse(cfs_rq, &rq->leaf_cfs_rq_list, leaf_cfs_rq_list)
+		update_cfs_rq_h_load(cfs_rq, NULL, NULL);
 
-	while ((se = READ_ONCE(cfs_rq->h_load_next)) != NULL) {
-		load = cfs_rq->h_load;
-		load = div64_ul(load * se->avg.load_avg,
-				cfs_rq_load_avg(cfs_rq) + 1);
-		cfs_rq = group_cfs_rq(se);
-		cfs_rq->h_load = load;
-		cfs_rq->last_h_load_update = now;
-	}
+	return decayed;
 }
 
 static unsigned long task_h_load(struct task_struct *p)
 {
 	struct cfs_rq *cfs_rq = task_cfs_rq(p);
 
-	update_cfs_rq_h_load(cfs_rq);
-	return div64_ul(p->se.avg.load_avg * cfs_rq->h_load,
+	return div64_ul(p->se.avg.load_avg * READ_ONCE(cfs_rq->h_load),
 			cfs_rq_load_avg(cfs_rq) + 1);
 }
 #else /* !CONFIG_FAIR_GROUP_SCHED: */
@@ -14949,7 +14969,7 @@ static inline void task_tick_core(struct
 /*
  * se_fi_update - Update the cfs_rq->zero_vruntime_fi in a CFS hierarchy if needed.
  */
-static void se_fi_update(const struct sched_entity *se, unsigned int fi_seq,
+static void se_fi_update(struct sched_entity *se, unsigned int fi_seq,
 			 bool forceidle)
 {
 	struct cfs_rq *cfs_rq;
@@ -15039,6 +15059,11 @@ static void task_tick_fair(struct rq *rq
 
 		se = &curr->se;
 		reweight_eevdf(cfs_rq, se, weight, se->on_rq);
+
+		if (!hrtick) {
+			for_each_sched_entity_bl(se, cfs_rq)
+				update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
+		}
 	}
 
 	if (hrtick)
@@ -15230,6 +15255,9 @@ static void set_next_task_fair(struct rq
 			weight = __calc_prop_weight(cfs_rq, se, weight);
 	}
 
+	for_each_sched_entity_bl(se, cfs_rq)
+		update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
+
 	if (throttled)
 		task_throttle_setup_work(p);
 
@@ -15428,6 +15456,8 @@ static int __sched_group_set_shares(stru
 			update_load_avg(cfs_rq, se, UPDATE_TG);
 			update_cfs_group(se);
 		}
+		for_each_sched_entity_bl(se, cfs_rq)
+			update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
 		rq_unlock_irqrestore(rq, &rf);
 	}
 
--- a/kernel/sched/pelt.h
+++ b/kernel/sched/pelt.h
@@ -5,6 +5,13 @@
 
 #include "sched-pelt.h"
 
+/*
+ * Pelt uses apprixmate 'us' as ns/1024; and then uses time segments of 1024
+ * 'us'. As a result each segment is in fact '1<<20' ns.
+ */
+#define PELT_SEGMENT_NS		(1<<20)
+#define PELT_SEGMENT_MASK	(PELT_SEGMENT_NS-1)
+
 int __update_load_avg_blocked_se(u64 now, struct sched_entity *se);
 int __update_load_avg_se(u64 now, struct cfs_rq *cfs_rq, struct sched_entity *se);
 int __update_load_avg_cfs_rq(u64 now, struct cfs_rq *cfs_rq);
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -736,7 +736,6 @@ struct cfs_rq {
 	 */
 	unsigned long		h_load;
 	u64			last_h_load_update;
-	struct sched_entity	*h_load_next;
 
 	struct rq		*rq;	/* CPU runqueue to which this cfs_rq is attached */
 

Re: [PATCH 4/4] sched/fair: Rework/fix task_h_load()

[Vincent Guittot]

On Fri, 28 Aug 2026 at 09:56, Peter Zijlstra <peterz@infradead.org> wrote:
>
> There are a number of issues with task_h_load():
>
>  - its hierarchy traversal is racy to the point of being broken; where
>    originally it was meant to be used under rq->lock, but lacking an assertion
>    for that fact, its use spread and violated this. The result is that the
>    back-link state is prone to races.
>
>  - it is rate-limited on jiffies, which is HZ, not the underlying PELT decay.
>
>  - since its update is tied to task_h_load() usage, the cfs_rq->h_load numbers are
>    not often 'up-to-date', rendering their output in sched/debug near useless.
>
> Rework the whole thing to keep a more up-to-date and less broken cfs_rq->h_load
> number.
>
> Move the back-link tracking into for_each_sched_entity(), such that any such
> loop sets up a path back. Use this to (optionally) re-compute cfs_rq->h_load on
> enqueue, dequeue, set_next and tick, all sites that hold rq->lock.
>
> This ensures that 'active' cgroups have reasonably up-to-date cfs_rq->h_load.
> Additionally, have __update_blocked_fair() update cfs_rq->h_load for all
> cgroups.
>
> Finally, replace the jiffy rate-limit with one that is tied to the PELT decay.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/sched/fair.c  |  108 ++++++++++++++++++++++++++++++++-------------------
>  kernel/sched/pelt.h  |    7 +++
>  kernel/sched/sched.h |    1
>  3 files changed, 76 insertions(+), 40 deletions(-)
>
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5541,6 +5541,44 @@ static inline bool skip_blocked_update(s
>         return true;
>  }
>
> +static inline void __update_cfs_rq_h_load(struct cfs_rq *cfs_rq,
> +                                         struct sched_entity *se,
> +                                         struct cfs_rq *p_cfs_rq)
> +{
> +       unsigned long load = cfs_rq->avg.load_avg;
> +
> +       if (cfs_rq != &cfs_rq->rq->cfs) {

A comment explaining that the 2 belows are used for the case where
for_each_sched_entity has not been called to set backlink would be
helpful

I haven't run tests yet (I will during the week) but the rework looks good to me




> +               if (!se)
> +                       se = &container_of(cfs_rq, struct cfs_tg_state, cfs_rq)->se;
> +               if (!p_cfs_rq)
> +                       p_cfs_rq = cfs_rq_of(se);
> +
> +               load = p_cfs_rq->h_load;
> +               load = div64_ul(load * se->avg.load_avg,
> +                               p_cfs_rq->avg.load_avg + 1);
> +       }
> +
> +       WRITE_ONCE(cfs_rq->h_load, load);
> +}
> +
> +static inline bool update_cfs_rq_h_load(struct cfs_rq *cfs_rq,
> +                                       struct sched_entity *se,
> +                                       struct cfs_rq *p_cfs_rq)
> +{
> +       /*
> +        * Mask out the segment bits, if the remaining bits match, then there
> +        * hasn't been a decay since the last time.
> +        */
> +       if ((cfs_rq->last_h_load_update & ~PELT_SEGMENT_MASK) ==
> +           (cfs_rq->avg.last_update_time & ~PELT_SEGMENT_MASK))
> +               return false;
> +
> +       __update_cfs_rq_h_load(cfs_rq, se, p_cfs_rq);
> +
> +       cfs_rq->last_h_load_update = cfs_rq->avg.last_update_time;
> +       return true;
> +}
> +
>  #else /* !CONFIG_FAIR_GROUP_SCHED: */
>
>  static inline void update_tg_load_avg(struct cfs_rq *cfs_rq) {}
> @@ -5554,6 +5592,10 @@ static inline int propagate_entity_load_
>
>  static inline void add_tg_cfs_propagate(struct cfs_rq *cfs_rq, long runnable_sum) {}
>
> +static inline bool update_cfs_rq_h_load(struct cfs_rq *cfs_rq,
> +                                       struct sched_entity *se,
> +                                       struct cfs_rq *p_cfs_rq) { return false; }
> +
>  #endif /* !CONFIG_FAIR_GROUP_SCHED */
>
>  #ifdef CONFIG_NO_HZ_COMMON
> @@ -7966,6 +8008,9 @@ static unsigned long enqueue_hierarchy(s
>                 flags = ENQUEUE_WAKEUP;
>         }
>
> +       for_each_sched_entity_bl(se, cfs_rq)
> +               update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
> +
>         return weight;
>  }
>
> @@ -8107,6 +8152,9 @@ static void dequeue_hierarchy(struct tas
>                 flags |= DEQUEUE_SLEEP;
>                 flags &= ~(DEQUEUE_DELAYED | DEQUEUE_SPECIAL);
>         }
> +
> +       for_each_sched_entity_bl(se, cfs_rq)
> +               update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
>  }
>
>  /*
> @@ -11288,51 +11336,23 @@ static bool __update_blocked_fair(struct
>                         *done = false;
>         }
>
> -       return decayed;
> -}
> -
> -/*
> - * Compute the hierarchical load factor for cfs_rq and all its ascendants.
> - * This needs to be done in a top-down fashion because the load of a child
> - * group is a fraction of its parents load.
> - */
> -static void update_cfs_rq_h_load(struct cfs_rq *cfs_rq)
> -{
> -       struct sched_entity *se = cfs_rq_se(cfs_rq);
> -       unsigned long now = jiffies;
> -       unsigned long load;
> -
> -       if (cfs_rq->last_h_load_update == now)
> -               return;
> -
> -       WRITE_ONCE(cfs_rq->h_load_next, NULL);
> -       for_each_sched_entity(se, cfs_rq) {
> -               WRITE_ONCE(cfs_rq->h_load_next, se);
> -               if (cfs_rq->last_h_load_update == now)
> -                       break;
> -       }
> -
> -       if (!se) {
> -               cfs_rq->h_load = cfs_rq_load_avg(cfs_rq);
> -               cfs_rq->last_h_load_update = now;
> -       }
> +       /*
> +        * The above (forward) leaf_cfs_rq_list traversal will have done
> +        * update_cfs_rq_load_avg() in a bottom-up fashion. Now iterate the
> +        * list backwards, such that we're ensured to have visited every
> +        * parent of the current group to update h_load in a top-down fashion.
> +        */
> +       list_for_each_entry_reverse(cfs_rq, &rq->leaf_cfs_rq_list, leaf_cfs_rq_list)
> +               update_cfs_rq_h_load(cfs_rq, NULL, NULL);
>
> -       while ((se = READ_ONCE(cfs_rq->h_load_next)) != NULL) {
> -               load = cfs_rq->h_load;
> -               load = div64_ul(load * se->avg.load_avg,
> -                               cfs_rq_load_avg(cfs_rq) + 1);
> -               cfs_rq = group_cfs_rq(se);
> -               cfs_rq->h_load = load;
> -               cfs_rq->last_h_load_update = now;
> -       }
> +       return decayed;
>  }
>
>  static unsigned long task_h_load(struct task_struct *p)
>  {
>         struct cfs_rq *cfs_rq = task_cfs_rq(p);
>
> -       update_cfs_rq_h_load(cfs_rq);
> -       return div64_ul(p->se.avg.load_avg * cfs_rq->h_load,
> +       return div64_ul(p->se.avg.load_avg * READ_ONCE(cfs_rq->h_load),
>                         cfs_rq_load_avg(cfs_rq) + 1);
>  }
>  #else /* !CONFIG_FAIR_GROUP_SCHED: */
> @@ -14949,7 +14969,7 @@ static inline void task_tick_core(struct
>  /*
>   * se_fi_update - Update the cfs_rq->zero_vruntime_fi in a CFS hierarchy if needed.
>   */
> -static void se_fi_update(const struct sched_entity *se, unsigned int fi_seq,
> +static void se_fi_update(struct sched_entity *se, unsigned int fi_seq,
>                          bool forceidle)
>  {
>         struct cfs_rq *cfs_rq;
> @@ -15039,6 +15059,11 @@ static void task_tick_fair(struct rq *rq
>
>                 se = &curr->se;
>                 reweight_eevdf(cfs_rq, se, weight, se->on_rq);
> +
> +               if (!hrtick) {
> +                       for_each_sched_entity_bl(se, cfs_rq)
> +                               update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
> +               }
>         }
>
>         if (hrtick)
> @@ -15230,6 +15255,9 @@ static void set_next_task_fair(struct rq
>                         weight = __calc_prop_weight(cfs_rq, se, weight);
>         }
>
> +       for_each_sched_entity_bl(se, cfs_rq)
> +               update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
> +
>         if (throttled)
>                 task_throttle_setup_work(p);
>
> @@ -15428,6 +15456,8 @@ static int __sched_group_set_shares(stru
>                         update_load_avg(cfs_rq, se, UPDATE_TG);
>                         update_cfs_group(se);
>                 }
> +               for_each_sched_entity_bl(se, cfs_rq)
> +                       update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
>                 rq_unlock_irqrestore(rq, &rf);
>         }
>
> --- a/kernel/sched/pelt.h
> +++ b/kernel/sched/pelt.h
> @@ -5,6 +5,13 @@
>
>  #include "sched-pelt.h"
>
> +/*
> + * Pelt uses apprixmate 'us' as ns/1024; and then uses time segments of 1024
> + * 'us'. As a result each segment is in fact '1<<20' ns.
> + */
> +#define PELT_SEGMENT_NS                (1<<20)
> +#define PELT_SEGMENT_MASK      (PELT_SEGMENT_NS-1)
> +
>  int __update_load_avg_blocked_se(u64 now, struct sched_entity *se);
>  int __update_load_avg_se(u64 now, struct cfs_rq *cfs_rq, struct sched_entity *se);
>  int __update_load_avg_cfs_rq(u64 now, struct cfs_rq *cfs_rq);
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -736,7 +736,6 @@ struct cfs_rq {
>          */
>         unsigned long           h_load;
>         u64                     last_h_load_update;
> -       struct sched_entity     *h_load_next;
>
>         struct rq               *rq;    /* CPU runqueue to which this cfs_rq is attached */
>
>
>

Re: [PATCH 4/4] sched/fair: Rework/fix task_h_load()

[Peter Zijlstra]

On Mon, Aug 31, 2026 at 12:10:42PM +0200, Vincent Guittot wrote:
> > +static inline void __update_cfs_rq_h_load(struct cfs_rq *cfs_rq,
> > +                                         struct sched_entity *se,
> > +                                         struct cfs_rq *p_cfs_rq)
> > +{
> > +       unsigned long load = cfs_rq->avg.load_avg;
> > +
> > +       if (cfs_rq != &cfs_rq->rq->cfs) {
> 
> A comment explaining that the 2 belows are used for the case where
> for_each_sched_entity has not been called to set backlink would be
> helpful

Fair enough; something like so?

		/*
		 * These last two arguments can be NULL then used outside of
		 * the for_each_sched_entity() hierarchy iteration. Like in
		 * __update_blocked_fair() where leaf_cfs_rq_list is iterated
		 * instead.
		 */

> > +               if (!se)
> > +                       se = &container_of(cfs_rq, struct cfs_tg_state, cfs_rq)->se;
> > +               if (!p_cfs_rq)
> > +                       p_cfs_rq = cfs_rq_of(se);
> > +
> > +               load = p_cfs_rq->h_load;
> > +               load = div64_ul(load * se->avg.load_avg,
> > +                               p_cfs_rq->avg.load_avg + 1);
> > +       }
> > +
> > +       WRITE_ONCE(cfs_rq->h_load, load);
> > +}
> > +
> > +static inline bool update_cfs_rq_h_load(struct cfs_rq *cfs_rq,
> > +                                       struct sched_entity *se,
> > +                                       struct cfs_rq *p_cfs_rq)
> > +{
> > +       /*
> > +        * Mask out the segment bits, if the remaining bits match, then there
> > +        * hasn't been a decay since the last time.
> > +        */
> > +       if ((cfs_rq->last_h_load_update & ~PELT_SEGMENT_MASK) ==
> > +           (cfs_rq->avg.last_update_time & ~PELT_SEGMENT_MASK))
> > +               return false;
> > +
> > +       __update_cfs_rq_h_load(cfs_rq, se, p_cfs_rq);
> > +
> > +       cfs_rq->last_h_load_update = cfs_rq->avg.last_update_time;
> > +       return true;
> > +}

Re: [PATCH 4/4] sched/fair: Rework/fix task_h_load()

[Vincent Guittot]

On Mon, 31 Aug 2026 at 12:37, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Aug 31, 2026 at 12:10:42PM +0200, Vincent Guittot wrote:
> > > +static inline void __update_cfs_rq_h_load(struct cfs_rq *cfs_rq,
> > > +                                         struct sched_entity *se,
> > > +                                         struct cfs_rq *p_cfs_rq)
> > > +{
> > > +       unsigned long load = cfs_rq->avg.load_avg;
> > > +
> > > +       if (cfs_rq != &cfs_rq->rq->cfs) {
> >
> > A comment explaining that the 2 belows are used for the case where
> > for_each_sched_entity has not been called to set backlink would be
> > helpful
>
> Fair enough; something like so?

yes, looks good

>
>                 /*
>                  * These last two arguments can be NULL then used outside of
 s/then/when/ ?

>                  * the for_each_sched_entity() hierarchy iteration. Like in
>                  * __update_blocked_fair() where leaf_cfs_rq_list is iterated
>                  * instead.
>                  */
>
> > > +               if (!se)
> > > +                       se = &container_of(cfs_rq, struct cfs_tg_state, cfs_rq)->se;
> > > +               if (!p_cfs_rq)
> > > +                       p_cfs_rq = cfs_rq_of(se);
> > > +
> > > +               load = p_cfs_rq->h_load;
> > > +               load = div64_ul(load * se->avg.load_avg,
> > > +                               p_cfs_rq->avg.load_avg + 1);
> > > +       }
> > > +
> > > +       WRITE_ONCE(cfs_rq->h_load, load);
> > > +}
> > > +
> > > +static inline bool update_cfs_rq_h_load(struct cfs_rq *cfs_rq,
> > > +                                       struct sched_entity *se,
> > > +                                       struct cfs_rq *p_cfs_rq)
> > > +{
> > > +       /*
> > > +        * Mask out the segment bits, if the remaining bits match, then there
> > > +        * hasn't been a decay since the last time.
> > > +        */
> > > +       if ((cfs_rq->last_h_load_update & ~PELT_SEGMENT_MASK) ==
> > > +           (cfs_rq->avg.last_update_time & ~PELT_SEGMENT_MASK))
> > > +               return false;
> > > +
> > > +       __update_cfs_rq_h_load(cfs_rq, se, p_cfs_rq);
> > > +
> > > +       cfs_rq->last_h_load_update = cfs_rq->avg.last_update_time;
> > > +       return true;
> > > +}

Re: [PATCH 4/4] sched/fair: Rework/fix task_h_load()

[Peter Zijlstra]

On Mon, Aug 31, 2026 at 02:06:14PM +0200, Vincent Guittot wrote:
> On Mon, 31 Aug 2026 at 12:37, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Mon, Aug 31, 2026 at 12:10:42PM +0200, Vincent Guittot wrote:
> > > > +static inline void __update_cfs_rq_h_load(struct cfs_rq *cfs_rq,
> > > > +                                         struct sched_entity *se,
> > > > +                                         struct cfs_rq *p_cfs_rq)
> > > > +{
> > > > +       unsigned long load = cfs_rq->avg.load_avg;
> > > > +
> > > > +       if (cfs_rq != &cfs_rq->rq->cfs) {
> > >
> > > A comment explaining that the 2 belows are used for the case where
> > > for_each_sched_entity has not been called to set backlink would be
> > > helpful
> >
> > Fair enough; something like so?
> 
> yes, looks good
> 
> >
> >                 /*
> >                  * These last two arguments can be NULL then used outside of
>  s/then/when/ ?

Just so, typing hard :/

> >                  * the for_each_sched_entity() hierarchy iteration. Like in
> >                  * __update_blocked_fair() where leaf_cfs_rq_list is iterated
> >                  * instead.
> >                  */

Re: [PATCH 4/4] sched/fair: Rework/fix task_h_load()

[Chen Yu]

On Fri, Aug 28, 2026 at 09:41:03AM +0200, Peter Zijlstra wrote:

[ ... ]

> @@ -15230,6 +15255,9 @@ static void set_next_task_fair(struct rq
>  			weight = __calc_prop_weight(cfs_rq, se, weight);
>  	}
>  
> +	for_each_sched_entity_bl(se, cfs_rq)
> +		update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
> +

It panics during bootup on a 192 Cores system,
[    9.445362][ T1687] Oops: general protection fault, kernel NULL pointer dereference 0x69: 0000 [#1] SMP NOPTI
[   18.987940][ T1687] CPU: 58 UID: 0 PID: 1687 Comm: systemd-udevd Not tainted 7.3.0-rc1-flat-hload+ #9 PREEMPTLAZY 
[   19.022374][ T1687] RIP: 0010:pick_task_fair+0x43/0xd0
[   19.175316][ T1687]  <TASK>
[   19.181239][ T1687]  __pick_next_task+0x49/0x1b0
[   19.189229][ T1687]  __schedule+0x14b/0x6b0
[   19.196711][ T1687]  preempt_schedule+0x3a/0x60
[   19.204543][ T1687]  preempt_schedule_thunk+0x16/0x40
[   19.212931][ T1687]  _raw_spin_unlock_irqrestore+0x2b/0x30
[   19.221833][ T1687]  autogroup_move_group+0xc5/0x160
[   19.230124][ T1687]  sched_autogroup_create_attach+0xa7/0x180
[   19.239280][ T1687]  ksys_setsid+0x12c/0x170
[   19.246753][ T1687]  __do_sys_setsid+0xe/0x20
[   19.254292][ T1687]  do_syscall_64+0xbc/0x470
[   19.307858][ T1687]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   19.316405][ T1687] RIP: 0033:0x7f347bfbcb9b

It seems that the crash is a read of se->sched_delayed on a NULL se inside
pick_next_entity(), so pick_eevdf() return NULL

After the following top->down backlink traverse,
for_each_sched_entity_bl(se, cfs_rq)
    update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);

cfs_rq is not the root->cfs_rq anymore, but a middle cfs_rq(autogroup
in above example). Meanwhile rq->cfs.curr remains NULL because the
rq->cfs.curr has been dequeued if the task is runnable and queued:
if (on_rq)
	__dequeue_entity(cfs_rq, se)

se = &p->se;
cfs_rq->curr = se; /*wrong cfs_rq*/

Then later _raw_spin_unlock_irqrestore triggers the scheduling
it picks from rq->cfs_rq.curr with a NULL tree.

Maybe we need to restore the rq->cfs_rq after the
for_each_sched_entity_bl()?
se = &p->se;
cfs_rq = &rq->cfs; <--
cfs_rq->curr = se;

thanks,
Chenyu

Re: [PATCH 4/4] sched/fair: Rework/fix task_h_load()

[Vincent Guittot]

On Wed, 2 Sept 2026 at 07:39, Chen Yu <yu.c.chen@intel.com> wrote:
>
> On Fri, Aug 28, 2026 at 09:41:03AM +0200, Peter Zijlstra wrote:
>
> [ ... ]
>
> > @@ -15230,6 +15255,9 @@ static void set_next_task_fair(struct rq
> >                       weight = __calc_prop_weight(cfs_rq, se, weight);
> >       }
> >
> > +     for_each_sched_entity_bl(se, cfs_rq)
> > +             update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
> > +
>
> It panics during bootup on a 192 Cores system,
> [    9.445362][ T1687] Oops: general protection fault, kernel NULL pointer dereference 0x69: 0000 [#1] SMP NOPTI
> [   18.987940][ T1687] CPU: 58 UID: 0 PID: 1687 Comm: systemd-udevd Not tainted 7.3.0-rc1-flat-hload+ #9 PREEMPTLAZY
> [   19.022374][ T1687] RIP: 0010:pick_task_fair+0x43/0xd0
> [   19.175316][ T1687]  <TASK>
> [   19.181239][ T1687]  __pick_next_task+0x49/0x1b0
> [   19.189229][ T1687]  __schedule+0x14b/0x6b0
> [   19.196711][ T1687]  preempt_schedule+0x3a/0x60
> [   19.204543][ T1687]  preempt_schedule_thunk+0x16/0x40
> [   19.212931][ T1687]  _raw_spin_unlock_irqrestore+0x2b/0x30
> [   19.221833][ T1687]  autogroup_move_group+0xc5/0x160
> [   19.230124][ T1687]  sched_autogroup_create_attach+0xa7/0x180
> [   19.239280][ T1687]  ksys_setsid+0x12c/0x170
> [   19.246753][ T1687]  __do_sys_setsid+0xe/0x20
> [   19.254292][ T1687]  do_syscall_64+0xbc/0x470
> [   19.307858][ T1687]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [   19.316405][ T1687] RIP: 0033:0x7f347bfbcb9b
>
> It seems that the crash is a read of se->sched_delayed on a NULL se inside
> pick_next_entity(), so pick_eevdf() return NULL

I faced the same crash while testing

>
> After the following top->down backlink traverse,
> for_each_sched_entity_bl(se, cfs_rq)
>     update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
>
> cfs_rq is not the root->cfs_rq anymore, but a middle cfs_rq(autogroup
> in above example). Meanwhile rq->cfs.curr remains NULL because the
> rq->cfs.curr has been dequeued if the task is runnable and queued:
> if (on_rq)
>         __dequeue_entity(cfs_rq, se)
>
> se = &p->se;
> cfs_rq->curr = se; /*wrong cfs_rq*/
>
> Then later _raw_spin_unlock_irqrestore triggers the scheduling
> it picks from rq->cfs_rq.curr with a NULL tree.
>
> Maybe we need to restore the rq->cfs_rq after the
> for_each_sched_entity_bl()?
> se = &p->se;
> cfs_rq = &rq->cfs; <--

Yes, this fixes it for me too

> cfs_rq->curr = se;
>
> thanks,
> Chenyu

Re: [PATCH 4/4] sched/fair: Rework/fix task_h_load()

[Peter Zijlstra]

On Wed, Sep 02, 2026 at 09:55:49AM +0200, Vincent Guittot wrote:
> On Wed, 2 Sept 2026 at 07:39, Chen Yu <yu.c.chen@intel.com> wrote:
> >
> > On Fri, Aug 28, 2026 at 09:41:03AM +0200, Peter Zijlstra wrote:
> >
> > [ ... ]
> >
> > > @@ -15230,6 +15255,9 @@ static void set_next_task_fair(struct rq
> > >                       weight = __calc_prop_weight(cfs_rq, se, weight);
> > >       }
> > >
> > > +     for_each_sched_entity_bl(se, cfs_rq)
> > > +             update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
> > > +
> >
> > It panics during bootup on a 192 Cores system,
> > [    9.445362][ T1687] Oops: general protection fault, kernel NULL pointer dereference 0x69: 0000 [#1] SMP NOPTI
> > [   18.987940][ T1687] CPU: 58 UID: 0 PID: 1687 Comm: systemd-udevd Not tainted 7.3.0-rc1-flat-hload+ #9 PREEMPTLAZY
> > [   19.022374][ T1687] RIP: 0010:pick_task_fair+0x43/0xd0
> > [   19.175316][ T1687]  <TASK>
> > [   19.181239][ T1687]  __pick_next_task+0x49/0x1b0
> > [   19.189229][ T1687]  __schedule+0x14b/0x6b0
> > [   19.196711][ T1687]  preempt_schedule+0x3a/0x60
> > [   19.204543][ T1687]  preempt_schedule_thunk+0x16/0x40
> > [   19.212931][ T1687]  _raw_spin_unlock_irqrestore+0x2b/0x30
> > [   19.221833][ T1687]  autogroup_move_group+0xc5/0x160
> > [   19.230124][ T1687]  sched_autogroup_create_attach+0xa7/0x180
> > [   19.239280][ T1687]  ksys_setsid+0x12c/0x170
> > [   19.246753][ T1687]  __do_sys_setsid+0xe/0x20
> > [   19.254292][ T1687]  do_syscall_64+0xbc/0x470
> > [   19.307858][ T1687]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> > [   19.316405][ T1687] RIP: 0033:0x7f347bfbcb9b
> >
> > It seems that the crash is a read of se->sched_delayed on a NULL se inside
> > pick_next_entity(), so pick_eevdf() return NULL
> 
> I faced the same crash while testing

Weirdly that crash didn't show up for me :-(, I had a few others that I
cured.

> >
> > After the following top->down backlink traverse,
> > for_each_sched_entity_bl(se, cfs_rq)
> >     update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
> >
> > cfs_rq is not the root->cfs_rq anymore, but a middle cfs_rq(autogroup
> > in above example). Meanwhile rq->cfs.curr remains NULL because the
> > rq->cfs.curr has been dequeued if the task is runnable and queued:
> > if (on_rq)
> >         __dequeue_entity(cfs_rq, se)
> >
> > se = &p->se;
> > cfs_rq->curr = se; /*wrong cfs_rq*/
> >
> > Then later _raw_spin_unlock_irqrestore triggers the scheduling
> > it picks from rq->cfs_rq.curr with a NULL tree.
> >
> > Maybe we need to restore the rq->cfs_rq after the
> > for_each_sched_entity_bl()?
> > se = &p->se;
> > cfs_rq = &rq->cfs; <--
> 
> Yes, this fixes it for me too

So I had this issue in task_tick_fair(), where
for_each_sched_entity_bl() clobbered cfs_rq, and fixed that by moving
things after reweight_eevdf() (which is what uses cfs_rq).

I at point I did actually look to see if anybody else would suffer that
same problem, but clearly I missed one.

I'm thinking the problem here is set_next_task_fair() ? I think I
misread the:

	se = &p->se;
	cfs_rq->curr = se;

to reset both se and cfs_rq, but clearly it doesn't. The below should
fix I suppose. Let me go and try and reproduce.


---
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -15287,9 +15287,6 @@ static void set_next_task_fair(struct rq
 			weight = __calc_prop_weight(cfs_rq, se, weight);
 	}
 
-	for_each_sched_entity_bl(se, cfs_rq)
-		update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
-
 	if (throttled)
 		task_throttle_setup_work(p);
 
@@ -15302,6 +15299,9 @@ static void set_next_task_fair(struct rq
 			set_protect_slice(cfs_rq, se);
 	}
 
+	for_each_sched_entity_bl(se, cfs_rq)
+		update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
+
 	if (task_on_rq_queued(p)) {
 		/*
 		 * Move the next running task to the front of the list, so our

Re: [PATCH 4/4] sched/fair: Rework/fix task_h_load()

[Peter Zijlstra]

On Wed, Sep 02, 2026 at 10:13:01AM +0200, Peter Zijlstra wrote:

> Let me go and try and reproduce.

Different physical machine.. *splat*, virtual machine it lives. Argh I
hate computers.

Anyway, confirmed on physical machine that triggered it, the blow seems
to cure things. I'll fold it in.

---
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -15287,9 +15287,6 @@ static void set_next_task_fair(struct rq
 			weight = __calc_prop_weight(cfs_rq, se, weight);
 	}
 
-	for_each_sched_entity_bl(se, cfs_rq)
-		update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
-
 	if (throttled)
 		task_throttle_setup_work(p);
 
@@ -15309,11 +15306,15 @@ static void set_next_task_fair(struct rq
 		 */
 		list_move(&se->group_node, &rq->cfs_tasks);
 	}
-	if (!first)
-		return;
 
 	WARN_ON_ONCE(se->sched_delayed);
 
+	for_each_sched_entity_bl(se, cfs_rq)
+		update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
+
+	if (!first)
+		return;
+
 	if (hrtick_enabled_fair(rq))
 		hrtick_start_fair(rq, p);
 

Re: [PATCH 4/4] sched/fair: Rework/fix task_h_load()

[Vincent Guittot]

On Wed, 2 Sept 2026 at 12:37, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Sep 02, 2026 at 10:13:01AM +0200, Peter Zijlstra wrote:
>
> > Let me go and try and reproduce.
>
> Different physical machine.. *splat*, virtual machine it lives. Argh I
> hate computers.
>
> Anyway, confirmed on physical machine that triggered it, the blow seems
> to cure things. I'll fold it in.
>
> ---
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -15287,9 +15287,6 @@ static void set_next_task_fair(struct rq
>                         weight = __calc_prop_weight(cfs_rq, se, weight);
>         }
>
> -       for_each_sched_entity_bl(se, cfs_rq)
> -               update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
> -
>         if (throttled)
>                 task_throttle_setup_work(p);
>
> @@ -15309,11 +15306,15 @@ static void set_next_task_fair(struct rq
>                  */
>                 list_move(&se->group_node, &rq->cfs_tasks);
>         }
> -       if (!first)
> -               return;
>
>         WARN_ON_ONCE(se->sched_delayed);
>
> +       for_each_sched_entity_bl(se, cfs_rq)
> +               update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
> +

should work too

> +       if (!first)
> +               return;
> +
>         if (hrtick_enabled_fair(rq))
>                 hrtick_start_fair(rq, p);
>
>
>

Re: [PATCH 4/4] sched/fair: Rework/fix task_h_load()

[Vincent Guittot]

On Wed, 2 Sept 2026 at 10:13, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Sep 02, 2026 at 09:55:49AM +0200, Vincent Guittot wrote:
> > On Wed, 2 Sept 2026 at 07:39, Chen Yu <yu.c.chen@intel.com> wrote:
> > >
> > > On Fri, Aug 28, 2026 at 09:41:03AM +0200, Peter Zijlstra wrote:
> > >
> > > [ ... ]
> > >
> > > > @@ -15230,6 +15255,9 @@ static void set_next_task_fair(struct rq
> > > >                       weight = __calc_prop_weight(cfs_rq, se, weight);
> > > >       }
> > > >
> > > > +     for_each_sched_entity_bl(se, cfs_rq)
> > > > +             update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
> > > > +
> > >
> > > It panics during bootup on a 192 Cores system,
> > > [    9.445362][ T1687] Oops: general protection fault, kernel NULL pointer dereference 0x69: 0000 [#1] SMP NOPTI
> > > [   18.987940][ T1687] CPU: 58 UID: 0 PID: 1687 Comm: systemd-udevd Not tainted 7.3.0-rc1-flat-hload+ #9 PREEMPTLAZY
> > > [   19.022374][ T1687] RIP: 0010:pick_task_fair+0x43/0xd0
> > > [   19.175316][ T1687]  <TASK>
> > > [   19.181239][ T1687]  __pick_next_task+0x49/0x1b0
> > > [   19.189229][ T1687]  __schedule+0x14b/0x6b0
> > > [   19.196711][ T1687]  preempt_schedule+0x3a/0x60
> > > [   19.204543][ T1687]  preempt_schedule_thunk+0x16/0x40
> > > [   19.212931][ T1687]  _raw_spin_unlock_irqrestore+0x2b/0x30
> > > [   19.221833][ T1687]  autogroup_move_group+0xc5/0x160
> > > [   19.230124][ T1687]  sched_autogroup_create_attach+0xa7/0x180
> > > [   19.239280][ T1687]  ksys_setsid+0x12c/0x170
> > > [   19.246753][ T1687]  __do_sys_setsid+0xe/0x20
> > > [   19.254292][ T1687]  do_syscall_64+0xbc/0x470
> > > [   19.307858][ T1687]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> > > [   19.316405][ T1687] RIP: 0033:0x7f347bfbcb9b
> > >
> > > It seems that the crash is a read of se->sched_delayed on a NULL se inside
> > > pick_next_entity(), so pick_eevdf() return NULL
> >
> > I faced the same crash while testing
>
> Weirdly that crash didn't show up for me :-(, I had a few others that I
> cured.

Some of my platforms didn't crash until I added +cpu in cgroup.sub_controller

>
> > >
> > > After the following top->down backlink traverse,
> > > for_each_sched_entity_bl(se, cfs_rq)
> > >     update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
> > >
> > > cfs_rq is not the root->cfs_rq anymore, but a middle cfs_rq(autogroup
> > > in above example). Meanwhile rq->cfs.curr remains NULL because the
> > > rq->cfs.curr has been dequeued if the task is runnable and queued:
> > > if (on_rq)
> > >         __dequeue_entity(cfs_rq, se)
> > >
> > > se = &p->se;
> > > cfs_rq->curr = se; /*wrong cfs_rq*/
> > >
> > > Then later _raw_spin_unlock_irqrestore triggers the scheduling
> > > it picks from rq->cfs_rq.curr with a NULL tree.
> > >
> > > Maybe we need to restore the rq->cfs_rq after the
> > > for_each_sched_entity_bl()?
> > > se = &p->se;
> > > cfs_rq = &rq->cfs; <--
> >
> > Yes, this fixes it for me too
>
> So I had this issue in task_tick_fair(), where
> for_each_sched_entity_bl() clobbered cfs_rq, and fixed that by moving
> things after reweight_eevdf() (which is what uses cfs_rq).

Yeah, all eevdf related things aim to use root cfs

>
> I at point I did actually look to see if anybody else would suffer that
> same problem, but clearly I missed one.
>
> I'm thinking the problem here is set_next_task_fair() ? I think I
> misread the:
>
>         se = &p->se;
>         cfs_rq->curr = se;
>
> to reset both se and cfs_rq, but clearly it doesn't. The below should
> fix I suppose. Let me go and try and reproduce.

it doesn't boot witht he below

>
>
> ---
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -15287,9 +15287,6 @@ static void set_next_task_fair(struct rq
>                         weight = __calc_prop_weight(cfs_rq, se, weight);
>         }
>
> -       for_each_sched_entity_bl(se, cfs_rq)
> -               update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
> -
>         if (throttled)
>                 task_throttle_setup_work(p);
>
> @@ -15302,6 +15299,9 @@ static void set_next_task_fair(struct rq
>                         set_protect_slice(cfs_rq, se);
>         }
>
> +       for_each_sched_entity_bl(se, cfs_rq)

Doesn't for_each_sched_entity_bl assume to start from root cfs as well ?


> +               update_cfs_rq_h_load(group_cfs_rq(se), se, cfs_rq);
> +

This modifies se, which is then used below in
list_move(se->group_node, &rq->cfs_tasks);
and
WARN_ON_ONCE(se->sched_delayed); below

Might be good to save pse = p->se


>         if (task_on_rq_queued(p)) {
>                 /*
>                  * Move the next running task to the front of the list, so our