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
Convert the test_bitmap to Kunit test in-place (without moving the file so that changes can be reviewed esaily). The test has been converted from kselftest module helper functions to kunit. Following major changes were done to achieve this: - Convert the init logic - Remove all __init* as kunit tests can be called multiple times - Use KUNIT_ASSERT_* macros for determining results.
Cc: kees@kernel.org Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com --- lib/test_bitmap.c | 624 +++++++++++++++++++++------------------------- 1 file changed, 284 insertions(+), 340 deletions(-)
diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c index 65a75d58ed9e4..41c82b8339ffc 100644 --- a/lib/test_bitmap.c +++ b/lib/test_bitmap.c @@ -13,17 +13,14 @@ #include <linux/slab.h> #include <linux/string.h> #include <linux/uaccess.h> - -#include "../tools/testing/selftests/kselftest_module.h" +#include <kunit/test.h>
#define EXP1_IN_BITS (sizeof(exp1) * 8)
-KSTM_MODULE_GLOBALS(); - -static char pbl_buffer[PAGE_SIZE] __initdata; -static char print_buf[PAGE_SIZE * 2] __initdata; +static char pbl_buffer[PAGE_SIZE]; +static char print_buf[PAGE_SIZE * 2];
-static const unsigned long exp1[] __initconst = { +static const unsigned long exp1[] = { BITMAP_FROM_U64(1), BITMAP_FROM_U64(2), BITMAP_FROM_U64(0x0000ffff), @@ -41,70 +38,59 @@ static const unsigned long exp1[] __initconst = { BITMAP_FROM_U64(0x80000000), };
-static const unsigned long exp2[] __initconst = { +static const unsigned long exp2[] = { BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0xffffffff77777777ULL), };
/* Fibonacci sequence */ -static const unsigned long exp2_to_exp3_mask[] __initconst = { +static const unsigned long exp2_to_exp3_mask[] = { BITMAP_FROM_U64(0x008000020020212eULL), }; /* exp3_0_1 = (exp2[0] & ~exp2_to_exp3_mask) | (exp2[1] & exp2_to_exp3_mask) */ -static const unsigned long exp3_0_1[] __initconst = { +static const unsigned long exp3_0_1[] = { BITMAP_FROM_U64(0x33b3333311313137ULL), }; /* exp3_1_0 = (exp2[1] & ~exp2_to_exp3_mask) | (exp2[0] & exp2_to_exp3_mask) */ -static const unsigned long exp3_1_0[] __initconst = { +static const unsigned long exp3_1_0[] = { BITMAP_FROM_U64(0xff7fffff77575751ULL), };
-static bool __init -__check_eq_ulong(const char *srcfile, unsigned int line, +static void +__check_eq_ulong(struct kunit *test, const char *srcfile, unsigned int line, const unsigned long exp_ulong, unsigned long x) { - if (exp_ulong != x) { - pr_err("[%s:%u] expected %lu, got %lu\n", - srcfile, line, exp_ulong, x); - return false; - } - return true; + KUNIT_ASSERT_EQ_MSG(test, exp_ulong, x, + "[%s:%u] expected %lu, got %lu\n", + srcfile, line, exp_ulong, x); }
-static bool __init -__check_eq_bitmap(const char *srcfile, unsigned int line, +static void +__check_eq_bitmap(struct kunit *test, const char *srcfile, unsigned int line, const unsigned long *exp_bmap, const unsigned long *bmap, unsigned int nbits) { - if (!bitmap_equal(exp_bmap, bmap, nbits)) { - pr_warn("[%s:%u] bitmaps contents differ: expected "%*pbl", got "%*pbl"\n", - srcfile, line, - nbits, exp_bmap, nbits, bmap); - return false; - } - return true; + KUNIT_ASSERT_TRUE_MSG(test, bitmap_equal(exp_bmap, bmap, nbits), + "[%s:%u] bitmaps contents differ: expected "%*pbl", got "%*pbl"\n", + srcfile, line, nbits, exp_bmap, nbits, bmap); }
-static bool __init -__check_eq_pbl(const char *srcfile, unsigned int line, +static void +__check_eq_pbl(struct kunit *test, const char *srcfile, unsigned int line, const char *expected_pbl, const unsigned long *bitmap, unsigned int nbits) { snprintf(pbl_buffer, sizeof(pbl_buffer), "%*pbl", nbits, bitmap); - if (strcmp(expected_pbl, pbl_buffer)) { - pr_warn("[%s:%u] expected "%s", got "%s"\n", - srcfile, line, - expected_pbl, pbl_buffer); - return false; - } - return true; + KUNIT_ASSERT_STREQ_MSG(test, expected_pbl, pbl_buffer, + "[%s:%u] expected "%s", got "%s"\n", + srcfile, line, expected_pbl, pbl_buffer); }
-static bool __init +static bool __check_eq_u32_array(const char *srcfile, unsigned int line, const u32 *exp_arr, unsigned int exp_len, const u32 *arr, unsigned int len) __used; -static bool __init +static bool __check_eq_u32_array(const char *srcfile, unsigned int line, const u32 *exp_arr, unsigned int exp_len, const u32 *arr, unsigned int len) @@ -128,100 +114,82 @@ __check_eq_u32_array(const char *srcfile, unsigned int line, return true; }
-static bool __init __check_eq_clump8(const char *srcfile, unsigned int line, - const unsigned int offset, - const unsigned int size, - const unsigned char *const clump_exp, - const unsigned long *const clump) +static void __check_eq_clump8(struct kunit *test, const char *srcfile, unsigned int line, + const unsigned int offset, + const unsigned int size, + const unsigned char *const clump_exp, + const unsigned long *const clump) { unsigned long exp;
- if (offset >= size) { - pr_warn("[%s:%u] bit offset for clump out-of-bounds: expected less than %u, got %u\n", - srcfile, line, size, offset); - return false; - } + KUNIT_ASSERT_LE_MSG(test, offset, size, + "[%s:%u] bit offset for clump out-of-bounds: expected less than %u, got %u\n", + srcfile, line, size, offset);
exp = clump_exp[offset / 8]; - if (!exp) { - pr_warn("[%s:%u] bit offset for zero clump: expected nonzero clump, got bit offset %u with clump value 0", - srcfile, line, offset); - return false; - } - - if (*clump != exp) { - pr_warn("[%s:%u] expected clump value of 0x%lX, got clump value of 0x%lX", - srcfile, line, exp, *clump); - return false; - } + KUNIT_ASSERT_TRUE_MSG(test, exp, + "[%s:%u] bit offset for zero clump: expected nonzero clump, got bit offset %u with clump value 0", + srcfile, line, offset);
- return true; + KUNIT_ASSERT_EQ_MSG(test, *clump, exp, + "[%s:%u] expected clump value of 0x%lX, got clump value of 0x%lX", + srcfile, line, exp, *clump); }
-static bool __init -__check_eq_str(const char *srcfile, unsigned int line, - const char *exp_str, const char *str, - unsigned int len) +static void +__check_eq_str(struct kunit *test, const char *srcfile, unsigned int line, + const char *exp_str, const char *str, + unsigned int len) { - bool eq; + KUNIT_EXPECT_STREQ_MSG(test, exp_str, str, + "[%s:%u] expected %s, got %s\n", + srcfile, line, exp_str, str);
- eq = strncmp(exp_str, str, len) == 0; - if (!eq) - pr_err("[%s:%u] expected %s, got %s\n", srcfile, line, exp_str, str); - - return eq; }
-#define __expect_eq(suffix, ...) \ +#define __expect_eq(test, suffix, ...) \ ({ \ - int result = 0; \ - total_tests++; \ - if (!__check_eq_ ## suffix(__FILE__, __LINE__, \ - ##__VA_ARGS__)) { \ - failed_tests++; \ - result = 1; \ - } \ - result; \ + __check_eq_ ## suffix(test, __FILE__, __LINE__, \ + ##__VA_ARGS__); \ })
-#define expect_eq_ulong(...) __expect_eq(ulong, ##__VA_ARGS__) -#define expect_eq_uint(x, y) expect_eq_ulong((unsigned int)(x), (unsigned int)(y)) -#define expect_eq_bitmap(...) __expect_eq(bitmap, ##__VA_ARGS__) -#define expect_eq_pbl(...) __expect_eq(pbl, ##__VA_ARGS__) -#define expect_eq_u32_array(...) __expect_eq(u32_array, ##__VA_ARGS__) -#define expect_eq_clump8(...) __expect_eq(clump8, ##__VA_ARGS__) -#define expect_eq_str(...) __expect_eq(str, ##__VA_ARGS__) +#define expect_eq_ulong(test, ...) __expect_eq(test, ulong, ##__VA_ARGS__) +#define expect_eq_uint(test, x, y) expect_eq_ulong(test, (unsigned int)(x), (unsigned int)(y)) +#define expect_eq_bitmap(test, ...) __expect_eq(test, bitmap, ##__VA_ARGS__) +#define expect_eq_pbl(test, ...) __expect_eq(test, pbl, ##__VA_ARGS__) +#define expect_eq_clump8(test, ...) __expect_eq(test, clump8, ##__VA_ARGS__) +#define expect_eq_str(test, ...) __expect_eq(test, str, ##__VA_ARGS__)
-static void __init test_zero_clear(void) +static void test_zero_clear(struct kunit *test) { DECLARE_BITMAP(bmap, 1024);
/* Known way to set all bits */ memset(bmap, 0xff, 128);
- expect_eq_pbl("0-22", bmap, 23); - expect_eq_pbl("0-1023", bmap, 1024); + expect_eq_pbl(test, "0-22", bmap, 23); + expect_eq_pbl(test, "0-1023", bmap, 1024);
/* single-word bitmaps */ bitmap_clear(bmap, 0, 9); - expect_eq_pbl("9-1023", bmap, 1024); + expect_eq_pbl(test, "9-1023", bmap, 1024);
bitmap_zero(bmap, 35); - expect_eq_pbl("64-1023", bmap, 1024); + expect_eq_pbl(test, "64-1023", bmap, 1024);
/* cross boundaries operations */ bitmap_clear(bmap, 79, 19); - expect_eq_pbl("64-78,98-1023", bmap, 1024); + expect_eq_pbl(test, "64-78,98-1023", bmap, 1024);
bitmap_zero(bmap, 115); - expect_eq_pbl("128-1023", bmap, 1024); + expect_eq_pbl(test, "128-1023", bmap, 1024);
/* Zeroing entire area */ bitmap_zero(bmap, 1024); - expect_eq_pbl("", bmap, 1024); + expect_eq_pbl(test, "", bmap, 1024); }
-static void __init test_find_nth_bit(void) +static void test_find_nth_bit(struct kunit *test) { unsigned long b, bit, cnt = 0; DECLARE_BITMAP(bmap, 64 * 3); @@ -236,62 +204,62 @@ static void __init test_find_nth_bit(void) __set_bit(80, bmap); __set_bit(123, bmap);
- expect_eq_uint(10, find_nth_bit(bmap, 64 * 3, 0)); - expect_eq_uint(20, find_nth_bit(bmap, 64 * 3, 1)); - expect_eq_uint(30, find_nth_bit(bmap, 64 * 3, 2)); - expect_eq_uint(40, find_nth_bit(bmap, 64 * 3, 3)); - expect_eq_uint(50, find_nth_bit(bmap, 64 * 3, 4)); - expect_eq_uint(60, find_nth_bit(bmap, 64 * 3, 5)); - expect_eq_uint(80, find_nth_bit(bmap, 64 * 3, 6)); - expect_eq_uint(123, find_nth_bit(bmap, 64 * 3, 7)); - expect_eq_uint(0, !!(find_nth_bit(bmap, 64 * 3, 8) < 64 * 3)); - - expect_eq_uint(10, find_nth_bit(bmap, 64 * 3 - 1, 0)); - expect_eq_uint(20, find_nth_bit(bmap, 64 * 3 - 1, 1)); - expect_eq_uint(30, find_nth_bit(bmap, 64 * 3 - 1, 2)); - expect_eq_uint(40, find_nth_bit(bmap, 64 * 3 - 1, 3)); - expect_eq_uint(50, find_nth_bit(bmap, 64 * 3 - 1, 4)); - expect_eq_uint(60, find_nth_bit(bmap, 64 * 3 - 1, 5)); - expect_eq_uint(80, find_nth_bit(bmap, 64 * 3 - 1, 6)); - expect_eq_uint(123, find_nth_bit(bmap, 64 * 3 - 1, 7)); - expect_eq_uint(0, !!(find_nth_bit(bmap, 64 * 3 - 1, 8) < 64 * 3 - 1)); + expect_eq_uint(test, 10, find_nth_bit(bmap, 64 * 3, 0)); + expect_eq_uint(test, 20, find_nth_bit(bmap, 64 * 3, 1)); + expect_eq_uint(test, 30, find_nth_bit(bmap, 64 * 3, 2)); + expect_eq_uint(test, 40, find_nth_bit(bmap, 64 * 3, 3)); + expect_eq_uint(test, 50, find_nth_bit(bmap, 64 * 3, 4)); + expect_eq_uint(test, 60, find_nth_bit(bmap, 64 * 3, 5)); + expect_eq_uint(test, 80, find_nth_bit(bmap, 64 * 3, 6)); + expect_eq_uint(test, 123, find_nth_bit(bmap, 64 * 3, 7)); + expect_eq_uint(test, 0, !!(find_nth_bit(bmap, 64 * 3, 8) < 64 * 3)); + + expect_eq_uint(test, 10, find_nth_bit(bmap, 64 * 3 - 1, 0)); + expect_eq_uint(test, 20, find_nth_bit(bmap, 64 * 3 - 1, 1)); + expect_eq_uint(test, 30, find_nth_bit(bmap, 64 * 3 - 1, 2)); + expect_eq_uint(test, 40, find_nth_bit(bmap, 64 * 3 - 1, 3)); + expect_eq_uint(test, 50, find_nth_bit(bmap, 64 * 3 - 1, 4)); + expect_eq_uint(test, 60, find_nth_bit(bmap, 64 * 3 - 1, 5)); + expect_eq_uint(test, 80, find_nth_bit(bmap, 64 * 3 - 1, 6)); + expect_eq_uint(test, 123, find_nth_bit(bmap, 64 * 3 - 1, 7)); + expect_eq_uint(test, 0, !!(find_nth_bit(bmap, 64 * 3 - 1, 8) < 64 * 3 - 1));
for_each_set_bit(bit, exp1, EXP1_IN_BITS) { b = find_nth_bit(exp1, EXP1_IN_BITS, cnt++); - expect_eq_uint(b, bit); + expect_eq_uint(test, b, bit); } }
-static void __init test_fill_set(void) +static void test_fill_set(struct kunit *test) { DECLARE_BITMAP(bmap, 1024);
/* Known way to clear all bits */ memset(bmap, 0x00, 128);
- expect_eq_pbl("", bmap, 23); - expect_eq_pbl("", bmap, 1024); + expect_eq_pbl(test, "", bmap, 23); + expect_eq_pbl(test, "", bmap, 1024);
/* single-word bitmaps */ bitmap_set(bmap, 0, 9); - expect_eq_pbl("0-8", bmap, 1024); + expect_eq_pbl(test, "0-8", bmap, 1024);
bitmap_fill(bmap, 35); - expect_eq_pbl("0-63", bmap, 1024); + expect_eq_pbl(test, "0-63", bmap, 1024);
/* cross boundaries operations */ bitmap_set(bmap, 79, 19); - expect_eq_pbl("0-63,79-97", bmap, 1024); + expect_eq_pbl(test, "0-63,79-97", bmap, 1024);
bitmap_fill(bmap, 115); - expect_eq_pbl("0-127", bmap, 1024); + expect_eq_pbl(test, "0-127", bmap, 1024);
/* Zeroing entire area */ bitmap_fill(bmap, 1024); - expect_eq_pbl("0-1023", bmap, 1024); + expect_eq_pbl(test, "0-1023", bmap, 1024); }
-static void __init test_copy(void) +static void test_copy(struct kunit *test) { DECLARE_BITMAP(bmap1, 1024); DECLARE_BITMAP(bmap2, 1024); @@ -302,20 +270,20 @@ static void __init test_copy(void) /* single-word bitmaps */ bitmap_set(bmap1, 0, 19); bitmap_copy(bmap2, bmap1, 23); - expect_eq_pbl("0-18", bmap2, 1024); + expect_eq_pbl(test, "0-18", bmap2, 1024);
bitmap_set(bmap2, 0, 23); bitmap_copy(bmap2, bmap1, 23); - expect_eq_pbl("0-18", bmap2, 1024); + expect_eq_pbl(test, "0-18", bmap2, 1024);
/* multi-word bitmaps */ bitmap_set(bmap1, 0, 109); bitmap_copy(bmap2, bmap1, 1024); - expect_eq_pbl("0-108", bmap2, 1024); + expect_eq_pbl(test, "0-108", bmap2, 1024);
bitmap_fill(bmap2, 1024); bitmap_copy(bmap2, bmap1, 1024); - expect_eq_pbl("0-108", bmap2, 1024); + expect_eq_pbl(test, "0-108", bmap2, 1024);
/* the following tests assume a 32- or 64-bit arch (even 128b * if we care) @@ -323,14 +291,14 @@ static void __init test_copy(void)
bitmap_fill(bmap2, 1024); bitmap_copy(bmap2, bmap1, 109); /* ... but 0-padded til word length */ - expect_eq_pbl("0-108,128-1023", bmap2, 1024); + expect_eq_pbl(test, "0-108,128-1023", bmap2, 1024);
bitmap_fill(bmap2, 1024); bitmap_copy(bmap2, bmap1, 97); /* ... but aligned on word length */ - expect_eq_pbl("0-108,128-1023", bmap2, 1024); + expect_eq_pbl(test, "0-108,128-1023", bmap2, 1024); }
-static void __init test_bitmap_region(void) +static void test_bitmap_region(struct kunit *test) { int pos, order;
@@ -341,21 +309,21 @@ static void __init test_bitmap_region(void) for (order = 0; order < 10; order++) { pos = bitmap_find_free_region(bmap, 1000, order); if (order == 0) - expect_eq_uint(pos, 0); + expect_eq_uint(test, pos, 0); else - expect_eq_uint(pos, order < 9 ? BIT(order) : -ENOMEM); + expect_eq_uint(test, pos, order < 9 ? BIT(order) : -ENOMEM); }
bitmap_release_region(bmap, 0, 0); for (order = 1; order < 9; order++) bitmap_release_region(bmap, BIT(order), order);
- expect_eq_uint(bitmap_weight(bmap, 1000), 0); + expect_eq_uint(test, bitmap_weight(bmap, 1000), 0); }
#define EXP2_IN_BITS (sizeof(exp2) * 8)
-static void __init test_replace(void) +static void test_replace(struct kunit *test) { unsigned int nbits = 64; unsigned int nlongs = DIV_ROUND_UP(nbits, BITS_PER_LONG); @@ -365,38 +333,38 @@ static void __init test_replace(void)
bitmap_zero(bmap, 1024); bitmap_replace(bmap, &exp2[0 * nlongs], &exp2[1 * nlongs], exp2_to_exp3_mask, nbits); - expect_eq_bitmap(bmap, exp3_0_1, nbits); + expect_eq_bitmap(test, bmap, exp3_0_1, nbits);
bitmap_zero(bmap, 1024); bitmap_replace(bmap, &exp2[1 * nlongs], &exp2[0 * nlongs], exp2_to_exp3_mask, nbits); - expect_eq_bitmap(bmap, exp3_1_0, nbits); + expect_eq_bitmap(test, bmap, exp3_1_0, nbits);
bitmap_fill(bmap, 1024); bitmap_replace(bmap, &exp2[0 * nlongs], &exp2[1 * nlongs], exp2_to_exp3_mask, nbits); - expect_eq_bitmap(bmap, exp3_0_1, nbits); + expect_eq_bitmap(test, bmap, exp3_0_1, nbits);
bitmap_fill(bmap, 1024); bitmap_replace(bmap, &exp2[1 * nlongs], &exp2[0 * nlongs], exp2_to_exp3_mask, nbits); - expect_eq_bitmap(bmap, exp3_1_0, nbits); + expect_eq_bitmap(test, bmap, exp3_1_0, nbits); }
-static const unsigned long sg_mask[] __initconst = { +static const unsigned long sg_mask[] = { BITMAP_FROM_U64(0x000000000000035aULL), };
-static const unsigned long sg_src[] __initconst = { +static const unsigned long sg_src[] = { BITMAP_FROM_U64(0x0000000000000667ULL), };
-static const unsigned long sg_gather_exp[] __initconst = { +static const unsigned long sg_gather_exp[] = { BITMAP_FROM_U64(0x0000000000000029ULL), };
-static const unsigned long sg_scatter_exp[] __initconst = { +static const unsigned long sg_scatter_exp[] = { BITMAP_FROM_U64(0x000000000000021aULL), };
-static void __init test_bitmap_sg(void) +static void test_bitmap_sg(struct kunit *test) { unsigned int nbits = 64; DECLARE_BITMAP(bmap_gather, 100); @@ -407,18 +375,18 @@ static void __init test_bitmap_sg(void) /* Simple gather call */ bitmap_zero(bmap_gather, 100); bitmap_gather(bmap_gather, sg_src, sg_mask, nbits); - expect_eq_bitmap(sg_gather_exp, bmap_gather, nbits); + expect_eq_bitmap(test, sg_gather_exp, bmap_gather, nbits);
/* Simple scatter call */ bitmap_zero(bmap_scatter, 100); bitmap_scatter(bmap_scatter, sg_src, sg_mask, nbits); - expect_eq_bitmap(sg_scatter_exp, bmap_scatter, nbits); + expect_eq_bitmap(test, sg_scatter_exp, bmap_scatter, nbits);
/* Scatter/gather relationship */ bitmap_zero(bmap_tmp, 100); bitmap_gather(bmap_tmp, bmap_scatter, sg_mask, nbits); bitmap_scatter(bmap_res, bmap_tmp, sg_mask, nbits); - expect_eq_bitmap(bmap_scatter, bmap_res, nbits); + expect_eq_bitmap(test, bmap_scatter, bmap_res, nbits); }
#define PARSE_TIME 0x1 @@ -432,7 +400,7 @@ struct test_bitmap_parselist{ const int flags; };
-static const struct test_bitmap_parselist parselist_tests[] __initconst = { +static const struct test_bitmap_parselist parselist_tests[] = { #define step (sizeof(u64) / sizeof(unsigned long))
{0, "0", &exp1[0], 8, 0}, @@ -517,7 +485,7 @@ static const struct test_bitmap_parselist parselist_tests[] __initconst = {
};
-static void __init test_bitmap_parselist(void) +static void test_bitmap_parselist(struct kunit *test) { int i; int err; @@ -531,21 +499,14 @@ static void __init test_bitmap_parselist(void) err = bitmap_parselist(ptest.in, bmap, ptest.nbits); time = ktime_get() - time;
- if (err != ptest.errno) { - pr_err("parselist: %d: input is %s, errno is %d, expected %d\n", - i, ptest.in, err, ptest.errno); - failed_tests++; - continue; - } + KUNIT_ASSERT_EQ_MSG(test, err, ptest.errno, + "parselist: %d: input is %s, errno is %d, expected %d\n", + i, ptest.in, err, ptest.errno);
- if (!err && ptest.expected - && !__bitmap_equal(bmap, ptest.expected, ptest.nbits)) { - pr_err("parselist: %d: input is %s, result is 0x%lx, expected 0x%lx\n", - i, ptest.in, bmap[0], - *ptest.expected); - failed_tests++; - continue; - } + KUNIT_ASSERT_FALSE_MSG(test, !err && ptest.expected && + !__bitmap_equal(bmap, ptest.expected, ptest.nbits), + "parselist: %d: input is %s, result is 0x%lx, expected 0x%lx\n", + i, ptest.in, bmap[0], *ptest.expected);
if (ptest.flags & PARSE_TIME) pr_info("parselist: %d: input is '%s' OK, Time: %llu\n", @@ -555,7 +516,7 @@ static void __init test_bitmap_parselist(void) } }
-static void __init test_bitmap_printlist(void) +static void test_bitmap_printlist(struct kunit *test) { unsigned long *bmap = kmalloc(PAGE_SIZE, GFP_KERNEL); char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL); @@ -575,17 +536,13 @@ static void __init test_bitmap_printlist(void) ret = bitmap_print_to_pagebuf(true, buf, bmap, PAGE_SIZE * 8); time = ktime_get() - time;
- if (ret != slen + 1) { - pr_err("bitmap_print_to_pagebuf: result is %d, expected %d\n", ret, slen); - failed_tests++; - goto out; - } + KUNIT_ASSERT_EQ_MSG(test, ret, slen + 1, + "bitmap_print_to_pagebuf: result is %d, expected %d\n", + ret, slen);
- if (strncmp(buf, expected, slen)) { - pr_err("bitmap_print_to_pagebuf: result is %s, expected %s\n", buf, expected); - failed_tests++; - goto out; - } + KUNIT_ASSERT_FALSE_MSG(test, strncmp(buf, expected, slen), + "bitmap_print_to_pagebuf: result is %s, expected %s\n", + buf, expected);
pr_info("bitmap_print_to_pagebuf: input is '%s', Time: %llu\n", buf, time); out: @@ -593,20 +550,20 @@ static void __init test_bitmap_printlist(void) kfree(bmap); }
-static const unsigned long parse_test[] __initconst = { +static const unsigned long parse_test[] = { BITMAP_FROM_U64(0), BITMAP_FROM_U64(1), BITMAP_FROM_U64(0xdeadbeef), BITMAP_FROM_U64(0x100000000ULL), };
-static const unsigned long parse_test2[] __initconst = { +static const unsigned long parse_test2[] = { BITMAP_FROM_U64(0x100000000ULL), BITMAP_FROM_U64(0xdeadbeef), BITMAP_FROM_U64(0x100000000ULL), BITMAP_FROM_U64(0xbaadf00ddeadbeef), BITMAP_FROM_U64(0x100000000ULL), BITMAP_FROM_U64(0x0badf00ddeadbeef), };
-static const struct test_bitmap_parselist parse_tests[] __initconst = { +static const struct test_bitmap_parselist parse_tests[] = { {0, "", &parse_test[0 * step], 32, 0}, {0, " ", &parse_test[0 * step], 32, 0}, {0, "0", &parse_test[0 * step], 32, 0}, @@ -633,7 +590,7 @@ static const struct test_bitmap_parselist parse_tests[] __initconst = { #undef step };
-static void __init test_bitmap_parse(void) +static void test_bitmap_parse(struct kunit *_test) { int i; int err; @@ -648,21 +605,14 @@ static void __init test_bitmap_parse(void) err = bitmap_parse(test.in, len, bmap, test.nbits); time = ktime_get() - time;
- if (err != test.errno) { - pr_err("parse: %d: input is %s, errno is %d, expected %d\n", - i, test.in, err, test.errno); - failed_tests++; - continue; - } + KUNIT_ASSERT_EQ_MSG(_test, err, test.errno, + "parse: %d: input is %s, errno is %d, expected %d\n", + i, test.in, err, test.errno);
- if (!err && test.expected - && !__bitmap_equal(bmap, test.expected, test.nbits)) { - pr_err("parse: %d: input is %s, result is 0x%lx, expected 0x%lx\n", - i, test.in, bmap[0], - *test.expected); - failed_tests++; - continue; - } + KUNIT_ASSERT_FALSE_MSG(_test, !err && test.expected && + !__bitmap_equal(bmap, test.expected, test.nbits), + "parse: %d: input is %s, result is 0x%lx, expected 0x%lx\n", + i, test.in, bmap[0], *test.expected);
if (test.flags & PARSE_TIME) pr_info("parse: %d: input is '%s' OK, Time: %llu\n", @@ -670,7 +620,7 @@ static void __init test_bitmap_parse(void) } }
-static void __init test_bitmap_arr32(void) +static void test_bitmap_arr32(struct kunit *test) { unsigned int nbits, next_bit; u32 arr[EXP1_IN_BITS / 32]; @@ -681,24 +631,22 @@ static void __init test_bitmap_arr32(void) for (nbits = 0; nbits < EXP1_IN_BITS; ++nbits) { bitmap_to_arr32(arr, exp1, nbits); bitmap_from_arr32(bmap2, arr, nbits); - expect_eq_bitmap(bmap2, exp1, nbits); + expect_eq_bitmap(test, bmap2, exp1, nbits);
next_bit = find_next_bit(bmap2, round_up(nbits, BITS_PER_LONG), nbits); - if (next_bit < round_up(nbits, BITS_PER_LONG)) { - pr_err("bitmap_copy_arr32(nbits == %d:" - " tail is not safely cleared: %d\n", - nbits, next_bit); - failed_tests++; - } + + KUNIT_ASSERT_GE_MSG(test, next_bit, round_up(nbits, BITS_PER_LONG), + "bitmap_copy_arr32(nbits == %d: tail is not safely cleared: %d\n", + nbits, next_bit);
if (nbits < EXP1_IN_BITS - 32) - expect_eq_uint(arr[DIV_ROUND_UP(nbits, 32)], - 0xa5a5a5a5); + expect_eq_uint(test, arr[DIV_ROUND_UP(nbits, 32)], + 0xa5a5a5a5); } }
-static void __init test_bitmap_arr64(void) +static void test_bitmap_arr64(struct kunit *test) { unsigned int nbits, next_bit; u64 arr[EXP1_IN_BITS / 64]; @@ -710,29 +658,25 @@ static void __init test_bitmap_arr64(void) memset(bmap2, 0xff, sizeof(arr)); bitmap_to_arr64(arr, exp1, nbits); bitmap_from_arr64(bmap2, arr, nbits); - expect_eq_bitmap(bmap2, exp1, nbits); + expect_eq_bitmap(test, bmap2, exp1, nbits);
next_bit = find_next_bit(bmap2, round_up(nbits, BITS_PER_LONG), nbits); - if (next_bit < round_up(nbits, BITS_PER_LONG)) { - pr_err("bitmap_copy_arr64(nbits == %d:" - " tail is not safely cleared: %d\n", nbits, next_bit); - failed_tests++; - } + KUNIT_ASSERT_GE_MSG(test, next_bit, round_up(nbits, BITS_PER_LONG), + "bitmap_copy_arr64(nbits == %d: tail is not safely cleared: %d\n", + nbits, next_bit);
- if ((nbits % 64) && - (arr[(nbits - 1) / 64] & ~GENMASK_ULL((nbits - 1) % 64, 0))) { - pr_err("bitmap_to_arr64(nbits == %d): tail is not safely cleared: 0x%016llx (must be 0x%016llx)\n", - nbits, arr[(nbits - 1) / 64], - GENMASK_ULL((nbits - 1) % 64, 0)); - failed_tests++; - } + KUNIT_ASSERT_FALSE_MSG(test, (nbits % 64) && + (arr[(nbits - 1) / 64] & ~GENMASK_ULL((nbits - 1) % 64, 0)), + "bitmap_to_arr64(nbits == %d): tail is not safely cleared: 0x%016llx (must be 0x%016llx)\n", + nbits, arr[(nbits - 1) / 64], + GENMASK_ULL((nbits - 1) % 64, 0));
if (nbits < EXP1_IN_BITS - 64) - expect_eq_uint(arr[DIV_ROUND_UP(nbits, 64)], 0xa5a5a5a5); + expect_eq_uint(test, arr[DIV_ROUND_UP(nbits, 64)], 0xa5a5a5a5); } }
-static void noinline __init test_mem_optimisations(void) +static noinline void test_mem_optimisations(struct kunit *test) { DECLARE_BITMAP(bmap1, 1024); DECLARE_BITMAP(bmap2, 1024); @@ -745,31 +689,26 @@ static void noinline __init test_mem_optimisations(void)
bitmap_set(bmap1, start, nbits); __bitmap_set(bmap2, start, nbits); - if (!bitmap_equal(bmap1, bmap2, 1024)) { - printk("set not equal %d %d\n", start, nbits); - failed_tests++; - } - if (!__bitmap_equal(bmap1, bmap2, 1024)) { - printk("set not __equal %d %d\n", start, nbits); - failed_tests++; - } + + KUNIT_ASSERT_TRUE_MSG(test, bitmap_equal(bmap1, bmap2, 1024), + "set not equal %d %d\n", start, nbits); + + KUNIT_ASSERT_TRUE_MSG(test, __bitmap_equal(bmap1, bmap2, 1024), + "set not __equal %d %d\n", start, nbits);
bitmap_clear(bmap1, start, nbits); __bitmap_clear(bmap2, start, nbits); - if (!bitmap_equal(bmap1, bmap2, 1024)) { - printk("clear not equal %d %d\n", start, nbits); - failed_tests++; - } - if (!__bitmap_equal(bmap1, bmap2, 1024)) { - printk("clear not __equal %d %d\n", start, - nbits); - failed_tests++; - } + + KUNIT_ASSERT_TRUE_MSG(test, bitmap_equal(bmap1, bmap2, 1024), + "clear not equal %d %d\n", start, nbits); + + KUNIT_ASSERT_TRUE_MSG(test, __bitmap_equal(bmap1, bmap2, 1024), + "clear not __equal %d %d\n", start, nbits); } } }
-static const unsigned char clump_exp[] __initconst = { +static const unsigned char clump_exp[] = { 0x01, /* 1 bit set */ 0x02, /* non-edge 1 bit set */ 0x00, /* zero bits set */ @@ -780,7 +719,7 @@ static const unsigned char clump_exp[] __initconst = { 0x05, /* non-adjacent 2 bits set */ };
-static void __init test_for_each_set_clump8(void) +static void test_for_each_set_clump8(struct kunit *test) { #define CLUMP_EXP_NUMBITS 64 DECLARE_BITMAP(bits, CLUMP_EXP_NUMBITS); @@ -799,10 +738,10 @@ static void __init test_for_each_set_clump8(void) bitmap_set(bits, 58, 1); /* 0x05 - part 2 */
for_each_set_clump8(start, clump, bits, CLUMP_EXP_NUMBITS) - expect_eq_clump8(start, CLUMP_EXP_NUMBITS, clump_exp, &clump); + expect_eq_clump8(test, start, CLUMP_EXP_NUMBITS, clump_exp, &clump); }
-static void __init test_for_each_set_bit_wrap(void) +static void test_for_each_set_bit_wrap(struct kunit *test) { DECLARE_BITMAP(orig, 500); DECLARE_BITMAP(copy, 500); @@ -823,11 +762,11 @@ static void __init test_for_each_set_bit_wrap(void) for_each_set_bit_wrap(bit, orig, 500, wr) bitmap_set(copy, bit, 1);
- expect_eq_bitmap(orig, copy, 500); + expect_eq_bitmap(test, orig, copy, 500); } }
-static void __init test_for_each_set_bit(void) +static void test_for_each_set_bit(struct kunit *test) { DECLARE_BITMAP(orig, 500); DECLARE_BITMAP(copy, 500); @@ -846,10 +785,10 @@ static void __init test_for_each_set_bit(void) for_each_set_bit(bit, orig, 500) bitmap_set(copy, bit, 1);
- expect_eq_bitmap(orig, copy, 500); + expect_eq_bitmap(test, orig, copy, 500); }
-static void __init test_for_each_set_bit_from(void) +static void test_for_each_set_bit_from(struct kunit *test) { DECLARE_BITMAP(orig, 500); DECLARE_BITMAP(copy, 500); @@ -875,11 +814,11 @@ static void __init test_for_each_set_bit_from(void)
bitmap_copy(tmp, orig, 500); bitmap_clear(tmp, 0, wr); - expect_eq_bitmap(tmp, copy, 500); + expect_eq_bitmap(test, tmp, copy, 500); } }
-static void __init test_for_each_clear_bit(void) +static void test_for_each_clear_bit(struct kunit *test) { DECLARE_BITMAP(orig, 500); DECLARE_BITMAP(copy, 500); @@ -898,10 +837,10 @@ static void __init test_for_each_clear_bit(void) for_each_clear_bit(bit, orig, 500) bitmap_clear(copy, bit, 1);
- expect_eq_bitmap(orig, copy, 500); + expect_eq_bitmap(test, orig, copy, 500); }
-static void __init test_for_each_clear_bit_from(void) +static void test_for_each_clear_bit_from(struct kunit *test) { DECLARE_BITMAP(orig, 500); DECLARE_BITMAP(copy, 500); @@ -927,11 +866,11 @@ static void __init test_for_each_clear_bit_from(void)
bitmap_copy(tmp, orig, 500); bitmap_set(tmp, 0, wr); - expect_eq_bitmap(tmp, copy, 500); + expect_eq_bitmap(test, tmp, copy, 500); } }
-static void __init test_for_each_set_bitrange(void) +static void test_for_each_set_bitrange(struct kunit *test) { DECLARE_BITMAP(orig, 500); DECLARE_BITMAP(copy, 500); @@ -950,10 +889,10 @@ static void __init test_for_each_set_bitrange(void) for_each_set_bitrange(s, e, orig, 500) bitmap_set(copy, s, e-s);
- expect_eq_bitmap(orig, copy, 500); + expect_eq_bitmap(test, orig, copy, 500); }
-static void __init test_for_each_clear_bitrange(void) +static void test_for_each_clear_bitrange(struct kunit *test) { DECLARE_BITMAP(orig, 500); DECLARE_BITMAP(copy, 500); @@ -972,10 +911,10 @@ static void __init test_for_each_clear_bitrange(void) for_each_clear_bitrange(s, e, orig, 500) bitmap_clear(copy, s, e-s);
- expect_eq_bitmap(orig, copy, 500); + expect_eq_bitmap(test, orig, copy, 500); }
-static void __init test_for_each_set_bitrange_from(void) +static void test_for_each_set_bitrange_from(struct kunit *test) { DECLARE_BITMAP(orig, 500); DECLARE_BITMAP(copy, 500); @@ -1001,11 +940,11 @@ static void __init test_for_each_set_bitrange_from(void)
bitmap_copy(tmp, orig, 500); bitmap_clear(tmp, 0, wr); - expect_eq_bitmap(tmp, copy, 500); + expect_eq_bitmap(test, tmp, copy, 500); } }
-static void __init test_for_each_clear_bitrange_from(void) +static void test_for_each_clear_bitrange_from(struct kunit *test) { DECLARE_BITMAP(orig, 500); DECLARE_BITMAP(copy, 500); @@ -1031,7 +970,7 @@ static void __init test_for_each_clear_bitrange_from(void)
bitmap_copy(tmp, orig, 500); bitmap_set(tmp, 0, wr); - expect_eq_bitmap(tmp, copy, 500); + expect_eq_bitmap(test, tmp, copy, 500); } }
@@ -1076,7 +1015,7 @@ static struct test_bitmap_cut test_cut[] = { }, };
-static void __init test_bitmap_cut(void) +static void test_bitmap_cut(struct kunit *test) { unsigned long b[5], *in = &b[1], *out = &b[0]; /* Partial overlap */ int i; @@ -1088,7 +1027,7 @@ static void __init test_bitmap_cut(void)
bitmap_cut(out, in, t->first, t->cut, t->nbits);
- expect_eq_bitmap(t->expected, out, t->nbits); + expect_eq_bitmap(test, t->expected, out, t->nbits); } }
@@ -1099,14 +1038,14 @@ struct test_bitmap_print { const char *list; };
-static const unsigned long small_bitmap[] __initconst = { +static const unsigned long small_bitmap[] = { BITMAP_FROM_U64(0x3333333311111111ULL), };
-static const char small_mask[] __initconst = "33333333,11111111\n"; -static const char small_list[] __initconst = "0,4,8,12,16,20,24,28,32-33,36-37,40-41,44-45,48-49,52-53,56-57,60-61\n"; +static const char small_mask[] = "33333333,11111111\n"; +static const char small_list[] = "0,4,8,12,16,20,24,28,32-33,36-37,40-41,44-45,48-49,52-53,56-57,60-61\n";
-static const unsigned long large_bitmap[] __initconst = { +static const unsigned long large_bitmap[] = { BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL), @@ -1129,28 +1068,28 @@ static const unsigned long large_bitmap[] __initconst = { BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL), };
-static const char large_mask[] __initconst = "33333333,11111111,33333333,11111111," - "33333333,11111111,33333333,11111111," - "33333333,11111111,33333333,11111111," - "33333333,11111111,33333333,11111111," - "33333333,11111111,33333333,11111111," - "33333333,11111111,33333333,11111111," - "33333333,11111111,33333333,11111111," - "33333333,11111111,33333333,11111111," - "33333333,11111111,33333333,11111111," - "33333333,11111111,33333333,11111111," - "33333333,11111111,33333333,11111111," - "33333333,11111111,33333333,11111111," - "33333333,11111111,33333333,11111111," - "33333333,11111111,33333333,11111111," - "33333333,11111111,33333333,11111111," - "33333333,11111111,33333333,11111111," - "33333333,11111111,33333333,11111111," - "33333333,11111111,33333333,11111111," - "33333333,11111111,33333333,11111111," - "33333333,11111111,33333333,11111111\n"; - -static const char large_list[] __initconst = /* more than 4KB */ +static const char large_mask[] = "33333333,11111111,33333333,11111111," + "33333333,11111111,33333333,11111111," + "33333333,11111111,33333333,11111111," + "33333333,11111111,33333333,11111111," + "33333333,11111111,33333333,11111111," + "33333333,11111111,33333333,11111111," + "33333333,11111111,33333333,11111111," + "33333333,11111111,33333333,11111111," + "33333333,11111111,33333333,11111111," + "33333333,11111111,33333333,11111111," + "33333333,11111111,33333333,11111111," + "33333333,11111111,33333333,11111111," + "33333333,11111111,33333333,11111111," + "33333333,11111111,33333333,11111111," + "33333333,11111111,33333333,11111111," + "33333333,11111111,33333333,11111111," + "33333333,11111111,33333333,11111111," + "33333333,11111111,33333333,11111111," + "33333333,11111111,33333333,11111111," + "33333333,11111111,33333333,11111111\n"; + +static const char large_list[] = /* more than 4KB */ "0,4,8,12,16,20,24,28,32-33,36-37,40-41,44-45,48-49,52-53,56-57,60-61,64,68,72,76,80,84,88,92,96-97,100-101,104-1" "05,108-109,112-113,116-117,120-121,124-125,128,132,136,140,144,148,152,156,160-161,164-165,168-169,172-173,176-1" "77,180-181,184-185,188-189,192,196,200,204,208,212,216,220,224-225,228-229,232-233,236-237,240-241,244-245,248-2" @@ -1192,12 +1131,12 @@ static const char large_list[] __initconst = /* more than 4KB */ "2489,2492-2493,2496,2500,2504,2508,2512,2516,2520,2524,2528-2529,2532-2533,2536-2537,2540-2541,2544-2545,2548-25" "49,2552-2553,2556-2557\n";
-static const struct test_bitmap_print test_print[] __initconst = { +static const struct test_bitmap_print test_print[] = { { small_bitmap, sizeof(small_bitmap) * BITS_PER_BYTE, small_mask, small_list }, { large_bitmap, sizeof(large_bitmap) * BITS_PER_BYTE, large_mask, large_list }, };
-static void __init test_bitmap_print_buf(void) +static void test_bitmap_print_buf(struct kunit *test) { int i;
@@ -1207,20 +1146,20 @@ static void __init test_bitmap_print_buf(void)
n = bitmap_print_bitmask_to_buf(print_buf, t->bitmap, t->nbits, 0, 2 * PAGE_SIZE); - expect_eq_uint(strlen(t->mask) + 1, n); - expect_eq_str(t->mask, print_buf, n); + expect_eq_uint(test, strlen(t->mask) + 1, n); + expect_eq_str(test, t->mask, print_buf, n);
n = bitmap_print_list_to_buf(print_buf, t->bitmap, t->nbits, 0, 2 * PAGE_SIZE); - expect_eq_uint(strlen(t->list) + 1, n); - expect_eq_str(t->list, print_buf, n); + expect_eq_uint(test, strlen(t->list) + 1, n); + expect_eq_str(test, t->list, print_buf, n);
/* test by non-zero offset */ if (strlen(t->list) > PAGE_SIZE) { n = bitmap_print_list_to_buf(print_buf, t->bitmap, t->nbits, PAGE_SIZE, PAGE_SIZE); - expect_eq_uint(strlen(t->list) + 1 - PAGE_SIZE, n); - expect_eq_str(t->list + PAGE_SIZE, print_buf, n); + expect_eq_uint(test, strlen(t->list) + 1 - PAGE_SIZE, n); + expect_eq_str(test, t->list + PAGE_SIZE, print_buf, n); } } } @@ -1229,7 +1168,7 @@ static void __init test_bitmap_print_buf(void) * FIXME: Clang breaks compile-time evaluations when KASAN and GCOV are enabled. * To workaround it, GCOV is force-disabled in Makefile for this configuration. */ -static void __init test_bitmap_const_eval(void) +static void test_bitmap_const_eval(struct kunit *test) { DECLARE_BITMAP(bitmap, BITS_PER_LONG); unsigned long initvar = BIT(2); @@ -1297,7 +1236,7 @@ static void __init test_bitmap_const_eval(void) /* * Helper function to test bitmap_write() overwriting the chosen byte pattern. */ -static void __init test_bitmap_write_helper(const char *pattern) +static void test_bitmap_write_helper(struct kunit *test, const char *pattern) { DECLARE_BITMAP(bitmap, TEST_BIT_LEN); DECLARE_BITMAP(exp_bitmap, TEST_BIT_LEN); @@ -1321,7 +1260,7 @@ static void __init test_bitmap_write_helper(const char *pattern) for (bit = 0; bit <= 1; bit++) { bitmap_write(bitmap, bit, i, 1); __assign_bit(i, exp_bitmap, bit); - expect_eq_bitmap(exp_bitmap, bitmap, + expect_eq_bitmap(test, exp_bitmap, bitmap, TEST_BIT_LEN); } } @@ -1331,7 +1270,7 @@ static void __init test_bitmap_write_helper(const char *pattern) bitmap_copy(exp_bitmap, pat_bitmap, TEST_BIT_LEN); for (i = 0; i < TEST_BIT_LEN; i++) { bitmap_write(bitmap, ~0UL, i, 0); - expect_eq_bitmap(exp_bitmap, bitmap, TEST_BIT_LEN); + expect_eq_bitmap(test, exp_bitmap, bitmap, TEST_BIT_LEN); }
for (nbits = BITS_PER_LONG; nbits >= 1; nbits--) { @@ -1344,14 +1283,14 @@ static void __init test_bitmap_write_helper(const char *pattern) for (n = 0; n < nbits; n++) __assign_bit(i + n, exp_bitmap, w & BIT(n)); bitmap_write(bitmap, w, i, nbits); - expect_eq_bitmap(exp_bitmap, bitmap, TEST_BIT_LEN); + expect_eq_bitmap(test, exp_bitmap, bitmap, TEST_BIT_LEN); r = bitmap_read(bitmap, i, nbits); - expect_eq_ulong(r, w); + expect_eq_ulong(test, r, w); } } }
-static void __init test_bitmap_read_write(void) +static void test_bitmap_read_write(struct kunit *test) { unsigned char *pattern[3] = {"", "all:1/2", "all"}; DECLARE_BITMAP(bitmap, TEST_BIT_LEN); @@ -1386,21 +1325,21 @@ static void __init test_bitmap_read_write(void)
bitmap_write(bitmap, 0b10101UL, i, 5); val = bitmap_read(bitmap, i, 5); - expect_eq_ulong(0b10101UL, val); + expect_eq_ulong(test, 0b10101UL, val);
bitmap_write(bitmap, 0b101UL, i + 5, 3); val = bitmap_read(bitmap, i + 5, 3); - expect_eq_ulong(0b101UL, val); + expect_eq_ulong(test, 0b101UL, val);
val = bitmap_read(bitmap, i, 8); - expect_eq_ulong(0b10110101UL, val); + expect_eq_ulong(test, 0b10110101UL, val); }
for (pi = 0; pi < ARRAY_SIZE(pattern); pi++) - test_bitmap_write_helper(pattern[pi]); + test_bitmap_write_helper(test, pattern[pi]); }
-static void __init test_bitmap_read_perf(void) +static void test_bitmap_read_perf(struct kunit *test) { DECLARE_BITMAP(bitmap, TEST_BIT_LEN); unsigned int cnt, nbits, i; @@ -1426,7 +1365,7 @@ static void __init test_bitmap_read_perf(void) pr_info("Time spent in %s:\t%llu\n", __func__, time); }
-static void __init test_bitmap_write_perf(void) +static void test_bitmap_write_perf(struct kunit *test) { DECLARE_BITMAP(bitmap, TEST_BIT_LEN); unsigned int cnt, nbits, i; @@ -1450,41 +1389,46 @@ static void __init test_bitmap_write_perf(void)
#undef TEST_BIT_LEN
-static void __init selftest(void) -{ - test_zero_clear(); - test_fill_set(); - test_copy(); - test_bitmap_region(); - test_replace(); - test_bitmap_sg(); - test_bitmap_arr32(); - test_bitmap_arr64(); - test_bitmap_parse(); - test_bitmap_parselist(); - test_bitmap_printlist(); - test_mem_optimisations(); - test_bitmap_cut(); - test_bitmap_print_buf(); - test_bitmap_const_eval(); - test_bitmap_read_write(); - test_bitmap_read_perf(); - test_bitmap_write_perf(); - - test_find_nth_bit(); - test_for_each_set_bit(); - test_for_each_set_bit_from(); - test_for_each_clear_bit(); - test_for_each_clear_bit_from(); - test_for_each_set_bitrange(); - test_for_each_clear_bitrange(); - test_for_each_set_bitrange_from(); - test_for_each_clear_bitrange_from(); - test_for_each_set_clump8(); - test_for_each_set_bit_wrap(); -} +static struct kunit_case bitmap_test_cases[] = { + KUNIT_CASE(test_zero_clear), + KUNIT_CASE(test_fill_set), + KUNIT_CASE(test_copy), + KUNIT_CASE(test_bitmap_region), + KUNIT_CASE(test_replace), + KUNIT_CASE(test_bitmap_sg), + KUNIT_CASE(test_bitmap_arr32), + KUNIT_CASE(test_bitmap_arr64), + KUNIT_CASE(test_bitmap_parse), + KUNIT_CASE(test_bitmap_parselist), + KUNIT_CASE(test_bitmap_printlist), + KUNIT_CASE(test_mem_optimisations), + KUNIT_CASE(test_bitmap_cut), + KUNIT_CASE(test_bitmap_print_buf), + KUNIT_CASE(test_bitmap_const_eval), + KUNIT_CASE(test_bitmap_read_write), + KUNIT_CASE(test_bitmap_read_perf), + KUNIT_CASE(test_bitmap_write_perf), + + KUNIT_CASE(test_find_nth_bit), + KUNIT_CASE(test_for_each_set_bit), + KUNIT_CASE(test_for_each_set_bit_from), + KUNIT_CASE(test_for_each_clear_bit), + KUNIT_CASE(test_for_each_clear_bit_from), + KUNIT_CASE(test_for_each_set_bitrange), + KUNIT_CASE(test_for_each_clear_bitrange), + KUNIT_CASE(test_for_each_set_bitrange_from), + KUNIT_CASE(test_for_each_clear_bitrange_from), + KUNIT_CASE(test_for_each_set_clump8), + KUNIT_CASE(test_for_each_set_bit_wrap), + {} +}; + +static struct kunit_suite bitmap_test_suite = { + .name = "bitmap", + .test_cases = bitmap_test_cases, +};
-KSTM_MODULE_LOADERS(test_bitmap); +kunit_test_suite(bitmap_test_suite); MODULE_AUTHOR("david decotigny david.decotigny@googlers.com"); MODULE_DESCRIPTION("Test cases for bitmap API"); MODULE_LICENSE("GPL");
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. - 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
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 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/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 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 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 forbitmaps 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/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 forbitmaps 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 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 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 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 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:
- 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.
- 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,
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
Remove the test_bitmap as it has been converted to kunit test.
Cc: kees@kernel.org Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com --- tools/testing/selftests/lib/Makefile | 2 +- tools/testing/selftests/lib/bitmap.sh | 3 --- tools/testing/selftests/lib/config | 1 - 3 files changed, 1 insertion(+), 5 deletions(-) delete mode 100755 tools/testing/selftests/lib/bitmap.sh
diff --git a/tools/testing/selftests/lib/Makefile b/tools/testing/selftests/lib/Makefile index ee71fc99d5b51..386c5887c0d65 100644 --- a/tools/testing/selftests/lib/Makefile +++ b/tools/testing/selftests/lib/Makefile @@ -4,6 +4,6 @@ # No binaries, but make sure arg-less "make" doesn't trigger "run_tests" all:
-TEST_PROGS := printf.sh bitmap.sh prime_numbers.sh scanf.sh strscpy.sh +TEST_PROGS := printf.sh prime_numbers.sh scanf.sh strscpy.sh
include ../lib.mk diff --git a/tools/testing/selftests/lib/bitmap.sh b/tools/testing/selftests/lib/bitmap.sh deleted file mode 100755 index 00a416fbc0ef0..0000000000000 --- a/tools/testing/selftests/lib/bitmap.sh +++ /dev/null @@ -1,3 +0,0 @@ -#!/bin/sh -# SPDX-License-Identifier: GPL-2.0 -$(dirname $0)/../kselftest/module.sh "bitmap" test_bitmap diff --git a/tools/testing/selftests/lib/config b/tools/testing/selftests/lib/config index 645839b50b0a2..7d3b1de29d3d6 100644 --- a/tools/testing/selftests/lib/config +++ b/tools/testing/selftests/lib/config @@ -1,6 +1,5 @@ CONFIG_TEST_PRINTF=m CONFIG_TEST_SCANF=m -CONFIG_TEST_BITMAP=m CONFIG_PRIME_NUMBERS=m CONFIG_TEST_STRSCPY=m CONFIG_TEST_BITOPS=m
On 7/26/24 05:06, Muhammad Usama Anjum wrote:
Remove the test_bitmap as it has been converted to kunit test.
Care to give some commit information on this change? This change will take the ability away to run bitmap tests during boot - as it will now be dependent on kunit
Why are we making changing like this without thinking through?
Cc: kees@kernel.org Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com
tools/testing/selftests/lib/Makefile | 2 +- tools/testing/selftests/lib/bitmap.sh | 3 --- tools/testing/selftests/lib/config | 1 - 3 files changed, 1 insertion(+), 5 deletions(-) delete mode 100755 tools/testing/selftests/lib/bitmap.sh
diff --git a/tools/testing/selftests/lib/Makefile b/tools/testing/selftests/lib/Makefile index ee71fc99d5b51..386c5887c0d65 100644 --- a/tools/testing/selftests/lib/Makefile +++ b/tools/testing/selftests/lib/Makefile @@ -4,6 +4,6 @@ # No binaries, but make sure arg-less "make" doesn't trigger "run_tests" all: -TEST_PROGS := printf.sh bitmap.sh prime_numbers.sh scanf.sh strscpy.sh +TEST_PROGS := printf.sh prime_numbers.sh scanf.sh strscpy.sh include ../lib.mk diff --git a/tools/testing/selftests/lib/bitmap.sh b/tools/testing/selftests/lib/bitmap.sh deleted file mode 100755 index 00a416fbc0ef0..0000000000000 --- a/tools/testing/selftests/lib/bitmap.sh +++ /dev/null @@ -1,3 +0,0 @@ -#!/bin/sh -# SPDX-License-Identifier: GPL-2.0 -$(dirname $0)/../kselftest/module.sh "bitmap" test_bitmap diff --git a/tools/testing/selftests/lib/config b/tools/testing/selftests/lib/config index 645839b50b0a2..7d3b1de29d3d6 100644 --- a/tools/testing/selftests/lib/config +++ b/tools/testing/selftests/lib/config @@ -1,6 +1,5 @@ CONFIG_TEST_PRINTF=m CONFIG_TEST_SCANF=m -CONFIG_TEST_BITMAP=m CONFIG_PRIME_NUMBERS=m CONFIG_TEST_STRSCPY=m CONFIG_TEST_BITOPS=m
thanks, -- Shuah
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.
thanks, -- Shuah
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.c...
Thanks, Yury
On 7/29/24 02:15, Muhammad Usama Anjum wrote:
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 isn't idea 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 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.
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.
+1 on this. Add new tests and look at the reports.
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
As I mentioned in my earlier message, I am going to nack all patches that convert existing selftests to kunit such as this one.
thanks, -- Shuah
On Tue, 30 Jul 2024 at 23:39, Shuah Khan skhan@linuxfoundation.org wrote:
On 7/29/24 02:15, Muhammad Usama Anjum wrote:
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 isn't idea for cases where people would want to check a subsystem on a running kernel - KUnit covers some use-cases and kselftest covers others.
Thanks, Shuah. That clarifies a lot. So the use-case we're concerned about is: - We have an existing running kernel, which has CONFIG_KUNIT=n. - Something breaks, and we want to check (for example) test_bitmap. - We can modprobe test_bitmap on the current kernel.
That's definitely something KUnit can't do at the moment (though the workaround of always building with CONFIG_KUNIT=m is working for Red Hat and Android, and seems the closest equivalent). Maybe we need to work out a way for KUnit tests to handle this case more smoothly. Because I don't think there's anything special about this test which makes this use-case more likely than any of the other KUnit tests we have.
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 are intended to be used.
I don't think it's clear that this _is_ how the test is intended to be used: the config help text (and earlier messages in this thread) were all about running tests at boot, which is distinct from "debugging a problem on a running system". I think this is where I've been most confused. We've been happy to assume the "testing at runtime" case is against a kernel with test configs enabled, too.
When it comes to test_bitmap specifically, I can't find any difference (save possibly for a couple of performance test functions) between it and any of the other tests we've converted to or written with KUnit in the past. I'd imagine (and correct me if I'm wrong) that it's pretty rare for there to be an issue which will be caught by test_bitmap on a running kernel, but not on a fresh boot / in a kernel with KUnit enabled. So the "debug a running system" situation seems to me only decrease coverage of some very rare situations. (Of course, that's ignoring that running the module against the current kernel might be most convenient, but that's a separate issue from coverage.)
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.
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.
+1 on this. Add new tests and look at the reports.
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
As I mentioned in my earlier message, I am going to nack all patches that convert existing selftests to kunit such as this one.
I should point out that we've already been porting a lot of tests in lib/ to KUnit for years, sometimes with the help of the test author, sometimes by encouraging it as a task for new contributors. And while most of those are not actually selftests (though we may have ported one or two), they differ from test_bitmap only in that there's a selftest wrapper script. The authors and users of those tests have thus far found this a worthwhile trade -- I've not heard any complaints -- so I'd rather we leave this as a "pause" on new conversions while we sort this out, rather than do something drastic like revert existing conversion.
Personally, I'd still prefer these sorts of conversions to go ahead -- I think the benefits outweightthe costs -- but I can totally get behind doing it on a case-per-case basis. I'd be disappointed if we NACKed changes like this in the case where the authors and users of that individual test would like it to be converted, though.
Regardless, I think there's more discussion to be had here, and I'll look into what changes we can make either to KUnit or to the testing documentation to make sure we're all on the same page.
-- David
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.c...
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"helpEnable 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.
- 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.
- 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 KUNITdefault KUNIT_ALL_TESTShelpThis builds the "bitmap_kunit" module that runs tests forbitmaps 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
On 7/30/24 21:06, David Gow wrote:
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.shCan 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"helpEnable 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.
- 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.
- 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).
Right. It appears the confusion is coming in because "What's the problem enabling CONFIG_KUNIT". The question is would you recommend enabling CONFIG_KUNIT in distribution kernels - there is a confusion that unit testing can be enabled in all environments.
The answer is no. Running tests should not require should not require CONFIG_KUNIT - I would call this not disagreement, but not understanding the role of unit (open box) vs closed box testing.
We want to users to be able to run as many tests possible without needing to enable KUnit or other config options to verify kernels they build. KUnit is a tool for kernel developers to verify - it is a unit testing framework that is where it brings value.
It isn't a replacement for selftests and other tests. I am not happy to seer tests that are supported as kselftests be changed to now require KUNIT to be enabled.
thanks, -- Shuah
linux-kselftest-mirror@lists.linaro.org
-
David Gow -
John Hubbard -
Muhammad Usama Anjum -
Randy Dunlap -
Shuah Khan -
Yury Norov