[PATCH 0/3] sched/nohz: Misc minor cleanups to the NOHZ idle balancing code

[PATCH 0/3] sched/nohz: Misc minor cleanups to the NOHZ idle balancing code

[Ingo Molnar]

Noticed a few minor details while I was reading this code:

Ingo Molnar (3):
  sched/nohz: Update idle load-balancing (ILB) comments
  sched/nohz: Use consistent variable names in find_new_ilb() and kick_ilb()
  sched/nohz: Remove weird error handling from find_new_ilb()

 kernel/sched/fair.c | 45 +++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

-- 
2.39.2

[PATCH 1/3] sched/nohz: Update idle load-balancing (ILB) comments

[Ingo Molnar]

 - Fix incorrect/misleading comments,

 - clarify some others,

 - fix typos & grammar,

 - and use more consistent style throughout.

Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: linux-kernel@vger.kernel.org
---
 kernel/sched/fair.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 04fbcbda97d5..8435179779e3 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -11547,14 +11547,15 @@ static inline int on_null_domain(struct rq *rq)
 
 #ifdef CONFIG_NO_HZ_COMMON
 /*
- * idle load balancing details
- * - When one of the busy CPUs notice that there may be an idle rebalancing
+ * NOHZ idle load balancing (ILB) details:
+ *
+ * - When one of the busy CPUs notices that there may be an idle rebalancing
  *   needed, they will kick the idle load balancer, which then does idle
  *   load balancing for all the idle CPUs.
- * - HK_TYPE_MISC CPUs are used for this task, because HK_TYPE_SCHED not set
+ *
+ * - HK_TYPE_MISC CPUs are used for this task, because HK_TYPE_SCHED is not set
  *   anywhere yet.
  */
-
 static inline int find_new_ilb(void)
 {
 	int ilb;
@@ -11575,8 +11576,10 @@ static inline int find_new_ilb(void)
 }
 
 /*
- * Kick a CPU to do the nohz balancing, if it is time for it. We pick any
- * idle CPU in the HK_TYPE_MISC housekeeping set (if there is one).
+ * Kick a CPU to do the NOHZ balancing, if it is time for it, via a cross-CPU
+ * SMP function call (IPI).
+ *
+ * We pick the first idle CPU in the HK_TYPE_MISC housekeeping set (if there is one).
  */
 static void kick_ilb(unsigned int flags)
 {
@@ -11604,7 +11607,7 @@ static void kick_ilb(unsigned int flags)
 
 	/*
 	 * This way we generate an IPI on the target CPU which
-	 * is idle. And the softirq performing nohz idle load balance
+	 * is idle, and the softirq performing NOHZ idle load balancing
 	 * will be run before returning from the IPI.
 	 */
 	smp_call_function_single_async(ilb_cpu, &cpu_rq(ilb_cpu)->nohz_csd);
@@ -11633,7 +11636,7 @@ static void nohz_balancer_kick(struct rq *rq)
 
 	/*
 	 * None are in tickless mode and hence no need for NOHZ idle load
-	 * balancing.
+	 * balancing:
 	 */
 	if (likely(!atomic_read(&nohz.nr_cpus)))
 		return;
@@ -11655,9 +11658,8 @@ static void nohz_balancer_kick(struct rq *rq)
 	sd = rcu_dereference(rq->sd);
 	if (sd) {
 		/*
-		 * If there's a CFS task and the current CPU has reduced
-		 * capacity; kick the ILB to see if there's a better CPU to run
-		 * on.
+		 * If there's a runnable CFS task and the current CPU has reduced
+		 * capacity, kick the ILB to see if there's a better CPU to run on:
 		 */
 		if (rq->cfs.h_nr_running >= 1 && check_cpu_capacity(rq, sd)) {
 			flags = NOHZ_STATS_KICK | NOHZ_BALANCE_KICK;
@@ -11709,11 +11711,11 @@ static void nohz_balancer_kick(struct rq *rq)
 	if (sds) {
 		/*
 		 * If there is an imbalance between LLC domains (IOW we could
-		 * increase the overall cache use), we need some less-loaded LLC
-		 * domain to pull some load. Likewise, we may need to spread
+		 * increase the overall cache utilization), we need a less-loaded LLC
+		 * domain to pull some load from. Likewise, we may need to spread
 		 * load within the current LLC domain (e.g. packed SMT cores but
 		 * other CPUs are idle). We can't really know from here how busy
-		 * the others are - so just get a nohz balance going if it looks
+		 * the others are - so just get a NOHZ balance going if it looks
 		 * like this LLC domain has tasks we could move.
 		 */
 		nr_busy = atomic_read(&sds->nr_busy_cpus);
-- 
2.39.2

[PATCH 2/3] sched/nohz: Use consistent variable names in find_new_ilb() and kick_ilb()

[Ingo Molnar]

Use 'ilb_cpu' consistently in both functions.

Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: linux-kernel@vger.kernel.org
---
 kernel/sched/fair.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 8435179779e3..d4e90d15bd77 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -11558,18 +11558,18 @@ static inline int on_null_domain(struct rq *rq)
  */
 static inline int find_new_ilb(void)
 {
-	int ilb;
 	const struct cpumask *hk_mask;
+	int ilb_cpu;
 
 	hk_mask = housekeeping_cpumask(HK_TYPE_MISC);
 
-	for_each_cpu_and(ilb, nohz.idle_cpus_mask, hk_mask) {
+	for_each_cpu_and(ilb_cpu, nohz.idle_cpus_mask, hk_mask) {
 
-		if (ilb == smp_processor_id())
+		if (ilb_cpu == smp_processor_id())
 			continue;
 
-		if (idle_cpu(ilb))
-			return ilb;
+		if (idle_cpu(ilb_cpu))
+			return ilb_cpu;
 	}
 
 	return nr_cpu_ids;
-- 
2.39.2

[PATCH 3/3] sched/nohz: Remove weird error handling from find_new_ilb()

[Ingo Molnar]

find_new_ilb() returns nr_cpu_ids on failure - which is a weird
choice in itself: not only is it a global variable, it is
a +1 out of bounds CPU index...

Its only user, kick_ilb(), then checks the return against nr_cpu_ids
to decide to return.

Instead of this, use a standard -1 return on failure to find an
idle CPU, as the argument is signed already.

Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: linux-kernel@vger.kernel.org
---
 kernel/sched/fair.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index d4e90d15bd77..dad60576cf56 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -11572,7 +11572,7 @@ static inline int find_new_ilb(void)
 			return ilb_cpu;
 	}
 
-	return nr_cpu_ids;
+	return -1;
 }
 
 /*
@@ -11593,8 +11593,7 @@ static void kick_ilb(unsigned int flags)
 		nohz.next_balance = jiffies+1;
 
 	ilb_cpu = find_new_ilb();
-
-	if (ilb_cpu >= nr_cpu_ids)
+	if (ilb_cpu < 0)
 		return;
 
 	/*
-- 
2.39.2

Re: [PATCH 3/3] sched/nohz: Remove weird error handling from find_new_ilb()

[Peter Zijlstra]

On Fri, Oct 06, 2023 at 12:25:18PM +0200, Ingo Molnar wrote:
> find_new_ilb() returns nr_cpu_ids on failure - which is a weird
> choice in itself: not only is it a global variable, it is
> a +1 out of bounds CPU index...

FWIW this is what all the cpumask bitops return when they've exhausted
the mask. Eg. no bits left set etc..

Re: [PATCH 3/3] sched/nohz: Remove weird error handling from find_new_ilb()

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Oct 06, 2023 at 12:25:18PM +0200, Ingo Molnar wrote:
> > find_new_ilb() returns nr_cpu_ids on failure - which is a weird
> > choice in itself: not only is it a global variable, it is
> > a +1 out of bounds CPU index...
> 
> FWIW this is what all the cpumask bitops return when they've exhausted
> the mask. Eg. no bits left set etc..

yeah, which then results in type-forcing uglies like:

   kernel/events/core.c:	if ((unsigned)cpu >= nr_cpu_ids) {
   kernel/events/core.c:   if ((unsigned)cpu >= nr_cpu_ids) {
   kernel/smp.c:	   if ((unsigned)cpu >= nr_cpu_ids || !cpu_online(cpu)) {

:-/

So I don't think this is a particularly well thought-out interface.

Thanks,

	Ingo

Re: [PATCH 0/3] sched/nohz: Misc minor cleanups to the NOHZ idle balancing code

[Joel Fernandes]

On Fri, Oct 06, 2023 at 12:25:15PM +0200, Ingo Molnar wrote:
> Noticed a few minor details while I was reading this code:
> 
> Ingo Molnar (3):
>   sched/nohz: Update idle load-balancing (ILB) comments
>   sched/nohz: Use consistent variable names in find_new_ilb() and kick_ilb()
>   sched/nohz: Remove weird error handling from find_new_ilb()

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

 - Joel

> 
>  kernel/sched/fair.c | 45 +++++++++++++++++++++++----------------------
>  1 file changed, 23 insertions(+), 22 deletions(-)
> 
> -- 
> 2.39.2
> 

[tip: sched/core] sched/nohz: Remove unnecessarily complex error handling pattern from find_new_ilb()

[tip-bot2 for Ingo Molnar]

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     f4bb5705114530cd775a5a649b666755b3efe7aa
Gitweb:        https://git.kernel.org/tip/f4bb5705114530cd775a5a649b666755b3efe7aa
Author:        Ingo Molnar <mingo@kernel.org>
AuthorDate:    Fri, 06 Oct 2023 12:25:18 +02:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Mon, 09 Oct 2023 12:21:23 +02:00

sched/nohz: Remove unnecessarily complex error handling pattern from find_new_ilb()

find_new_ilb() returns nr_cpu_ids on failure - which is the usual
cpumask bitops return pattern, but is weird & unnecessary in this
context: not only is it a global variable, it it is a +1 out of
bounds CPU index and also has different signedness ...

Its only user, kick_ilb(), then checks the return against nr_cpu_ids
to decide to return. There's no other use.

So instead of this, use a standard -1 return on failure to find an
idle CPU, as the argument is signed already.

Signed-off-by: Ingo Molnar <mingo@kernel.org>
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Link: https://lore.kernel.org/r/20231006102518.2452758-4-mingo@kernel.org
---
 kernel/sched/fair.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index f82b301..19bb4ac 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -11528,7 +11528,7 @@ static inline int find_new_ilb(void)
 			return ilb_cpu;
 	}
 
-	return nr_cpu_ids;
+	return -1;
 }
 
 /*
@@ -11549,8 +11549,7 @@ static void kick_ilb(unsigned int flags)
 		nohz.next_balance = jiffies+1;
 
 	ilb_cpu = find_new_ilb();
-
-	if (ilb_cpu >= nr_cpu_ids)
+	if (ilb_cpu < 0)
 		return;
 
 	/*

[tip: sched/core] sched/nohz: Use consistent variable names in find_new_ilb() and kick_ilb()

[tip-bot2 for Ingo Molnar]

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     b6dd6984832a2868f78879fce30d6965ae899d02
Gitweb:        https://git.kernel.org/tip/b6dd6984832a2868f78879fce30d6965ae899d02
Author:        Ingo Molnar <mingo@kernel.org>
AuthorDate:    Fri, 06 Oct 2023 12:25:17 +02:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Mon, 09 Oct 2023 12:21:23 +02:00

sched/nohz: Use consistent variable names in find_new_ilb() and kick_ilb()

Use 'ilb_cpu' consistently in both functions.

Signed-off-by: Ingo Molnar <mingo@kernel.org>
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Link: https://lore.kernel.org/r/20231006102518.2452758-3-mingo@kernel.org
---
 kernel/sched/fair.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 2b63a14..f82b301 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -11514,18 +11514,18 @@ static inline int on_null_domain(struct rq *rq)
  */
 static inline int find_new_ilb(void)
 {
-	int ilb;
 	const struct cpumask *hk_mask;
+	int ilb_cpu;
 
 	hk_mask = housekeeping_cpumask(HK_TYPE_MISC);
 
-	for_each_cpu_and(ilb, nohz.idle_cpus_mask, hk_mask) {
+	for_each_cpu_and(ilb_cpu, nohz.idle_cpus_mask, hk_mask) {
 
-		if (ilb == smp_processor_id())
+		if (ilb_cpu == smp_processor_id())
 			continue;
 
-		if (idle_cpu(ilb))
-			return ilb;
+		if (idle_cpu(ilb_cpu))
+			return ilb_cpu;
 	}
 
 	return nr_cpu_ids;

[tip: sched/core] sched/nohz: Update idle load-balancing (ILB) comments

[tip-bot2 for Ingo Molnar]

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     7ef7145a2b26b172ac6885c4cf3272a38bc0979a
Gitweb:        https://git.kernel.org/tip/7ef7145a2b26b172ac6885c4cf3272a38bc0979a
Author:        Ingo Molnar <mingo@kernel.org>
AuthorDate:    Fri, 06 Oct 2023 12:25:16 +02:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Mon, 09 Oct 2023 12:21:23 +02:00

sched/nohz: Update idle load-balancing (ILB) comments

 - Fix incorrect/misleading comments,

 - clarify some others,

 - fix typos & grammar,

 - and use more consistent style throughout.

Signed-off-by: Ingo Molnar <mingo@kernel.org>
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Link: https://lore.kernel.org/r/20231006102518.2452758-2-mingo@kernel.org
---
 kernel/sched/fair.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 52c498f..2b63a14 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -11503,14 +11503,15 @@ static inline int on_null_domain(struct rq *rq)
 
 #ifdef CONFIG_NO_HZ_COMMON
 /*
- * idle load balancing details
- * - When one of the busy CPUs notice that there may be an idle rebalancing
+ * NOHZ idle load balancing (ILB) details:
+ *
+ * - When one of the busy CPUs notices that there may be an idle rebalancing
  *   needed, they will kick the idle load balancer, which then does idle
  *   load balancing for all the idle CPUs.
- * - HK_TYPE_MISC CPUs are used for this task, because HK_TYPE_SCHED not set
+ *
+ * - HK_TYPE_MISC CPUs are used for this task, because HK_TYPE_SCHED is not set
  *   anywhere yet.
  */
-
 static inline int find_new_ilb(void)
 {
 	int ilb;
@@ -11531,8 +11532,10 @@ static inline int find_new_ilb(void)
 }
 
 /*
- * Kick a CPU to do the nohz balancing, if it is time for it. We pick any
- * idle CPU in the HK_TYPE_MISC housekeeping set (if there is one).
+ * Kick a CPU to do the NOHZ balancing, if it is time for it, via a cross-CPU
+ * SMP function call (IPI).
+ *
+ * We pick the first idle CPU in the HK_TYPE_MISC housekeeping set (if there is one).
  */
 static void kick_ilb(unsigned int flags)
 {
@@ -11560,7 +11563,7 @@ static void kick_ilb(unsigned int flags)
 
 	/*
 	 * This way we generate an IPI on the target CPU which
-	 * is idle. And the softirq performing nohz idle load balance
+	 * is idle, and the softirq performing NOHZ idle load balancing
 	 * will be run before returning from the IPI.
 	 */
 	smp_call_function_single_async(ilb_cpu, &cpu_rq(ilb_cpu)->nohz_csd);
@@ -11589,7 +11592,7 @@ static void nohz_balancer_kick(struct rq *rq)
 
 	/*
 	 * None are in tickless mode and hence no need for NOHZ idle load
-	 * balancing.
+	 * balancing:
 	 */
 	if (likely(!atomic_read(&nohz.nr_cpus)))
 		return;
@@ -11611,9 +11614,8 @@ static void nohz_balancer_kick(struct rq *rq)
 	sd = rcu_dereference(rq->sd);
 	if (sd) {
 		/*
-		 * If there's a CFS task and the current CPU has reduced
-		 * capacity; kick the ILB to see if there's a better CPU to run
-		 * on.
+		 * If there's a runnable CFS task and the current CPU has reduced
+		 * capacity, kick the ILB to see if there's a better CPU to run on:
 		 */
 		if (rq->cfs.h_nr_running >= 1 && check_cpu_capacity(rq, sd)) {
 			flags = NOHZ_STATS_KICK | NOHZ_BALANCE_KICK;
@@ -11665,11 +11667,11 @@ static void nohz_balancer_kick(struct rq *rq)
 	if (sds) {
 		/*
 		 * If there is an imbalance between LLC domains (IOW we could
-		 * increase the overall cache use), we need some less-loaded LLC
-		 * domain to pull some load. Likewise, we may need to spread
+		 * increase the overall cache utilization), we need a less-loaded LLC
+		 * domain to pull some load from. Likewise, we may need to spread
 		 * load within the current LLC domain (e.g. packed SMT cores but
 		 * other CPUs are idle). We can't really know from here how busy
-		 * the others are - so just get a nohz balance going if it looks
+		 * the others are - so just get a NOHZ balance going if it looks
 		 * like this LLC domain has tasks we could move.
 		 */
 		nr_busy = atomic_read(&sds->nr_busy_cpus);

Re: [PATCH 3/3] sched/nohz: Remove weird error handling from find_new_ilb()

[Valentin Schneider]

On 06/10/23 13:01, Ingo Molnar wrote:
> * Peter Zijlstra <peterz@infradead.org> wrote:
>
>> On Fri, Oct 06, 2023 at 12:25:18PM +0200, Ingo Molnar wrote:
>> > find_new_ilb() returns nr_cpu_ids on failure - which is a weird
>> > choice in itself: not only is it a global variable, it is
>> > a +1 out of bounds CPU index...
>> 
>> FWIW this is what all the cpumask bitops return when they've exhausted
>> the mask. Eg. no bits left set etc..
>
> yeah, which then results in type-forcing uglies like:
>
>    kernel/events/core.c:	if ((unsigned)cpu >= nr_cpu_ids) {
>    kernel/events/core.c:   if ((unsigned)cpu >= nr_cpu_ids) {
>    kernel/smp.c:	   if ((unsigned)cpu >= nr_cpu_ids || !cpu_online(cpu)) {

I can't see why we'd want smp_call_function_single*() /
generic_exec_single() to take a signed int as input, shouldn't this just be
unsigned?

The perf thing does look like it wants signed though...

>
> :-/
>
> So I don't think this is a particularly well thought-out interface.
>
> Thanks,
>
> 	Ingo

Re: [PATCH 1/3] sched/nohz: Update idle load-balancing (ILB) comments

[Valentin Schneider]

On 06/10/23 12:25, Ingo Molnar wrote:
>  - Fix incorrect/misleading comments,
>
>  - clarify some others,
>
>  - fix typos & grammar,
>
>  - and use more consistent style throughout.
>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Valentin Schneider <vschneid@redhat.com>

Re: [PATCH 2/3] sched/nohz: Use consistent variable names in find_new_ilb() and kick_ilb()

[Valentin Schneider]

On 06/10/23 12:25, Ingo Molnar wrote:
> Use 'ilb_cpu' consistently in both functions.
>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> Cc: linux-kernel@vger.kernel.org

Reviewed-by: Valentin Schneider <vschneid@redhat.com>