| Message ID | 20240726110658.2281070-3-usama.anjum@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | bitmap: Convert test_bitmap to kunit test | expand |
On 7/26/24 4:06 AM, Muhammad Usama Anjum wrote: > Rename module to bitmap_kunit and rename the configuration option > compliant with kunit framework. > > Cc: kees@kernel.org > Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > --- > MAINTAINERS | 2 +- > lib/Kconfig.debug | 15 ++++++++------- > lib/Makefile | 2 +- > lib/{test_bitmap.c => bitmap_kunit.c} | 0 > 4 files changed, 10 insertions(+), 9 deletions(-) > rename lib/{test_bitmap.c => bitmap_kunit.c} (100%) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 12b870712da4a..289b727344d64 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3814,13 +3814,13 @@ F: include/linux/find.h > F: include/linux/nodemask.h > F: include/linux/nodemask_types.h > F: include/vdso/bits.h > +F: lib/bitmap_kunit.c > F: lib/bitmap-str.c > F: lib/bitmap.c > F: lib/cpumask.c > F: lib/cpumask_kunit.c > F: lib/find_bit.c > F: lib/find_bit_benchmark.c > -F: lib/test_bitmap.c 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 thanks,
On 7/26/24 05:06, Muhammad Usama Anjum wrote: > Rename module to bitmap_kunit and rename the configuration option > compliant with kunit framework. > > Cc: kees@kernel.org > Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > --- > MAINTAINERS | 2 +- > lib/Kconfig.debug | 15 ++++++++------- > lib/Makefile | 2 +- > lib/{test_bitmap.c => bitmap_kunit.c} | 0 > 4 files changed, 10 insertions(+), 9 deletions(-) > rename lib/{test_bitmap.c => bitmap_kunit.c} (100%) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 12b870712da4a..289b727344d64 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3814,13 +3814,13 @@ F: include/linux/find.h > F: include/linux/nodemask.h > F: include/linux/nodemask_types.h > F: include/vdso/bits.h > +F: lib/bitmap_kunit.c > F: lib/bitmap-str.c > F: lib/bitmap.c > F: lib/cpumask.c > F: lib/cpumask_kunit.c > F: lib/find_bit.c > F: lib/find_bit_benchmark.c > -F: lib/test_bitmap.c > F: tools/include/linux/bitfield.h > F: tools/include/linux/bitmap.h > F: tools/include/linux/bits.h > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index a30c03a661726..6bb02990a73e7 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -2420,13 +2420,6 @@ config TEST_PRINTF > config TEST_SCANF > tristate "Test scanf() family of functions at runtime" > > -config TEST_BITMAP > - tristate "Test bitmap_*() family of functions at runtime" > - help > - Enable this option to test the bitmap functions at boot. > - > - If unsure, say N. > - 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. thanks, -- Shuah
On Fri, Jul 26, 2024 at 04:06:57PM +0500, Muhammad Usama Anjum wrote: > Rename module to bitmap_kunit and rename the configuration option > compliant with kunit framework. ... , so those enabling bitmaps testing in their configs by setting "CONFIG_TEST_BITMAP=y" will suddenly get it broken, and will likely not realize it until something nasty will happen. Sorry, NAK for config rename. > Cc: kees@kernel.org > Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > --- > MAINTAINERS | 2 +- > lib/Kconfig.debug | 15 ++++++++------- > lib/Makefile | 2 +- > lib/{test_bitmap.c => bitmap_kunit.c} | 0 > 4 files changed, 10 insertions(+), 9 deletions(-) > rename lib/{test_bitmap.c => bitmap_kunit.c} (100%) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 12b870712da4a..289b727344d64 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3814,13 +3814,13 @@ F: include/linux/find.h > F: include/linux/nodemask.h > F: include/linux/nodemask_types.h > F: include/vdso/bits.h > +F: lib/bitmap_kunit.c > F: lib/bitmap-str.c > F: lib/bitmap.c > F: lib/cpumask.c > F: lib/cpumask_kunit.c > F: lib/find_bit.c > F: lib/find_bit_benchmark.c > -F: lib/test_bitmap.c > F: tools/include/linux/bitfield.h > F: tools/include/linux/bitmap.h > F: tools/include/linux/bits.h > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index a30c03a661726..6bb02990a73e7 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -2420,13 +2420,6 @@ config TEST_PRINTF > config TEST_SCANF > tristate "Test scanf() family of functions at runtime" > > -config TEST_BITMAP > - tristate "Test bitmap_*() family of functions at runtime" > - help > - Enable this option to test the bitmap functions at boot. > - > - If unsure, say N. > - > config TEST_UUID > tristate "Test functions located in the uuid module at runtime" > > @@ -2813,6 +2806,14 @@ config USERCOPY_KUNIT_TEST > on the copy_to/from_user infrastructure, making sure basic > user/kernel boundary testing is working. > > +config BITMAP_KUNIT_TEST > + tristate "KUnit Test for bitmap_*() family of functions" > + depends on KUNIT > + default KUNIT_ALL_TESTS > + help > + This builds the "bitmap_kunit" module that runs tests for > + bitmaps int the kernel making sure that there isn't any bug. > + > config TEST_UDELAY > tristate "udelay test driver" > help > diff --git a/lib/Makefile b/lib/Makefile > index 322bb127b4dc6..37e7359a7065e 100644 > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -84,7 +84,6 @@ obj-$(CONFIG_TEST_DYNAMIC_DEBUG) += test_dynamic_debug.o > obj-$(CONFIG_TEST_PRINTF) += test_printf.o > obj-$(CONFIG_TEST_SCANF) += test_scanf.o > > -obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o > ifeq ($(CONFIG_CC_IS_CLANG)$(CONFIG_KASAN),yy) > # FIXME: Clang breaks test_bitmap_const_eval when KASAN and GCOV are enabled > GCOV_PROFILE_test_bitmap.o := n > @@ -388,6 +387,7 @@ CFLAGS_fortify_kunit.o += $(DISABLE_STRUCTLEAK_PLUGIN) > obj-$(CONFIG_FORTIFY_KUNIT_TEST) += fortify_kunit.o > obj-$(CONFIG_SIPHASH_KUNIT_TEST) += siphash_kunit.o > obj-$(CONFIG_USERCOPY_KUNIT_TEST) += usercopy_kunit.o > +obj-$(CONFIG_BITMAP_KUNIT_TEST) += bitmap_kunit.o > > obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o > > diff --git a/lib/test_bitmap.c b/lib/bitmap_kunit.c > similarity index 100% > rename from lib/test_bitmap.c > rename to lib/bitmap_kunit.c > -- > 2.39.2
On 7/26/24 11:45 PM, John Hubbard wrote: > On 7/26/24 4:06 AM, Muhammad Usama Anjum wrote: >> Rename module to bitmap_kunit and rename the configuration option >> compliant with kunit framework. >> >> Cc: kees@kernel.org >> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com> >> --- >> MAINTAINERS | 2 +- >> lib/Kconfig.debug | 15 ++++++++------- >> lib/Makefile | 2 +- >> lib/{test_bitmap.c => bitmap_kunit.c} | 0 >> 4 files changed, 10 insertions(+), 9 deletions(-) >> rename lib/{test_bitmap.c => bitmap_kunit.c} (100%) >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 12b870712da4a..289b727344d64 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -3814,13 +3814,13 @@ F: include/linux/find.h >> F: include/linux/nodemask.h >> F: include/linux/nodemask_types.h >> F: include/vdso/bits.h >> +F: lib/bitmap_kunit.c >> F: lib/bitmap-str.c >> F: lib/bitmap.c >> F: lib/cpumask.c >> F: lib/cpumask_kunit.c >> F: lib/find_bit.c >> F: lib/find_bit_benchmark.c >> -F: lib/test_bitmap.c > > 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? Yes, Thank you for mentioning it. I'd missed the naming scheme. I'll fix it. > > So: > > lib/tests/bitmap_kunit.c > > > [1] https://lore.kernel.org/20240724201354.make.730-kees@kernel.org > > thanks,
On 7/27/24 12:24 AM, Shuah Khan wrote: > On 7/26/24 05:06, Muhammad Usama Anjum wrote: >> Rename module to bitmap_kunit and rename the configuration option >> compliant with kunit framework. >> >> Cc: kees@kernel.org >> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com> >> --- >> MAINTAINERS | 2 +- >> lib/Kconfig.debug | 15 ++++++++------- >> lib/Makefile | 2 +- >> lib/{test_bitmap.c => bitmap_kunit.c} | 0 >> 4 files changed, 10 insertions(+), 9 deletions(-) >> rename lib/{test_bitmap.c => bitmap_kunit.c} (100%) >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 12b870712da4a..289b727344d64 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -3814,13 +3814,13 @@ F: include/linux/find.h >> F: include/linux/nodemask.h >> F: include/linux/nodemask_types.h >> F: include/vdso/bits.h >> +F: lib/bitmap_kunit.c >> F: lib/bitmap-str.c >> F: lib/bitmap.c >> F: lib/cpumask.c >> F: lib/cpumask_kunit.c >> F: lib/find_bit.c >> F: lib/find_bit_benchmark.c >> -F: lib/test_bitmap.c >> F: tools/include/linux/bitfield.h >> F: tools/include/linux/bitmap.h >> F: tools/include/linux/bits.h >> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug >> index a30c03a661726..6bb02990a73e7 100644 >> --- a/lib/Kconfig.debug >> +++ b/lib/Kconfig.debug >> @@ -2420,13 +2420,6 @@ config TEST_PRINTF >> config TEST_SCANF >> tristate "Test scanf() family of functions at runtime" >> -config TEST_BITMAP >> - tristate "Test bitmap_*() family of functions at runtime" >> - help >> - Enable this option to test the bitmap functions at boot. >> - >> - If unsure, say N. >> - > > This change will take away the ability to run bitmap tests during > boot on a non-kunit kernel. Kees, what opinion do you have on this? [1] has all the discussion though and my recent thoughts on why I sent this patch. > > 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. > > thanks, > -- Shuah >
On 7/27/24 10:35 PM, Yury Norov wrote: > On Fri, Jul 26, 2024 at 04:06:57PM +0500, Muhammad Usama Anjum wrote: >> Rename module to bitmap_kunit and rename the configuration option >> compliant with kunit framework. > > ... , so those enabling bitmaps testing in their configs by setting > "CONFIG_TEST_BITMAP=y" will suddenly get it broken, and will likely > not realize it until something nasty will happen. CONFIG_TEST_BITMAP was being enabled by the kselftest suite lib. The bitmap test and its config option would disappear. The same test can be run by just enabling KUNIT default config option: KUNIT_ALL_TESTS=y enables this bitmap config by default. > > Sorry, NAK for config rename. > >> Cc: kees@kernel.org >> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com> >> --- >> MAINTAINERS | 2 +- >> lib/Kconfig.debug | 15 ++++++++------- >> lib/Makefile | 2 +- >> lib/{test_bitmap.c => bitmap_kunit.c} | 0 >> 4 files changed, 10 insertions(+), 9 deletions(-) >> rename lib/{test_bitmap.c => bitmap_kunit.c} (100%) >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 12b870712da4a..289b727344d64 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -3814,13 +3814,13 @@ F: include/linux/find.h >> F: include/linux/nodemask.h >> F: include/linux/nodemask_types.h >> F: include/vdso/bits.h >> +F: lib/bitmap_kunit.c >> F: lib/bitmap-str.c >> F: lib/bitmap.c >> F: lib/cpumask.c >> F: lib/cpumask_kunit.c >> F: lib/find_bit.c >> F: lib/find_bit_benchmark.c >> -F: lib/test_bitmap.c >> F: tools/include/linux/bitfield.h >> F: tools/include/linux/bitmap.h >> F: tools/include/linux/bits.h >> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug >> index a30c03a661726..6bb02990a73e7 100644 >> --- a/lib/Kconfig.debug >> +++ b/lib/Kconfig.debug >> @@ -2420,13 +2420,6 @@ config TEST_PRINTF >> config TEST_SCANF >> tristate "Test scanf() family of functions at runtime" >> >> -config TEST_BITMAP >> - tristate "Test bitmap_*() family of functions at runtime" >> - help >> - Enable this option to test the bitmap functions at boot. >> - >> - If unsure, say N. >> - >> config TEST_UUID >> tristate "Test functions located in the uuid module at runtime" >> >> @@ -2813,6 +2806,14 @@ config USERCOPY_KUNIT_TEST >> on the copy_to/from_user infrastructure, making sure basic >> user/kernel boundary testing is working. >> >> +config BITMAP_KUNIT_TEST >> + tristate "KUnit Test for bitmap_*() family of functions" >> + depends on KUNIT >> + default KUNIT_ALL_TESTS >> + help >> + This builds the "bitmap_kunit" module that runs tests for >> + bitmaps int the kernel making sure that there isn't any bug. >> + >> config TEST_UDELAY >> tristate "udelay test driver" >> help >> diff --git a/lib/Makefile b/lib/Makefile >> index 322bb127b4dc6..37e7359a7065e 100644 >> --- a/lib/Makefile >> +++ b/lib/Makefile >> @@ -84,7 +84,6 @@ obj-$(CONFIG_TEST_DYNAMIC_DEBUG) += test_dynamic_debug.o >> obj-$(CONFIG_TEST_PRINTF) += test_printf.o >> obj-$(CONFIG_TEST_SCANF) += test_scanf.o >> >> -obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o >> ifeq ($(CONFIG_CC_IS_CLANG)$(CONFIG_KASAN),yy) >> # FIXME: Clang breaks test_bitmap_const_eval when KASAN and GCOV are enabled >> GCOV_PROFILE_test_bitmap.o := n >> @@ -388,6 +387,7 @@ CFLAGS_fortify_kunit.o += $(DISABLE_STRUCTLEAK_PLUGIN) >> obj-$(CONFIG_FORTIFY_KUNIT_TEST) += fortify_kunit.o >> obj-$(CONFIG_SIPHASH_KUNIT_TEST) += siphash_kunit.o >> obj-$(CONFIG_USERCOPY_KUNIT_TEST) += usercopy_kunit.o >> +obj-$(CONFIG_BITMAP_KUNIT_TEST) += bitmap_kunit.o >> >> obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o >> >> diff --git a/lib/test_bitmap.c b/lib/bitmap_kunit.c >> similarity index 100% >> rename from lib/test_bitmap.c >> rename to lib/bitmap_kunit.c >> -- >> 2.39.2 >
On 7/29/24 1:07 AM, Muhammad Usama Anjum wrote: > On 7/27/24 10:35 PM, Yury Norov wrote: >> On Fri, Jul 26, 2024 at 04:06:57PM +0500, Muhammad Usama Anjum wrote: >>> Rename module to bitmap_kunit and rename the configuration option >>> compliant with kunit framework. >> >> ... , so those enabling bitmaps testing in their configs by setting >> "CONFIG_TEST_BITMAP=y" will suddenly get it broken, and will likely >> not realize it until something nasty will happen. > CONFIG_TEST_BITMAP was being enabled by the kselftest suite lib. The bitmap > test and its config option would disappear. The same test can be run by > just enabling KUNIT default config option: > > KUNIT_ALL_TESTS=y enables this bitmap config by default. > >> >> Sorry, NAK for config rename. >> I agree with Yury. Using KUNIT takes away test coverage for people who are willing to run selftests but not use KUNIT.
On 7/29/24 7:09 PM, Randy Dunlap wrote: > > > On 7/29/24 1:07 AM, Muhammad Usama Anjum wrote: >> On 7/27/24 10:35 PM, Yury Norov wrote: >>> On Fri, Jul 26, 2024 at 04:06:57PM +0500, Muhammad Usama Anjum wrote: >>>> Rename module to bitmap_kunit and rename the configuration option >>>> compliant with kunit framework. >>> >>> ... , so those enabling bitmaps testing in their configs by setting >>> "CONFIG_TEST_BITMAP=y" will suddenly get it broken, and will likely >>> not realize it until something nasty will happen. >> CONFIG_TEST_BITMAP was being enabled by the kselftest suite lib. The bitmap >> test and its config option would disappear. The same test can be run by >> just enabling KUNIT default config option: >> >> KUNIT_ALL_TESTS=y enables this bitmap config by default. >> >>> >>> Sorry, NAK for config rename. >>> > > I agree with Yury. Using KUNIT takes away test coverage for people who > are willing to run selftests but not use KUNIT. How is a kselftest useful when it doesn't even check results of the tests and report failures when they happen?
On Mon, 29 Jul 2024 at 22:09, Randy Dunlap <rdunlap@infradead.org> wrote: > > > > On 7/29/24 1:07 AM, Muhammad Usama Anjum wrote: > > On 7/27/24 10:35 PM, Yury Norov wrote: > >> On Fri, Jul 26, 2024 at 04:06:57PM +0500, Muhammad Usama Anjum wrote: > >>> Rename module to bitmap_kunit and rename the configuration option > >>> compliant with kunit framework. > >> > >> ... , so those enabling bitmaps testing in their configs by setting > >> "CONFIG_TEST_BITMAP=y" will suddenly get it broken, and will likely > >> not realize it until something nasty will happen. > > CONFIG_TEST_BITMAP was being enabled by the kselftest suite lib. The bitmap > > test and its config option would disappear. The same test can be run by > > just enabling KUNIT default config option: > > > > KUNIT_ALL_TESTS=y enables this bitmap config by default. > > > >> > >> Sorry, NAK for config rename. > >> > > I agree with Yury. Using KUNIT takes away test coverage for people who > are willing to run selftests but not use KUNIT. I can see the point that renaming the config option is just churn, but is there a reason people would run the bitmap selftest but be unable or unwilling to use KUnit? Beyond a brief period of adjustment (which could probably be made quite minimal with a wrapper script or something), there shouldn't really be any fundamental difference: KUnit tests can already run at boot, be configured with a config option, and write output to the kernel log. There's nothing really being taken away here, and the bonus of having easier access to run the tests with KUnit's tooling (or have them automatically run by systems which run KUnit tests) would seem worthwhile to me, especially since it's optional. And CONFIG_KUNIT shouldn't be heavy enough to cause problems. Obviously I can't force people to use KUnit, but this is exactly the sort of test which would fit KUnit well, and -- forgive me if I'm missing something -- the only real argument against it I'm hearing is "it's different". And while that's valid (as I said, churn for churn's sake isn't great), none of the "people who are willing to run selftests but not use KUnit" have given reasons why. Especially since this is the sort of test (testing in-kernel functions) we're encouraging people to write with KUnit in Documentation/dev-tools/testing-overview.rst -- if there are good reasons people are refusing to run these, maybe we need to fix those or change the recommendation. -- David
On 7/30/24 12:51 AM, Muhammad Usama Anjum wrote: > On 7/29/24 7:09 PM, Randy Dunlap wrote: >> >> >> On 7/29/24 1:07 AM, Muhammad Usama Anjum wrote: >>> On 7/27/24 10:35 PM, Yury Norov wrote: >>>> On Fri, Jul 26, 2024 at 04:06:57PM +0500, Muhammad Usama Anjum wrote: >>>>> Rename module to bitmap_kunit and rename the configuration option >>>>> compliant with kunit framework. >>>> >>>> ... , so those enabling bitmaps testing in their configs by setting >>>> "CONFIG_TEST_BITMAP=y" will suddenly get it broken, and will likely >>>> not realize it until something nasty will happen. >>> CONFIG_TEST_BITMAP was being enabled by the kselftest suite lib. The bitmap >>> test and its config option would disappear. The same test can be run by >>> just enabling KUNIT default config option: >>> >>> KUNIT_ALL_TESTS=y enables this bitmap config by default. >>> >>>> >>>> Sorry, NAK for config rename. >>>> >> >> I agree with Yury. Using KUNIT takes away test coverage for people who >> are willing to run selftests but not use KUNIT. > How is a kselftest useful when it doesn't even check results of the tests > and report failures when they happen? That should be an easy fix then.
On 7/30/24 04:10, David Gow wrote: > On Mon, 29 Jul 2024 at 22:09, Randy Dunlap <rdunlap@infradead.org> wrote: >> >> >> >> On 7/29/24 1:07 AM, Muhammad Usama Anjum wrote: >>> On 7/27/24 10:35 PM, Yury Norov wrote: >>>> On Fri, Jul 26, 2024 at 04:06:57PM +0500, Muhammad Usama Anjum wrote: >>>>> Rename module to bitmap_kunit and rename the configuration option >>>>> compliant with kunit framework. >>>> >>>> ... , so those enabling bitmaps testing in their configs by setting >>>> "CONFIG_TEST_BITMAP=y" will suddenly get it broken, and will likely >>>> not realize it until something nasty will happen. >>> CONFIG_TEST_BITMAP was being enabled by the kselftest suite lib. The bitmap >>> test and its config option would disappear. The same test can be run by >>> just enabling KUNIT default config option: >>> >>> KUNIT_ALL_TESTS=y enables this bitmap config by default. >>> >>>> >>>> Sorry, NAK for config rename. >>>> >> >> I agree with Yury. Using KUNIT takes away test coverage for people who >> are willing to run selftests but not use KUNIT. This is incorrect. There are selftest that are used to - regression test a subsystem or a abi during boot - spot check on a running system to debug and test by loading a test module. > > I can see the point that renaming the config option is just churn, but > is there a reason people would run the bitmap selftest but be unable > or unwilling to use KUnit? > > Beyond a brief period of adjustment (which could probably be made > quite minimal with a wrapper script or something), there shouldn't > really be any fundamental difference: KUnit tests can already run at > boot, be configured with a config option, and write output to the > kernel log. There's nothing really being taken away here, and the > bonus of having easier access to run the tests with KUnit's tooling > (or have them automatically run by systems which run KUnit tests) > would seem worthwhile to me, especially since it's optional. And > CONFIG_KUNIT shouldn't be heavy enough to cause problems. > > Obviously I can't force people to use KUnit, but this is exactly the > sort of test which would fit KUnit well, and -- forgive me if I'm > missing something -- the only real argument against it I'm hearing is > "it's different". And while that's valid (as I said, churn for churn's > sake isn't great), none of the "people who are willing to run > selftests but not use KUnit" have given reasons why. Especially since > this is the sort of test (testing in-kernel functions) we're > encouraging people to write with KUnit in > Documentation/dev-tools/testing-overview.rst -- if there are good > reasons people are refusing to run these, maybe we need to fix those > or change the recommendation. It isn't about kunit vs. kselftest. It is about overall kernel validation and ability to test effectively by developers and users. KUnit isn't ideal for cases where people would want to check a subsystem on a running kernel - KUnit covers some use-cases and kselftest covers others. What happens if we are debugging a problem that requires us to debug on a running system? Please don't go converting kselftest into kunit without understanding how these frameworks are intended to be used. Yes kselftest results need to be looked at. Write a parser which can be improved. What you are doing is reducing the coverage and talking away the ability to debug and test on running system. Fix what needs to be fixed instead of deleting tests. Think through the use-cases before advocating KUnit is suitable for all testing needs and talking about being able to force or not force people to use one or the other. Reports aren't everything. The primary reason we have these tests is for developers to be able to test. Reports can be improved and shouldn't come at the expense of coverage and testing. Any patch that does that will be NACKed. thanks, -- Shuah
On Tue, Jul 30, 2024 at 06:10:55PM +0800, David Gow wrote: > On Mon, 29 Jul 2024 at 22:09, Randy Dunlap <rdunlap@infradead.org> wrote: > > > > > > > > On 7/29/24 1:07 AM, Muhammad Usama Anjum wrote: > > > On 7/27/24 10:35 PM, Yury Norov wrote: > > >> On Fri, Jul 26, 2024 at 04:06:57PM +0500, Muhammad Usama Anjum wrote: > > >>> Rename module to bitmap_kunit and rename the configuration option > > >>> compliant with kunit framework. > > >> > > >> ... , so those enabling bitmaps testing in their configs by setting > > >> "CONFIG_TEST_BITMAP=y" will suddenly get it broken, and will likely > > >> not realize it until something nasty will happen. > > > CONFIG_TEST_BITMAP was being enabled by the kselftest suite lib. The bitmap > > > test and its config option would disappear. The same test can be run by > > > just enabling KUNIT default config option: > > > > > > KUNIT_ALL_TESTS=y enables this bitmap config by default. > > > > > >> > > >> Sorry, NAK for config rename. > > >> > > > > I agree with Yury. Using KUNIT takes away test coverage for people who > > are willing to run selftests but not use KUNIT. > > I can see the point that renaming the config option is just churn, but > is there a reason people would run the bitmap selftest but be unable > or unwilling to use KUnit? > > Beyond a brief period of adjustment (which could probably be made > quite minimal with a wrapper script or something), there shouldn't > really be any fundamental difference: KUnit tests can already run at > boot, be configured with a config option, and write output to the > kernel log. There's nothing really being taken away here, and the > bonus of having easier access to run the tests with KUnit's tooling > (or have them automatically run by systems which run KUnit tests) > would seem worthwhile to me, especially since it's optional. And > CONFIG_KUNIT shouldn't be heavy enough to cause problems. > > Obviously I can't force people to use KUnit, but this is exactly the > sort of test which would fit KUnit well, and -- forgive me if I'm > missing something -- the only real argument against it I'm hearing is > "it's different". And while that's valid (as I said, churn for churn's > sake isn't great), none of the "people who are willing to run > selftests but not use KUnit" have given reasons why. Especially since > this is the sort of test (testing in-kernel functions) we're > encouraging people to write with KUnit in > Documentation/dev-tools/testing-overview.rst -- if there are good > reasons people are refusing to run these, maybe we need to fix those > or change the recommendation. This doesn't work like this, and never did. Against every change of that sort there's always a strong, valid and self-contained argument: don't touch something that works. However, reviewers provided more than one reason against this rework. Every person has their own reasoning. For me it's history wipe and change of a method how we enable the test. For John, Shuah, Randy and others there's a bunch of other reasons. And my question is still unanswered: what exactly is getting better with this switch to KUNIT, comparing to the old behavior? From KUNIT development perspective, I'd look at this situation as an opportunity to improve the framework. If people don't like such things, I'd leave them alone with their habits and write some sort of compatibility layer for KUNIT, such that you can integrate the test that you like into your framework with no or minimal changes to the original code. Thanks, Yury
On 7/30/24 09:55, Shuah Khan wrote: > On 7/30/24 04:10, David Gow wrote: >> On Mon, 29 Jul 2024 at 22:09, Randy Dunlap <rdunlap@infradead.org> wrote: >>> >>> >>> >>> On 7/29/24 1:07 AM, Muhammad Usama Anjum wrote: >>>> On 7/27/24 10:35 PM, Yury Norov wrote: >>>>> On Fri, Jul 26, 2024 at 04:06:57PM +0500, Muhammad Usama Anjum wrote: >>>>>> Rename module to bitmap_kunit and rename the configuration option >>>>>> compliant with kunit framework. >>>>> >>>>> ... , so those enabling bitmaps testing in their configs by setting >>>>> "CONFIG_TEST_BITMAP=y" will suddenly get it broken, and will likely >>>>> not realize it until something nasty will happen. >>>> CONFIG_TEST_BITMAP was being enabled by the kselftest suite lib. The bitmap >>>> test and its config option would disappear. The same test can be run by >>>> just enabling KUNIT default config option: >>>> >>>> KUNIT_ALL_TESTS=y enables this bitmap config by default. >>>> >>>>> >>>>> Sorry, NAK for config rename. >>>>> >>> >>> I agree with Yury. Using KUNIT takes away test coverage for people who >>> are willing to run selftests but not use KUNIT. > > This is incorrect. There are selftest that are used to > > - regression test a subsystem or a abi during boot > - spot check on a running system to debug and test by loading > a test module. > >> >> I can see the point that renaming the config option is just churn, but >> is there a reason people would run the bitmap selftest but be unable >> or unwilling to use KUnit? >> >> Beyond a brief period of adjustment (which could probably be made >> quite minimal with a wrapper script or something), there shouldn't >> really be any fundamental difference: KUnit tests can already run at >> boot, be configured with a config option, and write output to the >> kernel log. There's nothing really being taken away here, and the >> bonus of having easier access to run the tests with KUnit's tooling >> (or have them automatically run by systems which run KUnit tests) >> would seem worthwhile to me, especially since it's optional. And >> CONFIG_KUNIT shouldn't be heavy enough to cause problems. >> Shouldn't be is the operative word? This doesn't help people who want run a run bitmap test on a running system. This is a wrong direction to go to say all testing has to be done under kunit. What happened to the effort to run selftests as is under KUnit? What is the motivation to convert all tests to kunit instead of trying to provide support to run kselftest under kunit environment? We discussed this a few years ago as I recall. Let's work on that instead of removing existing selftests and regressing current use-cases? Can we look into providing: 1. running kselftest under kunit environment without changes as user space applications? 2. Leave kselftests alone so we don't weaken kernel testing thanks, -- Shuah
On 7/30/24 11:17 AM, Shuah Khan wrote: > On 7/30/24 09:55, Shuah Khan wrote: >> On 7/30/24 04:10, David Gow wrote: >>> On Mon, 29 Jul 2024 at 22:09, Randy Dunlap <rdunlap@infradead.org> ... >>> I can see the point that renaming the config option is just churn, but >>> is there a reason people would run the bitmap selftest but be unable >>> or unwilling to use KUnit? >>> >>> Beyond a brief period of adjustment (which could probably be made >>> quite minimal with a wrapper script or something), there shouldn't >>> really be any fundamental difference: KUnit tests can already run at >>> boot, be configured with a config option, and write output to the >>> kernel log. There's nothing really being taken away here, and the >>> bonus of having easier access to run the tests with KUnit's tooling >>> (or have them automatically run by systems which run KUnit tests) >>> would seem worthwhile to me, especially since it's optional. And >>> CONFIG_KUNIT shouldn't be heavy enough to cause problems. >>> > > Shouldn't be is the operative word? This doesn't help people who > want run a run bitmap test on a running system. This is a wrong > direction to go to say all testing has to be done under kunit. > > What happened to the effort to run selftests as is under KUnit? What > is the motivation to convert all tests to kunit instead of trying to > provide support to run kselftest under kunit environment? > > We discussed this a few years ago as I recall. Let's work on that > instead of removing existing selftests and regressing current use-cases? > > Can we look into providing: > > 1. running kselftest under kunit environment without changes > as user space applications? Yes. I suggested this earlier: if something fits neatly into a KUnit test, then with some additional work, it can also be run from kselftest. Just supporting both would be very nice, because people don't have to change anything about their testing flow. > 2. Leave kselftests alone so we don't weaken kernel testing Or augment them as above, so that we don't weaken kernel testing, yes. thanks,
diff --git a/MAINTAINERS b/MAINTAINERS index 12b870712da4a..289b727344d64 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3814,13 +3814,13 @@ F: include/linux/find.h F: include/linux/nodemask.h F: include/linux/nodemask_types.h F: include/vdso/bits.h +F: lib/bitmap_kunit.c F: lib/bitmap-str.c F: lib/bitmap.c F: lib/cpumask.c F: lib/cpumask_kunit.c F: lib/find_bit.c F: lib/find_bit_benchmark.c -F: lib/test_bitmap.c F: tools/include/linux/bitfield.h F: tools/include/linux/bitmap.h F: tools/include/linux/bits.h diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index a30c03a661726..6bb02990a73e7 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -2420,13 +2420,6 @@ config TEST_PRINTF config TEST_SCANF tristate "Test scanf() family of functions at runtime" -config TEST_BITMAP - tristate "Test bitmap_*() family of functions at runtime" - help - Enable this option to test the bitmap functions at boot. - - If unsure, say N. - config TEST_UUID tristate "Test functions located in the uuid module at runtime" @@ -2813,6 +2806,14 @@ config USERCOPY_KUNIT_TEST on the copy_to/from_user infrastructure, making sure basic user/kernel boundary testing is working. +config BITMAP_KUNIT_TEST + tristate "KUnit Test for bitmap_*() family of functions" + depends on KUNIT + default KUNIT_ALL_TESTS + help + This builds the "bitmap_kunit" module that runs tests for + bitmaps int the kernel making sure that there isn't any bug. + config TEST_UDELAY tristate "udelay test driver" help diff --git a/lib/Makefile b/lib/Makefile index 322bb127b4dc6..37e7359a7065e 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -84,7 +84,6 @@ obj-$(CONFIG_TEST_DYNAMIC_DEBUG) += test_dynamic_debug.o obj-$(CONFIG_TEST_PRINTF) += test_printf.o obj-$(CONFIG_TEST_SCANF) += test_scanf.o -obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o ifeq ($(CONFIG_CC_IS_CLANG)$(CONFIG_KASAN),yy) # FIXME: Clang breaks test_bitmap_const_eval when KASAN and GCOV are enabled GCOV_PROFILE_test_bitmap.o := n @@ -388,6 +387,7 @@ CFLAGS_fortify_kunit.o += $(DISABLE_STRUCTLEAK_PLUGIN) obj-$(CONFIG_FORTIFY_KUNIT_TEST) += fortify_kunit.o obj-$(CONFIG_SIPHASH_KUNIT_TEST) += siphash_kunit.o obj-$(CONFIG_USERCOPY_KUNIT_TEST) += usercopy_kunit.o +obj-$(CONFIG_BITMAP_KUNIT_TEST) += bitmap_kunit.o obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o diff --git a/lib/test_bitmap.c b/lib/bitmap_kunit.c similarity index 100% rename from lib/test_bitmap.c rename to lib/bitmap_kunit.c
Rename module to bitmap_kunit and rename the configuration option compliant with kunit framework. Cc: kees@kernel.org Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com> --- MAINTAINERS | 2 +- lib/Kconfig.debug | 15 ++++++++------- lib/Makefile | 2 +- lib/{test_bitmap.c => bitmap_kunit.c} | 0 4 files changed, 10 insertions(+), 9 deletions(-) rename lib/{test_bitmap.c => bitmap_kunit.c} (100%)