Message-ID: <Z6eaDuXnT_rjVSNS@thinkpad>
Date: Sat, 8 Feb 2025 12:53:27 -0500
From: Yury Norov <yury.norov@...il.com>
To: Tamir Duberstein <tamird@...il.com>
Cc: David Gow <davidgow@...gle.com>, 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>,
	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 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@...il.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@...il.com>

Powered by blists - more mailing lists