[v1] bitmap: convert self-test to KUnit

[PATCH 0/3] bitmap: convert self-test to KUnit

Posted by Tamir Duberstein 1 year ago

This is one of just 3 remaining "Test Module" kselftests (the others
being printf and scanf), the rest having been converted to KUnit.

I tested this using:

$ tools/testing/kunit/kunit.py run --arch arm64 --make_options LLVM=1 bitmap.

I've already sent out a conversion series for each of printf[0] and scanf[1].

There was a previous attempt[2] to do this in July 2024. Please bear
with me as I try to understand and address the objections from that
time. I've spoken with Muhammad Usama Anjum, the author of that series,
and received their approval to "take over" this work. Here we go...

On 7/26/24 11:45 PM, John Hubbard wrote:
> 
> This changes the situation from "works for Linus' tab completion
> case", to "causes a tab completion problem"! :)
> 
> I think a tests/ subdir is how we eventually decided to do this [1],
> right?
> 
> So:
> 
>     lib/tests/bitmap_kunit.c
> 
> [1] https://lore.kernel.org/20240724201354.make.730-kees@kernel.org

This is true and unfortunate, but not trivial to fix because new
kallsyms tests were placed in lib/tests in commit 84b4a51fce4c
("selftests: add new kallsyms selftests")  *after* the KUnit filename
best practices were adopted.

I propose that the KUnit maintainers blaze this trail using
`string_kunit.c` which currently still lives in lib/ despite the KUnit
docs giving it as an example at lib/tests/.

On 7/27/24 12:24 AM, Shuah Khan wrote:
> 
> This change will take away the ability to run bitmap tests during
> boot on a non-kunit kernel.
> 
> Nack on this change. I wan to see all tests that are being removed
> from lib because they have been converted - also it doesn't make
> sense to convert some tests like this one that add the ability test
> during boot.

This point was also discussed in another thread[3] in which:

On 7/27/24 12:35 AM, Shuah Khan wrote:
> 
> Please make sure you aren't taking away the ability to run these tests during
> boot. 
>
> It doesn't make sense to convert every single test especially when it
> is intended to be run during boot without dependencies - not as a kunit test
> but a regression test during boot.
> 
> bitmap is one example - pay attention to the config help test - bitmap
> one clearly states it runs regression testing during boot. Any test that
> says that isn't a candidate for conversion.
> 
> I am going to nack any such conversions.

The crux of the argument seems to be that the config help text is taken
to describe the author's intent with the fragment "at boot". I think
this may be a case of confirmation bias: I see at least the following
KUnit tests with "at boot" in their help text:
- CPUMASK_KUNIT_TEST
- BITFIELD_KUNIT
- CHECKSUM_KUNIT
- UTIL_MACROS_KUNIT

It seems to me that the inference being made is that any test that runs
"at boot" is intended to be run by both developers and users, but I find
no evidence that bitmap in particular would ever provide additional
value when run by users.

There's further discussion about KUnit not being "ideal for cases where
people would want to check a subsystem on a running kernel", but I find
no evidence that bitmap in particular is actually testing the running
kernel; it is a unit test of the bitmap functions, which is also stated
in the config help text.

David Gow made many of the same points in his final reply[4], which was
never replied to.

Link: https://lore.kernel.org/all/20250207-printf-kunit-convert-v2-0-057b23860823@gmail.com/T/#u [0]
Link: https://lore.kernel.org/all/20250207-scanf-kunit-convert-v4-0-a23e2afaede8@gmail.com/T/#u [1]
Link: https://lore.kernel.org/all/20240726110658.2281070-1-usama.anjum@collabora.com/T/#u [2]
Link: https://lore.kernel.org/all/327831fb-47ab-4555-8f0b-19a8dbcaacd7@collabora.com/T/#u [3]
Link: https://lore.kernel.org/all/CABVgOSmMoPD3JfzVd4VTkzGL2fZCo8LfwzaVSzeFimPrhgLa5w@mail.gmail.com/ [4]

Thanks for your attention.

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
Tamir Duberstein (3):
      bitmap: remove _check_eq_u32_array
      bitmap: convert self-test to KUnit
      bitmap: break kunit into test cases

 MAINTAINERS                           |   2 +-
 arch/m68k/configs/amiga_defconfig     |   1 -
 arch/m68k/configs/apollo_defconfig    |   1 -
 arch/m68k/configs/atari_defconfig     |   1 -
 arch/m68k/configs/bvme6000_defconfig  |   1 -
 arch/m68k/configs/hp300_defconfig     |   1 -
 arch/m68k/configs/mac_defconfig       |   1 -
 arch/m68k/configs/multi_defconfig     |   1 -
 arch/m68k/configs/mvme147_defconfig   |   1 -
 arch/m68k/configs/mvme16x_defconfig   |   1 -
 arch/m68k/configs/q40_defconfig       |   1 -
 arch/m68k/configs/sun3_defconfig      |   1 -
 arch/m68k/configs/sun3x_defconfig     |   1 -
 arch/powerpc/configs/ppc64_defconfig  |   1 -
 lib/Kconfig.debug                     |  24 +-
 lib/Makefile                          |   2 +-
 lib/{test_bitmap.c => bitmap_kunit.c} | 454 +++++++++++++---------------------
 tools/testing/selftests/lib/bitmap.sh |   3 -
 tools/testing/selftests/lib/config    |   1 -
 19 files changed, 195 insertions(+), 304 deletions(-)
---
base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
change-id: 20250207-bitmap-kunit-convert-92d3147b2eee

Best regards,
-- 
Tamir Duberstein <tamird@gmail.com>

Re: [PATCH 0/3] bitmap: convert self-test to KUnit

Posted by Yury Norov 12 months ago

On Fri, Feb 07, 2025 at 03:14:01PM -0500, Tamir Duberstein wrote:
> This is one of just 3 remaining "Test Module" kselftests (the others
> being printf and scanf), the rest having been converted to KUnit.
> 
> I tested this using:
> 
> $ tools/testing/kunit/kunit.py run --arch arm64 --make_options LLVM=1 bitmap.
> 
> I've already sent out a conversion series for each of printf[0] and scanf[1].
> 
> There was a previous attempt[2] to do this in July 2024. Please bear
> with me as I try to understand and address the objections from that
> time. I've spoken with Muhammad Usama Anjum, the author of that series,
> and received their approval to "take over" this work. Here we go...

Take over means that you'd at least add the Co-developed-by tag.

> 
> On 7/26/24 11:45 PM, John Hubbard wrote:
> > 
> > This changes the situation from "works for Linus' tab completion
> > case", to "causes a tab completion problem"! :)
> > 
> > I think a tests/ subdir is how we eventually decided to do this [1],
> > right?
> > 
> > So:
> > 
> >     lib/tests/bitmap_kunit.c
> > 
> > [1] https://lore.kernel.org/20240724201354.make.730-kees@kernel.org
> 
> This is true and unfortunate, but not trivial to fix because new
> kallsyms tests were placed in lib/tests in commit 84b4a51fce4c
> ("selftests: add new kallsyms selftests")  *after* the KUnit filename
> best practices were adopted.
> 
> I propose that the KUnit maintainers blaze this trail using
> `string_kunit.c` which currently still lives in lib/ despite the KUnit
> docs giving it as an example at lib/tests/.
> 
> On 7/27/24 12:24 AM, Shuah Khan wrote:
> > 
> > This change will take away the ability to run bitmap tests during
> > boot on a non-kunit kernel.
> > 
> > Nack on this change. I wan to see all tests that are being removed
> > from lib because they have been converted - also it doesn't make
> > sense to convert some tests like this one that add the ability test
> > during boot.
> 
> This point was also discussed in another thread[3] in which:
> 
> On 7/27/24 12:35 AM, Shuah Khan wrote:
> > 
> > Please make sure you aren't taking away the ability to run these tests during
> > boot. 
> >
> > It doesn't make sense to convert every single test especially when it
> > is intended to be run during boot without dependencies - not as a kunit test
> > but a regression test during boot.
> > 
> > bitmap is one example - pay attention to the config help test - bitmap
> > one clearly states it runs regression testing during boot. Any test that
> > says that isn't a candidate for conversion.
> > 
> > I am going to nack any such conversions.
> 
> The crux of the argument seems to be that the config help text is taken
> to describe the author's intent with the fragment "at boot". I think
 
KUNIT is disabled in defconfig, at least on x86_64. It is also disabled
on my Ubuntu 24.04 machine. If I take your patches, I'll be unable to
boot-test bitmaps. Even worse, I'll be unable to build the standalone
test from sources as a module and load it later.

Or I misunderstand it, and there's a way to build some particular KUNIT
test without enabling KUNIT in config and/or re-compiling the whole kernel?
Please teach me, if so

Unless you give me a way to build and run the test in true
production environment, I'm not going with KUNITs. Sorry.

> this may be a case of confirmation bias: I see at least the following
> KUnit tests with "at boot" in their help text:
> - CPUMASK_KUNIT_TEST

This one doesn't count because the test was not converted, it's
originally written as a KUNIT test. I am happy when people bring new
tests in the most comfortable way for them, and I don't want to push
them to use this framework or another. So I didn't object, and I'm
thankful for this contribution to Sander.

> - BITFIELD_KUNIT

Same here. Plus, it was written long before I took over bitfields.

> - CHECKSUM_KUNIT
> - UTIL_MACROS_KUNIT

> It seems to me that the inference being made is that any test that runs
> "at boot" is intended to be run by both developers and users, but I find
> no evidence that bitmap in particular would ever provide additional
> value when run by users.

This is my evidence: sometimes people report performance or whatever
issues on their systems, suspecting bitmaps guilty. I ask them to run
the bitmap or find_bit test to narrow the problem. Sometimes I need to
test a hardware I have no access to, and I have to (kindly!) ask people
to build a small test and run it. I don't want to ask them to rebuild
the whole kernel, or even to build something else.

https://lore.kernel.org/all/YuWk3titnOiQACzC@yury-laptop/

> There's further discussion about KUnit not being "ideal for cases where
> people would want to check a subsystem on a running kernel", but I find
> no evidence that bitmap in particular is actually testing the running
> kernel; it is a unit test of the bitmap functions, which is also stated
> in the config help text.
> 
> David Gow made many of the same points in his final reply[4], which was
> never replied to.

Nice summary for the discussion. Unfortunately you missed my concerns.
Which are:

Pros:
 - Now we switch to KUNITs because KUNITs are so good

Cons:
 - Wipes git history;
 - Bloats the test's source code;
 - Adds dependencies;
 - Doesn't run on most popular distros and defconfig;

So, no.

Thanks,
Yury

> Link: https://lore.kernel.org/all/20250207-printf-kunit-convert-v2-0-057b23860823@gmail.com/T/#u [0]
> Link: https://lore.kernel.org/all/20250207-scanf-kunit-convert-v4-0-a23e2afaede8@gmail.com/T/#u [1]
> Link: https://lore.kernel.org/all/20240726110658.2281070-1-usama.anjum@collabora.com/T/#u [2]
> Link: https://lore.kernel.org/all/327831fb-47ab-4555-8f0b-19a8dbcaacd7@collabora.com/T/#u [3]
> Link: https://lore.kernel.org/all/CABVgOSmMoPD3JfzVd4VTkzGL2fZCo8LfwzaVSzeFimPrhgLa5w@mail.gmail.com/ [4]
> 
> Thanks for your attention.
> 
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
> Tamir Duberstein (3):
>       bitmap: remove _check_eq_u32_array
>       bitmap: convert self-test to KUnit
>       bitmap: break kunit into test cases
> 
>  MAINTAINERS                           |   2 +-
>  arch/m68k/configs/amiga_defconfig     |   1 -
>  arch/m68k/configs/apollo_defconfig    |   1 -
>  arch/m68k/configs/atari_defconfig     |   1 -
>  arch/m68k/configs/bvme6000_defconfig  |   1 -
>  arch/m68k/configs/hp300_defconfig     |   1 -
>  arch/m68k/configs/mac_defconfig       |   1 -
>  arch/m68k/configs/multi_defconfig     |   1 -
>  arch/m68k/configs/mvme147_defconfig   |   1 -
>  arch/m68k/configs/mvme16x_defconfig   |   1 -
>  arch/m68k/configs/q40_defconfig       |   1 -
>  arch/m68k/configs/sun3_defconfig      |   1 -
>  arch/m68k/configs/sun3x_defconfig     |   1 -
>  arch/powerpc/configs/ppc64_defconfig  |   1 -
>  lib/Kconfig.debug                     |  24 +-
>  lib/Makefile                          |   2 +-
>  lib/{test_bitmap.c => bitmap_kunit.c} | 454 +++++++++++++---------------------
>  tools/testing/selftests/lib/bitmap.sh |   3 -
>  tools/testing/selftests/lib/config    |   1 -
>  19 files changed, 195 insertions(+), 304 deletions(-)
> ---
> base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
> change-id: 20250207-bitmap-kunit-convert-92d3147b2eee
> 
> Best regards,
> -- 
> Tamir Duberstein <tamird@gmail.com>

Re: [PATCH 0/3] bitmap: convert self-test to KUnit

Posted by Geert Uytterhoeven 12 months ago

Hi Yuri,

On Sat, 8 Feb 2025 at 18:53, Yury Norov <yury.norov@gmail.com> wrote:
> On Fri, Feb 07, 2025 at 03:14:01PM -0500, Tamir Duberstein wrote:
> > On 7/27/24 12:35 AM, Shuah Khan wrote:
> > > Please make sure you aren't taking away the ability to run these tests during
> > > boot.
> > >
> > > It doesn't make sense to convert every single test especially when it
> > > is intended to be run during boot without dependencies - not as a kunit test
> > > but a regression test during boot.
> > >
> > > bitmap is one example - pay attention to the config help test - bitmap
> > > one clearly states it runs regression testing during boot. Any test that
> > > says that isn't a candidate for conversion.
> > >
> > > I am going to nack any such conversions.
> >
> > The crux of the argument seems to be that the config help text is taken
> > to describe the author's intent with the fragment "at boot". I think

IMO, "at boot" is a misnomer, as most tests can be either builtin
or modular.

> KUNIT is disabled in defconfig, at least on x86_64. It is also disabled
> on my Ubuntu 24.04 machine. If I take your patches, I'll be unable to

I think distros should start setting CONFIG_KUNIT=m.

> boot-test bitmaps. Even worse, I'll be unable to build the standalone
> test from sources as a module and load it later.

If you could build the standalone test from sources as a module,
surely you can build the converted standalone test and KUNIT itself as
modules, and load both of them later?

> Or I misunderstand it, and there's a way to build some particular KUNIT
> test without enabling KUNIT in config and/or re-compiling the whole kernel?
> Please teach me, if so
>
> Unless you give me a way to build and run the test in true
> production environment, I'm not going with KUNITs. Sorry.

FTR, this is why I've been advocating for making all tests modular, and
for not letting any test select (possibly unwanted) extra functionality.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

distro support for CONFIG_KUNIT: [PATCH 0/3] bitmap: convert self-test to KUnit

Posted by John Hubbard 12 months ago

On 2/9/25 11:54 PM, Geert Uytterhoeven wrote:
> On Sat, 8 Feb 2025 at 18:53, Yury Norov <yury.norov@gmail.com> wrote:
>> On Fri, Feb 07, 2025 at 03:14:01PM -0500, Tamir Duberstein wrote:
>>> On 7/27/24 12:35 AM, Shuah Khan wrote:
...
>>> The crux of the argument seems to be that the config help text is taken
>>> to describe the author's intent with the fragment "at boot". I think
> 
> IMO, "at boot" is a misnomer, as most tests can be either builtin
> or modular.

Right.

> 
>> KUNIT is disabled in defconfig, at least on x86_64. It is also disabled
>> on my Ubuntu 24.04 machine. If I take your patches, I'll be unable to

OK so I just bought a shiny new test machine, and installed one of the
big name distros on it, hoping they've moved ahead and bought into the kunit
story...

$ grep KUNIT /boot/config-6.8.0-52-generic
# CONFIG_KUNIT is not set

...gagghh! No such luck. One more data point, in support of Yuri's complaint. :)

> 
> I think distros should start setting CONFIG_KUNIT=m.

Yes they should! kunit really does have important advantages for many use
cases, including bitmaps here, and "CONFIG_KUNIT is not set" is the main
obstacle.

Let me add a few people to Cc who might be able to influence some distros.

thanks,
-- 
John Hubbard