| Message ID | 20240726110658.2281070-1-usama.anjum@collabora.com |
|---|---|
| Headers | show |
| Series | bitmap: Convert test_bitmap to kunit test | expand |
On Fri, Jul 26, 2024 at 01:26:48PM -0600, Shuah Khan wrote: > On 7/26/24 05:06, Muhammad Usama Anjum wrote: > > In this series, test_bitmap is being converted to kunit test. Multiple > > patches will make the review process smooth. > > > > - Patch-1: Convert the tests in lib/test_bitmap.c to kunit > > - Patch-2: Rename the lib/test_bitmap.c to lib/bitmap_kunit.c and other > > configuration options > > - Patch-3: Remove the bitmap.sh selftest > > > > Muhammad Usama Anjum (3): > > bitmap: convert test_bitmap to KUnit test > > bitmap: Rename module > > selftests: lib: remove test_bitmap > > > > MAINTAINERS | 2 +- > > lib/Kconfig.debug | 15 +- > > lib/Makefile | 2 +- > > lib/{test_bitmap.c => bitmap_kunit.c} | 624 ++++++++++++-------------- > > tools/testing/selftests/lib/Makefile | 2 +- > > tools/testing/selftests/lib/bitmap.sh | 3 - > > tools/testing/selftests/lib/config | 1 - > > 7 files changed, 295 insertions(+), 354 deletions(-) > > rename lib/{test_bitmap.c => bitmap_kunit.c} (70%) > > delete mode 100755 tools/testing/selftests/lib/bitmap.sh > > > > Can you tell me how this conversion helps? > > It is removing the ability to run bitmap tests during boot. > It doesn't make sense to blindly convert all test under lib > to kunit - Nack on this change or any change that takes away > the ability to run tests and makes them dependent on kunit. Hi Muhammad, In addition to Shuah's and John's reasoning. This patch wipes the test history (git blame will point on you for most of the test), breaks boot-time testing support, messes with config names and usability, and drops kselftest support for ... exactly, what? KUNIT engine here doesn't improve on readability, neither shorten the test length, to my taste. If you'd like to contribute to bitmaps testing - I'm all for that. This is the very core and performance-sensitive piece of kernel, and any extra-coverage is always welcome. But I think the best way would be either adding new cases to the existing test, or writing a new test, KUNIT-based, if you like. Thanks, Yury
On 7/27/24 11:10 PM, Yury Norov wrote: > On Fri, Jul 26, 2024 at 01:26:48PM -0600, Shuah Khan wrote: >> On 7/26/24 05:06, Muhammad Usama Anjum wrote: >>> In this series, test_bitmap is being converted to kunit test. Multiple >>> patches will make the review process smooth. >>> >>> - Patch-1: Convert the tests in lib/test_bitmap.c to kunit >>> - Patch-2: Rename the lib/test_bitmap.c to lib/bitmap_kunit.c and other >>> configuration options >>> - Patch-3: Remove the bitmap.sh selftest >>> >>> Muhammad Usama Anjum (3): >>> bitmap: convert test_bitmap to KUnit test >>> bitmap: Rename module >>> selftests: lib: remove test_bitmap >>> >>> MAINTAINERS | 2 +- >>> lib/Kconfig.debug | 15 +- >>> lib/Makefile | 2 +- >>> lib/{test_bitmap.c => bitmap_kunit.c} | 624 ++++++++++++-------------- >>> tools/testing/selftests/lib/Makefile | 2 +- >>> tools/testing/selftests/lib/bitmap.sh | 3 - >>> tools/testing/selftests/lib/config | 1 - >>> 7 files changed, 295 insertions(+), 354 deletions(-) >>> rename lib/{test_bitmap.c => bitmap_kunit.c} (70%) >>> delete mode 100755 tools/testing/selftests/lib/bitmap.sh >>> >> >> Can you tell me how this conversion helps? >> >> It is removing the ability to run bitmap tests during boot. >> It doesn't make sense to blindly convert all test under lib >> to kunit - Nack on this change or any change that takes away >> the ability to run tests and makes them dependent on kunit. > > Hi Muhammad, > > In addition to Shuah's and John's reasoning. This patch wipes the > test history (git blame will point on you for most of the test), When files are renamed, their history isn't lost. We just need to use --follow option with git log to get complete history[1]. > breaks boot-time testing support, messes with config names and > usability, and drops kselftest support for ... exactly, what? AFAIU the kselftest wasn't detected the test results that's why I started thinking on which could be best way to detect if any failure happens in this test. Triggering the test from kselftest doesn't grantee the test it would pass every time until we check results. For this kind of in-kernel testing, kunit is best suites. Please find earlier discussion [2]. > > KUNIT engine here doesn't improve on readability, neither shorten > the test length, to my taste. > > If you'd like to contribute to bitmaps testing - I'm all for that. > This is the very core and performance-sensitive piece of kernel, > and any extra-coverage is always welcome. > > But I think the best way would be either adding new cases to the > existing test, or writing a new test, KUNIT-based, if you like. > [1] https://git-scm.com/docs/git-log#Documentation/git-log.txt---follow [2] https://lore.kernel.org/all/327831fb-47ab-4555-8f0b-19a8dbcaacd7@collabora.com > Thanks, > Yury >
On 7/27/24 12:26 AM, Shuah Khan wrote: > On 7/26/24 05:06, Muhammad Usama Anjum wrote: >> In this series, test_bitmap is being converted to kunit test. Multiple >> patches will make the review process smooth. >> >> - Patch-1: Convert the tests in lib/test_bitmap.c to kunit >> - Patch-2: Rename the lib/test_bitmap.c to lib/bitmap_kunit.c and other >> configuration options >> - Patch-3: Remove the bitmap.sh selftest >> >> Muhammad Usama Anjum (3): >> bitmap: convert test_bitmap to KUnit test >> bitmap: Rename module >> selftests: lib: remove test_bitmap >> >> MAINTAINERS | 2 +- >> lib/Kconfig.debug | 15 +- >> lib/Makefile | 2 +- >> lib/{test_bitmap.c => bitmap_kunit.c} | 624 ++++++++++++-------------- >> tools/testing/selftests/lib/Makefile | 2 +- >> tools/testing/selftests/lib/bitmap.sh | 3 - >> tools/testing/selftests/lib/config | 1 - >> 7 files changed, 295 insertions(+), 354 deletions(-) >> rename lib/{test_bitmap.c => bitmap_kunit.c} (70%) >> delete mode 100755 tools/testing/selftests/lib/bitmap.sh >> > > Can you tell me how this conversion helps? > > It is removing the ability to run bitmap tests during boot. > It doesn't make sense to blindly convert all test under lib > to kunit - Nack on this change or any change that takes away > the ability to run tests and makes them dependent on kunit. Let's discuss this on discussion thread [1]. [1] https://lore.kernel.org/all/a3083ad4-e9dc-40da-bf57-8391bcd96a6c@collabora.com > > thanks, > -- Shuah
On 7/29/24 02:29, Muhammad Usama Anjum wrote: > On 7/27/24 12:26 AM, Shuah Khan wrote: >> On 7/26/24 05:06, Muhammad Usama Anjum wrote: >>> In this series, test_bitmap is being converted to kunit test. Multiple >>> patches will make the review process smooth. >>> >>> - Patch-1: Convert the tests in lib/test_bitmap.c to kunit >>> - Patch-2: Rename the lib/test_bitmap.c to lib/bitmap_kunit.c and other >>> configuration options >>> - Patch-3: Remove the bitmap.sh selftest >>> >>> Muhammad Usama Anjum (3): >>> bitmap: convert test_bitmap to KUnit test >>> bitmap: Rename module >>> selftests: lib: remove test_bitmap >>> >>> MAINTAINERS | 2 +- >>> lib/Kconfig.debug | 15 +- >>> lib/Makefile | 2 +- >>> lib/{test_bitmap.c => bitmap_kunit.c} | 624 ++++++++++++-------------- >>> tools/testing/selftests/lib/Makefile | 2 +- >>> tools/testing/selftests/lib/bitmap.sh | 3 - >>> tools/testing/selftests/lib/config | 1 - >>> 7 files changed, 295 insertions(+), 354 deletions(-) >>> rename lib/{test_bitmap.c => bitmap_kunit.c} (70%) >>> delete mode 100755 tools/testing/selftests/lib/bitmap.sh >>> >> >> Can you tell me how this conversion helps? >> >> It is removing the ability to run bitmap tests during boot. >> It doesn't make sense to blindly convert all test under lib >> to kunit - Nack on this change or any change that takes away >> the ability to run tests and makes them dependent on kunit. > Let's discuss this on discussion thread [1]. So - it doesn't mean that it is a done deal. Each patch will be reviewed on individual basis. This test in particular clearly its use-case right in the config which was deleted without understanding it. -config TEST_BITMAP - tristate "Test bitmap_*() family of functions at runtime" - help - Enable this option to test the bitmap functions at boot. This line above is the important piece of information which tells you how the test is intended to be used. 1. You can enable this option and boot the kernel to check for regressions. 2. You can load the module on a running kernel to check for health. Converting it to kunit drops support for these two use-cases which are user-space regressions. You don't want to do that. - - 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. And this isn't complete even. I don'ty see tristate in here. I responded to the thread that started this flurry of conversion activity stating that converting tests without thinking through the use-cases isn't what we want to do. 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, 30 Jul 2024 at 23:49, Shuah Khan <skhan@linuxfoundation.org> wrote: > > On 7/29/24 02:29, Muhammad Usama Anjum wrote: > > On 7/27/24 12:26 AM, Shuah Khan wrote: > >> On 7/26/24 05:06, Muhammad Usama Anjum wrote: > >>> In this series, test_bitmap is being converted to kunit test. Multiple > >>> patches will make the review process smooth. > >>> > >>> - Patch-1: Convert the tests in lib/test_bitmap.c to kunit > >>> - Patch-2: Rename the lib/test_bitmap.c to lib/bitmap_kunit.c and other > >>> configuration options > >>> - Patch-3: Remove the bitmap.sh selftest > >>> > >>> Muhammad Usama Anjum (3): > >>> bitmap: convert test_bitmap to KUnit test > >>> bitmap: Rename module > >>> selftests: lib: remove test_bitmap > >>> > >>> MAINTAINERS | 2 +- > >>> lib/Kconfig.debug | 15 +- > >>> lib/Makefile | 2 +- > >>> lib/{test_bitmap.c => bitmap_kunit.c} | 624 ++++++++++++-------------- > >>> tools/testing/selftests/lib/Makefile | 2 +- > >>> tools/testing/selftests/lib/bitmap.sh | 3 - > >>> tools/testing/selftests/lib/config | 1 - > >>> 7 files changed, 295 insertions(+), 354 deletions(-) > >>> rename lib/{test_bitmap.c => bitmap_kunit.c} (70%) > >>> delete mode 100755 tools/testing/selftests/lib/bitmap.sh > >>> > >> > >> Can you tell me how this conversion helps? > >> > >> It is removing the ability to run bitmap tests during boot. > >> It doesn't make sense to blindly convert all test under lib > >> to kunit - Nack on this change or any change that takes away > >> the ability to run tests and makes them dependent on kunit. > > Let's discuss this on discussion thread [1]. > > So - it doesn't mean that it is a done deal. Each patch will be > reviewed on individual basis. This test in particular clearly its > use-case right in the config which was deleted without understanding > it. > > -config TEST_BITMAP > - tristate "Test bitmap_*() family of functions at runtime" > - help > - Enable this option to test the bitmap functions at boot. > > > This line above is the important piece of information which tells > you how the test is intended to be used. > > 1. You can enable this option and boot the kernel to check for regressions. Converting the test to KUnit _does not break this_. You can still enable this option and boot the kernel to test for regressions if it's a KUnit test. > 2. You can load the module on a running kernel to check for health. This is where the disagreement lies in my mind. While you can do this with KUnit, the kernel does have to have been built with CONFIG_KUNIT enabled (either built-in or as a module). (Indeed, KUnit adds the ability to trigger a test at runtime even if it's built-in, via debugfs, so it's possible to do so even if CONFIG_MODULES is disabled. That's a pretty niche use-case, I admit, but I'd guess that the bitmap functionailty breaking in a way which disappears when CONFIG_KUNIT is enabled is as well.) In general, though, my interpretation of "Test ... at runtime" in config option descriptions has not precluded introducing the CONFIG_KUNIT dependency. We already have several KUnit tests with that in their description, several of which were ported to KUnit later. > > Converting it to kunit drops support for these two use-cases which > are user-space regressions. You don't want to do that. > > - > - 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. > > And this isn't complete even. I don'ty see tristate in here. (This looks like it's tristate to me?) > > I responded to the thread that started this flurry of conversion activity > stating that converting tests without thinking through the use-cases > isn't what we want to do. > > 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
In this series, test_bitmap is being converted to kunit test. Multiple patches will make the review process smooth. - Patch-1: Convert the tests in lib/test_bitmap.c to kunit - Patch-2: Rename the lib/test_bitmap.c to lib/bitmap_kunit.c and other configuration options - Patch-3: Remove the bitmap.sh selftest Muhammad Usama Anjum (3): bitmap: convert test_bitmap to KUnit test bitmap: Rename module selftests: lib: remove test_bitmap MAINTAINERS | 2 +- lib/Kconfig.debug | 15 +- lib/Makefile | 2 +- lib/{test_bitmap.c => bitmap_kunit.c} | 624 ++++++++++++-------------- tools/testing/selftests/lib/Makefile | 2 +- tools/testing/selftests/lib/bitmap.sh | 3 - tools/testing/selftests/lib/config | 1 - 7 files changed, 295 insertions(+), 354 deletions(-) rename lib/{test_bitmap.c => bitmap_kunit.c} (70%) delete mode 100755 tools/testing/selftests/lib/bitmap.sh