[PATCH v4 0/5] cpumask: Fix invalid uniprocessor assumptions

[PATCH v4 0/5] cpumask: Fix invalid uniprocessor assumptions

[Sander Vanheule]

On uniprocessor builds, it is currently assumed that any cpumask will
contain the single CPU: cpu0. This assumption is used to provide
optimised implementations.

The current assumption also appears to be wrong, by ignoring the fact
that users can provide empty cpumask-s. This can result in bugs as
explained in [1].

This series introduces some basic tests, and updates the optimisations
for uniprocessor builds.

The x86 patch was written after the kernel test robot [2] ran into a
failed build. I have tried to list the files potentially affected by the
changes to cpumask.h, in an attempt to find any other cases that fail on
!SMP. I've gone through some of the files manually, and ran a few cross
builds, but nothing else popped up. I (build) checked about half of the
potientally affected files, but I do not have the resources to do them
all. I hope we can fix other issues if/when they pop up later.

[1] https://lore.kernel.org/all/20220530082552.46113-1-sander@svanheule.net/
[2] https://lore.kernel.org/all/202206060858.wA0FOzRy-lkp@intel.com/

Changes since v3:
Link: https://lore.kernel.org/all/cover.1654410109.git.sander@svanheule.net/
  - Guard againts CPU hotpluggin while testing cpu online/present masks
  - Add fix for cpu_llc_shared_map on x86

Changes since v2:
Link: https://lore.kernel.org/all/cover.1654362935.git.sander@svanheule.net/
  - Put new tests after patch fixes
  - Update for_each_* macros

Changes since v1:
Link: https://lore.kernel.org/all/cover.1654201862.git.sander@svanheule.net/
  - Place tests in lib/test_cpumask.c
  - Drop the modified UP code in favor of the generic SMP implementation
  - Update declaration of cpumask_next_wrap()

Sander Vanheule (5):
  x86/cacheinfo: move shared cache map definitions
  cpumask: Fix invalid uniprocessor mask assumption
  lib/test: Introduce cpumask KUnit test suite
  cpumask: Add UP optimised for_each_*_cpu versions
  cpumask: Update cpumask_next_wrap() signature

 arch/x86/kernel/cpu/cacheinfo.c |   6 ++
 arch/x86/kernel/smpboot.c       |   4 -
 include/linux/cpumask.h         | 108 +++++++------------------
 lib/Kconfig.debug               |   9 +++
 lib/Makefile                    |   4 +-
 lib/cpumask.c                   |   2 +
 lib/test_cpumask.c              | 138 ++++++++++++++++++++++++++++++++
 7 files changed, 184 insertions(+), 87 deletions(-)
 create mode 100644 lib/test_cpumask.c

-- 
2.36.1

[PATCH v4 1/5] x86/cacheinfo: move shared cache map definitions

[Sander Vanheule]

The maps to keep track of shared caches between CPUs on SMP systems are
declared in asm/smp.h, among them specifically cpu_llc_shared_map.
These maps are externally defined in cpu/smpboot.c. The latter is only
compiled on CONFIG_SMP=y, which means the declared extern symbols from
asm/smp.h do not have a corresponding definition on uniprocessor builds.

The inline cpu_llc_shared_mask() function from asm/smp.h refers to the
map declaration mentioned above. This function is referenced in
cacheinfo.c inside for_each_cpu() loop macros, to provide cpumask for
the loop. On uniprocessor builds, the symbol for the cpu_llc_shared_map
does not exist. However, the current implementation of for_each_cpu()
also (wrongly) ignores the provided mask.

By sheer luck, the compiler thus optimises out this unused reference to
cpu_llc_shared_map, and the linker therefore does not require the
cpu_llc_shared_mask to actually exist on uniprocessor builds. Only on
SMP bulids does smpboot.o exist to provide the required symbols.

To no longer rely on compiler optimisations for successful uniprocessor
builds, move the definitions of cpu_llc_shared_map and
cpu_l2c_shared_map from smpboot.c to cacheinfo.c.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 arch/x86/kernel/cpu/cacheinfo.c | 6 ++++++
 arch/x86/kernel/smpboot.c       | 4 ----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
index fe98a1465be6..66556833d7af 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -29,6 +29,12 @@
 #define LVL_3		4
 #define LVL_TRACE	5
 
+/* Shared last level cache maps */
+DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_llc_shared_map);
+
+/* Shared L2 cache maps */
+DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_l2c_shared_map);
+
 struct _cache_table {
 	unsigned char descriptor;
 	char cache_type;
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 5e7f9532a10d..f24227bc3220 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -95,10 +95,6 @@ EXPORT_PER_CPU_SYMBOL(cpu_core_map);
 DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_die_map);
 EXPORT_PER_CPU_SYMBOL(cpu_die_map);
 
-DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_llc_shared_map);
-
-DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_l2c_shared_map);
-
 /* Per CPU bogomips and other parameters */
 DEFINE_PER_CPU_READ_MOSTLY(struct cpuinfo_x86, cpu_info);
 EXPORT_PER_CPU_SYMBOL(cpu_info);
-- 
2.36.1

[PATCH v4 2/5] cpumask: Fix invalid uniprocessor mask assumption

[Sander Vanheule]

On uniprocessor builds, any CPU mask is assumed to contain exactly one
CPU (cpu0). This assumption ignores the existence of empty masks,
resulting in incorrect behaviour.
cpumask_first_zero(), cpumask_next_zero(), and for_each_cpu_not() don't
provide behaviour matching the assumption that a UP mask is always "1",
and instead provide behaviour matching the empty mask.

Drop the incorrectly optimised code and use the generic implementations
in all cases.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---

Notes:
    Changes since v3:
    - Add back UP-optimised cpumask_local_spread, cpumask_any_distribute,
      cpumask_any_and_distribute
    
    Changes since v1:
    - Drop UP implementations instead of trying to fix them

 include/linux/cpumask.h | 99 ++++++++---------------------------------
 lib/Makefile            |  3 +-
 lib/cpumask.c           |  2 +
 3 files changed, 22 insertions(+), 82 deletions(-)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index fe29ac7cc469..7fbef41b3093 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -116,85 +116,6 @@ static __always_inline unsigned int cpumask_check(unsigned int cpu)
 	return cpu;
 }
 
-#if NR_CPUS == 1
-/* Uniprocessor.  Assume all masks are "1". */
-static inline unsigned int cpumask_first(const struct cpumask *srcp)
-{
-	return 0;
-}
-
-static inline unsigned int cpumask_first_zero(const struct cpumask *srcp)
-{
-	return 0;
-}
-
-static inline unsigned int cpumask_first_and(const struct cpumask *srcp1,
-					     const struct cpumask *srcp2)
-{
-	return 0;
-}
-
-static inline unsigned int cpumask_last(const struct cpumask *srcp)
-{
-	return 0;
-}
-
-/* Valid inputs for n are -1 and 0. */
-static inline unsigned int cpumask_next(int n, const struct cpumask *srcp)
-{
-	return n+1;
-}
-
-static inline unsigned int cpumask_next_zero(int n, const struct cpumask *srcp)
-{
-	return n+1;
-}
-
-static inline unsigned int cpumask_next_and(int n,
-					    const struct cpumask *srcp,
-					    const struct cpumask *andp)
-{
-	return n+1;
-}
-
-static inline unsigned int cpumask_next_wrap(int n, const struct cpumask *mask,
-					     int start, bool wrap)
-{
-	/* cpu0 unless stop condition, wrap and at cpu0, then nr_cpumask_bits */
-	return (wrap && n == 0);
-}
-
-/* cpu must be a valid cpu, ie 0, so there's no other choice. */
-static inline unsigned int cpumask_any_but(const struct cpumask *mask,
-					   unsigned int cpu)
-{
-	return 1;
-}
-
-static inline unsigned int cpumask_local_spread(unsigned int i, int node)
-{
-	return 0;
-}
-
-static inline int cpumask_any_and_distribute(const struct cpumask *src1p,
-					     const struct cpumask *src2p) {
-	return cpumask_first_and(src1p, src2p);
-}
-
-static inline int cpumask_any_distribute(const struct cpumask *srcp)
-{
-	return cpumask_first(srcp);
-}
-
-#define for_each_cpu(cpu, mask)			\
-	for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)
-#define for_each_cpu_not(cpu, mask)		\
-	for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)
-#define for_each_cpu_wrap(cpu, mask, start)	\
-	for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask, (void)(start))
-#define for_each_cpu_and(cpu, mask1, mask2)	\
-	for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask1, (void)mask2)
-#else
 /**
  * cpumask_first - get the first cpu in a cpumask
  * @srcp: the cpumask pointer
@@ -260,10 +181,29 @@ static inline unsigned int cpumask_next_zero(int n, const struct cpumask *srcp)
 
 int __pure cpumask_next_and(int n, const struct cpumask *, const struct cpumask *);
 int __pure cpumask_any_but(const struct cpumask *mask, unsigned int cpu);
+
+#if NR_CPUS == 1
+/* Uniprocessor: there is only one valid CPU */
+static inline unsigned int cpumask_local_spread(unsigned int i, int node)
+{
+	return 0;
+}
+
+static inline int cpumask_any_and_distribute(const struct cpumask *src1p,
+					     const struct cpumask *src2p) {
+	return cpumask_first_and(src1p, src2p);
+}
+
+static inline int cpumask_any_distribute(const struct cpumask *srcp)
+{
+	return cpumask_first(srcp);
+}
+#else
 unsigned int cpumask_local_spread(unsigned int i, int node);
 int cpumask_any_and_distribute(const struct cpumask *src1p,
 			       const struct cpumask *src2p);
 int cpumask_any_distribute(const struct cpumask *srcp);
+#endif /* NR_CPUS */
 
 /**
  * for_each_cpu - iterate over every cpu in a mask
@@ -324,7 +264,6 @@ extern int cpumask_next_wrap(int n, const struct cpumask *mask, int start, bool
 	for ((cpu) = -1;						\
 		(cpu) = cpumask_next_and((cpu), (mask1), (mask2)),	\
 		(cpu) < nr_cpu_ids;)
-#endif /* SMP */
 
 #define CPU_BITS_NONE						\
 {								\
diff --git a/lib/Makefile b/lib/Makefile
index f99bf61f8bbc..bcc7e8ea0cde 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -34,10 +34,9 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \
 	 is_single_threaded.o plist.o decompress.o kobject_uevent.o \
 	 earlycpio.o seq_buf.o siphash.o dec_and_lock.o \
 	 nmi_backtrace.o nodemask.o win_minmax.o memcat_p.o \
-	 buildid.o
+	 buildid.o cpumask.o
 
 lib-$(CONFIG_PRINTK) += dump_stack.o
-lib-$(CONFIG_SMP) += cpumask.o
 
 lib-y	+= kobject.o klist.o
 obj-y	+= lockref.o
diff --git a/lib/cpumask.c b/lib/cpumask.c
index a971a82d2f43..b9728513a4d4 100644
--- a/lib/cpumask.c
+++ b/lib/cpumask.c
@@ -192,6 +192,7 @@ void __init free_bootmem_cpumask_var(cpumask_var_t mask)
 }
 #endif
 
+#if NR_CPUS > 1
 /**
  * cpumask_local_spread - select the i'th cpu with local numa cpu's first
  * @i: index number
@@ -279,3 +280,4 @@ int cpumask_any_distribute(const struct cpumask *srcp)
 	return next;
 }
 EXPORT_SYMBOL(cpumask_any_distribute);
+#endif /* NR_CPUS */
-- 
2.36.1

[PATCH v4 3/5] lib/test: Introduce cpumask KUnit test suite

[Sander Vanheule]

Add a basic suite of tests for cpumask, providing some tests for empty
and completely filled cpumasks.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

Notes:
    Changes since v3:
    - Test for_each_cpu*() over empty mask and cpu_possible_mask
    - Add Andy's Reviewed-by
    - Use num_{online,present,possible}_cpus() for builtin masks
    - Guard against CPU hotplugging during test for dynamic builtin CPU masks
    
    Changes since v2:
    - Rework for_each_* test macros, as suggested by Yury
    
    Changes since v1:
    - New patch

 lib/Kconfig.debug  |   9 +++
 lib/Makefile       |   1 +
 lib/test_cpumask.c | 138 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 148 insertions(+)
 create mode 100644 lib/test_cpumask.c

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 2e24db4bff19..04aaa20d50f9 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2021,6 +2021,15 @@ config LKDTM
 	Documentation on how to use the module can be found in
 	Documentation/fault-injection/provoke-crashes.rst
 
+config TEST_CPUMASK
+	tristate "cpumask tests" if !KUNIT_ALL_TESTS
+	depends on KUNIT
+	default KUNIT_ALL_TESTS
+	help
+	  Enable to turn on cpumask tests, running at boot or module load time.
+
+	  If unsure, say N.
+
 config TEST_LIST_SORT
 	tristate "Linked list sorting test" if !KUNIT_ALL_TESTS
 	depends on KUNIT
diff --git a/lib/Makefile b/lib/Makefile
index bcc7e8ea0cde..de3e47453fe8 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -99,6 +99,7 @@ obj-$(CONFIG_TEST_HMM) += test_hmm.o
 obj-$(CONFIG_TEST_FREE_PAGES) += test_free_pages.o
 obj-$(CONFIG_KPROBES_SANITY_TEST) += test_kprobes.o
 obj-$(CONFIG_TEST_REF_TRACKER) += test_ref_tracker.o
+obj-$(CONFIG_TEST_CPUMASK) += test_cpumask.o
 CFLAGS_test_fprobe.o += $(CC_FLAGS_FTRACE)
 obj-$(CONFIG_FPROBE_SANITY_TEST) += test_fprobe.o
 #
diff --git a/lib/test_cpumask.c b/lib/test_cpumask.c
new file mode 100644
index 000000000000..a31a1622f1f6
--- /dev/null
+++ b/lib/test_cpumask.c
@@ -0,0 +1,138 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * KUnit tests for cpumask.
+ *
+ * Author: Sander Vanheule <sander@svanheule.net>
+ */
+
+#include <kunit/test.h>
+#include <linux/cpu.h>
+#include <linux/cpumask.h>
+
+#define EXPECT_FOR_EACH_CPU_EQ(test, mask)			\
+	do {							\
+		const cpumask_t *m = (mask);			\
+		int mask_weight = cpumask_weight(m);		\
+		int cpu, iter = 0;				\
+		for_each_cpu(cpu, m)				\
+			iter++;					\
+		KUNIT_EXPECT_EQ((test), mask_weight, iter);	\
+	} while (0)
+
+#define EXPECT_FOR_EACH_CPU_NOT_EQ(test, mask)					\
+	do {									\
+		const cpumask_t *m = (mask);					\
+		int mask_weight = cpumask_weight(m);				\
+		int cpu, iter = 0;						\
+		for_each_cpu_not(cpu, m)					\
+			iter++;							\
+		KUNIT_EXPECT_EQ((test), nr_cpu_ids - mask_weight, iter);	\
+	} while (0)
+
+#define EXPECT_FOR_EACH_CPU_WRAP_EQ(test, mask)			\
+	do {							\
+		const cpumask_t *m = (mask);			\
+		int mask_weight = cpumask_weight(m);		\
+		int cpu, iter = 0;				\
+		for_each_cpu_wrap(cpu, m, nr_cpu_ids / 2)	\
+			iter++;					\
+		KUNIT_EXPECT_EQ((test), mask_weight, iter);	\
+	} while (0)
+
+#define EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, name)		\
+	do {							\
+		int mask_weight = num_##name##_cpus();		\
+		int cpu, iter = 0;				\
+		for_each_##name##_cpu(cpu)			\
+			iter++;					\
+		KUNIT_EXPECT_EQ((test), mask_weight, iter);	\
+	} while (0)
+
+static cpumask_t mask_empty;
+static cpumask_t mask_all;
+
+static void test_cpumask_weight(struct kunit *test)
+{
+	KUNIT_EXPECT_TRUE(test, cpumask_empty(&mask_empty));
+	KUNIT_EXPECT_TRUE(test, cpumask_full(cpu_possible_mask));
+	KUNIT_EXPECT_TRUE(test, cpumask_full(&mask_all));
+
+	KUNIT_EXPECT_EQ(test, 0, cpumask_weight(&mask_empty));
+	KUNIT_EXPECT_EQ(test, nr_cpu_ids, cpumask_weight(cpu_possible_mask));
+	KUNIT_EXPECT_EQ(test, nr_cpumask_bits, cpumask_weight(&mask_all));
+}
+
+static void test_cpumask_first(struct kunit *test)
+{
+	KUNIT_EXPECT_LE(test, nr_cpu_ids, cpumask_first(&mask_empty));
+	KUNIT_EXPECT_EQ(test, 0, cpumask_first(cpu_possible_mask));
+
+	KUNIT_EXPECT_EQ(test, 0, cpumask_first_zero(&mask_empty));
+	KUNIT_EXPECT_LE(test, nr_cpu_ids, cpumask_first_zero(cpu_possible_mask));
+}
+
+static void test_cpumask_last(struct kunit *test)
+{
+	KUNIT_EXPECT_LE(test, nr_cpumask_bits, cpumask_last(&mask_empty));
+	KUNIT_EXPECT_EQ(test, nr_cpumask_bits - 1, cpumask_last(cpu_possible_mask));
+}
+
+static void test_cpumask_next(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, 0, cpumask_next_zero(-1, &mask_empty));
+	KUNIT_EXPECT_LE(test, nr_cpu_ids, cpumask_next_zero(-1, cpu_possible_mask));
+
+	KUNIT_EXPECT_LE(test, nr_cpu_ids, cpumask_next(-1, &mask_empty));
+	KUNIT_EXPECT_EQ(test, 0, cpumask_next(-1, cpu_possible_mask));
+}
+
+static void test_cpumask_iterators(struct kunit *test)
+{
+	EXPECT_FOR_EACH_CPU_EQ(test, &mask_empty);
+	EXPECT_FOR_EACH_CPU_NOT_EQ(test, &mask_empty);
+	EXPECT_FOR_EACH_CPU_WRAP_EQ(test, &mask_empty);
+
+	EXPECT_FOR_EACH_CPU_EQ(test, cpu_possible_mask);
+	EXPECT_FOR_EACH_CPU_NOT_EQ(test, cpu_possible_mask);
+	EXPECT_FOR_EACH_CPU_WRAP_EQ(test, cpu_possible_mask);
+}
+
+static void test_cpumask_iterators_builtin(struct kunit *test)
+{
+	EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, possible);
+
+	/* Ensure the dynamic masks are stable while running the tests */
+	cpu_hotplug_disable();
+
+	EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, online);
+	EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, present);
+
+	cpu_hotplug_enable();
+}
+
+static int test_cpumask_init(struct kunit *test)
+{
+	cpumask_clear(&mask_empty);
+	cpumask_setall(&mask_all);
+
+	return 0;
+}
+
+static struct kunit_case test_cpumask_cases[] = {
+	KUNIT_CASE(test_cpumask_weight),
+	KUNIT_CASE(test_cpumask_first),
+	KUNIT_CASE(test_cpumask_last),
+	KUNIT_CASE(test_cpumask_next),
+	KUNIT_CASE(test_cpumask_iterators),
+	KUNIT_CASE(test_cpumask_iterators_builtin),
+	{}
+};
+
+static struct kunit_suite test_cpumask_suite = {
+	.name = "cpumask",
+	.init = test_cpumask_init,
+	.test_cases = test_cpumask_cases,
+};
+kunit_test_suite(test_cpumask_suite);
+
+MODULE_LICENSE("GPL");
-- 
2.36.1

[PATCH v4 4/5] cpumask: Add UP optimised for_each_*_cpu versions

[Sander Vanheule]

On uniprocessor builds, the following loops will always run over a mask
that contains one enabled CPU (cpu0):
    - for_each_possible_cpu
    - for_each_online_cpu
    - for_each_present_cpu

Provide uniprocessor-specific macros for these loops, that always run
exactly once.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 include/linux/cpumask.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 7fbef41b3093..6c5b4ee000f2 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -750,9 +750,16 @@ extern const DECLARE_BITMAP(cpu_all_bits, NR_CPUS);
 /* First bits of cpu_bit_bitmap are in fact unset. */
 #define cpu_none_mask to_cpumask(cpu_bit_bitmap[0])
 
+#if NR_CPUS == 1
+/* Uniprocessor: the possible/online/present masks are always "1" */
+#define for_each_possible_cpu(cpu)	for ((cpu) = 0; (cpu) < 1; (cpu)++)
+#define for_each_online_cpu(cpu)	for ((cpu) = 0; (cpu) < 1; (cpu)++)
+#define for_each_present_cpu(cpu)	for ((cpu) = 0; (cpu) < 1; (cpu)++)
+#else
 #define for_each_possible_cpu(cpu) for_each_cpu((cpu), cpu_possible_mask)
 #define for_each_online_cpu(cpu)   for_each_cpu((cpu), cpu_online_mask)
 #define for_each_present_cpu(cpu)  for_each_cpu((cpu), cpu_present_mask)
+#endif
 
 /* Wrappers for arch boot code to manipulate normally-constant masks */
 void init_cpu_present(const struct cpumask *src);
-- 
2.36.1

[PATCH v4 5/5] cpumask: Update cpumask_next_wrap() signature

[Sander Vanheule]

The extern specifier is not needed for this declaration, so drop it. The
function also depends only on the input parameters, and has no side
effects, so it can be marked __pure like other functions in cpumask.h.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

Notes:
    Changes since v3:
    - Add Andy's Reviewed-by
    
    Changes since v1:
    - Split off patch from other changes

 include/linux/cpumask.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 6c5b4ee000f2..523857884ae4 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -229,7 +229,7 @@ int cpumask_any_distribute(const struct cpumask *srcp);
 		(cpu) = cpumask_next_zero((cpu), (mask)),	\
 		(cpu) < nr_cpu_ids;)
 
-extern int cpumask_next_wrap(int n, const struct cpumask *mask, int start, bool wrap);
+int __pure cpumask_next_wrap(int n, const struct cpumask *mask, int start, bool wrap);
 
 /**
  * for_each_cpu_wrap - iterate over every cpu in a mask, starting at a specified location
-- 
2.36.1

Re: [PATCH v4 0/5] cpumask: Fix invalid uniprocessor assumptions

[Andrew Morton]

On Sat,  2 Jul 2022 18:08:23 +0200 Sander Vanheule <sander@svanheule.net> wrote:

> On uniprocessor builds, it is currently assumed that any cpumask will
> contain the single CPU: cpu0. This assumption is used to provide
> optimised implementations.
> 
> The current assumption also appears to be wrong, by ignoring the fact
> that users can provide empty cpumask-s. This can result in bugs as
> explained in [1].

It's a little unkind to send people off to some link to explain the
very core issue which this patchset addresses!  So I enhanced this
paragraph:

: The current assumption also appears to be wrong, by ignoring the fact that
: users can provide empty cpumasks.  This can result in bugs as explained in
: [1] - for_each_cpu() will run one iteration of the loop even when passed
: an empty cpumask.

> This series introduces some basic tests, and updates the optimisations
> for uniprocessor builds.
> 
> The x86 patch was written after the kernel test robot [2] ran into a
> failed build. I have tried to list the files potentially affected by the
> changes to cpumask.h, in an attempt to find any other cases that fail on
> !SMP. I've gone through some of the files manually, and ran a few cross
> builds, but nothing else popped up. I (build) checked about half of the
> potientally affected files, but I do not have the resources to do them
> all. I hope we can fix other issues if/when they pop up later.
> 
> [1] https://lore.kernel.org/all/20220530082552.46113-1-sander@svanheule.net/
> [2] https://lore.kernel.org/all/202206060858.wA0FOzRy-lkp@intel.com/

Re: [PATCH v4 2/5] cpumask: Fix invalid uniprocessor mask assumption

[Yury Norov]

On Sat, Jul 02, 2022 at 06:08:25PM +0200, Sander Vanheule wrote:
> On uniprocessor builds, any CPU mask is assumed to contain exactly one
> CPU (cpu0). This assumption ignores the existence of empty masks,
> resulting in incorrect behaviour.
> cpumask_first_zero(), cpumask_next_zero(), and for_each_cpu_not() don't
> provide behaviour matching the assumption that a UP mask is always "1",
> and instead provide behaviour matching the empty mask.
> 
> Drop the incorrectly optimised code and use the generic implementations
> in all cases.
> 
> Signed-off-by: Sander Vanheule <sander@svanheule.net>

Suggested-by: Yury Norov <yury.norov@gmail.com>

> ---
> 
> Notes:
>     Changes since v3:
>     - Add back UP-optimised cpumask_local_spread, cpumask_any_distribute,
>       cpumask_any_and_distribute
>     
>     Changes since v1:
>     - Drop UP implementations instead of trying to fix them
> 
>  include/linux/cpumask.h | 99 ++++++++---------------------------------
>  lib/Makefile            |  3 +-
>  lib/cpumask.c           |  2 +
>  3 files changed, 22 insertions(+), 82 deletions(-)
> 
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index fe29ac7cc469..7fbef41b3093 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -116,85 +116,6 @@ static __always_inline unsigned int cpumask_check(unsigned int cpu)
>  	return cpu;
>  }
>  
> -#if NR_CPUS == 1
> -/* Uniprocessor.  Assume all masks are "1". */
> -static inline unsigned int cpumask_first(const struct cpumask *srcp)
> -{
> -	return 0;
> -}
> -
> -static inline unsigned int cpumask_first_zero(const struct cpumask *srcp)
> -{
> -	return 0;
> -}
> -
> -static inline unsigned int cpumask_first_and(const struct cpumask *srcp1,
> -					     const struct cpumask *srcp2)
> -{
> -	return 0;
> -}
> -
> -static inline unsigned int cpumask_last(const struct cpumask *srcp)
> -{
> -	return 0;
> -}
> -
> -/* Valid inputs for n are -1 and 0. */
> -static inline unsigned int cpumask_next(int n, const struct cpumask *srcp)
> -{
> -	return n+1;
> -}
> -
> -static inline unsigned int cpumask_next_zero(int n, const struct cpumask *srcp)
> -{
> -	return n+1;
> -}
> -
> -static inline unsigned int cpumask_next_and(int n,
> -					    const struct cpumask *srcp,
> -					    const struct cpumask *andp)
> -{
> -	return n+1;
> -}
> -
> -static inline unsigned int cpumask_next_wrap(int n, const struct cpumask *mask,
> -					     int start, bool wrap)
> -{
> -	/* cpu0 unless stop condition, wrap and at cpu0, then nr_cpumask_bits */
> -	return (wrap && n == 0);
> -}
> -
> -/* cpu must be a valid cpu, ie 0, so there's no other choice. */
> -static inline unsigned int cpumask_any_but(const struct cpumask *mask,
> -					   unsigned int cpu)
> -{
> -	return 1;
> -}
> -
> -static inline unsigned int cpumask_local_spread(unsigned int i, int node)
> -{
> -	return 0;
> -}
> -
> -static inline int cpumask_any_and_distribute(const struct cpumask *src1p,
> -					     const struct cpumask *src2p) {
> -	return cpumask_first_and(src1p, src2p);
> -}
> -
> -static inline int cpumask_any_distribute(const struct cpumask *srcp)
> -{
> -	return cpumask_first(srcp);
> -}
> -
> -#define for_each_cpu(cpu, mask)			\
> -	for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)
> -#define for_each_cpu_not(cpu, mask)		\
> -	for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)
> -#define for_each_cpu_wrap(cpu, mask, start)	\
> -	for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask, (void)(start))
> -#define for_each_cpu_and(cpu, mask1, mask2)	\
> -	for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask1, (void)mask2)
> -#else
>  /**
>   * cpumask_first - get the first cpu in a cpumask
>   * @srcp: the cpumask pointer
> @@ -260,10 +181,29 @@ static inline unsigned int cpumask_next_zero(int n, const struct cpumask *srcp)
>  
>  int __pure cpumask_next_and(int n, const struct cpumask *, const struct cpumask *);
>  int __pure cpumask_any_but(const struct cpumask *mask, unsigned int cpu);
> +
> +#if NR_CPUS == 1
> +/* Uniprocessor: there is only one valid CPU */
> +static inline unsigned int cpumask_local_spread(unsigned int i, int node)
> +{
> +	return 0;
> +}
> +
> +static inline int cpumask_any_and_distribute(const struct cpumask *src1p,
> +					     const struct cpumask *src2p) {
> +	return cpumask_first_and(src1p, src2p);
> +}
> +
> +static inline int cpumask_any_distribute(const struct cpumask *srcp)
> +{
> +	return cpumask_first(srcp);
> +}
> +#else
>  unsigned int cpumask_local_spread(unsigned int i, int node);
>  int cpumask_any_and_distribute(const struct cpumask *src1p,
>  			       const struct cpumask *src2p);
>  int cpumask_any_distribute(const struct cpumask *srcp);
> +#endif /* NR_CPUS */
>  
>  /**
>   * for_each_cpu - iterate over every cpu in a mask
> @@ -324,7 +264,6 @@ extern int cpumask_next_wrap(int n, const struct cpumask *mask, int start, bool
>  	for ((cpu) = -1;						\
>  		(cpu) = cpumask_next_and((cpu), (mask1), (mask2)),	\
>  		(cpu) < nr_cpu_ids;)
> -#endif /* SMP */
>  
>  #define CPU_BITS_NONE						\
>  {								\
> diff --git a/lib/Makefile b/lib/Makefile
> index f99bf61f8bbc..bcc7e8ea0cde 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -34,10 +34,9 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \
>  	 is_single_threaded.o plist.o decompress.o kobject_uevent.o \
>  	 earlycpio.o seq_buf.o siphash.o dec_and_lock.o \
>  	 nmi_backtrace.o nodemask.o win_minmax.o memcat_p.o \
> -	 buildid.o
> +	 buildid.o cpumask.o
>  
>  lib-$(CONFIG_PRINTK) += dump_stack.o
> -lib-$(CONFIG_SMP) += cpumask.o
>  
>  lib-y	+= kobject.o klist.o
>  obj-y	+= lockref.o
> diff --git a/lib/cpumask.c b/lib/cpumask.c
> index a971a82d2f43..b9728513a4d4 100644
> --- a/lib/cpumask.c
> +++ b/lib/cpumask.c
> @@ -192,6 +192,7 @@ void __init free_bootmem_cpumask_var(cpumask_var_t mask)
>  }
>  #endif
>  
> +#if NR_CPUS > 1
>  /**
>   * cpumask_local_spread - select the i'th cpu with local numa cpu's first
>   * @i: index number
> @@ -279,3 +280,4 @@ int cpumask_any_distribute(const struct cpumask *srcp)
>  	return next;
>  }
>  EXPORT_SYMBOL(cpumask_any_distribute);
> +#endif /* NR_CPUS */
> -- 
> 2.36.1

Re: [PATCH v4 3/5] lib/test: Introduce cpumask KUnit test suite

[Yury Norov]

On Sat, Jul 02, 2022 at 06:08:26PM +0200, Sander Vanheule wrote:
> Add a basic suite of tests for cpumask, providing some tests for empty
> and completely filled cpumasks.
> 
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> 
> Notes:
>     Changes since v3:
>     - Test for_each_cpu*() over empty mask and cpu_possible_mask
>     - Add Andy's Reviewed-by
>     - Use num_{online,present,possible}_cpus() for builtin masks
>     - Guard against CPU hotplugging during test for dynamic builtin CPU masks
>     
>     Changes since v2:
>     - Rework for_each_* test macros, as suggested by Yury

We have a special tag for it:

Suggested-by: Yury Norov <yury.norov@gmail.com>

>     
>     Changes since v1:
>     - New patch
> 
>  lib/Kconfig.debug  |   9 +++
>  lib/Makefile       |   1 +
>  lib/test_cpumask.c | 138 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 148 insertions(+)
>  create mode 100644 lib/test_cpumask.c
> 
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 2e24db4bff19..04aaa20d50f9 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2021,6 +2021,15 @@ config LKDTM
>  	Documentation on how to use the module can be found in
>  	Documentation/fault-injection/provoke-crashes.rst
>  
> +config TEST_CPUMASK
> +	tristate "cpumask tests" if !KUNIT_ALL_TESTS
> +	depends on KUNIT
> +	default KUNIT_ALL_TESTS
> +	help
> +	  Enable to turn on cpumask tests, running at boot or module load time.
> +
> +	  If unsure, say N.
> +
>  config TEST_LIST_SORT
>  	tristate "Linked list sorting test" if !KUNIT_ALL_TESTS
>  	depends on KUNIT
> diff --git a/lib/Makefile b/lib/Makefile
> index bcc7e8ea0cde..de3e47453fe8 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -99,6 +99,7 @@ obj-$(CONFIG_TEST_HMM) += test_hmm.o
>  obj-$(CONFIG_TEST_FREE_PAGES) += test_free_pages.o
>  obj-$(CONFIG_KPROBES_SANITY_TEST) += test_kprobes.o
>  obj-$(CONFIG_TEST_REF_TRACKER) += test_ref_tracker.o
> +obj-$(CONFIG_TEST_CPUMASK) += test_cpumask.o
>  CFLAGS_test_fprobe.o += $(CC_FLAGS_FTRACE)
>  obj-$(CONFIG_FPROBE_SANITY_TEST) += test_fprobe.o
>  #
> diff --git a/lib/test_cpumask.c b/lib/test_cpumask.c
> new file mode 100644
> index 000000000000..a31a1622f1f6
> --- /dev/null
> +++ b/lib/test_cpumask.c
> @@ -0,0 +1,138 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * KUnit tests for cpumask.
> + *
> + * Author: Sander Vanheule <sander@svanheule.net>
> + */
> +
> +#include <kunit/test.h>
> +#include <linux/cpu.h>
> +#include <linux/cpumask.h>
> +
> +#define EXPECT_FOR_EACH_CPU_EQ(test, mask)			\
> +	do {							\
> +		const cpumask_t *m = (mask);			\
> +		int mask_weight = cpumask_weight(m);		\
> +		int cpu, iter = 0;				\
> +		for_each_cpu(cpu, m)				\
> +			iter++;					\
> +		KUNIT_EXPECT_EQ((test), mask_weight, iter);	\
> +	} while (0)
> +
> +#define EXPECT_FOR_EACH_CPU_NOT_EQ(test, mask)					\
> +	do {									\
> +		const cpumask_t *m = (mask);					\
> +		int mask_weight = cpumask_weight(m);				\
> +		int cpu, iter = 0;						\
> +		for_each_cpu_not(cpu, m)					\
> +			iter++;							\
> +		KUNIT_EXPECT_EQ((test), nr_cpu_ids - mask_weight, iter);	\
> +	} while (0)
> +
> +#define EXPECT_FOR_EACH_CPU_WRAP_EQ(test, mask)			\
> +	do {							\
> +		const cpumask_t *m = (mask);			\
> +		int mask_weight = cpumask_weight(m);		\
> +		int cpu, iter = 0;				\
> +		for_each_cpu_wrap(cpu, m, nr_cpu_ids / 2)	\
> +			iter++;					\
> +		KUNIT_EXPECT_EQ((test), mask_weight, iter);	\
> +	} while (0)
> +
> +#define EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, name)		\
> +	do {							\
> +		int mask_weight = num_##name##_cpus();		\
> +		int cpu, iter = 0;				\
> +		for_each_##name##_cpu(cpu)			\
> +			iter++;					\
> +		KUNIT_EXPECT_EQ((test), mask_weight, iter);	\
> +	} while (0)
> +
> +static cpumask_t mask_empty;
> +static cpumask_t mask_all;
> +
> +static void test_cpumask_weight(struct kunit *test)
> +{
> +	KUNIT_EXPECT_TRUE(test, cpumask_empty(&mask_empty));
> +	KUNIT_EXPECT_TRUE(test, cpumask_full(cpu_possible_mask));
> +	KUNIT_EXPECT_TRUE(test, cpumask_full(&mask_all));
> +
> +	KUNIT_EXPECT_EQ(test, 0, cpumask_weight(&mask_empty));
> +	KUNIT_EXPECT_EQ(test, nr_cpu_ids, cpumask_weight(cpu_possible_mask));
> +	KUNIT_EXPECT_EQ(test, nr_cpumask_bits, cpumask_weight(&mask_all));
> +}
> +
> +static void test_cpumask_first(struct kunit *test)
> +{
> +	KUNIT_EXPECT_LE(test, nr_cpu_ids, cpumask_first(&mask_empty));
> +	KUNIT_EXPECT_EQ(test, 0, cpumask_first(cpu_possible_mask));
> +
> +	KUNIT_EXPECT_EQ(test, 0, cpumask_first_zero(&mask_empty));
> +	KUNIT_EXPECT_LE(test, nr_cpu_ids, cpumask_first_zero(cpu_possible_mask));
> +}
> +
> +static void test_cpumask_last(struct kunit *test)
> +{
> +	KUNIT_EXPECT_LE(test, nr_cpumask_bits, cpumask_last(&mask_empty));
> +	KUNIT_EXPECT_EQ(test, nr_cpumask_bits - 1, cpumask_last(cpu_possible_mask));
> +}
> +
> +static void test_cpumask_next(struct kunit *test)
> +{
> +	KUNIT_EXPECT_EQ(test, 0, cpumask_next_zero(-1, &mask_empty));
> +	KUNIT_EXPECT_LE(test, nr_cpu_ids, cpumask_next_zero(-1, cpu_possible_mask));
> +
> +	KUNIT_EXPECT_LE(test, nr_cpu_ids, cpumask_next(-1, &mask_empty));
> +	KUNIT_EXPECT_EQ(test, 0, cpumask_next(-1, cpu_possible_mask));
> +}
> +
> +static void test_cpumask_iterators(struct kunit *test)
> +{
> +	EXPECT_FOR_EACH_CPU_EQ(test, &mask_empty);
> +	EXPECT_FOR_EACH_CPU_NOT_EQ(test, &mask_empty);
> +	EXPECT_FOR_EACH_CPU_WRAP_EQ(test, &mask_empty);
> +
> +	EXPECT_FOR_EACH_CPU_EQ(test, cpu_possible_mask);
> +	EXPECT_FOR_EACH_CPU_NOT_EQ(test, cpu_possible_mask);
> +	EXPECT_FOR_EACH_CPU_WRAP_EQ(test, cpu_possible_mask);
> +}
> +
> +static void test_cpumask_iterators_builtin(struct kunit *test)
> +{
> +	EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, possible);
> +
> +	/* Ensure the dynamic masks are stable while running the tests */
> +	cpu_hotplug_disable();
> +
> +	EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, online);
> +	EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, present);
> +
> +	cpu_hotplug_enable();
> +}
> +
> +static int test_cpumask_init(struct kunit *test)
> +{
> +	cpumask_clear(&mask_empty);
> +	cpumask_setall(&mask_all);
> +
> +	return 0;
> +}
> +
> +static struct kunit_case test_cpumask_cases[] = {
> +	KUNIT_CASE(test_cpumask_weight),
> +	KUNIT_CASE(test_cpumask_first),
> +	KUNIT_CASE(test_cpumask_last),
> +	KUNIT_CASE(test_cpumask_next),
> +	KUNIT_CASE(test_cpumask_iterators),
> +	KUNIT_CASE(test_cpumask_iterators_builtin),
> +	{}
> +};
> +
> +static struct kunit_suite test_cpumask_suite = {
> +	.name = "cpumask",
> +	.init = test_cpumask_init,
> +	.test_cases = test_cpumask_cases,
> +};
> +kunit_test_suite(test_cpumask_suite);
> +
> +MODULE_LICENSE("GPL");
> -- 
> 2.36.1

Re: [PATCH v4 4/5] cpumask: Add UP optimised for_each_*_cpu versions

[Yury Norov]

On Sat, Jul 02, 2022 at 06:08:27PM +0200, Sander Vanheule wrote:
> On uniprocessor builds, the following loops will always run over a mask
> that contains one enabled CPU (cpu0):
>     - for_each_possible_cpu
>     - for_each_online_cpu
>     - for_each_present_cpu
> 
> Provide uniprocessor-specific macros for these loops, that always run
> exactly once.
> 
> Signed-off-by: Sander Vanheule <sander@svanheule.net>

Acked-by: Yury Norov <yury.norov@gmail.com>

> ---
>  include/linux/cpumask.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index 7fbef41b3093..6c5b4ee000f2 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -750,9 +750,16 @@ extern const DECLARE_BITMAP(cpu_all_bits, NR_CPUS);
>  /* First bits of cpu_bit_bitmap are in fact unset. */
>  #define cpu_none_mask to_cpumask(cpu_bit_bitmap[0])
>  
> +#if NR_CPUS == 1
> +/* Uniprocessor: the possible/online/present masks are always "1" */
> +#define for_each_possible_cpu(cpu)	for ((cpu) = 0; (cpu) < 1; (cpu)++)
> +#define for_each_online_cpu(cpu)	for ((cpu) = 0; (cpu) < 1; (cpu)++)
> +#define for_each_present_cpu(cpu)	for ((cpu) = 0; (cpu) < 1; (cpu)++)
> +#else
>  #define for_each_possible_cpu(cpu) for_each_cpu((cpu), cpu_possible_mask)
>  #define for_each_online_cpu(cpu)   for_each_cpu((cpu), cpu_online_mask)
>  #define for_each_present_cpu(cpu)  for_each_cpu((cpu), cpu_present_mask)
> +#endif
>  
>  /* Wrappers for arch boot code to manipulate normally-constant masks */
>  void init_cpu_present(const struct cpumask *src);
> -- 
> 2.36.1

Re: [PATCH v4 4/5] cpumask: Add UP optimised for_each_*_cpu versions

[Yury Norov]

On Sat, Jul 02, 2022 at 02:47:55PM -0700, Yury Norov wrote:
> On Sat, Jul 02, 2022 at 06:08:27PM +0200, Sander Vanheule wrote:
> > On uniprocessor builds, the following loops will always run over a mask
> > that contains one enabled CPU (cpu0):
> >     - for_each_possible_cpu
> >     - for_each_online_cpu
> >     - for_each_present_cpu
> > 
> > Provide uniprocessor-specific macros for these loops, that always run
> > exactly once.
> > 
> > Signed-off-by: Sander Vanheule <sander@svanheule.net>
> 
> Acked-by: Yury Norov <yury.norov@gmail.com>

I think this patch should go before #2 to avoid possible issues while
bisecting...

> 
> > ---
> >  include/linux/cpumask.h | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> > index 7fbef41b3093..6c5b4ee000f2 100644
> > --- a/include/linux/cpumask.h
> > +++ b/include/linux/cpumask.h
> > @@ -750,9 +750,16 @@ extern const DECLARE_BITMAP(cpu_all_bits, NR_CPUS);
> >  /* First bits of cpu_bit_bitmap are in fact unset. */
> >  #define cpu_none_mask to_cpumask(cpu_bit_bitmap[0])
> >  
> > +#if NR_CPUS == 1
> > +/* Uniprocessor: the possible/online/present masks are always "1" */
> > +#define for_each_possible_cpu(cpu)	for ((cpu) = 0; (cpu) < 1; (cpu)++)
> > +#define for_each_online_cpu(cpu)	for ((cpu) = 0; (cpu) < 1; (cpu)++)
> > +#define for_each_present_cpu(cpu)	for ((cpu) = 0; (cpu) < 1; (cpu)++)
> > +#else
> >  #define for_each_possible_cpu(cpu) for_each_cpu((cpu), cpu_possible_mask)
> >  #define for_each_online_cpu(cpu)   for_each_cpu((cpu), cpu_online_mask)
> >  #define for_each_present_cpu(cpu)  for_each_cpu((cpu), cpu_present_mask)
> > +#endif
> >  
> >  /* Wrappers for arch boot code to manipulate normally-constant masks */
> >  void init_cpu_present(const struct cpumask *src);
> > -- 
> > 2.36.1

Re: [PATCH v4 3/5] lib/test: Introduce cpumask KUnit test suite

[Sander Vanheule]

On Sat, 2022-07-02 at 14:45 -0700, Yury Norov wrote:
> > On Sat, Jul 02, 2022 at 06:08:26PM +0200, Sander Vanheule wrote:
> > > > Add a basic suite of tests for cpumask, providing some tests for empty
> > > > and completely filled cpumasks.
> > > > 
> > > > Signed-off-by: Sander Vanheule <sander@svanheule.net>
> > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > ---
> > > > 
> > > > Notes:
> > > >     Changes since v3:
> > > >     - Test for_each_cpu*() over empty mask and cpu_possible_mask
> > > >     - Add Andy's Reviewed-by
> > > >     - Use num_{online,present,possible}_cpus() for builtin masks
> > > >     - Guard against CPU hotplugging during test for dynamic builtin CPU masks
> > > >     
> > > >     Changes since v2:
> > > >     - Rework for_each_* test macros, as suggested by Yury
> > 
> > We have a special tag for it:
> > 
> > Suggested-by: Yury Norov <yury.norov@gmail.com>

Of course, sorry I forgot about this. I should've included this already when introducing this patch
anyway.

Best,
Sander

Re: [PATCH v4 4/5] cpumask: Add UP optimised for_each_*_cpu versions

[Sander Vanheule]

On Sat, 2022-07-02 at 14:50 -0700, Yury Norov wrote:
> On Sat, Jul 02, 2022 at 02:47:55PM -0700, Yury Norov wrote:
> > On Sat, Jul 02, 2022 at 06:08:27PM +0200, Sander Vanheule wrote:
> > > On uniprocessor builds, the following loops will always run over a mask
> > > that contains one enabled CPU (cpu0):
> > >     - for_each_possible_cpu
> > >     - for_each_online_cpu
> > >     - for_each_present_cpu
> > > 
> > > Provide uniprocessor-specific macros for these loops, that always run
> > > exactly once.
> > > 
> > > Signed-off-by: Sander Vanheule <sander@svanheule.net>
> > 
> > Acked-by: Yury Norov <yury.norov@gmail.com>
> 
> I think this patch should go before #2 to avoid possible issues while
> bisecting...

The old assumption was valid in this case, but I agree it makes sense to move this patch forward in
the series. Also to avoid a cycle of optimised/generic/optimised, even if the behaviour doesn't
change.

Best,
Sander

> > 
> > > ---
> > >  include/linux/cpumask.h | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> > > index 7fbef41b3093..6c5b4ee000f2 100644
> > > --- a/include/linux/cpumask.h
> > > +++ b/include/linux/cpumask.h
> > > @@ -750,9 +750,16 @@ extern const DECLARE_BITMAP(cpu_all_bits, NR_CPUS);
> > >  /* First bits of cpu_bit_bitmap are in fact unset. */
> > >  #define cpu_none_mask to_cpumask(cpu_bit_bitmap[0])
> > >  
> > > +#if NR_CPUS == 1
> > > +/* Uniprocessor: the possible/online/present masks are always "1" */
> > > +#define for_each_possible_cpu(cpu)     for ((cpu) = 0; (cpu) < 1; (cpu)++)
> > > +#define for_each_online_cpu(cpu)       for ((cpu) = 0; (cpu) < 1; (cpu)++)
> > > +#define for_each_present_cpu(cpu)      for ((cpu) = 0; (cpu) < 1; (cpu)++)
> > > +#else
> > >  #define for_each_possible_cpu(cpu) for_each_cpu((cpu), cpu_possible_mask)
> > >  #define for_each_online_cpu(cpu)   for_each_cpu((cpu), cpu_online_mask)
> > >  #define for_each_present_cpu(cpu)  for_each_cpu((cpu), cpu_present_mask)
> > > +#endif
> > >  
> > >  /* Wrappers for arch boot code to manipulate normally-constant masks */
> > >  void init_cpu_present(const struct cpumask *src);
> > > -- 
> > > 2.36.1

Re: [PATCH v4 0/5] cpumask: Fix invalid uniprocessor assumptions

[Sander Vanheule]

On Sat, 2022-07-02 at 13:38 -0700, Andrew Morton wrote:
> On Sat,  2 Jul 2022 18:08:23 +0200 Sander Vanheule <sander@svanheule.net> wrote:
> 
> > On uniprocessor builds, it is currently assumed that any cpumask will
> > contain the single CPU: cpu0. This assumption is used to provide
> > optimised implementations.
> > 
> > The current assumption also appears to be wrong, by ignoring the fact
> > that users can provide empty cpumask-s. This can result in bugs as
> > explained in [1].
> 
> It's a little unkind to send people off to some link to explain the
> very core issue which this patchset addresses!  So I enhanced this
> paragraph:
> 
> : The current assumption also appears to be wrong, by ignoring the fact that
> : users can provide empty cpumasks.  This can result in bugs as explained in
> : [1] - for_each_cpu() will run one iteration of the loop even when passed
> : an empty cpumask.

Makes sense to add this, sorry for the inconvenience.

Just to make sure, since I'm not familiar with the process for patches going through the mm tree,
can I still send a v5 to move the last patch forward in the series, and to include Yury's tags?

Best,
Sander

> > This series introduces some basic tests, and updates the optimisations
> > for uniprocessor builds.
> > 
> > The x86 patch was written after the kernel test robot [2] ran into a
> > failed build. I have tried to list the files potentially affected by the
> > changes to cpumask.h, in an attempt to find any other cases that fail on
> > !SMP. I've gone through some of the files manually, and ran a few cross
> > builds, but nothing else popped up. I (build) checked about half of the
> > potientally affected files, but I do not have the resources to do them
> > all. I hope we can fix other issues if/when they pop up later.
> > 
> > [1] https://lore.kernel.org/all/20220530082552.46113-1-sander@svanheule.net/
> > [2] https://lore.kernel.org/all/202206060858.wA0FOzRy-lkp@intel.com/
> 

Re: [PATCH v4 2/5] cpumask: Fix invalid uniprocessor mask assumption

[Sander Vanheule]

On Sat, 2022-07-02 at 14:42 -0700, Yury Norov wrote:
> On Sat, Jul 02, 2022 at 06:08:25PM +0200, Sander Vanheule wrote:
> > On uniprocessor builds, any CPU mask is assumed to contain exactly one
> > CPU (cpu0). This assumption ignores the existence of empty masks,
> > resulting in incorrect behaviour.
> > cpumask_first_zero(), cpumask_next_zero(), and for_each_cpu_not() don't
> > provide behaviour matching the assumption that a UP mask is always "1",
> > and instead provide behaviour matching the empty mask.
> > 
> > Drop the incorrectly optimised code and use the generic implementations
> > in all cases.
> > 
> > Signed-off-by: Sander Vanheule <sander@svanheule.net>
> 
> Suggested-by: Yury Norov <yury.norov@gmail.com>
> 

To be honest, I was somewhat taken aback by this Suggested-by. I certainly appreciate the feedback
you've provided, and I think the patch has gotten better for it, so I do want to acknowledge that.
During reviews of other patches however, I haven't had a reviewer request this for suggestions that
couldn't be spun off into a separate patch. If you are OK with this patch, a Reviewed-by would
definitely be warranted and seems more appropriate IMHO.


Best,
Sander

> > ---
> > 
> > Notes:
> >     Changes since v3:
> >     - Add back UP-optimised cpumask_local_spread, cpumask_any_distribute,
> >       cpumask_any_and_distribute
> >     
> >     Changes since v1:
> >     - Drop UP implementations instead of trying to fix them
> > 
> >  include/linux/cpumask.h | 99 ++++++++---------------------------------
> >  lib/Makefile            |  3 +-
> >  lib/cpumask.c           |  2 +
> >  3 files changed, 22 insertions(+), 82 deletions(-)
> > 
> > diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> > index fe29ac7cc469..7fbef41b3093 100644
> > --- a/include/linux/cpumask.h
> > +++ b/include/linux/cpumask.h
> > @@ -116,85 +116,6 @@ static __always_inline unsigned int cpumask_check(unsigned int cpu)
> >         return cpu;
> >  }
> >  
> > -#if NR_CPUS == 1
> > -/* Uniprocessor.  Assume all masks are "1". */
> > -static inline unsigned int cpumask_first(const struct cpumask *srcp)
> > -{
> > -       return 0;
> > -}
> > -
> > -static inline unsigned int cpumask_first_zero(const struct cpumask *srcp)
> > -{
> > -       return 0;
> > -}
> > -
> > -static inline unsigned int cpumask_first_and(const struct cpumask *srcp1,
> > -                                            const struct cpumask *srcp2)
> > -{
> > -       return 0;
> > -}
> > -
> > -static inline unsigned int cpumask_last(const struct cpumask *srcp)
> > -{
> > -       return 0;
> > -}
> > -
> > -/* Valid inputs for n are -1 and 0. */
> > -static inline unsigned int cpumask_next(int n, const struct cpumask *srcp)
> > -{
> > -       return n+1;
> > -}
> > -
> > -static inline unsigned int cpumask_next_zero(int n, const struct cpumask *srcp)
> > -{
> > -       return n+1;
> > -}
> > -
> > -static inline unsigned int cpumask_next_and(int n,
> > -                                           const struct cpumask *srcp,
> > -                                           const struct cpumask *andp)
> > -{
> > -       return n+1;
> > -}
> > -
> > -static inline unsigned int cpumask_next_wrap(int n, const struct cpumask *mask,
> > -                                            int start, bool wrap)
> > -{
> > -       /* cpu0 unless stop condition, wrap and at cpu0, then nr_cpumask_bits */
> > -       return (wrap && n == 0);
> > -}
> > -
> > -/* cpu must be a valid cpu, ie 0, so there's no other choice. */
> > -static inline unsigned int cpumask_any_but(const struct cpumask *mask,
> > -                                          unsigned int cpu)
> > -{
> > -       return 1;
> > -}
> > -
> > -static inline unsigned int cpumask_local_spread(unsigned int i, int node)
> > -{
> > -       return 0;
> > -}
> > -
> > -static inline int cpumask_any_and_distribute(const struct cpumask *src1p,
> > -                                            const struct cpumask *src2p) {
> > -       return cpumask_first_and(src1p, src2p);
> > -}
> > -
> > -static inline int cpumask_any_distribute(const struct cpumask *srcp)
> > -{
> > -       return cpumask_first(srcp);
> > -}
> > -
> > -#define for_each_cpu(cpu, mask)                        \
> > -       for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)
> > -#define for_each_cpu_not(cpu, mask)            \
> > -       for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)
> > -#define for_each_cpu_wrap(cpu, mask, start)    \
> > -       for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask, (void)(start))
> > -#define for_each_cpu_and(cpu, mask1, mask2)    \
> > -       for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask1, (void)mask2)
> > -#else
> >  /**
> >   * cpumask_first - get the first cpu in a cpumask
> >   * @srcp: the cpumask pointer
> > @@ -260,10 +181,29 @@ static inline unsigned int cpumask_next_zero(int n, const struct cpumask
> > *srcp)
> >  
> >  int __pure cpumask_next_and(int n, const struct cpumask *, const struct cpumask *);
> >  int __pure cpumask_any_but(const struct cpumask *mask, unsigned int cpu);
> > +
> > +#if NR_CPUS == 1
> > +/* Uniprocessor: there is only one valid CPU */
> > +static inline unsigned int cpumask_local_spread(unsigned int i, int node)
> > +{
> > +       return 0;
> > +}
> > +
> > +static inline int cpumask_any_and_distribute(const struct cpumask *src1p,
> > +                                            const struct cpumask *src2p) {
> > +       return cpumask_first_and(src1p, src2p);
> > +}
> > +
> > +static inline int cpumask_any_distribute(const struct cpumask *srcp)
> > +{
> > +       return cpumask_first(srcp);
> > +}
> > +#else
> >  unsigned int cpumask_local_spread(unsigned int i, int node);
> >  int cpumask_any_and_distribute(const struct cpumask *src1p,
> >                                const struct cpumask *src2p);
> >  int cpumask_any_distribute(const struct cpumask *srcp);
> > +#endif /* NR_CPUS */
> >  
> >  /**
> >   * for_each_cpu - iterate over every cpu in a mask
> > @@ -324,7 +264,6 @@ extern int cpumask_next_wrap(int n, const struct cpumask *mask, int start,
> > bool
> >         for ((cpu) = -1;                                                \
> >                 (cpu) = cpumask_next_and((cpu), (mask1), (mask2)),      \
> >                 (cpu) < nr_cpu_ids;)
> > -#endif /* SMP */
> >  
> >  #define CPU_BITS_NONE                                          \
> >  {                                                              \
> > diff --git a/lib/Makefile b/lib/Makefile
> > index f99bf61f8bbc..bcc7e8ea0cde 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -34,10 +34,9 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \
> >          is_single_threaded.o plist.o decompress.o kobject_uevent.o \
> >          earlycpio.o seq_buf.o siphash.o dec_and_lock.o \
> >          nmi_backtrace.o nodemask.o win_minmax.o memcat_p.o \
> > -        buildid.o
> > +        buildid.o cpumask.o
> >  
> >  lib-$(CONFIG_PRINTK) += dump_stack.o
> > -lib-$(CONFIG_SMP) += cpumask.o
> >  
> >  lib-y  += kobject.o klist.o
> >  obj-y  += lockref.o
> > diff --git a/lib/cpumask.c b/lib/cpumask.c
> > index a971a82d2f43..b9728513a4d4 100644
> > --- a/lib/cpumask.c
> > +++ b/lib/cpumask.c
> > @@ -192,6 +192,7 @@ void __init free_bootmem_cpumask_var(cpumask_var_t mask)
> >  }
> >  #endif
> >  
> > +#if NR_CPUS > 1
> >  /**
> >   * cpumask_local_spread - select the i'th cpu with local numa cpu's first
> >   * @i: index number
> > @@ -279,3 +280,4 @@ int cpumask_any_distribute(const struct cpumask *srcp)
> >         return next;
> >  }
> >  EXPORT_SYMBOL(cpumask_any_distribute);
> > +#endif /* NR_CPUS */
> > -- 
> > 2.36.1

Re: [PATCH v4 0/5] cpumask: Fix invalid uniprocessor assumptions

[Andrew Morton]

On Sun, 03 Jul 2022 09:50:51 +0200 Sander Vanheule <sander@svanheule.net> wrote:

> On Sat, 2022-07-02 at 13:38 -0700, Andrew Morton wrote:
> > On Sat,  2 Jul 2022 18:08:23 +0200 Sander Vanheule <sander@svanheule.net> wrote:
> > 
> > > On uniprocessor builds, it is currently assumed that any cpumask will
> > > contain the single CPU: cpu0. This assumption is used to provide
> > > optimised implementations.
> > > 
> > > The current assumption also appears to be wrong, by ignoring the fact
> > > that users can provide empty cpumask-s. This can result in bugs as
> > > explained in [1].
> > 
> > It's a little unkind to send people off to some link to explain the
> > very core issue which this patchset addresses!  So I enhanced this
> > paragraph:
> > 
> > : The current assumption also appears to be wrong, by ignoring the fact that
> > : users can provide empty cpumasks.  This can result in bugs as explained in
> > : [1] - for_each_cpu() will run one iteration of the loop even when passed
> > : an empty cpumask.
> 
> Makes sense to add this, sorry for the inconvenience.
> 
> Just to make sure, since I'm not familiar with the process for patches going through the mm tree,

Patches enter -mm in quilt form and are published in the (rebasing)
mm-unstable branch
git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.  Once they have
stopped changing and have been stabilized, I move them into the
non-rebasing mm-stable branch.

> can I still send a v5 to move the last patch forward in the series, and to include Yury's tags?

I already added Yury's ack.  Please tell me the specific patch ordering
and I'll take care of that.

Re: [PATCH v4 0/5] cpumask: Fix invalid uniprocessor assumptions

[Sander Vanheule]

Hi Andrew,

On Sun, 2022-07-03 at 13:39 -0700, Andrew Morton wrote:
> On Sun, 03 Jul 2022 09:50:51 +0200 Sander Vanheule <sander@svanheule.net>
> wrote:
> 
> > On Sat, 2022-07-02 at 13:38 -0700, Andrew Morton wrote:
> > > On Sat,  2 Jul 2022 18:08:23 +0200 Sander Vanheule <sander@svanheule.net>
> > > wrote:
> > > 
> > > > On uniprocessor builds, it is currently assumed that any cpumask will
> > > > contain the single CPU: cpu0. This assumption is used to provide
> > > > optimised implementations.
> > > > 
> > > > The current assumption also appears to be wrong, by ignoring the fact
> > > > that users can provide empty cpumask-s. This can result in bugs as
> > > > explained in [1].
> > > 
> > > It's a little unkind to send people off to some link to explain the
> > > very core issue which this patchset addresses!  So I enhanced this
> > > paragraph:
> > > 
> > > : The current assumption also appears to be wrong, by ignoring the fact
> > > that
> > > : users can provide empty cpumasks.  This can result in bugs as explained
> > > in
> > > : [1] - for_each_cpu() will run one iteration of the loop even when passed
> > > : an empty cpumask.
> > 
> > Makes sense to add this, sorry for the inconvenience.
> > 
> > Just to make sure, since I'm not familiar with the process for patches going
> > through the mm tree,
> 
> Patches enter -mm in quilt form and are published in the (rebasing)
> mm-unstable branch
> git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.  Once they have
> stopped changing and have been stabilized, I move them into the
> non-rebasing mm-stable branch.
> 
> > can I still send a v5 to move the last patch forward in the series, and to
> > include Yury's tags?
> 
> I already added Yury's ack.  Please tell me the specific patch ordering
> and I'll take care of that.
> 

The updated patch order should be:
   x86/cacheinfo: move shared cache map definitions
   cpumask: add UP optimised for_each_*_cpu versions
   cpumask: fix invalid uniprocessor mask assumption
   lib/test: introduce cpumask KUnit test suite
   cpumask: update cpumask_next_wrap() signature

Reordering the patches on my tree didn't produce any conflicts.

Best,
Sander

Re: [PATCH v4 0/5] cpumask: Fix invalid uniprocessor assumptions

[Andrew Morton]

On Sun, 10 Jul 2022 08:51:08 +0200 Sander Vanheule <sander@svanheule.net> wrote:

> > > can I still send a v5 to move the last patch forward in the series, and to
> > > include Yury's tags?
> > 
> > I already added Yury's ack.  Please tell me the specific patch ordering
> > and I'll take care of that.
> > 
> 
> The updated patch order should be:
>    x86/cacheinfo: move shared cache map definitions
>    cpumask: add UP optimised for_each_*_cpu versions
>    cpumask: fix invalid uniprocessor mask assumption
>    lib/test: introduce cpumask KUnit test suite
>    cpumask: update cpumask_next_wrap() signature
> 
> Reordering the patches on my tree didn't produce any conflicts.

Got it, thanks.

Re: [PATCH v4 3/5] lib/test: Introduce cpumask KUnit test suite

[Maíra Canal]

> Add a basic suite of tests for cpumask, providing some tests for empty
> and completely filled cpumasks.
> 
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

The tests test_cpumask_weight and test_cpumask_last are failing on all
architectures, as can be seen on [1]. Also this test doesn't follow the
standard style for KUnit tests [2].

[1]
https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20220718/testrun/10865066/suite/kunit/tests/
[2] https://docs.kernel.org/dev-tools/kunit/style.html

CC: Brendan Higgins <brendanhiggins@google.com>
CC: David Gow <davidgow@google.com>

Best Regards,
- Maíra Canal

> ---
> 
> Notes:
>     Changes since v3:
>     - Test for_each_cpu*() over empty mask and cpu_possible_mask
>     - Add Andy's Reviewed-by
>     - Use num_{online,present,possible}_cpus() for builtin masks
>     - Guard against CPU hotplugging during test for dynamic builtin CPU masks
>     
>     Changes since v2:
>     - Rework for_each_* test macros, as suggested by Yury
>     
>     Changes since v1:
>     - New patch
> 
>  lib/Kconfig.debug  |   9 +++
>  lib/Makefile       |   1 +
>  lib/test_cpumask.c | 138 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 148 insertions(+)
>  create mode 100644 lib/test_cpumask.c
> 
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 2e24db4bff19..04aaa20d50f9 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2021,6 +2021,15 @@ config LKDTM
>  	Documentation on how to use the module can be found in
>  	Documentation/fault-injection/provoke-crashes.rst
>  
> +config TEST_CPUMASK
> +	tristate "cpumask tests" if !KUNIT_ALL_TESTS
> +	depends on KUNIT
> +	default KUNIT_ALL_TESTS
> +	help
> +	  Enable to turn on cpumask tests, running at boot or module load time.
> +
> +	  If unsure, say N.
> +
>  config TEST_LIST_SORT
>  	tristate "Linked list sorting test" if !KUNIT_ALL_TESTS
>  	depends on KUNIT
> diff --git a/lib/Makefile b/lib/Makefile
> index bcc7e8ea0cde..de3e47453fe8 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -99,6 +99,7 @@ obj-$(CONFIG_TEST_HMM) += test_hmm.o
>  obj-$(CONFIG_TEST_FREE_PAGES) += test_free_pages.o
>  obj-$(CONFIG_KPROBES_SANITY_TEST) += test_kprobes.o
>  obj-$(CONFIG_TEST_REF_TRACKER) += test_ref_tracker.o
> +obj-$(CONFIG_TEST_CPUMASK) += test_cpumask.o
>  CFLAGS_test_fprobe.o += $(CC_FLAGS_FTRACE)
>  obj-$(CONFIG_FPROBE_SANITY_TEST) += test_fprobe.o
>  #
> diff --git a/lib/test_cpumask.c b/lib/test_cpumask.c
> new file mode 100644
> index 000000000000..a31a1622f1f6
> --- /dev/null
> +++ b/lib/test_cpumask.c
> @@ -0,0 +1,138 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * KUnit tests for cpumask.
> + *
> + * Author: Sander Vanheule <sander@svanheule.net>
> + */
> +
> +#include <kunit/test.h>
> +#include <linux/cpu.h>
> +#include <linux/cpumask.h>
> +
> +#define EXPECT_FOR_EACH_CPU_EQ(test, mask)			\
> +	do {							\
> +		const cpumask_t *m = (mask);			\
> +		int mask_weight = cpumask_weight(m);		\
> +		int cpu, iter = 0;				\
> +		for_each_cpu(cpu, m)				\
> +			iter++;					\
> +		KUNIT_EXPECT_EQ((test), mask_weight, iter);	\
> +	} while (0)
> +
> +#define EXPECT_FOR_EACH_CPU_NOT_EQ(test, mask)					\
> +	do {									\
> +		const cpumask_t *m = (mask);					\
> +		int mask_weight = cpumask_weight(m);				\
> +		int cpu, iter = 0;						\
> +		for_each_cpu_not(cpu, m)					\
> +			iter++;							\
> +		KUNIT_EXPECT_EQ((test), nr_cpu_ids - mask_weight, iter);	\
> +	} while (0)
> +
> +#define EXPECT_FOR_EACH_CPU_WRAP_EQ(test, mask)			\
> +	do {							\
> +		const cpumask_t *m = (mask);			\
> +		int mask_weight = cpumask_weight(m);		\
> +		int cpu, iter = 0;				\
> +		for_each_cpu_wrap(cpu, m, nr_cpu_ids / 2)	\
> +			iter++;					\
> +		KUNIT_EXPECT_EQ((test), mask_weight, iter);	\
> +	} while (0)
> +
> +#define EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, name)		\
> +	do {							\
> +		int mask_weight = num_##name##_cpus();		\
> +		int cpu, iter = 0;				\
> +		for_each_##name##_cpu(cpu)			\
> +			iter++;					\
> +		KUNIT_EXPECT_EQ((test), mask_weight, iter);	\
> +	} while (0)
> +
> +static cpumask_t mask_empty;
> +static cpumask_t mask_all;
> +
> +static void test_cpumask_weight(struct kunit *test)
> +{
> +	KUNIT_EXPECT_TRUE(test, cpumask_empty(&mask_empty));
> +	KUNIT_EXPECT_TRUE(test, cpumask_full(cpu_possible_mask));
> +	KUNIT_EXPECT_TRUE(test, cpumask_full(&mask_all));
> +
> +	KUNIT_EXPECT_EQ(test, 0, cpumask_weight(&mask_empty));
> +	KUNIT_EXPECT_EQ(test, nr_cpu_ids, cpumask_weight(cpu_possible_mask));
> +	KUNIT_EXPECT_EQ(test, nr_cpumask_bits, cpumask_weight(&mask_all));
> +}
> +
> +static void test_cpumask_first(struct kunit *test)
> +{
> +	KUNIT_EXPECT_LE(test, nr_cpu_ids, cpumask_first(&mask_empty));
> +	KUNIT_EXPECT_EQ(test, 0, cpumask_first(cpu_possible_mask));
> +
> +	KUNIT_EXPECT_EQ(test, 0, cpumask_first_zero(&mask_empty));
> +	KUNIT_EXPECT_LE(test, nr_cpu_ids, cpumask_first_zero(cpu_possible_mask));
> +}
> +
> +static void test_cpumask_last(struct kunit *test)
> +{
> +	KUNIT_EXPECT_LE(test, nr_cpumask_bits, cpumask_last(&mask_empty));
> +	KUNIT_EXPECT_EQ(test, nr_cpumask_bits - 1, cpumask_last(cpu_possible_mask));
> +}
> +
> +static void test_cpumask_next(struct kunit *test)
> +{
> +	KUNIT_EXPECT_EQ(test, 0, cpumask_next_zero(-1, &mask_empty));
> +	KUNIT_EXPECT_LE(test, nr_cpu_ids, cpumask_next_zero(-1, cpu_possible_mask));
> +
> +	KUNIT_EXPECT_LE(test, nr_cpu_ids, cpumask_next(-1, &mask_empty));
> +	KUNIT_EXPECT_EQ(test, 0, cpumask_next(-1, cpu_possible_mask));
> +}
> +
> +static void test_cpumask_iterators(struct kunit *test)
> +{
> +	EXPECT_FOR_EACH_CPU_EQ(test, &mask_empty);
> +	EXPECT_FOR_EACH_CPU_NOT_EQ(test, &mask_empty);
> +	EXPECT_FOR_EACH_CPU_WRAP_EQ(test, &mask_empty);
> +
> +	EXPECT_FOR_EACH_CPU_EQ(test, cpu_possible_mask);
> +	EXPECT_FOR_EACH_CPU_NOT_EQ(test, cpu_possible_mask);
> +	EXPECT_FOR_EACH_CPU_WRAP_EQ(test, cpu_possible_mask);
> +}
> +
> +static void test_cpumask_iterators_builtin(struct kunit *test)
> +{
> +	EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, possible);
> +
> +	/* Ensure the dynamic masks are stable while running the tests */
> +	cpu_hotplug_disable();
> +
> +	EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, online);
> +	EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, present);
> +
> +	cpu_hotplug_enable();
> +}
> +
> +static int test_cpumask_init(struct kunit *test)
> +{
> +	cpumask_clear(&mask_empty);
> +	cpumask_setall(&mask_all);
> +
> +	return 0;
> +}
> +
> +static struct kunit_case test_cpumask_cases[] = {
> +	KUNIT_CASE(test_cpumask_weight),
> +	KUNIT_CASE(test_cpumask_first),
> +	KUNIT_CASE(test_cpumask_last),
> +	KUNIT_CASE(test_cpumask_next),
> +	KUNIT_CASE(test_cpumask_iterators),
> +	KUNIT_CASE(test_cpumask_iterators_builtin),
> +	{}
> +};
> +
> +static struct kunit_suite test_cpumask_suite = {
> +	.name = "cpumask",
> +	.init = test_cpumask_init,
> +	.test_cases = test_cpumask_cases,
> +};
> +kunit_test_suite(test_cpumask_suite);
> +
> +MODULE_LICENSE("GPL");
> -- 
> 2.36.1

Re: [PATCH v4 3/5] lib/test: Introduce cpumask KUnit test suite

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 10173 bytes --]

On Wed, Jul 20, 2022 at 5:31 AM Maíra Canal <mairacanal@riseup.net> wrote:
>
> > Add a basic suite of tests for cpumask, providing some tests for empty
> > and completely filled cpumasks.
> >
> > Signed-off-by: Sander Vanheule <sander@svanheule.net>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> The tests test_cpumask_weight and test_cpumask_last are failing on all
> architectures, as can be seen on [1]. Also this test doesn't follow the
> standard style for KUnit tests [2].
>
> [1]
> https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20220718/testrun/10865066/suite/kunit/tests/
> [2] https://docs.kernel.org/dev-tools/kunit/style.html
>
> CC: Brendan Higgins <brendanhiggins@google.com>
> CC: David Gow <davidgow@google.com>
>
> Best Regards,
> - Maíra Canal
>

Hmm... this test passes on the default kunit_tool configs for UML and
x86_64, which are all without SMP.

It looks like the flaw is that, if CONFIG_NR_CPUS is greater than the
actual number of CPUs present, then the cpu_possible_mask (correctly)
won't be full.

I'm not sure what the right fix is: but removing the checks for
cpu_possible_mask being full is probably the way to go. Unless we want
to plumb through some actual detail about the underlying system and
check against that, it doesn't make sense. (Or, we could generate an
artificial "possilbe_mask" which is always full, and test the cpu
against that. But we sort-of already do that with mask_all anyway.)

So, my recommendation for a fix would be:
- Get rid of "KUNIT_EXPECT_TRUE(test, cpumask_full(cpu_possible_mask));"
- Replace "KUNIT_EXPECT_EQ(test, nr_cpumask_bits - 1,
cpumask_last(cpu_possible_mask));" with a KUNIT_EXPECT_GE()
- _Maybe_ add some debug logging with the cpumask value being checked,
as it's a pain to tell from the expectation failure messages. e.g.,
kunit_info(test, "cpu_possible_mask = '%*pb[l]'\n",
cpumask_pr_args(cpu_possible_mask));


Cheers,
-- David

> > ---
> >
> > Notes:
> >     Changes since v3:
> >     - Test for_each_cpu*() over empty mask and cpu_possible_mask
> >     - Add Andy's Reviewed-by
> >     - Use num_{online,present,possible}_cpus() for builtin masks
> >     - Guard against CPU hotplugging during test for dynamic builtin CPU masks
> >
> >     Changes since v2:
> >     - Rework for_each_* test macros, as suggested by Yury
> >
> >     Changes since v1:
> >     - New patch
> >
> >  lib/Kconfig.debug  |   9 +++
> >  lib/Makefile       |   1 +
> >  lib/test_cpumask.c | 138 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 148 insertions(+)
> >  create mode 100644 lib/test_cpumask.c
> >
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 2e24db4bff19..04aaa20d50f9 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -2021,6 +2021,15 @@ config LKDTM
> >       Documentation on how to use the module can be found in
> >       Documentation/fault-injection/provoke-crashes.rst
> >
> > +config TEST_CPUMASK
> > +     tristate "cpumask tests" if !KUNIT_ALL_TESTS
> > +     depends on KUNIT
> > +     default KUNIT_ALL_TESTS
> > +     help
> > +       Enable to turn on cpumask tests, running at boot or module load time.
> > +
> > +       If unsure, say N.
> > +
> >  config TEST_LIST_SORT
> >       tristate "Linked list sorting test" if !KUNIT_ALL_TESTS
> >       depends on KUNIT
> > diff --git a/lib/Makefile b/lib/Makefile
> > index bcc7e8ea0cde..de3e47453fe8 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -99,6 +99,7 @@ obj-$(CONFIG_TEST_HMM) += test_hmm.o
> >  obj-$(CONFIG_TEST_FREE_PAGES) += test_free_pages.o
> >  obj-$(CONFIG_KPROBES_SANITY_TEST) += test_kprobes.o
> >  obj-$(CONFIG_TEST_REF_TRACKER) += test_ref_tracker.o
> > +obj-$(CONFIG_TEST_CPUMASK) += test_cpumask.o
> >  CFLAGS_test_fprobe.o += $(CC_FLAGS_FTRACE)
> >  obj-$(CONFIG_FPROBE_SANITY_TEST) += test_fprobe.o
> >  #
> > diff --git a/lib/test_cpumask.c b/lib/test_cpumask.c
> > new file mode 100644
> > index 000000000000..a31a1622f1f6
> > --- /dev/null
> > +++ b/lib/test_cpumask.c
> > @@ -0,0 +1,138 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * KUnit tests for cpumask.
> > + *
> > + * Author: Sander Vanheule <sander@svanheule.net>
> > + */
> > +
> > +#include <kunit/test.h>
> > +#include <linux/cpu.h>
> > +#include <linux/cpumask.h>
> > +
> > +#define EXPECT_FOR_EACH_CPU_EQ(test, mask)                   \
> > +     do {                                                    \
> > +             const cpumask_t *m = (mask);                    \
> > +             int mask_weight = cpumask_weight(m);            \
> > +             int cpu, iter = 0;                              \
> > +             for_each_cpu(cpu, m)                            \
> > +                     iter++;                                 \
> > +             KUNIT_EXPECT_EQ((test), mask_weight, iter);     \
> > +     } while (0)
> > +
> > +#define EXPECT_FOR_EACH_CPU_NOT_EQ(test, mask)                                       \
> > +     do {                                                                    \
> > +             const cpumask_t *m = (mask);                                    \
> > +             int mask_weight = cpumask_weight(m);                            \
> > +             int cpu, iter = 0;                                              \
> > +             for_each_cpu_not(cpu, m)                                        \
> > +                     iter++;                                                 \
> > +             KUNIT_EXPECT_EQ((test), nr_cpu_ids - mask_weight, iter);        \
> > +     } while (0)
> > +
> > +#define EXPECT_FOR_EACH_CPU_WRAP_EQ(test, mask)                      \
> > +     do {                                                    \
> > +             const cpumask_t *m = (mask);                    \
> > +             int mask_weight = cpumask_weight(m);            \
> > +             int cpu, iter = 0;                              \
> > +             for_each_cpu_wrap(cpu, m, nr_cpu_ids / 2)       \
> > +                     iter++;                                 \
> > +             KUNIT_EXPECT_EQ((test), mask_weight, iter);     \
> > +     } while (0)
> > +
> > +#define EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, name)           \
> > +     do {                                                    \
> > +             int mask_weight = num_##name##_cpus();          \
> > +             int cpu, iter = 0;                              \
> > +             for_each_##name##_cpu(cpu)                      \
> > +                     iter++;                                 \
> > +             KUNIT_EXPECT_EQ((test), mask_weight, iter);     \
> > +     } while (0)
> > +
> > +static cpumask_t mask_empty;
> > +static cpumask_t mask_all;
> > +
> > +static void test_cpumask_weight(struct kunit *test)
> > +{
> > +     KUNIT_EXPECT_TRUE(test, cpumask_empty(&mask_empty));
> > +     KUNIT_EXPECT_TRUE(test, cpumask_full(cpu_possible_mask));
> > +     KUNIT_EXPECT_TRUE(test, cpumask_full(&mask_all));
> > +
> > +     KUNIT_EXPECT_EQ(test, 0, cpumask_weight(&mask_empty));
> > +     KUNIT_EXPECT_EQ(test, nr_cpu_ids, cpumask_weight(cpu_possible_mask));
> > +     KUNIT_EXPECT_EQ(test, nr_cpumask_bits, cpumask_weight(&mask_all));
> > +}
> > +
> > +static void test_cpumask_first(struct kunit *test)
> > +{
> > +     KUNIT_EXPECT_LE(test, nr_cpu_ids, cpumask_first(&mask_empty));
> > +     KUNIT_EXPECT_EQ(test, 0, cpumask_first(cpu_possible_mask));
> > +
> > +     KUNIT_EXPECT_EQ(test, 0, cpumask_first_zero(&mask_empty));
> > +     KUNIT_EXPECT_LE(test, nr_cpu_ids, cpumask_first_zero(cpu_possible_mask));
> > +}
> > +
> > +static void test_cpumask_last(struct kunit *test)
> > +{
> > +     KUNIT_EXPECT_LE(test, nr_cpumask_bits, cpumask_last(&mask_empty));
> > +     KUNIT_EXPECT_EQ(test, nr_cpumask_bits - 1, cpumask_last(cpu_possible_mask));
> > +}
> > +
> > +static void test_cpumask_next(struct kunit *test)
> > +{
> > +     KUNIT_EXPECT_EQ(test, 0, cpumask_next_zero(-1, &mask_empty));
> > +     KUNIT_EXPECT_LE(test, nr_cpu_ids, cpumask_next_zero(-1, cpu_possible_mask));
> > +
> > +     KUNIT_EXPECT_LE(test, nr_cpu_ids, cpumask_next(-1, &mask_empty));
> > +     KUNIT_EXPECT_EQ(test, 0, cpumask_next(-1, cpu_possible_mask));
> > +}
> > +
> > +static void test_cpumask_iterators(struct kunit *test)
> > +{
> > +     EXPECT_FOR_EACH_CPU_EQ(test, &mask_empty);
> > +     EXPECT_FOR_EACH_CPU_NOT_EQ(test, &mask_empty);
> > +     EXPECT_FOR_EACH_CPU_WRAP_EQ(test, &mask_empty);
> > +
> > +     EXPECT_FOR_EACH_CPU_EQ(test, cpu_possible_mask);
> > +     EXPECT_FOR_EACH_CPU_NOT_EQ(test, cpu_possible_mask);
> > +     EXPECT_FOR_EACH_CPU_WRAP_EQ(test, cpu_possible_mask);
> > +}
> > +
> > +static void test_cpumask_iterators_builtin(struct kunit *test)
> > +{
> > +     EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, possible);
> > +
> > +     /* Ensure the dynamic masks are stable while running the tests */
> > +     cpu_hotplug_disable();
> > +
> > +     EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, online);
> > +     EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, present);
> > +
> > +     cpu_hotplug_enable();
> > +}
> > +
> > +static int test_cpumask_init(struct kunit *test)
> > +{
> > +     cpumask_clear(&mask_empty);
> > +     cpumask_setall(&mask_all);
> > +
> > +     return 0;
> > +}
> > +
> > +static struct kunit_case test_cpumask_cases[] = {
> > +     KUNIT_CASE(test_cpumask_weight),
> > +     KUNIT_CASE(test_cpumask_first),
> > +     KUNIT_CASE(test_cpumask_last),
> > +     KUNIT_CASE(test_cpumask_next),
> > +     KUNIT_CASE(test_cpumask_iterators),
> > +     KUNIT_CASE(test_cpumask_iterators_builtin),
> > +     {}
> > +};
> > +
> > +static struct kunit_suite test_cpumask_suite = {
> > +     .name = "cpumask",
> > +     .init = test_cpumask_init,
> > +     .test_cases = test_cpumask_cases,
> > +};
> > +kunit_test_suite(test_cpumask_suite);
> > +
> > +MODULE_LICENSE("GPL");
> > --
> > 2.36.1
>
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH v4 3/5] lib/test: Introduce cpumask KUnit test suite

[Sander Vanheule]

Hi David, Maíra,

On Wed, 2022-07-20 at 13:24 +0800, David Gow wrote:
> On Wed, Jul 20, 2022 at 5:31 AM Maíra Canal <mairacanal@riseup.net> wrote:
> > 
> > > Add a basic suite of tests for cpumask, providing some tests for empty
> > > and completely filled cpumasks.
> > > 
> > > Signed-off-by: Sander Vanheule <sander@svanheule.net>
> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > The tests test_cpumask_weight and test_cpumask_last are failing on all
> > architectures, as can be seen on [1]. Also this test doesn't follow the
> > standard style for KUnit tests [2].
> > 
> > [1]
> > https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20220718/testrun/10865066/suite/kunit/tests/
> > [2] https://docs.kernel.org/dev-tools/kunit/style.html
> > 
> > CC: Brendan Higgins <brendanhiggins@google.com>
> > CC: David Gow <davidgow@google.com>
> > 
> > Best Regards,
> > - Maíra Canal
> > 
> 
> Hmm... this test passes on the default kunit_tool configs for UML and
> x86_64, which are all without SMP.
> 
> It looks like the flaw is that, if CONFIG_NR_CPUS is greater than the
> actual number of CPUs present, then the cpu_possible_mask (correctly)
> won't be full.
> 
> I'm not sure what the right fix is: but removing the checks for
> cpu_possible_mask being full is probably the way to go. Unless we want
> to plumb through some actual detail about the underlying system and
> check against that, it doesn't make sense. (Or, we could generate an
> artificial "possilbe_mask" which is always full, and test the cpu
> against that. But we sort-of already do that with mask_all anyway.)

The description of cpu_possible_mask does indeed allow for it to not be filled
completely.

> 
> So, my recommendation for a fix would be:
> - Get rid of "KUNIT_EXPECT_TRUE(test, cpumask_full(cpu_possible_mask));"

As per the above, I'll remove this (faulty) check.

> - Replace "KUNIT_EXPECT_EQ(test, nr_cpumask_bits - 1,
> cpumask_last(cpu_possible_mask));" with a KUNIT_EXPECT_GE()

I think we can actually use KUNIT_EXPECT_EQ(test, nr_cpu_ids - 1, ...) here.

Since cpumask_first() on the same mask must return at most nr_cpu_ids - 1 for a
valid result, cpumask_last() cannot return anything larger than this value. 
This implies that cpu_possible_mask cannot contain gaps if its weight equals
nr_cpu_ids (which is checked in test_cpumask_weight).

> - _Maybe_ add some debug logging with the cpumask value being checked,
> as it's a pain to tell from the expectation failure messages. e.g.,
> kunit_info(test, "cpu_possible_mask = '%*pb[l]'\n",
> cpumask_pr_args(cpu_possible_mask));

That would be a useful addition, I'll see where I can add it.

Best,
Sander
> 

Re: [PATCH v4 3/5] lib/test: Introduce cpumask KUnit test suite

[Sander Vanheule]

Hi,

On Tue, 2022-07-19 at 18:31 -0300, Maíra Canal wrote:
> > Add a basic suite of tests for cpumask, providing some tests for empty
> > and completely filled cpumasks.
> > 
> > Signed-off-by: Sander Vanheule <sander@svanheule.net>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> The tests test_cpumask_weight and test_cpumask_last are failing on all
> architectures, as can be seen on [1]. Also this test doesn't follow the
> standard style for KUnit tests [2].
> 
> [1]
> https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20220718/testrun/10865066/suite/kunit/tests/
> [2] https://docs.kernel.org/dev-tools/kunit/style.html

Thanks for the feedback, I wasn't aware of the style guidelines. See my reply to
David's message for the issues with the cpu_possible_mask tests.

Andrew, would you like me to resubmit the entire series, or can I just send a
new version of this patch?

Best,
Sander