| Message ID | 20250207-bitmap-kunit-convert-v1-0-c520675343b6@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | bitmap: convert self-test to KUnit | expand |
On Sat, 8 Feb 2025 at 04:14, Tamir Duberstein <tamird@gmail.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@gmail.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@gmail.com> >
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>
On Sat, Feb 8, 2025 at 12:53 PM Yury Norov <yury.norov@gmail.com> wrote: > > [...] > > Take over means that you'd at least add the Co-developed-by tag. I didn't use their code - the thing being "taken over" is the work of having these debates with the maintainers. > [...] > > 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 is a question for David -- I don't know if this is possible. > [...] > > 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/ This is compelling evidence, and it was not previously raised. Thank you. I notice that two things are true about the performance test part of test_bitmap.c: - It's a minority of the code in the file (48 lines out of 1462). - There are no assertions in it. Do you also find value in running the testing portion on other people's machines, to which you don't have access? > [...] > > 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; I was very careful to minimize churn, and the result is 249 lines on which I'd now own the blame (228 with `-w`). Still, it's a valid con. > - Bloats the test's source code; The test is 74 lines shorter after this series. > - Adds dependencies; > - Doesn't run on most popular distros and defconfig; Yep, I understand your concerns much better now - and I'm grateful for your having taken the time to explain and show receipts. Still, I wonder if we can get the best of both worlds - either by finding what you need in KUnit, or by moving the testing bit to KUnit and keeping the performance bit where it is. Thanks. Tamir
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
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,
On 10.02.25 20:35, John Hubbard wrote: > 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, Fedora has it. CS-10 has it (-> RHEL-10): redhat/configs/common/generic/CONFIG_KUNIT:CONFIG_KUNIT=m https://gitlab.com/redhat/centos-stream/src/kernel/centos-stream-10/-/blob/main/redhat/configs/common/generic/CONFIG_KUNIT?ref_type=heads CS-9 has it (-> RHEL-9): redhat/configs/common/generic/CONFIG_KUNIT:CONFIG_KUNIT=m https://gitlab.com/redhat/centos-stream/src/kernel/centos-stream-9/-/blob/main/redhat/configs/common/generic/CONFIG_KUNIT?ref_type=heads So I think from the RH side everything is properly set? Let me CC Nico, he did some KUNIT work in the past.
On Mon, Feb 10, 2025 at 11:35:48AM -0800, John Hubbard wrote: > 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. Hi John, Geert, Tamir, Can you please explain in details which advantages KUNIT brings to the test_bitmap.c, find_bit_benchmark.c and other low-level tests? I'm not strongly against moving under KUNIT's hat, but I do: - respect commitment of my contributors, so I don't want to wipe git history for no serious reason; - respect time of my testers, so no extra dependencies; - respect time of reviewers. Tamir, If it comes to v2, can you please begin your series with an exhaustive and clear answer to the following questions: - What do the tests miss now? - What do _you_ need from the tests? Describe your test scenario. - How exactly KUNIT helps you testing bitmaps and friends better? - Is there a way to meet your needs with a less invasive approach, particularly without run-time dependencies? Thanks, Yury
On Mon, Feb 10, 2025 at 3:20 PM Yury Norov <yury.norov@gmail.com> wrote: > > On Mon, Feb 10, 2025 at 11:35:48AM -0800, John Hubbard wrote: > > 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. > > Hi John, Geert, Tamir, > > Can you please explain in details which advantages KUNIT brings to > the test_bitmap.c, find_bit_benchmark.c and other low-level tests? I can try, but I'm not the expert, and David Gow can probably elaborate further. As I understand it the main benefit of KUnit is standardization and speed (and standardization _is_ speed). KUnit makes it very easy for me, a person who has not previously contributed to any of the bitmap code, to run those tests, and it requires zero configuration, it all just works. It's basically just `tools/testing/kunit/kunit.py run bitmap`, and I get the test results in a human-readable format. The same benefit applies on the author side: test facilities are standardized, so once you get to know the tools, all the tests start to look the same: you can jump in and contribute without having to first learn the so-called local "testing framework". The important part is that this all applies to ~all other tests written in KUnit. I can even run them *all* trivially: `tools/testing/kunit/kunit.py run`. Anecdotally I've also noticed there are bots running those KUnit tests e.g. see https://lore.kernel.org/all/20250207-blackholedev-kunit-convert-v1-1-8ef0dc1ff881@gmail.com/ where a test I converted was immediately flagged by a robot as having dubious type coercion. None of these are must-haves, they are just (to me) a nice way to make the kernel more approachable for new contributors. As for pure benchmarks like find_bit_benchmark.c and test_bitmap.c::`test_bitmap_{read,write}_perf` specifically: I believe the benefits are super limited or even negative: AFAIK KUnit is designed to generally suppress output (in the userspace reporter, not in the kernel) unless a test fails, so I wouldn't hurry to use it for these. > I'm not strongly against moving under KUNIT's hat, but I do: > - respect commitment of my contributors, so I don't want to wipe git > history for no serious reason; > - respect time of my testers, so no extra dependencies; > - respect time of reviewers. These are valid concerns. Certainly the testing case is the most compelling and folks are clearly interested in lowering those barriers. I don't have any influence in this area, but I am grateful to John for starting the conversation. As I mentioned in the previous thread: I think we could keep `test_bitmap_{read,write}_perf` in test_bitmap.c and get the best of both worlds. WDYT? > Tamir, > > If it comes to v2, can you please begin your series with an exhaustive > and clear answer to the following questions: > - What do the tests miss now? > - What do _you_ need from the tests? Describe your test scenario. > - How exactly KUNIT helps you testing bitmaps and friends better? > - Is there a way to meet your needs with a less invasive approach, > particularly without run-time dependencies? Hopefully I've answered these above. I can include some of it in a v2, but perhaps the general pitch for KUnit is better placed in documentation or slides from a conference? Cheers. Tamir
On Mon, Feb 10, 2025 at 12:46 PM David Hildenbrand <david@redhat.com> wrote: > > On 10.02.25 20:35, John Hubbard wrote: > > 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, > > > Fedora has it. > > CS-10 has it (-> RHEL-10): > redhat/configs/common/generic/CONFIG_KUNIT:CONFIG_KUNIT=m > > https://gitlab.com/redhat/centos-stream/src/kernel/centos-stream-10/-/blob/main/redhat/configs/common/generic/CONFIG_KUNIT?ref_type=heads > > CS-9 has it (-> RHEL-9): > redhat/configs/common/generic/CONFIG_KUNIT:CONFIG_KUNIT=m > > https://gitlab.com/redhat/centos-stream/src/kernel/centos-stream-9/-/blob/main/redhat/configs/common/generic/CONFIG_KUNIT?ref_type=heads > > So I think from the RH side everything is properly set? > > Let me CC Nico, he did some KUNIT work in the past. Yeah that is correct! I enabled KUNIT in our environments a few years ago. We enable it as a module and use our own wrapper to exercise the code. For RHEL and Centos these kunit modules are only shipped internally for testing; However fedora-rawhide makes these modules available in the kernel-modules-internal package. To test this you can follow this to install rawhide-vm: https://developer.fedoraproject.org/tools/virt-builder/about.html then inside the vm: yum install kernel-modules-internal add the kunit.enable=1 to the cmdline reboot vm modprobe kunit modprobe <test_name> Hopefully that helps! -- Nico > > > -- > Cheers, > > David / dhildenb >
On 2/10/25 3:10 PM, Nico Pache wrote: >>> ... >> Fedora has it. >> >> CS-10 has it (-> RHEL-10): >> redhat/configs/common/generic/CONFIG_KUNIT:CONFIG_KUNIT=m >> >> https://gitlab.com/redhat/centos-stream/src/kernel/centos-stream-10/-/blob/main/redhat/configs/common/generic/CONFIG_KUNIT?ref_type=heads >> >> CS-9 has it (-> RHEL-9): >> redhat/configs/common/generic/CONFIG_KUNIT:CONFIG_KUNIT=m >> >> https://gitlab.com/redhat/centos-stream/src/kernel/centos-stream-9/-/blob/main/redhat/configs/common/generic/CONFIG_KUNIT?ref_type=heads >> >> So I think from the RH side everything is properly set? >> >> Let me CC Nico, he did some KUNIT work in the past. > > Yeah that is correct! I enabled KUNIT in our environments a few years > ago. We enable it as a module and use our own wrapper to exercise the > code. For RHEL and Centos these kunit modules are only shipped > internally for testing; However fedora-rawhide makes these modules > available in the kernel-modules-internal package. > > To test this you can follow this to install rawhide-vm: > https://developer.fedoraproject.org/tools/virt-builder/about.html > then inside the vm: > yum install kernel-modules-internal > add the kunit.enable=1 to the cmdline > reboot vm > modprobe kunit > modprobe <test_name> > > Hopefully that helps! > -- Nico Great news, thanks for the quick answers for Red Hat. "Already done" is as good as it gets, for this kind of question. :) thanks,
On Tue, 11 Feb 2025 at 04:20, Yury Norov <yury.norov@gmail.com> wrote: > > On Mon, Feb 10, 2025 at 11:35:48AM -0800, John Hubbard wrote: > > 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. > > Hi John, Geert, Tamir, > > Can you please explain in details which advantages KUNIT brings to > the test_bitmap.c, find_bit_benchmark.c and other low-level tests? > First, I'd agree with Tamir and others who've pointed out that KUnit is not really intended for benchmarks, so I definitely don't want to oversell it for find_bit_benchmark.c and others (though it has been used for benchmarks in a few cases, it's not perfect). The main advantages of KUnit are that: - it's already used for a bunch of lib/ tests (so quite familiar to a number of contributors/users), - tests can be run at boot without needing any userspace at all (if built-in), - KUnit tests are already being automatically run as regression tests, and it's relatively easy to run "all tests" associated with a config - there's a whole bunch of tooling which automates configuring, building, and running a kernel with these tests, across several architectures (e.g., passing --arch arm64 or --arch m68k to automatically build and run the tests in QEMU for those architectures) - there's tooling to nicely print, format, and summarise the results of any number of suites. - and, as a result of the above, it's very quick and easy to run a test (or whole bunch of tests) in an isolated environment. KUnit also has very few runtime dependencies from a developer point-of-view (python, optionally QEMU). It is, however, less focused on the "debug a running system" case, hence the requirement for CONFIG_KUNIT to be enabled, and the tooling largely being focused on the "run all of the tests in UML or a VM" case. > I'm not strongly against moving under KUNIT's hat, but I do: > - respect commitment of my contributors, so I don't want to wipe git > history for no serious reason; It should be possible to preserve the git history during a port, though it admittedly will have a bunch of porting changes which make it more difficult to navigate with git blame. But it should at least be better than a total wipe. > - respect time of my testers, so no extra dependencies; I think KUnit is a bit of a mixed bag here. In many cases, KUnit tests can be run pretty quickly, either because the tester's environment already has CONFIG_KUNIT enabled, or possibly under some kind of virtualization with the tooling (either QEMU, or on x86, User-Mode-Linux). But, as mentioned above, I'd not consider it ideal for performance testing. I am very sympathetic to not wanting to add a dependency here. I think KUnit is a pretty minimal dependency as far as they go (it selects CONFIG_GLOB and nothing else, needs no userspace, and even the tooling only really requires python on top of the basic toolchain), but it's definitely a further dependency for the 'test-the-currently-running-kernel' use-case. > - respect time of reviewers. This is an area where I think KUnit could be an advantage, as it'd be easy for reviewers to simply run ./tools/testing/kunit/kunit.py run bitmap as a spot check on the code. And if they want to test on other architectures, the --arch option can be used to do so quickly. (And, of course, it can still be loaded as a module (or even built-in) on a physical system or other VM, albeit with the requirement for CONFIG_KUNIT.) As for distros enabling CONFIG_KUNIT by default, as mentioned, my understanding is that the Fedora/Red Hat distros are doing it, as are some branches of Android, but I doubt anyone else is. This makes sense: enabling CONFIG_KUNIT isn't free (though the overhead should be very low if no test is running, just a few static branches and an extra check in the module loader), and there's always been some reticence (which I've shared) in enabling test features on production kernels. So it's definitely something I was recommending against a few years ago. However, we've definitely made sure that CONFIG_KUNIT is less invasive, with things like hooks being kept behind static branches, and made the decision to have _running_ a test taint the kernel, so it should be relatively safe to just have the framework enabled (and even safe to have it loaded), and pretty obvious when a test is run. We've also got a bunch of extra options to disable running tests automatically when they're built-in/loaded, both at runtime and compile time. So I'd definitely be happy with other distros choosing to enable CONFIG_KUNIT by default now: I think it's much safer than it was, and you won't be going against the grain. But equally, I'd understand having it disabled in production: the cost, while very small, is nonzero. Cheers, -- David