cpumask: Fix invalid uniprocessor assumptions

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/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/

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 <[email protected]>

---

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

include/linux/cpumask.h +19 -80

index fe29ac7cc469..7fbef41b3093 100644
@@ -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						\
 {								\

lib/Makefile +1 -2

index f99bf61f8bbc..bcc7e8ea0cde 100644
@@ -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

lib/cpumask.c +2 -0

index a971a82d2f43..b9728513a4d4 100644
@@ -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 */

Signature · 1 line

2.36.1

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

Signed-off-by Sander Vanheule <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

---

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

lib/Kconfig.debug +9 -0

index 2e24db4bff19..04aaa20d50f9 100644
@@ -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

lib/Makefile +1 -0

index bcc7e8ea0cde..de3e47453fe8 100644
@@ -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
 #

lib/test_cpumask.c new file +138 -0

new file mode 100644
index 000000000000..a31a1622f1f6
@@ -0,0 +1,138 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * KUnit tests for cpumask.
+ *
+ * Author: Sander Vanheule <[email protected]>
+ */
+
+#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");

Signature · 1 line

2.36.1

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 <[email protected]>

---
 include/linux/cpumask.h | 7 +++++++
 1 file changed, 7 insertions(+)

include/linux/cpumask.h +7 -0

index 7fbef41b3093..6c5b4ee000f2 100644
@@ -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);

Signature · 1 line

2.36.1

Quoted 6 lines · 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

Quoted 4 lines

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

Signed-off-by Sander Vanheule <[email protected]>

Acked-by Yury Norov <[email protected]>

Quoted 28 lines

---
 include/linux/cpumask.h | 7 +++++++
 1 file changed, 7 insertions(+)

include/linux/cpumask.h +7 -0

index 7fbef41b3093..6c5b4ee000f2 100644
@@ -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);

Signature · 1 line

2.36.1

Quoted 18 lines · Sander Vanheule

Quoted 16 lines

Quoted 16 lines

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

Signed-off-by Sander Vanheule <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

---

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

Quoted 3 lines

We have a special tag for it:

Suggested-by Yury Norov <[email protected]>

Quoted 10 lines · Yury Norov

Quoted 9 lines · 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.

Quoted 3 lines

Quoted 1 line

Signed-off-by Sander Vanheule <[email protected]>

Acked-by Yury Norov <[email protected]>

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

Quoted 28 lines

Quoted 28 lines

---
 include/linux/cpumask.h | 7 +++++++

 1 file changed, 7 insertions(+)

include/linux/cpumask.h +0 -0

index 7fbef41b3093..6c5b4ee000f2 100644
@@ -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])

include/linux/cpumask.h +6 -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);

Signature · 1 line

2.36.1

Quoted 7 lines · On Sat,  2 Jul 2022 18:08:23 +0200 Sander Vanheule <[email protected]> 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.

Quoted 13 lines

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/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/

Quoted 7 lines · 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.

Quoted 4 lines

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

Signed-off-by Sander Vanheule <[email protected]>

Suggested-by Yury Norov <[email protected]>

Quoted 180 lines

---

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

include/linux/cpumask.h +0 -0

index fe29ac7cc469..7fbef41b3093 100644
@@ -116,85 +116,6 @@ static __always_inline unsigned int cpumask_check(unsigned int cpu)
         return cpu;
  }

include/linux/cpumask.h +0 -79

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

include/linux/cpumask.h +0 -0

@@ -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);

include/linux/cpumask.h +18 -0

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

include/linux/cpumask.h +0 -0

@@ -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                                          \
 {                                                              \

lib/Makefile +1 -1

index f99bf61f8bbc..bcc7e8ea0cde 100644
@@ -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

lib/cpumask.c +0 -0

index a971a82d2f43..b9728513a4d4 100644
@@ -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

lib/cpumask.c +1 -0

@@ -279,3 +280,4 @@ int cpumask_any_distribute(const struct cpumask *srcp)
         return next;
  }
  EXPORT_SYMBOL(cpumask_any_distribute);
+#endif /* NR_CPUS */

Signature · 1 line

2.36.1

Quoted 14 lines · Andrew Morton

Quoted 7 lines · On Sat,  2 Jul 2022 18:08:23 +0200 Sander Vanheule <[email protected]> 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:

Quoted 4 lines

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

Quoted 23 lines · On Sun, 03 Jul 2022 09:50:51 +0200 Sander Vanheule <[email protected]> wrote:

Quoted 15 lines · Andrew Morton

Quoted 7 lines · On Sat,  2 Jul 2022 18:08:23 +0200 Sander Vanheule <[email protected]> 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:

Quoted 6 lines

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

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

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

Signed-off-by Sander Vanheule <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

---

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

lib/Kconfig.debug +9 -0

index 2e24db4bff19..04aaa20d50f9 100644
@@ -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

lib/Makefile +1 -0

index bcc7e8ea0cde..de3e47453fe8 100644
@@ -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
 #

lib/test_cpumask.c new file +138 -0

new file mode 100644
index 000000000000..a31a1622f1f6
@@ -0,0 +1,138 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * KUnit tests for cpumask.
+ *
+ * Author: Sander Vanheule <[email protected]>
+ */
+
+#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");

Signature · 1 line

2.36.1

Quoted 6 lines

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

Signed-off-by Sander Vanheule <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

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 <[email protected]>

CC David Gow <[email protected]>

Best Regards,
- Maíra Canal

Quoted 199 lines

---

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

lib/Kconfig.debug +9 -0

index 2e24db4bff19..04aaa20d50f9 100644
@@ -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

lib/Makefile +1 -0

index bcc7e8ea0cde..de3e47453fe8 100644
@@ -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
 #

lib/test_cpumask.c new file +138 -1

new file mode 100644
index 000000000000..a31a1622f1f6
@@ -0,0 +1,138 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * KUnit tests for cpumask.
+ *
+ * Author: Sander Vanheule <[email protected]>
+ */
+
+#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

Quoted 15 lines · Maíra Canal

Quoted 6 lines

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

Signed-off-by Sander Vanheule <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

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

Quoted 6 lines

CC Brendan Higgins <[email protected]>

CC David Gow <[email protected]>

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

Quoted 5 lines

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

Signed-off-by Sander Vanheule <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

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