From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7BA7D292B44 for ; Tue, 2 Dec 2025 16:10:05 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764691805; cv=none; b=ROtcMZCtL0enMp6QNbWHkaQXzfV0FfTJtnXK1CqKC8mifiVyk8SiVeWilbA53r2se218CoAF5J+049BvTiw64lTGHLsuWK832btDQr7xJ8BID7GMo3EjSo7pY1t0Wfpxpn9jtr8TIM+FoXKU2agchJGLOqrCMQ+5mevLKi3RjC8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764691805; c=relaxed/simple; bh=a85uFPrEDsqKOo7bVzP14oiOB2nHEJiJ4G+CHHaPqr8=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ekMlpBso8jcLPoheqRVcIi2yrh3Stsocexlm0PZUr/OzMxKzDxeZIPedwEX+mRkeNTGXjlgrjKVqzgit4BGwprEH76NG783FqCJ+9TNc48W+k1KIECU7+sFOKHeFM9wLpEQrH5myP+I0xZyy3YqSE9ioEnc5m3pyN739M2XHlac= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=twBIllnG; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="twBIllnG" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 62517C4CEF1; Tue, 2 Dec 2025 16:10:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1764691805; bh=a85uFPrEDsqKOo7bVzP14oiOB2nHEJiJ4G+CHHaPqr8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=twBIllnGSfsQzMxDBQxjs/eV/VvrdKQ/b0hSz33zyXDXHAoSwws8oBnemSalZnC86 IgyBpGkWXtQcehw87rhwDv6MMOeZYAudL6CtJPbKmvuc+xS/4l8J5FCGlXAURjHsEN T39+q4z0IMwF+3rkFYOz+C2LZp5V3lCIheWNYphIZOhTVagCCAYYUd+oCcF2dYcZAs 0b6er8NDCWcOEo1bAuoWphfrKWCza5c78riMjoGrdCWjz50wfy5tBs/y1kzlAJ9Xx/ F/wuj//6J5j7hIZ+1IAmVLNOWexkoKeIxp/8JjXCzPgGuCkn+sWcdNKK2sRxTk4Ojk /mN2rMxMwgffw== Date: Tue, 2 Dec 2025 17:09:59 +0100 From: Ingo Molnar To: Vincent Guittot Cc: linux-kernel@vger.kernel.org, Peter Zijlstra , Frederic Weisbecker , Shrikanth Hegde , Juri Lelli , Dietmar Eggemann , Valentin Schneider , Linus Torvalds , Mel Gorman , Steven Rostedt , Thomas Gleixner Subject: [PATCH 1/1 -v3] sched/fair: Sort out 'blocked_load*' namespace noise Message-ID: References: <20251202081304.3103393-1-mingo@kernel.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: * Vincent Guittot wrote: > On Tue, 2 Dec 2025 at 10:35, Ingo Molnar wrote: > > > > > > * Vincent Guittot wrote: > > > > > On Tue, 2 Dec 2025 at 09:13, Ingo Molnar 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 Okay, agreed - the reworked -v3 version is attached. I've optimistically added your Reviewed-by tag as well. :-) Thanks, Ingo - Change from -v2: also rename to cfs_rq_has_blocked_load(). ===================> >From 395fc683e48f6fe5f36082691681d0d64d1a48ff Mon Sep 17 00:00:00 2001 From: Ingo Molnar Date: Tue, 2 Dec 2025 10:35:06 +0100 Subject: [PATCH] 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. Suggested-by: Vincent Guittot Signed-off-by: Ingo Molnar Reviewed-by: Vincent Guittot Cc: Peter Zijlstra Cc: Frederic Weisbecker Cc: Shrikanth Hegde Link: https://patch.msgid.link/aS6yvxyc3JfMxxQW@gmail.com --- kernel/sched/fair.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index b6043ec4885b..76f5e4b78b30 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -7140,7 +7140,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 */ @@ -9776,7 +9776,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; @@ -9809,16 +9809,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_has_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_has_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) @@ -9875,7 +9875,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; } @@ -9935,7 +9935,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; @@ -9956,7 +9956,7 @@ static void __sched_balance_update_blocked_averages(struct rq *rq) decayed |= __update_blocked_others(rq, &done); decayed |= __update_blocked_fair(rq, &done); - update_blocked_load_status(rq, !done); + update_has_blocked_load_status(rq, !done); if (decayed) cpufreq_update_util(rq, 0); } @@ -12452,7 +12452,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; @@ -12613,9 +12613,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; @@ -12631,7 +12631,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(); @@ -12644,7 +12644,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) @@ -12685,8 +12685,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. @@ -12694,12 +12694,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(); @@ -12766,7 +12766,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); } /* @@ -12828,7 +12828,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;