Message-ID: <CABVgOSne6yfQ7NmSPkYSci6Ej1tKsijqKaZxR7NsLfPcDEq99g@mail.gmail.com>
Date: Sat, 8 Feb 2025 17:07:01 +0800
From: David Gow <davidgow@...gle.com>
To: Tamir Duberstein <tamird@...il.com>
Cc: John Hubbard <jhubbard@...dia.com>, Andrew Morton <akpm@...ux-foundation.org>, 
	Geert Uytterhoeven <geert@...ux-m68k.org>, Madhavan Srinivasan <maddy@...ux.ibm.com>, 
	Michael Ellerman <mpe@...erman.id.au>, Nicholas Piggin <npiggin@...il.com>, 
	Christophe Leroy <christophe.leroy@...roup.eu>, Naveen N Rao <naveen@...nel.org>, 
	Yury Norov <yury.norov@...il.com>, Rasmus Villemoes <linux@...musvillemoes.dk>, 
	Shuah Khan <shuah@...nel.org>, Kees Cook <kees@...nel.org>, 
	Muhammad Usama Anjum <usama.anjum@...labora.com>, linux-kernel@...r.kernel.org, 
	linux-m68k@...ts.linux-m68k.org, linuxppc-dev@...ts.ozlabs.org, 
	linux-kselftest@...r.kernel.org
Subject: Re: [PATCH 0/3] bitmap: convert self-test to KUnit

On Sat, 8 Feb 2025 at 04:14, Tamir Duberstein <tamird@...il.com> wrote:
>
> This is one of just 3 remaining "Test Module" kselftests (the others
> being printf and scanf), the rest having been converted to KUnit.

Thanks a lot, Tamir: these are great!

>
> I tested this using:
>
> $ tools/testing/kunit/kunit.py run --arch arm64 --make_options LLVM=1 bitmap.

I have also tested this across several architectures, including arm,
m68k, i386, x86_64, powerpc64, and UML, and it works well here.

For anyone put off by the long command, testing it under UML is
$ ./tools/testing/kunit/kunit.py run bitmap

It should also automatically run on boot for any kernel with it
built-in, and run when the module is loaded if it's enabled as a
module.

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

I admit to never quite understanding the "at boot" wording as an
objection here, as KUnit tests can run at boot (and do by default),
and are often regression tests. I'd not object if anyone wanted this
stated more clearly in the new config option's help text, though.

The line between 'developers' and 'users' in the kernel world is
necessarily thin, but I equally think users who would want to be able
to run test modules are unlikely to be unable to run KUnit tests if
they so desire. The only difficulty (which I admit could be annoying)
is that it's not possible to run the test against a kernel built with
CONFIG_KUNIT=n. Personally, I have my doubts that anyone is deriving
value from running test_bitmap against a system which was not compiled
for testing -- particularly since it's now quite common for distros to
ship kernels to users with CONFIG_KUNIT=m (IIRC, Red Hat is doing
this, and Android at least were considering it).

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

Again, I think the only issue here is the CONFIG_KUNIT=n argument
above. This is a real issue, but I can't imagine a case where someone
has a running system which has broken due to an issue in the bitmap
code which can't easily be reproduced on a fresh kernel with
CONFIG_KUNIT enabled. (Though that could just be a limitation of my
imagination, so if that has happened to someone, I'd love to hear the
story!)

>
> 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@...il.com>
> ---

My only remaining concerns are that:
- We've not misinterpreted any of the objections to the previous
versions (and ideally that everyone's happy, or at least contentedly
resigned, for this to go through),
- This goes upstream in a way that minimises the conflicts in the
various defconfigs. Given the number of these ports, it'd be great to
either get them to all go through in the same tree or otherwise make
sure the resolutions (while trivial) are as non-annoying as possible.

Cheers,
-- David


> 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@...il.com>
>

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5294 bytes)

Powered by blists - more mailing lists