[PATCH v2 0/5] cpumask: KUnit test suite fixes and improvements

[PATCH v2 0/5] cpumask: KUnit test suite fixes and improvements

[Sander Vanheule]

This series fixes the reported issues, and implements the suggested
improvements, for the version of the cpumask tests [1] that was merged
with commit c41e8866c28c ("lib/test: introduce cpumask KUnit test
suite").

These changes include fixes for the tests, and better alignment with the
KUnit style guidelines.

[1] https://lore.kernel.org/all/85217b5de6d62257313ad7fde3e1969421acad75.1659077534.git.sander@svanheule.net/

Changes since v1:
Link: https://lore.kernel.org/lkml/cover.1660068429.git.sander@svanheule.net/
- Collect tags
- Rewrite commit message of "lib/test_cpumask: drop cpu_possible_mask
  full test"
- Also CC KUnit mailing list

Sander Vanheule (5):
  lib/test_cpumask: drop cpu_possible_mask full test
  lib/test_cpumask: fix cpu_possible_mask last test
  lib/test_cpumask: follow KUnit style guidelines
  lib/cpumask_kunit: log mask contents
  lib/cpumask_kunit: add tests file to MAINTAINERS

 MAINTAINERS                             |  1 +
 lib/Kconfig.debug                       |  7 +++++--
 lib/Makefile                            |  2 +-
 lib/{test_cpumask.c => cpumask_kunit.c} | 13 +++++++++++--
 4 files changed, 18 insertions(+), 5 deletions(-)
 rename lib/{test_cpumask.c => cpumask_kunit.c} (90%)

-- 
2.37.2

[PATCH v2 1/5] lib/test_cpumask: drop cpu_possible_mask full test

[Sander Vanheule]

When the number of CPUs that can possibly be brought online is known at
boot time, e.g. when HOTPLUG is disabled, nr_cpu_ids may be smaller than
NR_CPUS. In that case, cpu_possible_mask would not be completely filled,
and cpumask_full(cpu_possible_mask) can return false for valid system
configurations.

Fixes: c41e8866c28c ("lib/test: introduce cpumask KUnit test suite")
Link: https://lore.kernel.org/lkml/346cb279-8e75-24b0-7d12-9803f2b41c73@riseup.net/
Reported-by: Maíra Canal <mairacanal@riseup.net>
Signed-off-by: Sander Vanheule <sander@svanheule.net>
Tested-by: Maíra Canal <mairacanal@riseup.net>
Reviewed-by: David Gow <davidgow@google.com>
---
Changes in v2:
Rewrite commit message to explain why this test is wrong

 lib/test_cpumask.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/test_cpumask.c b/lib/test_cpumask.c
index a31a1622f1f6..4ebf9f5805f3 100644
--- a/lib/test_cpumask.c
+++ b/lib/test_cpumask.c
@@ -54,7 +54,6 @@ 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));
-- 
2.37.2

Re: [PATCH v2 1/5] lib/test_cpumask: drop cpu_possible_mask full test

[Yury Norov]

On Sat, Aug 20, 2022 at 05:03:09PM +0200, Sander Vanheule wrote:
> When the number of CPUs that can possibly be brought online is known at
> boot time, e.g. when HOTPLUG is disabled, nr_cpu_ids may be smaller than
> NR_CPUS. In that case, cpu_possible_mask would not be completely filled,
> and cpumask_full(cpu_possible_mask) can return false for valid system
> configurations.

It doesn't mean we can just give up. You can check validity of possible
cpumask like this: 
        KUNIT_EXPECT_EQ(test, nr_cpu_ids, cpumask_first_zero(&mask_all))
        KUNIT_EXPECT_EQ(test, NR_CPUS, cpumask_first(&mask_all))
 
> Fixes: c41e8866c28c ("lib/test: introduce cpumask KUnit test suite")
> Link: https://lore.kernel.org/lkml/346cb279-8e75-24b0-7d12-9803f2b41c73@riseup.net/
> Reported-by: Maíra Canal <mairacanal@riseup.net>
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> Tested-by: Maíra Canal <mairacanal@riseup.net>
> Reviewed-by: David Gow <davidgow@google.com>
> ---
> Changes in v2:
> Rewrite commit message to explain why this test is wrong
> 
>  lib/test_cpumask.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/lib/test_cpumask.c b/lib/test_cpumask.c
> index a31a1622f1f6..4ebf9f5805f3 100644
> --- a/lib/test_cpumask.c
> +++ b/lib/test_cpumask.c
> @@ -54,7 +54,6 @@ 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));
> -- 
> 2.37.2

Re: [PATCH v2 1/5] lib/test_cpumask: drop cpu_possible_mask full test

[Sander Vanheule]

Hi Yury,

On Sat, 2022-08-20 at 14:35 -0700, Yury Norov wrote:
> On Sat, Aug 20, 2022 at 05:03:09PM +0200, Sander Vanheule wrote:
> > When the number of CPUs that can possibly be brought online is known at
> > boot time, e.g. when HOTPLUG is disabled, nr_cpu_ids may be smaller than
> > NR_CPUS. In that case, cpu_possible_mask would not be completely filled,
> > and cpumask_full(cpu_possible_mask) can return false for valid system
> > configurations.
> 
> It doesn't mean we can just give up. You can check validity of possible
> cpumask like this: 
>         KUNIT_EXPECT_EQ(test, nr_cpu_ids, cpumask_first_zero(&mask_all))
>         KUNIT_EXPECT_EQ(test, NR_CPUS, cpumask_first(&mask_all))

Did you mean cpu_possible_mask, or mask_all?

For cpu_possible_mask, these tests are in test_cpumask_first(), albeit under a
slightly different form. Together with the tests in test_cpumask_weight() and
test_cpumask_last(), cpu_possible_mask is already one of the more constrained
masks.

For mask_all, the mask is filled up with nr_cpumask_bits <= NR_CPUS. I could add
cpumask_first(), cpumask_first_zero(), and cpumask_last() tests though.

More tests could be also added for cpu_all_mask, since this does have all
NR_CPUS bits set, but I think that belongs in a separate patch.

I think the extra mask_all and cpu_all_mask test are out of scope for this
patch, but they could be added in another patch (for 6.1).

Best,
Sander

>  
> > Fixes: c41e8866c28c ("lib/test: introduce cpumask KUnit test suite")
> > Link:
> > https://lore.kernel.org/lkml/346cb279-8e75-24b0-7d12-9803f2b41c73@riseup.net/
> > Reported-by: Maíra Canal <mairacanal@riseup.net>
> > Signed-off-by: Sander Vanheule <sander@svanheule.net>
> > Tested-by: Maíra Canal <mairacanal@riseup.net>
> > Reviewed-by: David Gow <davidgow@google.com>
> > ---
> > Changes in v2:
> > Rewrite commit message to explain why this test is wrong
> > 
> >  lib/test_cpumask.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/lib/test_cpumask.c b/lib/test_cpumask.c
> > index a31a1622f1f6..4ebf9f5805f3 100644
> > --- a/lib/test_cpumask.c
> > +++ b/lib/test_cpumask.c
> > @@ -54,7 +54,6 @@ 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));
> > -- 
> > 2.37.2

Re: [PATCH v2 1/5] lib/test_cpumask: drop cpu_possible_mask full test

[Sander Vanheule]

Hi Yury,

Replying back in plaintext, as you sent an HTML message.

On Sun, 2022-08-21 at 09:18 -0400, Yury Norov wrote:
> 
> 
> On Sun, Aug 21, 2022, 09:08 Sander Vanheule <sander@svanheule.net> wrote:
> > Hi Yury,
> > 
> > On Sat, 2022-08-20 at 14:35 -0700, Yury Norov wrote:
> > > On Sat, Aug 20, 2022 at 05:03:09PM +0200, Sander Vanheule wrote:
> > > > When the number of CPUs that can possibly be brought online is known at
> > > > boot time, e.g. when HOTPLUG is disabled, nr_cpu_ids may be smaller than
> > > > NR_CPUS. In that case, cpu_possible_mask would not be completely filled,
> > > > and cpumask_full(cpu_possible_mask) can return false for valid system
> > > > configurations.
> > > 
> > > It doesn't mean we can just give up. You can check validity of possible
> > > cpumask like this: 
> > >         KUNIT_EXPECT_EQ(test, nr_cpu_ids, cpumask_first_zero(&mask_all))
> > >         KUNIT_EXPECT_EQ(test, NR_CPUS, cpumask_first(&mask_all))
> > 
> > Did you mean cpu_possible_mask, or mask_all?
> 
> cpu_possble_as of curse.
> 
> > For cpu_possible_mask, these tests are in test_cpumask_first(), albeit under
> > a
> > slightly different form. Together with the tests in test_cpumask_weight()
> > and
> > test_cpumask_last(), cpu_possible_mask is already one of the more
> > constrained
> > masks.
> > 
> > 
> > For mask_all, the mask is filled up with nr_cpumask_bits <= NR_CPUS. I could
> > add
> > cpumask_first(), cpumask_first_zero(), and cpumask_last() tests though.
> > 
> > More tests could be also added for cpu_all_mask, since this does have all
> > NR_CPUS bits set, but I think that belongs in a separate patch.
> > 
> > I think the extra mask_all and cpu_all_mask test are out of scope for this
> > patch, but they could be added in another patch (for 6.1).
> 
> If you think that possible mask is tested by other parts, then can you notice
> that in comments?

Sure, I'll update the commit message to note the other constraints on
cpu_possible_mask.

Best,
Sander

[PATCH v2 2/5] lib/test_cpumask: fix cpu_possible_mask last test

[Sander Vanheule]

Since cpumask_first() on the cpu_possible_mask must return at most
nr_cpu_ids - 1 for a valid result, cpumask_last() cannot return anything
larger than this value.  As test_cpumask_weight() also verifies that the
total weight of cpu_possible_mask must equal nr_cpu_ids, the last bit
set in this mask must be at nr_cpu_ids - 1.

Fixes: c41e8866c28c ("lib/test: introduce cpumask KUnit test suite")
Link: https://lore.kernel.org/lkml/346cb279-8e75-24b0-7d12-9803f2b41c73@riseup.net/
Reported-by: Maíra Canal <mairacanal@riseup.net>
Signed-off-by: Sander Vanheule <sander@svanheule.net>
Tested-by: Maíra Canal <mairacanal@riseup.net>
Reviewed-by: David Gow <davidgow@google.com>
---
 lib/test_cpumask.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/test_cpumask.c b/lib/test_cpumask.c
index 4ebf9f5805f3..4d353614d853 100644
--- a/lib/test_cpumask.c
+++ b/lib/test_cpumask.c
@@ -73,7 +73,7 @@ static void test_cpumask_first(struct kunit *test)
 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));
+	KUNIT_EXPECT_EQ(test, nr_cpu_ids - 1, cpumask_last(cpu_possible_mask));
 }
 
 static void test_cpumask_next(struct kunit *test)
-- 
2.37.2

[PATCH v2 3/5] lib/test_cpumask: follow KUnit style guidelines

[Sander Vanheule]

The cpumask test suite doesn't follow the KUnit style guidelines, as
laid out in Documentation/dev-tools/kunit/style.rst.  The file is
renamed to lib/cpumask_kunit.c to clearly distinguish it from other,
non-KUnit, tests.

Link: https://lore.kernel.org/lkml/346cb279-8e75-24b0-7d12-9803f2b41c73@riseup.net/
Suggested-by: Maíra Canal <mairacanal@riseup.net>
Signed-off-by: Sander Vanheule <sander@svanheule.net>
Reviewed-by: Maíra Canal <mairacanal@riseup.net>
Reviewed-by: David Gow <davidgow@google.com>
---
 lib/Kconfig.debug                       | 7 +++++--
 lib/Makefile                            | 2 +-
 lib/{test_cpumask.c => cpumask_kunit.c} | 0
 3 files changed, 6 insertions(+), 3 deletions(-)
 rename lib/{test_cpumask.c => cpumask_kunit.c} (100%)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 072e4b289c13..bcbe60d6c80c 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2029,13 +2029,16 @@ 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
+config CPUMASK_KUNIT_TEST
+	tristate "KUnit test for cpumask" 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.
 
+	  For more information on KUnit and unit tests in general, please refer
+	  to the KUnit documentation in Documentation/dev-tools/kunit/.
+
 	  If unsure, say N.
 
 config TEST_LIST_SORT
diff --git a/lib/Makefile b/lib/Makefile
index 5927d7fa0806..ffabc30a27d4 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -60,6 +60,7 @@ obj-$(CONFIG_TEST_BPF) += test_bpf.o
 obj-$(CONFIG_TEST_FIRMWARE) += test_firmware.o
 obj-$(CONFIG_TEST_BITOPS) += test_bitops.o
 CFLAGS_test_bitops.o += -Werror
+obj-$(CONFIG_CPUMASK_KUNIT_TEST) += cpumask_kunit.o
 obj-$(CONFIG_TEST_SYSCTL) += test_sysctl.o
 obj-$(CONFIG_TEST_SIPHASH) += test_siphash.o
 obj-$(CONFIG_HASH_KUNIT_TEST) += test_hash.o
@@ -100,7 +101,6 @@ 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/cpumask_kunit.c
similarity index 100%
rename from lib/test_cpumask.c
rename to lib/cpumask_kunit.c
-- 
2.37.2

[PATCH v2 4/5] lib/cpumask_kunit: log mask contents

[Sander Vanheule]

For extra context, log the contents of the masks under test.  This
should help with finding out why a certain test fails.

Link: https://lore.kernel.org/lkml/CABVgOSkPXBc-PWk1zBZRQ_Tt+Sz1ruFHBj3ixojymZF=Vi4tpQ@mail.gmail.com/
Suggested-by: David Gow <davidgow@google.com>
Signed-off-by: Sander Vanheule <sander@svanheule.net>
Reviewed-by: David Gow <davidgow@google.com>
---
 lib/cpumask_kunit.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/cpumask_kunit.c b/lib/cpumask_kunit.c
index 4d353614d853..0f8059a5e93b 100644
--- a/lib/cpumask_kunit.c
+++ b/lib/cpumask_kunit.c
@@ -51,6 +51,10 @@
 static cpumask_t mask_empty;
 static cpumask_t mask_all;
 
+#define STR_MASK(m)			#m
+#define TEST_CPUMASK_PRINT(test, mask)	\
+	kunit_info(test, "%s = '%*pbl'\n", STR_MASK(mask), nr_cpumask_bits, cpumask_bits(mask))
+
 static void test_cpumask_weight(struct kunit *test)
 {
 	KUNIT_EXPECT_TRUE(test, cpumask_empty(&mask_empty));
@@ -103,6 +107,9 @@ static void test_cpumask_iterators_builtin(struct kunit *test)
 	/* Ensure the dynamic masks are stable while running the tests */
 	cpu_hotplug_disable();
 
+	TEST_CPUMASK_PRINT(test, cpu_online_mask);
+	TEST_CPUMASK_PRINT(test, cpu_present_mask);
+
 	EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, online);
 	EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, present);
 
@@ -114,6 +121,9 @@ static int test_cpumask_init(struct kunit *test)
 	cpumask_clear(&mask_empty);
 	cpumask_setall(&mask_all);
 
+	TEST_CPUMASK_PRINT(test, &mask_all);
+	TEST_CPUMASK_PRINT(test, cpu_possible_mask);
+
 	return 0;
 }
 
-- 
2.37.2

Re: [PATCH v2 4/5] lib/cpumask_kunit: log mask contents

[Yury Norov]

On Sat, Aug 20, 2022 at 05:03:12PM +0200, Sander Vanheule wrote:
> For extra context, log the contents of the masks under test.  This
> should help with finding out why a certain test fails.
> 
> Link: https://lore.kernel.org/lkml/CABVgOSkPXBc-PWk1zBZRQ_Tt+Sz1ruFHBj3ixojymZF=Vi4tpQ@mail.gmail.com/
> Suggested-by: David Gow <davidgow@google.com>
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> Reviewed-by: David Gow <davidgow@google.com>
> ---
>  lib/cpumask_kunit.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/lib/cpumask_kunit.c b/lib/cpumask_kunit.c
> index 4d353614d853..0f8059a5e93b 100644
> --- a/lib/cpumask_kunit.c
> +++ b/lib/cpumask_kunit.c
> @@ -51,6 +51,10 @@
>  static cpumask_t mask_empty;
>  static cpumask_t mask_all;
>  
> +#define STR_MASK(m)			#m
> +#define TEST_CPUMASK_PRINT(test, mask)	\
> +	kunit_info(test, "%s = '%*pbl'\n", STR_MASK(mask), nr_cpumask_bits, cpumask_bits(mask))
> +
>  static void test_cpumask_weight(struct kunit *test)
>  {
>  	KUNIT_EXPECT_TRUE(test, cpumask_empty(&mask_empty));
> @@ -103,6 +107,9 @@ static void test_cpumask_iterators_builtin(struct kunit *test)
>  	/* Ensure the dynamic masks are stable while running the tests */
>  	cpu_hotplug_disable();
>  
> +	TEST_CPUMASK_PRINT(test, cpu_online_mask);
> +	TEST_CPUMASK_PRINT(test, cpu_present_mask);
> +
>  	EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, online);
>  	EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, present);
>  
> @@ -114,6 +121,9 @@ static int test_cpumask_init(struct kunit *test)
>  	cpumask_clear(&mask_empty);
>  	cpumask_setall(&mask_all);
>  
> +	TEST_CPUMASK_PRINT(test, &mask_all);
> +	TEST_CPUMASK_PRINT(test, cpu_possible_mask);
> +

It sort of breaks the rule of silence. Can you make this print conditional
on a test failure? If everything is OK, who wants to look into details? 

>  	return 0;
>  }
>  
> -- 
> 2.37.2

Re: [PATCH v2 4/5] lib/cpumask_kunit: log mask contents

[Sander Vanheule]

On Sat, 2022-08-20 at 14:46 -0700, Yury Norov wrote:
> On Sat, Aug 20, 2022 at 05:03:12PM +0200, Sander Vanheule wrote:
> > For extra context, log the contents of the masks under test.  This
> > should help with finding out why a certain test fails.
> > 
> > Link:
> > https://lore.kernel.org/lkml/CABVgOSkPXBc-PWk1zBZRQ_Tt+Sz1ruFHBj3ixojymZF=Vi4tpQ@mail.gmail.com/
> > Suggested-by: David Gow <davidgow@google.com>
> > Signed-off-by: Sander Vanheule <sander@svanheule.net>
> > Reviewed-by: David Gow <davidgow@google.com>
> > ---
> >  lib/cpumask_kunit.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/lib/cpumask_kunit.c b/lib/cpumask_kunit.c
> > index 4d353614d853..0f8059a5e93b 100644
> > --- a/lib/cpumask_kunit.c
> > +++ b/lib/cpumask_kunit.c
> > @@ -51,6 +51,10 @@
> >  static cpumask_t mask_empty;
> >  static cpumask_t mask_all;
> >  
> > +#define STR_MASK(m)                    #m
> > +#define TEST_CPUMASK_PRINT(test, mask) \
> > +       kunit_info(test, "%s = '%*pbl'\n", STR_MASK(mask), nr_cpumask_bits,
> > cpumask_bits(mask))
> > +
> >  static void test_cpumask_weight(struct kunit *test)
> >  {
> >         KUNIT_EXPECT_TRUE(test, cpumask_empty(&mask_empty));
> > @@ -103,6 +107,9 @@ static void test_cpumask_iterators_builtin(struct kunit
> > *test)
> >         /* Ensure the dynamic masks are stable while running the tests */
> >         cpu_hotplug_disable();
> >  
> > +       TEST_CPUMASK_PRINT(test, cpu_online_mask);
> > +       TEST_CPUMASK_PRINT(test, cpu_present_mask);
> > +
> >         EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, online);
> >         EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, present);
> >  
> > @@ -114,6 +121,9 @@ static int test_cpumask_init(struct kunit *test)
> >         cpumask_clear(&mask_empty);
> >         cpumask_setall(&mask_all);
> >  
> > +       TEST_CPUMASK_PRINT(test, &mask_all);
> > +       TEST_CPUMASK_PRINT(test, cpu_possible_mask);
> > +
> 
> It sort of breaks the rule of silence. Can you make this print conditional
> on a test failure? If everything is OK, who wants to look into details? 

I will change the macros to the _MSG versions, and log the mask there.

I implemented this with kunit_info() as David proposed. AFAICT I can't call 
kunit_info() only when the test fails, because the EXPECT_ macros don't return
any result.

Best,
Sander

> 
> >         return 0;
> >  }
> >  
> > -- 
> > 2.37.2

Re: [PATCH v2 4/5] lib/cpumask_kunit: log mask contents

[Maíra Canal]

On 8/21/22 10:13, Sander Vanheule wrote:
> On Sat, 2022-08-20 at 14:46 -0700, Yury Norov wrote:
>> On Sat, Aug 20, 2022 at 05:03:12PM +0200, Sander Vanheule wrote:
>>> For extra context, log the contents of the masks under test.  This
>>> should help with finding out why a certain test fails.
>>>
>>> Link:
>>> https://lore.kernel.org/lkml/CABVgOSkPXBc-PWk1zBZRQ_Tt+Sz1ruFHBj3ixojymZF=Vi4tpQ@mail.gmail.com/
>>> Suggested-by: David Gow <davidgow@google.com>
>>> Signed-off-by: Sander Vanheule <sander@svanheule.net>
>>> Reviewed-by: David Gow <davidgow@google.com>
>>> ---
>>>  lib/cpumask_kunit.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/lib/cpumask_kunit.c b/lib/cpumask_kunit.c
>>> index 4d353614d853..0f8059a5e93b 100644
>>> --- a/lib/cpumask_kunit.c
>>> +++ b/lib/cpumask_kunit.c
>>> @@ -51,6 +51,10 @@
>>>  static cpumask_t mask_empty;
>>>  static cpumask_t mask_all;
>>>  
>>> +#define STR_MASK(m)                    #m
>>> +#define TEST_CPUMASK_PRINT(test, mask) \
>>> +       kunit_info(test, "%s = '%*pbl'\n", STR_MASK(mask), nr_cpumask_bits,
>>> cpumask_bits(mask))
>>> +
>>>  static void test_cpumask_weight(struct kunit *test)
>>>  {
>>>         KUNIT_EXPECT_TRUE(test, cpumask_empty(&mask_empty));
>>> @@ -103,6 +107,9 @@ static void test_cpumask_iterators_builtin(struct kunit
>>> *test)
>>>         /* Ensure the dynamic masks are stable while running the tests */
>>>         cpu_hotplug_disable();
>>>  
>>> +       TEST_CPUMASK_PRINT(test, cpu_online_mask);
>>> +       TEST_CPUMASK_PRINT(test, cpu_present_mask);
>>> +
>>>         EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, online);
>>>         EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, present);
>>>  
>>> @@ -114,6 +121,9 @@ static int test_cpumask_init(struct kunit *test)
>>>         cpumask_clear(&mask_empty);
>>>         cpumask_setall(&mask_all);
>>>  
>>> +       TEST_CPUMASK_PRINT(test, &mask_all);
>>> +       TEST_CPUMASK_PRINT(test, cpu_possible_mask);
>>> +
>>
>> It sort of breaks the rule of silence. Can you make this print conditional
>> on a test failure? If everything is OK, who wants to look into details? 
> 
> I will change the macros to the _MSG versions, and log the mask there.
> 
> I implemented this with kunit_info() as David proposed. AFAICT I can't call 
> kunit_info() only when the test fails, because the EXPECT_ macros don't return
> any result.

Maybe you can use KUNIT_EXPECT_EQ_MSG to print a more detailed error and
avoid printing the info when the test doesn't fail.

Best Regards,
- Maíra Canal

> 
> Best,
> Sander
> 
>>
>>>         return 0;
>>>  }
>>>  
>>> -- 
>>> 2.37.2
> 

Re: [PATCH v2 4/5] lib/cpumask_kunit: log mask contents

[Sander Vanheule]

Hi Maíra,

On Sun, 2022-08-21 at 11:02 -0300, Maíra Canal wrote:
> 
> 
> On 8/21/22 10:13, Sander Vanheule wrote:
> > On Sat, 2022-08-20 at 14:46 -0700, Yury Norov wrote:
> > > On Sat, Aug 20, 2022 at 05:03:12PM +0200, Sander Vanheule wrote:
> > > > For extra context, log the contents of the masks under test.  This
> > > > should help with finding out why a certain test fails.
> > > > 
> > > > Link:
> > > > https://lore.kernel.org/lkml/CABVgOSkPXBc-PWk1zBZRQ_Tt+Sz1ruFHBj3ixojymZF=Vi4tpQ@mail.gmail.com/
> > > > Suggested-by: David Gow <davidgow@google.com>
> > > > Signed-off-by: Sander Vanheule <sander@svanheule.net>
> > > > Reviewed-by: David Gow <davidgow@google.com>
> > > > ---
> > > >  lib/cpumask_kunit.c | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/lib/cpumask_kunit.c b/lib/cpumask_kunit.c
> > > > index 4d353614d853..0f8059a5e93b 100644
> > > > --- a/lib/cpumask_kunit.c
> > > > +++ b/lib/cpumask_kunit.c
> > > > @@ -51,6 +51,10 @@
> > > >  static cpumask_t mask_empty;
> > > >  static cpumask_t mask_all;
> > > >  
> > > > +#define STR_MASK(m)                    #m
> > > > +#define TEST_CPUMASK_PRINT(test, mask) \
> > > > +       kunit_info(test, "%s = '%*pbl'\n", STR_MASK(mask),
> > > > nr_cpumask_bits,
> > > > cpumask_bits(mask))
> > > > +
> > > >  static void test_cpumask_weight(struct kunit *test)
> > > >  {
> > > >         KUNIT_EXPECT_TRUE(test, cpumask_empty(&mask_empty));
> > > > @@ -103,6 +107,9 @@ static void test_cpumask_iterators_builtin(struct
> > > > kunit
> > > > *test)
> > > >         /* Ensure the dynamic masks are stable while running the tests
> > > > */
> > > >         cpu_hotplug_disable();
> > > >  
> > > > +       TEST_CPUMASK_PRINT(test, cpu_online_mask);
> > > > +       TEST_CPUMASK_PRINT(test, cpu_present_mask);
> > > > +
> > > >         EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, online);
> > > >         EXPECT_FOR_EACH_CPU_BUILTIN_EQ(test, present);
> > > >  
> > > > @@ -114,6 +121,9 @@ static int test_cpumask_init(struct kunit *test)
> > > >         cpumask_clear(&mask_empty);
> > > >         cpumask_setall(&mask_all);
> > > >  
> > > > +       TEST_CPUMASK_PRINT(test, &mask_all);
> > > > +       TEST_CPUMASK_PRINT(test, cpu_possible_mask);
> > > > +
> > > 
> > > It sort of breaks the rule of silence. Can you make this print conditional
> > > on a test failure? If everything is OK, who wants to look into details? 
> > 
> > I will change the macros to the _MSG versions, and log the mask there.
> > 
> > I implemented this with kunit_info() as David proposed. AFAICT I can't call 
> > kunit_info() only when the test fails, because the EXPECT_ macros don't
> > return
> > any result.
> 
> Maybe you can use KUNIT_EXPECT_EQ_MSG to print a more detailed error and
> avoid printing the info when the test doesn't fail.

Yes, this is what I currently have for use with the _MSG() variants of the
macros:

+#define MASK_MSG(m)                    \
+       "%s contains %sCPUs %*pbl", #m, (cpumask_weight(m) ? "" : "no "), nr_cpumask_bits, cpumask_bits(m)
+

For example, with (bogus) KUNIT_EXPECT_TRUE_MSG(test, ..., MASK_MSG(mask)) this
becomes (trimmed):

$ ./tools/testing/kunit/kunit.py run  --build_dir=build-um cpumask
[...]
[18:15:33] # test_cpumask_weight: EXPECTATION FAILED at lib/cpumask_kunit.c:60
[18:15:33] Expected cpumask_empty(((struct cpumask *)(1 ? (cpu_all_bits) : (void *)sizeof(__check_is_bitmap(cpu_all_bits))))) to be true, but is false
[18:15:33] 
[18:15:33] cpu_all_mask contains CPUs 0
[18:15:33] # test_cpumask_weight: EXPECTATION FAILED at lib/cpumask_kunit.c:61
[18:15:33] Expected cpumask_full(&mask_empty) to be true, but is false
[18:15:33] 
[18:15:33] &mask_empty contains no CPUs
[18:15:33] not ok 1 - test_cpumask_weight
[18:15:33] [FAILED] test_cpumask_weight
[...]

Or on a real system:
[    1.246805]     # test_cpumask_weight: EXPECTATION FAILED at lib/cpumask_kunit.c:59
[    1.246805]     Expected cpumask_empty(((struct cpumask *)(1 ? (cpu_all_bits) : (void *)sizeof(__check_is_bitmap(cpu_all_bits))))) to be true, but is false
[    1.246805] 
[    1.246805] cpu_all_mask contains CPUs 0-1
[    1.249756]     not ok 1 - test_cpumask_weight

I will send an updated series tomorrow, in case David or others have more more comments.

Best,
Sander

[PATCH v2 5/5] lib/cpumask_kunit: add tests file to MAINTAINERS

[Sander Vanheule]

cpumask related files are listed under the BITMAP API section, so file
with the tests for cpumask should be added to that list.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
Reviewed-by: David Gow <davidgow@google.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index f512b430c7cb..0f41174be0d3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3612,6 +3612,7 @@ F:	include/linux/find.h
 F:	include/linux/nodemask.h
 F:	lib/bitmap.c
 F:	lib/cpumask.c
+F:	lib/cpumask_kunit.c
 F:	lib/find_bit.c
 F:	lib/find_bit_benchmark.c
 F:	lib/test_bitmap.c
-- 
2.37.2

Re: [PATCH v2 0/5] cpumask: KUnit test suite fixes and improvements

[Yury Norov]

On Sat, Aug 20, 2022 at 05:03:08PM +0200, Sander Vanheule wrote:
> This series fixes the reported issues, and implements the suggested
> improvements, for the version of the cpumask tests [1] that was merged
> with commit c41e8866c28c ("lib/test: introduce cpumask KUnit test
> suite").
> 
> These changes include fixes for the tests, and better alignment with the
> KUnit style guidelines.

I wrote a couple comments, but the series looks OK to me in general.
So for 2, 3 and 5:
Acked-by: Yury Norov <yury.norov@gmail.com>

It's named as 'fix', but it fixes a test, and the kernel code itself
looks correct. So, do you want to take it into 6.0-rc, or in 6.1?

I'm OK to do it this way or another, but for later -rc's it may look
too noisy. And I'm not sure where to put a threshold.

Thanks,
Yury
 
> [1] https://lore.kernel.org/all/85217b5de6d62257313ad7fde3e1969421acad75.1659077534.git.sander@svanheule.net/
> 
> Changes since v1:
> Link: https://lore.kernel.org/lkml/cover.1660068429.git.sander@svanheule.net/
> - Collect tags
> - Rewrite commit message of "lib/test_cpumask: drop cpu_possible_mask
>   full test"
> - Also CC KUnit mailing list
> 
> Sander Vanheule (5):
>   lib/test_cpumask: drop cpu_possible_mask full test
>   lib/test_cpumask: fix cpu_possible_mask last test
>   lib/test_cpumask: follow KUnit style guidelines
>   lib/cpumask_kunit: log mask contents
>   lib/cpumask_kunit: add tests file to MAINTAINERS
> 
>  MAINTAINERS                             |  1 +
>  lib/Kconfig.debug                       |  7 +++++--
>  lib/Makefile                            |  2 +-
>  lib/{test_cpumask.c => cpumask_kunit.c} | 13 +++++++++++--
>  4 files changed, 18 insertions(+), 5 deletions(-)
>  rename lib/{test_cpumask.c => cpumask_kunit.c} (90%)
> 
> -- 
> 2.37.2

Re: [PATCH v2 0/5] cpumask: KUnit test suite fixes and improvements

[Sander Vanheule]

Hi Yury,

On Sat, 2022-08-20 at 15:06 -0700, Yury Norov wrote:
> On Sat, Aug 20, 2022 at 05:03:08PM +0200, Sander Vanheule wrote:
> > This series fixes the reported issues, and implements the suggested
> > improvements, for the version of the cpumask tests [1] that was merged
> > with commit c41e8866c28c ("lib/test: introduce cpumask KUnit test
> > suite").
> > 
> > These changes include fixes for the tests, and better alignment with the
> > KUnit style guidelines.
> 
> I wrote a couple comments, but the series looks OK to me in general.
> So for 2, 3 and 5:
> Acked-by: Yury Norov <yury.norov@gmail.com>
> 
> It's named as 'fix', but it fixes a test, and the kernel code itself
> looks correct. So, do you want to take it into 6.0-rc, or in 6.1?
> 
> I'm OK to do it this way or another, but for later -rc's it may look
> too noisy. And I'm not sure where to put a threshold.

Broken tests are worse than no tests IMHO, so I would at least like patches 1
and 2 to be merged for 6.0-rc. I don't want people to end up with false
positives, like Maíra did, for an entire release cycle.

Preferably I would also like to see 3 in 6.0-rc, so no renames will be needed in
6.1 anymore. Not that I expect anything to depend on this symbol (or filename)
by then, but I feel it's better not to risk that by waiting for 6.1.

Patches 4 and 5 can go with 6.1, as far as I'm concerned. Especially as the mask
logging patch (4) may need some work still.

Best,
Sander