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@xxxxxxxxxx > > 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@xxxxxxxxx/T/#u [0] > Link: https://lore.kernel.org/all/20250207-scanf-kunit-convert-v4-0-a23e2afaede8@xxxxxxxxx/T/#u [1] > Link: https://lore.kernel.org/all/20240726110658.2281070-1-usama.anjum@xxxxxxxxxxxxx/T/#u [2] > Link: https://lore.kernel.org/all/327831fb-47ab-4555-8f0b-19a8dbcaacd7@xxxxxxxxxxxxx/T/#u [3] > Link: https://lore.kernel.org/all/CABVgOSmMoPD3JfzVd4VTkzGL2fZCo8LfwzaVSzeFimPrhgLa5w@xxxxxxxxxxxxxx/ [4] > > Thanks for your attention. > > Signed-off-by: Tamir Duberstein <tamird@xxxxxxxxx> > --- > 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@xxxxxxxxx>
