Message-ID: <CAKfTPtAAi56vVim2W5s8U0YD5oZSTsX5iz+uX-fPKDOMr46B8g@mail.gmail.com>
Date: Tue, 2 Dec 2025 11:13:55 +0100
From: Vincent Guittot <vincent.guittot@...aro.org>
To: Ingo Molnar <mingo@...nel.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: Re: [PATCH 1/1 -v2] sched/fair: Sort out 'blocked_load*' namespace noise

On Tue, 2 Dec 2025 at 10:35, Ingo Molnar <mingo@...nel.org> wrote:
>
>
> * 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.

I would prefer using 'has_blocked_load' as I find it easier to get
that it's the same info saved in different places

Thanks

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