Received-SPF: pass (google.com: domain of linux-kernel+bounces-31724-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: Ming Lei <ming.lei@redhat.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org,
	Thomas Gleixner <tglx@linutronix.de>,
	Ming Lei <ming.lei@redhat.com>,
	Yury Norov <yury.norov@gmail.com>
Subject: [PATCH] lib/group_cpus.c: simplify grp_spread_init_one()
Date: Sat, 20 Jan 2024 14:55:43 +0800
Message-ID: <20240120065543.739203-1-ming.lei@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

What the inner loop needs to do is to assign each cpu in `siblmsk & nmsk`.

Clean it by using cpumask_next_and(), meantime add helper of grp_assign_cpu().

So grp_spread_init_one() becomes more readable now.

Cc: Yury Norov <yury.norov@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---

Hello Andrew,

Please consider to use this one to replace the 1st, 2nd & 4th patches
in Yury's V5:

- avoid to add new kernel API
- avoid to update iterator variable inside loop, which is tricky
- fix bug in the 4th patch
- add grp_assign_cpu() to make code more readable & clean

 lib/group_cpus.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/lib/group_cpus.c b/lib/group_cpus.c
index ee272c4cefcc..6cbc7379d954 100644
--- a/lib/group_cpus.c
+++ b/lib/group_cpus.c
@@ -11,32 +11,36 @@
 
 #ifdef CONFIG_SMP
 
+static bool grp_assign_cpu(int cpu, struct cpumask *irqmsk,
+		struct cpumask *nmsk)
+{
+	if (cpu >= nr_cpu_ids)
+		return false;
+
+	cpumask_clear_cpu(cpu, nmsk);
+	cpumask_set_cpu(cpu, irqmsk);
+	return true;
+}
+
 static void grp_spread_init_one(struct cpumask *irqmsk, struct cpumask *nmsk,
 				unsigned int cpus_per_grp)
 {
 	const struct cpumask *siblmsk;
-	int cpu, sibl;
-
-	for ( ; cpus_per_grp > 0; ) {
-		cpu = cpumask_first(nmsk);
+	int cpu = -1, sibl;
 
-		/* Should not happen, but I'm too lazy to think about it */
-		if (cpu >= nr_cpu_ids)
+	while (cpus_per_grp > 0) {
+		cpu = cpumask_next(cpu, nmsk);
+		if (!grp_assign_cpu(cpu, irqmsk, nmsk))
 			return;
-
-		cpumask_clear_cpu(cpu, nmsk);
-		cpumask_set_cpu(cpu, irqmsk);
 		cpus_per_grp--;
 
 		/* If the cpu has siblings, use them first */
 		siblmsk = topology_sibling_cpumask(cpu);
-		for (sibl = -1; cpus_per_grp > 0; ) {
-			sibl = cpumask_next(sibl, siblmsk);
-			if (sibl >= nr_cpu_ids)
+		sibl = cpu;
+		while (cpus_per_grp > 0) {
+			sibl = cpumask_next_and(sibl, siblmsk, nmsk);
+			if (!grp_assign_cpu(sibl, irqmsk, nmsk))
 				break;
-			if (!cpumask_test_and_clear_cpu(sibl, nmsk))
-				continue;
-			cpumask_set_cpu(sibl, irqmsk);
 			cpus_per_grp--;
 		}
 	}
-- 
2.41.0