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").
[1] https://lore.kernel.org/all/85217b5de6d62257313ad7fde3e1969421acad75.1659077534.git.sander@svanheule.net/
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.1
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]>
---
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.1
cpu_possible_mask is not necessarily completely filled. That means
running a check on cpumask_full() doesn't make sense, so drop the test.
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]>
Cc: David Gow <[email protected]>
---
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.1
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]>
---
MAINTAINERS | 1 +
1 file changed, 1 insertion(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 868bbf31603d..21ff272c2c10 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3601,6 +3601,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.1
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/[email protected]/
Reported-by: Maíra Canal <[email protected]>
Signed-off-by: Sander Vanheule <[email protected]>
Cc: David Gow <[email protected]>
---
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.1
On 8/9/22 15:08, Sander Vanheule wrote:
> 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/[email protected]/
> Reported-by: Maíra Canal <[email protected]>
> Signed-off-by: Sander Vanheule <[email protected]>
Tested-by: Maíra Canal <[email protected]>
> Cc: David Gow <[email protected]>
> ---
> 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)
On 8/9/22 15:08, Sander Vanheule wrote:
> cpu_possible_mask is not necessarily completely filled. That means
> running a check on cpumask_full() doesn't make sense, so drop the test.
>
> 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]>
> Cc: David Gow <[email protected]>
> ---
> 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));
On Wed, Aug 10, 2022 at 2:09 AM Sander Vanheule <[email protected]> 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]>
> ---
Thanks!
Another option would be to only print this on test failure, but it's
fine as-is as well.
Reviewed-by: David Gow <[email protected]>
Cheers,
-- David
> 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.1
>
On Wed, Aug 10, 2022 at 2:09 AM Sander Vanheule <[email protected]> wrote:
>
> 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]>
Cheers,
-- David
> MAINTAINERS | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 868bbf31603d..21ff272c2c10 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3601,6 +3601,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.1
>
On Wed, Aug 10, 2022 at 2:09 AM Sander Vanheule <[email protected]> wrote:
>
> cpu_possible_mask is not necessarily completely filled. That means
> running a check on cpumask_full() doesn't make sense, so drop the test.
>
> 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]>
> Cc: David Gow <[email protected]>
> ---
Looks good to me. It'd maybe be worth noting _why_ cpu_possible_mask
is not always filled (i.e., that the number of available CPUs might
not match the maximum number of CPUs the kernel is built to support),
but it's probably not worth doing a new version of the patch series
just for that.
Reviewed-by: David Gow <[email protected]>
Cheers,
-- David
> 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.1
>
On Wed, Aug 10, 2022 at 2:09 AM Sander Vanheule <[email protected]> wrote:
>
> 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/[email protected]/
> Reported-by: Maíra Canal <[email protected]>
> Signed-off-by: Sander Vanheule <[email protected]>
> Cc: David Gow <[email protected]>
> ---
Much better, thanks.
Reviewed-by: David Gow <[email protected]>
Cheers,
-- David
> 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.1
>
Hi Maíra,
On Tue, 2022-08-09 at 19:26 -0300, Maíra Canal wrote:
> On 8/9/22 15:08, Sander Vanheule wrote:
> > cpu_possible_mask is not necessarily completely filled. That means
> > running a check on cpumask_full() doesn't make sense, so drop the test.
> >
> > 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]>
Thanks for testing again, and sorry for the trouble!
Best,
Sander
>
> > Cc: David Gow <[email protected]>
> > ---
> > 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));
Hi David,
On Wed, 2022-08-10 at 12:06 +0800, David Gow wrote:
> On Wed, Aug 10, 2022 at 2:09 AM Sander Vanheule <[email protected]> wrote:
> >
> > cpu_possible_mask is not necessarily completely filled. That means
> > running a check on cpumask_full() doesn't make sense, so drop the test.
> >
> > 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]>
> > Cc: David Gow <[email protected]>
> > ---
>
> Looks good to me. It'd maybe be worth noting _why_ cpu_possible_mask
> is not always filled (i.e., that the number of available CPUs might
> not match the maximum number of CPUs the kernel is built to support),
> but it's probably not worth doing a new version of the patch series
> just for that.
>
> Reviewed-by: David Gow <[email protected]>
Thanks for the reviews!
Perhaps the commit message could be replaced by:
"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) may return false for valid system
configurations."
Best,
Sander
>
> Cheers,
> -- David
>
>
> > 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.1
> >
On Wed, Aug 10, 2022 at 4:45 PM Sander Vanheule <[email protected]> wrote:
>
> Hi David,
>
> On Wed, 2022-08-10 at 12:06 +0800, David Gow wrote:
> > On Wed, Aug 10, 2022 at 2:09 AM Sander Vanheule <[email protected]> wrote:
> > >
> > > cpu_possible_mask is not necessarily completely filled. That means
> > > running a check on cpumask_full() doesn't make sense, so drop the test.
> > >
> > > 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]>
> > > Cc: David Gow <[email protected]>
> > > ---
> >
> > Looks good to me. It'd maybe be worth noting _why_ cpu_possible_mask
> > is not always filled (i.e., that the number of available CPUs might
> > not match the maximum number of CPUs the kernel is built to support),
> > but it's probably not worth doing a new version of the patch series
> > just for that.
> >
> > Reviewed-by: David Gow <[email protected]>
>
> Thanks for the reviews!
>
> Perhaps the commit message could be replaced by:
>
> "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) may return false for valid system
> configurations."
>
Sounds good to me! Thanks!
-- David