Message-ID: <aS6yvxyc3JfMxxQW@gmail.com>
Date: Tue, 2 Dec 2025 10:34:55 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Vincent Guittot <vincent.guittot@...aro.org>
Cc: linux-kernel@...r.kernel.org, Peter Zijlstra <peterz@...radead.org>,
	Frederic Weisbecker <frederic@...nel.org>,
	Shrikanth Hegde <sshegde@...ux.ibm.com>,
	Juri Lelli <juri.lelli@...hat.com>,
	Dietmar Eggemann <dietmar.eggemann@....com>,
	Valentin Schneider <vschneid@...hat.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Mel Gorman <mgorman@...e.de>, Steven Rostedt <rostedt@...dmis.org>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: [PATCH 1/1 -v2] sched/fair: Sort out 'blocked_load*' namespace noise


* Vincent Guittot <vincent.guittot@...aro.org> wrote:

> On Tue, 2 Dec 2025 at 09:13, Ingo Molnar <mingo@...nel.org> wrote:
> >
> > There's three separate, independent pieces of logic in the
> > scheduler that are named 'has_blocked':
> >
> >    1) nohz.has_blocked,
> >    2) rq->has_blocked_load - both of these relate to NOHZ balancing,
> >
> >    3) and cfs_rq_has_blocked(), which operates on SMP load-balancing
> >       averages.
> >
> > While reviewing this code I noticed a couple of inconsistencies:
> >
> >   - nohz.has_blocked sometimes gets handled via a local variable
> >     that is named 'has_blocked_load' - but it's the runqueue
> >     that has the has_blocked_load field, not the nohz structure ...
> >
> >   - The cfs_rq_has_blocked() function does SMP load-balancing and
> >     has no relation to NOHZ has_blocked logic.
> >
> >   - The update_blocked_load_status() function, which sets the
> >     rq->has_blocked_load field, has a parameter named 'has_blocked',
> >     but that's the field name of the nohz structure.
> >
> > To sort all of this out, standardize on 3 distinct patterns:
> >
> >   (1) nohz.has_blocked related functions and variables use the
> >       'has_blocked' nomenclature,
> >
> >   (2) rq->has_blocked_load related functions and variables use
> >       'has_blocked_load',
> >
> >   (3) and cfs_rq_has_blocked() uses 'has_blocked_load_avg'.
> 
> They are all implementing the same feature: update the blocked pelt
> signal of idle rqs.

Yeah, should have said 3 separate layers of logic that 
each deal with the same thing, when writing the 
changelog I missed how update_blocked_load_status() 
feeds into rq->has_blocked_load via !done PELT signal 
we get back from the load-balancers and didn't look 
further. :-/

> If we want some renaming, we should use the same naming for all to
> show that it's all about the same thing
> 
> nohz.has_blocked_load()
> cfs_rq_has_blocked_load()
> rq->has_blocked_load()

I'd still argue that greppability of the 3 layers might 
have a small code readability value:

  git grep 'has_blocked\>'                kernel/sched/
  git grep 'has_blocked_load\>'           kernel/sched/
  git grep 'has_blocked_load_avg\>'       kernel/sched/

... and I've fixed up the changelogs to say:

	There's three separate layers of logic in the scheduler that
	deal with 'has_blocked' handling of the NOHZ code:

	  (1) nohz.has_blocked,
	  (2) rq->has_blocked_load, deal with NOHZ idle balancing,
	  (3) and cfs_rq_has_blocked(), which is part of the layer
	      that is passing the SMP load-balancing signal to the
	      NOHZ layers.

	To make it easier to separate them, split these 3 shared-mixed
	uses of 'has_blocked' name patterns into 3 distinct and greppable
	patterns:

	  (1) nohz.has_blocked related functions and variables use
	      'has_blocked',

	  (2) rq->has_blocked_load related functions and variables use
	      'has_blocked_load',

	  (3) and cfs_rq_has_blocked() uses 'has_blocked_load_avg'.

... but if you still object to that notion, we can also 
do your suggestion - see the patch below. Both variants 
are fine to me, no strong preferences, as long as the 
names remove the existing random noise. :-)

In fact, on a second thought, I slightly prefer your 
suggestion, as 'has_blocked_load' has a proper noun.

Thanks,

	Ingo

========================>
From: Ingo Molnar <mingo@...nel.org>
Subject: sched/fair: Sort out 'blocked_load*' namespace noise

There's three layers of logic in the scheduler that
deal with 'has_blocked' (load) handling of the NOHZ code:

  (1) nohz.has_blocked,
  (2) rq->has_blocked_load, deal with NOHZ idle balancing,
  (3) and cfs_rq_has_blocked(), which is part of the layer
      that is passing the SMP load-balancing signal to the
      NOHZ layers.

The 'has_blocked' and 'has_blocked_load' names are used 
in a mixed fashion, sometimes within the same function.

Standardize on 'has_blocked_load' to make it all easy 
to read and easy to grep.

No change in functionality.

Signed-off-by: Ingo Molnar <mingo@...nel.org>

 kernel/sched/fair.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index ff647e92a314..c6d3d01945bb 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7137,7 +7137,7 @@ static DEFINE_PER_CPU(cpumask_var_t, should_we_balance_tmpmask);
 static struct {
 	cpumask_var_t idle_cpus_mask;
 	atomic_t nr_cpus;
-	int has_blocked;		/* Idle CPUS has blocked load */
+	int has_blocked_load;		/* Idle CPUS has blocked load */
 	int needs_update;		/* Newly idle CPUs need their next_balance collated */
 	unsigned long next_balance;     /* in jiffy units */
 	unsigned long next_blocked;	/* Next update of blocked load in jiffies */
@@ -9767,7 +9767,7 @@ static void attach_tasks(struct lb_env *env)
 }
 
 #ifdef CONFIG_NO_HZ_COMMON
-static inline bool cfs_rq_has_blocked(struct cfs_rq *cfs_rq)
+static inline bool cfs_rq_has_blocked_load(struct cfs_rq *cfs_rq)
 {
 	if (cfs_rq->avg.load_avg)
 		return true;
@@ -9800,16 +9800,16 @@ static inline void update_blocked_load_tick(struct rq *rq)
 	WRITE_ONCE(rq->last_blocked_load_update_tick, jiffies);
 }
 
-static inline void update_blocked_load_status(struct rq *rq, bool has_blocked)
+static inline void update_blocked_load_status(struct rq *rq, bool has_blocked_load)
 {
-	if (!has_blocked)
+	if (!has_blocked_load)
 		rq->has_blocked_load = 0;
 }
 #else /* !CONFIG_NO_HZ_COMMON: */
-static inline bool cfs_rq_has_blocked(struct cfs_rq *cfs_rq) { return false; }
+static inline bool cfs_rq_has_blocked_load(struct cfs_rq *cfs_rq) { return false; }
 static inline bool others_have_blocked(struct rq *rq) { return false; }
 static inline void update_blocked_load_tick(struct rq *rq) {}
-static inline void update_blocked_load_status(struct rq *rq, bool has_blocked) {}
+static inline void update_blocked_load_status(struct rq *rq, bool has_blocked_load) {}
 #endif /* !CONFIG_NO_HZ_COMMON */
 
 static bool __update_blocked_others(struct rq *rq, bool *done)
@@ -9866,7 +9866,7 @@ static bool __update_blocked_fair(struct rq *rq, bool *done)
 			list_del_leaf_cfs_rq(cfs_rq);
 
 		/* Don't need periodic decay once load/util_avg are null */
-		if (cfs_rq_has_blocked(cfs_rq))
+		if (cfs_rq_has_blocked_load(cfs_rq))
 			*done = false;
 	}
 
@@ -9926,7 +9926,7 @@ static bool __update_blocked_fair(struct rq *rq, bool *done)
 	bool decayed;
 
 	decayed = update_cfs_rq_load_avg(cfs_rq_clock_pelt(cfs_rq), cfs_rq);
-	if (cfs_rq_has_blocked(cfs_rq))
+	if (cfs_rq_has_blocked_load(cfs_rq))
 		*done = false;
 
 	return decayed;
@@ -12443,7 +12443,7 @@ static void nohz_balancer_kick(struct rq *rq)
 	if (likely(!atomic_read(&nohz.nr_cpus)))
 		return;
 
-	if (READ_ONCE(nohz.has_blocked) &&
+	if (READ_ONCE(nohz.has_blocked_load) &&
 	    time_after(now, READ_ONCE(nohz.next_blocked)))
 		flags = NOHZ_STATS_KICK;
 
@@ -12604,9 +12604,9 @@ void nohz_balance_enter_idle(int cpu)
 
 	/*
 	 * The tick is still stopped but load could have been added in the
-	 * meantime. We set the nohz.has_blocked flag to trig a check of the
+	 * meantime. We set the nohz.has_blocked_load flag to trig a check of the
 	 * *_avg. The CPU is already part of nohz.idle_cpus_mask so the clear
-	 * of nohz.has_blocked can only happen after checking the new load
+	 * of nohz.has_blocked_load can only happen after checking the new load
 	 */
 	if (rq->nohz_tick_stopped)
 		goto out;
@@ -12622,7 +12622,7 @@ void nohz_balance_enter_idle(int cpu)
 
 	/*
 	 * Ensures that if nohz_idle_balance() fails to observe our
-	 * @idle_cpus_mask store, it must observe the @has_blocked
+	 * @idle_cpus_mask store, it must observe the @has_blocked_load
 	 * and @needs_update stores.
 	 */
 	smp_mb__after_atomic();
@@ -12635,7 +12635,7 @@ void nohz_balance_enter_idle(int cpu)
 	 * Each time a cpu enter idle, we assume that it has blocked load and
 	 * enable the periodic update of the load of idle CPUs
 	 */
-	WRITE_ONCE(nohz.has_blocked, 1);
+	WRITE_ONCE(nohz.has_blocked_load, 1);
 }
 
 static bool update_nohz_stats(struct rq *rq)
@@ -12676,8 +12676,8 @@ static void _nohz_idle_balance(struct rq *this_rq, unsigned int flags)
 
 	/*
 	 * We assume there will be no idle load after this update and clear
-	 * the has_blocked flag. If a cpu enters idle in the mean time, it will
-	 * set the has_blocked flag and trigger another update of idle load.
+	 * the has_blocked_load flag. If a cpu enters idle in the mean time, it will
+	 * set the has_blocked_load flag and trigger another update of idle load.
 	 * Because a cpu that becomes idle, is added to idle_cpus_mask before
 	 * setting the flag, we are sure to not clear the state and not
 	 * check the load of an idle cpu.
@@ -12685,12 +12685,12 @@ static void _nohz_idle_balance(struct rq *this_rq, unsigned int flags)
 	 * Same applies to idle_cpus_mask vs needs_update.
 	 */
 	if (flags & NOHZ_STATS_KICK)
-		WRITE_ONCE(nohz.has_blocked, 0);
+		WRITE_ONCE(nohz.has_blocked_load, 0);
 	if (flags & NOHZ_NEXT_KICK)
 		WRITE_ONCE(nohz.needs_update, 0);
 
 	/*
-	 * Ensures that if we miss the CPU, we must see the has_blocked
+	 * Ensures that if we miss the CPU, we must see the has_blocked_load
 	 * store from nohz_balance_enter_idle().
 	 */
 	smp_mb();
@@ -12757,7 +12757,7 @@ static void _nohz_idle_balance(struct rq *this_rq, unsigned int flags)
 abort:
 	/* There is still blocked load, enable periodic update */
 	if (has_blocked_load)
-		WRITE_ONCE(nohz.has_blocked, 1);
+		WRITE_ONCE(nohz.has_blocked_load, 1);
 }
 
 /*
@@ -12819,7 +12819,7 @@ static void nohz_newidle_balance(struct rq *this_rq)
 		return;
 
 	/* Don't need to update blocked load of idle CPUs*/
-	if (!READ_ONCE(nohz.has_blocked) ||
+	if (!READ_ONCE(nohz.has_blocked_load) ||
 	    time_before(jiffies, READ_ONCE(nohz.next_blocked)))
 		return;
 

Powered by blists - more mailing lists