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/[email protected]/
- 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
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/[email protected]/
Reported-by: Maíra Canal <[email protected]>
Signed-off-by: Sander Vanheule <[email protected]>
Tested-by: Maíra Canal <[email protected]>
Reviewed-by: David Gow <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Sander Vanheule <[email protected]>
Reviewed-by: David Gow <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: David Gow <[email protected]>
---
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
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/[email protected]/
Suggested-by: Maíra Canal <[email protected]>
Signed-off-by: Sander Vanheule <[email protected]>
Reviewed-by: Maíra Canal <[email protected]>
Reviewed-by: David Gow <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Sander Vanheule <[email protected]>
> Reviewed-by: David Gow <[email protected]>
> ---
> 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
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/[email protected]/
> Reported-by: Ma?ra Canal <[email protected]>
> Signed-off-by: Sander Vanheule <[email protected]>
> Tested-by: Ma?ra Canal <[email protected]>
> Reviewed-by: David Gow <[email protected]>
> ---
> 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
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 <[email protected]>
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/[email protected]/
> - 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
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/[email protected]/
> > Reported-by: Maíra Canal <[email protected]>
> > Signed-off-by: Sander Vanheule <[email protected]>
> > Tested-by: Maíra Canal <[email protected]>
> > Reviewed-by: David Gow <[email protected]>
> > ---
> > 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
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 <[email protected]>
>
> 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
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 <[email protected]>
> > Signed-off-by: Sander Vanheule <[email protected]>
> > Reviewed-by: David Gow <[email protected]>
> > ---
> > 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
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 <[email protected]> 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
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 <[email protected]>
>>> Signed-off-by: Sander Vanheule <[email protected]>
>>> Reviewed-by: David Gow <[email protected]>
>>> ---
>>> 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
>
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 <[email protected]>
> > > > Signed-off-by: Sander Vanheule <[email protected]>
> > > > Reviewed-by: David Gow <[email protected]>
> > > > ---
> > > > 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