| Message ID | 20210826012626.1163705-7-isabellabdoamaral@usp.br (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | test_hash.c: refactor into KUnit | expand |
On Thu, Aug 26, 2021 at 9:26 AM Isabella Basso <isabellabdoamaral@usp.br> wrote: > > Use KUnit framework to make tests more easily integrable with CIs. Even > though these tests are not yet properly written as unit tests this > change should help in debugging. Thanks -- I think KUnit is a good fit for these tests. I've tested the series, and it works well for me (but again, I'm no expert on the hashing code). I've left a few comments below, but there's nothing major which seems to actually break the test, so this series is nevertheless: Tested-by: David Gow <davidgow@google.com> Cheers, -- David > > Also drop module support and remove kernel messages (i.e. through > pr_info) as KUnit handles all debugging output. To clarify, are you actually dropping support for building this as a module, or just letting KUnit handle it for you? Ideally, this will still work as a module, even if it also works as a built-in test. (Given that the Kconfig entry still is 'tristate', I assume this is the case.) > > Co-developed-by: Augusto Durães Camargo <augusto.duraes33@gmail.com> > Signed-off-by: Augusto Durães Camargo <augusto.duraes33@gmail.com> > Co-developed-by: Enzo Ferreira <ferreiraenzoa@gmail.com> > Signed-off-by: Enzo Ferreira <ferreiraenzoa@gmail.com> > Signed-off-by: Isabella Basso <isabellabdoamaral@usp.br> > --- > lib/Kconfig.debug | 28 ++++--- > lib/Makefile | 2 +- > lib/test_hash.c | 197 ++++++++++++++-------------------------------- > 3 files changed, 79 insertions(+), 148 deletions(-) > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index 5e5894d98c50..adefb03a7e16 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -2221,15 +2221,6 @@ config TEST_RHASHTABLE > > If unsure, say N. > > -config TEST_HASH > - tristate "Perform selftest on hash functions" > - help > - Enable this option to test the kernel's integer (<linux/hash.h>), and > - string (<linux/stringhash.h>) hash functions on boot (or module load). > - > - This is intended to help people writing architecture-specific > - optimized versions. If unsure, say N. > - > config TEST_SIPHASH > tristate "Perform selftest on siphash functions" > help > @@ -2378,6 +2369,25 @@ config BITFIELD_KUNIT > > If unsure, say N. > > +config HASH_KUNIT_TEST > + tristate "KUnit Test for integer hash functions" if !KUNIT_ALL_TESTS > + depends on KUNIT > + default KUNIT_ALL_TESTS > + help > + Enable this option to test the kernel's string (<linux/stringhash.h>), and > + integer (<linux/hash.h>) hash functions on boot. > + > + KUnit tests run during boot and output the results to the debug log > + in TAP format (https://testanything.org/). Only useful for kernel devs > + running the KUnit test harness, and not intended for inclusion into a > + production build. > + > + For more information on KUnit and unit tests in general please refer > + to the KUnit documentation in Documentation/dev-tools/kunit/. > + > + This is intended to help people writing architecture-specific > + optimized versions. If unsure, say N. > + > config RESOURCE_KUNIT_TEST > tristate "KUnit test for resource API" > depends on KUNIT > diff --git a/lib/Makefile b/lib/Makefile > index c2e81d0eb31c..0bc336d9d036 100644 > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -62,7 +62,7 @@ obj-$(CONFIG_TEST_BITOPS) += test_bitops.o > CFLAGS_test_bitops.o += -Werror > obj-$(CONFIG_TEST_SYSCTL) += test_sysctl.o > obj-$(CONFIG_TEST_SIPHASH) += test_siphash.o > -obj-$(CONFIG_TEST_HASH) += test_hash.o > +obj-$(CONFIG_HASH_KUNIT_TEST) += test_hash.o > obj-$(CONFIG_TEST_IDA) += test_ida.o > obj-$(CONFIG_KASAN_KUNIT_TEST) += test_kasan.o > CFLAGS_test_kasan.o += -fno-builtin > diff --git a/lib/test_hash.c b/lib/test_hash.c > index c168823b0963..84590bbf47dc 100644 > --- a/lib/test_hash.c > +++ b/lib/test_hash.c > @@ -14,14 +14,10 @@ > * and hash_64(). > */ > > -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt "\n" > - > -#include <linux/compiler.h> > #include <linux/types.h> > -#include <linux/module.h> > #include <linux/hash.h> > #include <linux/stringhash.h> > -#include <linux/printk.h> > +#include <kunit/test.h> > > #define SIZE 256 /* Run time is cubic in SIZE */ > > @@ -29,7 +25,7 @@ static u32 string_or; /* stores or-ed string output */ > static u32 hash_or[2][33] = { { 0, } }; /* stores or-ed hash output */ > > /* 32-bit XORSHIFT generator. Seed must not be zero. */ > -static u32 __init __attribute_const__ > +static u32 __attribute_const__ > xorshift(u32 seed) > { > seed ^= seed << 13; > @@ -39,7 +35,7 @@ xorshift(u32 seed) > } > > /* Given a non-zero x, returns a non-zero byte. */ > -static u8 __init __attribute_const__ > +static u8 __attribute_const__ > mod255(u32 x) > { > x = (x & 0xffff) + (x >> 16); /* 1 <= x <= 0x1fffe */ > @@ -50,8 +46,7 @@ mod255(u32 x) > } > > /* Fill the buffer with non-zero bytes. */ > -static void __init > -fill_buf(char *buf, size_t len, u32 seed) > +static void fill_buf(char *buf, size_t len, u32 seed) > { > size_t i; > > @@ -62,41 +57,31 @@ fill_buf(char *buf, size_t len, u32 seed) > } > > #ifdef HAVE_ARCH__HASH_32 > -static bool __init > -test_int_hash32(u32 *h0, u32 *h1, u32 *h2) > +static bool test_int_hash32(struct kunit *test, u32 *h0, u32 *h1, u32 *h2) > { > hash_or[1][0] |= *h2 = __hash_32_generic(h0); > #if HAVE_ARCH__HASH_32 == 1 > - if (*h1 != *h2) { > - pr_err("__hash_32(%#x) = %#x != __hash_32_generic() = %#x", > - *h0, *h1, *h2); > - return false; > - } > + KUNIT_ASSERT_EQ_MSG(test, *h1, *h2, > + "__hash_32(%#x) = %#x != __hash_32_generic() = %#x", > + *h0, *h1, *h2); Should this (and others) be EXPECTations rather than ASSERTions? I imagine we'd want to continue the test even if this doesn't match. (I know that the existing function returns early here, but I'd argue that __hash_32() and __hash_32_generic() producing different results is a separate issue than the final ORed result turning out differently, and we shouldn't change the latter by exiting out early from the former. Equally, there's probably an argument for splitting out the __hash_32() vs __hash_32_generic() tests completely from the _or tests, but that would be more work.) > #endif > - return true; > } > #endif > > #ifdef HAVE_ARCH_HASH_64 > -static bool __init > -test_int_hash64(unsigned long long h64, u32 *h0, u32 *h1, u32 *h2, u32 const *m, int k) > +static bool test_int_hash64(struct kunit *test, unsigned long long h64, u32 *h0, u32 *h1, > + u32 *h2, u32 const *m, int k) > { > *h2 = hash_64_generic(*h64, *k); > #if HAVE_ARCH_HASH_64 == 1 > - if (*h1 != *h2) { > - pr_err("hash_64(%#llx, %d) = %#x != hash_64_generic() = %#x", > - *h64, *k, *h1, *h2); > - return false; > - } > + KUNIT_ASSERT_EQ_MSG(test, *h1, *h2, > + "hash_64(%#llx, %d) = %#x != hash_64_generic() = %#x", > + *h64, *k, *h1, *h2); > #else > - if (*h2 > *m) { > - pr_err("hash_64_generic(%#llx, %d) = %#x > %#x", > - *h64, *k, *h1, *m); > - return false; > - } > + KUNIT_ASSERT_LE_MSG(test, *h1, *h2, > + "hash_64_generic(%#llx, %d) = %#x > %#x", > + *h64, *k, *h1, *m); > #endif > - return true; > - > } > #endif > > @@ -109,8 +94,7 @@ test_int_hash64(unsigned long long h64, u32 *h0, u32 *h1, u32 *h2, u32 const *m, > * inline, the code being tested is actually in the module, and you can > * recompile and re-test the module without rebooting. > */ > -static bool __init > -test_int_hash(unsigned long long h64) > +static void test_int_hash(struct kunit *test, unsigned long long h64) > { > int k; > u32 h0 = (u32)h64, h1; > @@ -122,7 +106,7 @@ test_int_hash(unsigned long long h64) > /* Test __hash32 */ > hash_or[0][0] |= h1 = __hash_32(h0); > #ifdef HAVE_ARCH__HASH_32 > - if (!test_int_hash32(&h0, &h1, &h2)) > + if (!test_int_hash32(test, &h0, &h1, &h2)) > return false; > #endif > > @@ -132,27 +116,22 @@ test_int_hash(unsigned long long h64) > > /* Test hash_32 */ > hash_or[0][k] |= h1 = hash_32(h0, k); > - if (h1 > m) { > - pr_err("hash_32(%#x, %d) = %#x > %#x", h0, k, h1, m); > - return false; > - } > + KUNIT_ASSERT_LE_MSG(test, h1, m, > + "hash_32(%#x, %d) = %#x > %#x", > + h0, k, h1, m); > > /* Test hash_64 */ > hash_or[1][k] |= h1 = hash_64(h64, k); > - if (h1 > m) { > - pr_err("hash_64(%#llx, %d) = %#x > %#x", h64, k, h1, m); > - return false; > - } > + KUNIT_ASSERT_LE_MSG(test, h1, m, > + "hash_64(%#llx, %d) = %#x > %#x", > + h64, k, h1, m); > #ifdef HAVE_ARCH_HASH_64 > - if (!test_int_hash64(&h64, &h0, &h1, &h2, &m, &k)) > - return false; > + test_int_hash64(test, &h64, &h0, &h1, &h2, &m, &k); > #endif > } > - > - return true; > } > > -static int __init test_string_or(void) > +static void test_string_or(struct kunit *test) > { > char buf[SIZE+1]; > int i, j; > @@ -173,19 +152,14 @@ static int __init test_string_or(void) > } /* j */ > > /* The OR of all the hash values should cover all the bits */ > - if (~string_or) { > - pr_err("OR of all string hash results = %#x != %#x", > - string_or, -1u); > - return -EINVAL; > - } > - > - return 0; > + KUNIT_ASSERT_FALSE_MSG(test, ~string_or, > + "OR of all string hash results = %#x != %#x", > + string_or, -1u); > } > > -static int __init test_hash_or(void) > +static void test_hash_or(struct kunit *test) > { > char buf[SIZE+1]; > - unsigned tests = 0; > unsigned long long h64 = 0; > int i, j; > > @@ -201,39 +175,27 @@ static int __init test_hash_or(void) > u32 h0 = full_name_hash(buf+i, buf+i, j-i); > > /* Check that hashlen_string gets the length right */ > - if (hashlen_len(hashlen) != j-i) { > - pr_err("hashlen_string(%d..%d) returned length" > - " %u, expected %d", > - i, j, hashlen_len(hashlen), j-i); > - return -EINVAL; > - } > + KUNIT_ASSERT_EQ_MSG(test, hashlen_len(hashlen), j-i, > + "hashlen_string(%d..%d) returned length %u, expected %d", > + i, j, hashlen_len(hashlen), j-i); > /* Check that the hashes match */ > - if (hashlen_hash(hashlen) != h0) { > - pr_err("hashlen_string(%d..%d) = %08x != " > - "full_name_hash() = %08x", > - i, j, hashlen_hash(hashlen), h0); > - return -EINVAL; > - } > + KUNIT_ASSERT_EQ_MSG(test, hashlen_hash(hashlen), h0, > + "hashlen_string(%d..%d) = %08x != full_name_hash() = %08x", > + i, j, hashlen_hash(hashlen), h0); > > h64 = h64 << 32 | h0; /* For use with hash_64 */ > - if (!test_int_hash(h64)) > - return -EINVAL; > - tests++; > + test_int_hash(test, h64); > } /* i */ > } /* j */ > > - if (~hash_or[0][0]) { > - pr_err("OR of all __hash_32 results = %#x != %#x", > - hash_or[0][0], -1u); > - return -EINVAL; > - } > + KUNIT_ASSERT_FALSE_MSG(test, ~hash_or[0][0], > + "OR of all __hash_32 results = %#x != %#x", > + hash_or[0][0], -1u); > #ifdef HAVE_ARCH__HASH_32 > #if HAVE_ARCH__HASH_32 != 1 /* Test is pointless if results match */ > - if (~hash_or[1][0]) { > - pr_err("OR of all __hash_32_generic results = %#x != %#x", > - hash_or[1][0], -1u); > - return -EINVAL; > - } > + KUNIT_ASSERT_FALSE_MSG(test, ~hash_or[1][0], > + "OR of all __hash_32_generic results = %#x != %#x", > + hash_or[1][0], -1u); > #endif > #endif > > @@ -241,65 +203,24 @@ static int __init test_hash_or(void) > for (i = 1; i <= 32; i++) { > u32 const m = ((u32)2 << (i-1)) - 1; /* Low i bits set */ > > - if (hash_or[0][i] != m) { > - pr_err("OR of all hash_32(%d) results = %#x " > - "(%#x expected)", i, hash_or[0][i], m); > - return -EINVAL; > - } > - if (hash_or[1][i] != m) { > - pr_err("OR of all hash_64(%d) results = %#x " > - "(%#x expected)", i, hash_or[1][i], m); > - return -EINVAL; > - } > + KUNIT_ASSERT_EQ_MSG(test, hash_or[0][i], m, > + "OR of all hash_32(%d) results = %#x (%#x expected)", > + i, hash_or[0][i], m); > + KUNIT_ASSERT_EQ_MSG(test, hash_or[1][i], m, > + "OR of all hash_64(%d) results = %#x (%#x expected)", > + i, hash_or[1][i], m); > } > - > - pr_notice("%u tests passed.", tests); > - > - return 0; > } > > -static void __init notice_skipped_tests(void) > -{ > - /* Issue notices about skipped tests. */ > -#ifdef HAVE_ARCH__HASH_32 > -#if HAVE_ARCH__HASH_32 != 1 > - pr_info("__hash_32() is arch-specific; not compared to generic."); > -#endif > -#else > - pr_info("__hash_32() has no arch implementation to test."); > -#endif > -#ifdef HAVE_ARCH_HASH_64 > -#if HAVE_ARCH_HASH_64 != 1 > - pr_info("hash_64() is arch-specific; not compared to generic."); > -#endif > -#else > - pr_info("hash_64() has no arch implementation to test."); > -#endif > -} > - > -static int __init > -test_hash_init(void) > -{ > - int ret; > - > - ret = test_string_or(); > - if (ret < 0) > - return ret; > - > - ret = test_hash_or(); > - if (ret < 0) > - return ret; > - > - notice_skipped_tests(); > - > - return ret; > -} > - > -static void __exit test_hash_exit(void) > -{ > -} > +static struct kunit_case hash_test_cases[] = { > + KUNIT_CASE(test_string_or), > + KUNIT_CASE(test_hash_or), Ideally, these could be split up further into separate __hash_32(), hash_32(), and hash_64() tests. Maybe even with separate tests for architecture-specific versus generic implementations as mentioned above, if that made sense. It'd require enough reworking of the tests and expected results though, that I wouldn't necessarily want to force such a significant change at the same time as this more straightforward port to KUnit. > + {} > +}; > > -module_init(test_hash_init); /* Does everything */ > -module_exit(test_hash_exit); /* Does nothing */ > +static struct kunit_suite hash_test_suite = { > + .name = "hash_tests", It might be worth just naming this "hash", as the fact that it's a KUnit suite already tells us it contains tests. > + .test_cases = hash_test_cases, > +}; > > -MODULE_LICENSE("GPL"); It's probably worth keeping this in as it's still GPLed, and can be built as a module. > +kunit_test_suite(hash_test_suite); > -- > 2.33.0 >
Hi Isabella,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.14-rc7 next-20210825]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Isabella-Basso/test_hash-c-refactor-into-KUnit/20210826-092911
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 73f3af7b4611d77bdaea303fb639333eb28e37d7
config: m68k-randconfig-r013-20210826 (attached as .config)
compiler: m68k-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/89fe4c8eec63dcabd6d3e8f3f71f3c2248d18dab
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Isabella-Basso/test_hash-c-refactor-into-KUnit/20210826-092911
git checkout 89fe4c8eec63dcabd6d3e8f3f71f3c2248d18dab
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
lib/test_hash.c: In function 'test_int_hash32':
>> lib/test_hash.c:62:50: warning: passing argument 1 of '__hash_32_generic' makes integer from pointer without a cast [-Wint-conversion]
62 | hash_or[1][0] |= *h2 = __hash_32_generic(h0);
| ^~
| |
| u32 * {aka unsigned int *}
In file included from lib/test_hash.c:18:
include/linux/hash.h:60:41: note: expected 'u32' {aka 'unsigned int'} but argument is of type 'u32 *' {aka 'unsigned int *'}
60 | static inline u32 __hash_32_generic(u32 val)
| ~~~~^~~
lib/test_hash.c:68:1: error: no return statement in function returning non-void [-Werror=return-type]
68 | }
| ^
lib/test_hash.c: In function 'test_int_hash':
lib/test_hash.c:110:24: error: 'return' with a value, in function returning void [-Werror=return-type]
110 | return false;
| ^~~~~
lib/test_hash.c:97:13: note: declared here
97 | static void test_int_hash(struct kunit *test, unsigned long long h64)
| ^~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/__hash_32_generic +62 lib/test_hash.c
468a9428521e7d00 George Spelvin 2016-05-26 58
7d8047a5cceb3123 Isabella Basso 2021-08-25 59 #ifdef HAVE_ARCH__HASH_32
89fe4c8eec63dcab Isabella Basso 2021-08-25 60 static bool test_int_hash32(struct kunit *test, u32 *h0, u32 *h1, u32 *h2)
7d8047a5cceb3123 Isabella Basso 2021-08-25 61 {
7d8047a5cceb3123 Isabella Basso 2021-08-25 @62 hash_or[1][0] |= *h2 = __hash_32_generic(h0);
7d8047a5cceb3123 Isabella Basso 2021-08-25 63 #if HAVE_ARCH__HASH_32 == 1
89fe4c8eec63dcab Isabella Basso 2021-08-25 64 KUNIT_ASSERT_EQ_MSG(test, *h1, *h2,
89fe4c8eec63dcab Isabella Basso 2021-08-25 65 "__hash_32(%#x) = %#x != __hash_32_generic() = %#x",
7d8047a5cceb3123 Isabella Basso 2021-08-25 66 *h0, *h1, *h2);
7d8047a5cceb3123 Isabella Basso 2021-08-25 67 #endif
7d8047a5cceb3123 Isabella Basso 2021-08-25 68 }
7d8047a5cceb3123 Isabella Basso 2021-08-25 69 #endif
7d8047a5cceb3123 Isabella Basso 2021-08-25 70
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Isabella,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.14-rc7 next-20210825]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Isabella-Basso/test_hash-c-refactor-into-KUnit/20210826-092911
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 73f3af7b4611d77bdaea303fb639333eb28e37d7
config: parisc-randconfig-r014-20210825 (attached as .config)
compiler: hppa64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/89fe4c8eec63dcabd6d3e8f3f71f3c2248d18dab
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Isabella-Basso/test_hash-c-refactor-into-KUnit/20210826-092911
git checkout 89fe4c8eec63dcabd6d3e8f3f71f3c2248d18dab
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=parisc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>):
lib/test_hash.c: In function 'test_int_hash32':
lib/test_hash.c:62:50: warning: passing argument 1 of '__hash_32_generic' makes integer from pointer without a cast [-Wint-conversion]
62 | hash_or[1][0] |= *h2 = __hash_32_generic(h0);
| ^~
| |
| u32 * {aka unsigned int *}
In file included from lib/test_hash.c:18:
include/linux/hash.h:60:41: note: expected 'u32' {aka 'unsigned int'} but argument is of type 'u32 *' {aka 'unsigned int *'}
60 | static inline u32 __hash_32_generic(u32 val)
| ~~~~^~~
lib/test_hash.c:68:1: error: no return statement in function returning non-void [-Werror=return-type]
68 | }
| ^
lib/test_hash.c: In function 'test_int_hash64':
>> lib/test_hash.c:75:31: error: invalid type argument of unary '*' (have 'long long unsigned int')
75 | *h2 = hash_64_generic(*h64, *k);
| ^~~~
>> lib/test_hash.c:75:37: error: invalid type argument of unary '*' (have 'int')
75 | *h2 = hash_64_generic(*h64, *k);
| ^~
In file included from lib/test_hash.c:20:
lib/test_hash.c:79:29: error: invalid type argument of unary '*' (have 'long long unsigned int')
79 | *h64, *k, *h1, *h2);
| ^~~~
include/kunit/test.h:775:30: note: in definition of macro 'KUNIT_ASSERTION'
775 | ##__VA_ARGS__); \
| ^~~~~~~~~~~
include/kunit/test.h:891:9: note: in expansion of macro 'KUNIT_BASE_BINARY_ASSERTION'
891 | KUNIT_BASE_BINARY_ASSERTION(test, \
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
include/kunit/test.h:980:9: note: in expansion of macro 'KUNIT_BASE_EQ_MSG_ASSERTION'
980 | KUNIT_BASE_EQ_MSG_ASSERTION(test, \
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
include/kunit/test.h:1644:9: note: in expansion of macro 'KUNIT_BINARY_EQ_MSG_ASSERTION'
1644 | KUNIT_BINARY_EQ_MSG_ASSERTION(test, \
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
lib/test_hash.c:77:9: note: in expansion of macro 'KUNIT_ASSERT_EQ_MSG'
77 | KUNIT_ASSERT_EQ_MSG(test, *h1, *h2,
| ^~~~~~~~~~~~~~~~~~~
lib/test_hash.c:79:35: error: invalid type argument of unary '*' (have 'int')
79 | *h64, *k, *h1, *h2);
| ^~
include/kunit/test.h:775:30: note: in definition of macro 'KUNIT_ASSERTION'
775 | ##__VA_ARGS__); \
| ^~~~~~~~~~~
include/kunit/test.h:891:9: note: in expansion of macro 'KUNIT_BASE_BINARY_ASSERTION'
891 | KUNIT_BASE_BINARY_ASSERTION(test, \
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
include/kunit/test.h:980:9: note: in expansion of macro 'KUNIT_BASE_EQ_MSG_ASSERTION'
980 | KUNIT_BASE_EQ_MSG_ASSERTION(test, \
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
include/kunit/test.h:1644:9: note: in expansion of macro 'KUNIT_BINARY_EQ_MSG_ASSERTION'
1644 | KUNIT_BINARY_EQ_MSG_ASSERTION(test, \
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
lib/test_hash.c:77:9: note: in expansion of macro 'KUNIT_ASSERT_EQ_MSG'
77 | KUNIT_ASSERT_EQ_MSG(test, *h1, *h2,
| ^~~~~~~~~~~~~~~~~~~
lib/test_hash.c:85:1: error: no return statement in function returning non-void [-Werror=return-type]
85 | }
| ^
lib/test_hash.c: In function 'test_int_hash':
lib/test_hash.c:110:24: error: 'return' with a value, in function returning void [-Werror=return-type]
110 | return false;
| ^~~~~
lib/test_hash.c:97:13: note: declared here
97 | static void test_int_hash(struct kunit *test, unsigned long long h64)
| ^~~~~~~~~~~~~
>> lib/test_hash.c:129:39: warning: passing argument 2 of 'test_int_hash64' makes integer from pointer without a cast [-Wint-conversion]
129 | test_int_hash64(test, &h64, &h0, &h1, &h2, &m, &k);
| ^~~~
| |
| long long unsigned int *
lib/test_hash.c:72:68: note: expected 'long long unsigned int' but argument is of type 'long long unsigned int *'
72 | static bool test_int_hash64(struct kunit *test, unsigned long long h64, u32 *h0, u32 *h1,
| ~~~~~~~~~~~~~~~~~~~^~~
lib/test_hash.c:129:64: warning: passing argument 7 of 'test_int_hash64' makes integer from pointer without a cast [-Wint-conversion]
129 | test_int_hash64(test, &h64, &h0, &h1, &h2, &m, &k);
| ^~
| |
| int *
lib/test_hash.c:73:44: note: expected 'int' but argument is of type 'int *'
73 | u32 *h2, u32 const *m, int k)
| ~~~~^
cc1: some warnings being treated as errors
vim +75 lib/test_hash.c
7d8047a5cceb31 Isabella Basso 2021-08-25 70
7d8047a5cceb31 Isabella Basso 2021-08-25 71 #ifdef HAVE_ARCH_HASH_64
89fe4c8eec63dc Isabella Basso 2021-08-25 72 static bool test_int_hash64(struct kunit *test, unsigned long long h64, u32 *h0, u32 *h1,
89fe4c8eec63dc Isabella Basso 2021-08-25 73 u32 *h2, u32 const *m, int k)
7d8047a5cceb31 Isabella Basso 2021-08-25 74 {
7d8047a5cceb31 Isabella Basso 2021-08-25 @75 *h2 = hash_64_generic(*h64, *k);
7d8047a5cceb31 Isabella Basso 2021-08-25 76 #if HAVE_ARCH_HASH_64 == 1
89fe4c8eec63dc Isabella Basso 2021-08-25 77 KUNIT_ASSERT_EQ_MSG(test, *h1, *h2,
89fe4c8eec63dc Isabella Basso 2021-08-25 78 "hash_64(%#llx, %d) = %#x != hash_64_generic() = %#x",
7d8047a5cceb31 Isabella Basso 2021-08-25 79 *h64, *k, *h1, *h2);
7d8047a5cceb31 Isabella Basso 2021-08-25 80 #else
89fe4c8eec63dc Isabella Basso 2021-08-25 81 KUNIT_ASSERT_LE_MSG(test, *h1, *h2,
89fe4c8eec63dc Isabella Basso 2021-08-25 82 "hash_64_generic(%#llx, %d) = %#x > %#x",
7d8047a5cceb31 Isabella Basso 2021-08-25 83 *h64, *k, *h1, *m);
7d8047a5cceb31 Isabella Basso 2021-08-25 84 #endif
7d8047a5cceb31 Isabella Basso 2021-08-25 85 }
7d8047a5cceb31 Isabella Basso 2021-08-25 86 #endif
7d8047a5cceb31 Isabella Basso 2021-08-25 87
468a9428521e7d George Spelvin 2016-05-26 88 /*
468a9428521e7d George Spelvin 2016-05-26 89 * Test the various integer hash functions. h64 (or its low-order bits)
468a9428521e7d George Spelvin 2016-05-26 90 * is the integer to hash. hash_or accumulates the OR of the hash values,
468a9428521e7d George Spelvin 2016-05-26 91 * which are later checked to see that they cover all the requested bits.
468a9428521e7d George Spelvin 2016-05-26 92 *
468a9428521e7d George Spelvin 2016-05-26 93 * Because these functions (as opposed to the string hashes) are all
468a9428521e7d George Spelvin 2016-05-26 94 * inline, the code being tested is actually in the module, and you can
468a9428521e7d George Spelvin 2016-05-26 95 * recompile and re-test the module without rebooting.
468a9428521e7d George Spelvin 2016-05-26 96 */
89fe4c8eec63dc Isabella Basso 2021-08-25 97 static void test_int_hash(struct kunit *test, unsigned long long h64)
468a9428521e7d George Spelvin 2016-05-26 98 {
468a9428521e7d George Spelvin 2016-05-26 99 int k;
7d8047a5cceb31 Isabella Basso 2021-08-25 100 u32 h0 = (u32)h64, h1;
7d8047a5cceb31 Isabella Basso 2021-08-25 101
7d8047a5cceb31 Isabella Basso 2021-08-25 102 #if defined HAVE_ARCH__HASH_32 || defined HAVE_ARCH_HASH_64
7d8047a5cceb31 Isabella Basso 2021-08-25 103 u32 h2;
7d8047a5cceb31 Isabella Basso 2021-08-25 104 #endif
468a9428521e7d George Spelvin 2016-05-26 105
468a9428521e7d George Spelvin 2016-05-26 106 /* Test __hash32 */
468a9428521e7d George Spelvin 2016-05-26 107 hash_or[0][0] |= h1 = __hash_32(h0);
468a9428521e7d George Spelvin 2016-05-26 108 #ifdef HAVE_ARCH__HASH_32
89fe4c8eec63dc Isabella Basso 2021-08-25 109 if (!test_int_hash32(test, &h0, &h1, &h2))
468a9428521e7d George Spelvin 2016-05-26 110 return false;
468a9428521e7d George Spelvin 2016-05-26 111 #endif
468a9428521e7d George Spelvin 2016-05-26 112
468a9428521e7d George Spelvin 2016-05-26 113 /* Test k = 1..32 bits */
468a9428521e7d George Spelvin 2016-05-26 114 for (k = 1; k <= 32; k++) {
468a9428521e7d George Spelvin 2016-05-26 115 u32 const m = ((u32)2 << (k-1)) - 1; /* Low k bits set */
468a9428521e7d George Spelvin 2016-05-26 116
468a9428521e7d George Spelvin 2016-05-26 117 /* Test hash_32 */
468a9428521e7d George Spelvin 2016-05-26 118 hash_or[0][k] |= h1 = hash_32(h0, k);
89fe4c8eec63dc Isabella Basso 2021-08-25 119 KUNIT_ASSERT_LE_MSG(test, h1, m,
89fe4c8eec63dc Isabella Basso 2021-08-25 120 "hash_32(%#x, %d) = %#x > %#x",
89fe4c8eec63dc Isabella Basso 2021-08-25 121 h0, k, h1, m);
d41da448b96d6d Isabella Basso 2021-08-25 122
468a9428521e7d George Spelvin 2016-05-26 123 /* Test hash_64 */
468a9428521e7d George Spelvin 2016-05-26 124 hash_or[1][k] |= h1 = hash_64(h64, k);
89fe4c8eec63dc Isabella Basso 2021-08-25 125 KUNIT_ASSERT_LE_MSG(test, h1, m,
89fe4c8eec63dc Isabella Basso 2021-08-25 126 "hash_64(%#llx, %d) = %#x > %#x",
89fe4c8eec63dc Isabella Basso 2021-08-25 127 h64, k, h1, m);
468a9428521e7d George Spelvin 2016-05-26 128 #ifdef HAVE_ARCH_HASH_64
89fe4c8eec63dc Isabella Basso 2021-08-25 @129 test_int_hash64(test, &h64, &h0, &h1, &h2, &m, &k);
468a9428521e7d George Spelvin 2016-05-26 130 #endif
468a9428521e7d George Spelvin 2016-05-26 131 }
468a9428521e7d George Spelvin 2016-05-26 132 }
468a9428521e7d George Spelvin 2016-05-26 133
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi, David, On Thu, Aug 26, 2021 at 1:26 AM David Gow <davidgow@google.com> wrote: > > On Thu, Aug 26, 2021 at 9:26 AM Isabella Basso <isabellabdoamaral@usp.br> wrote: > > > > Use KUnit framework to make tests more easily integrable with CIs. Even > > though these tests are not yet properly written as unit tests this > > change should help in debugging. > > Thanks -- I think KUnit is a good fit for these tests. I've tested the > series, and it works well for me (but again, I'm no expert on the > hashing code). > > I've left a few comments below, but there's nothing major which seems > to actually break the test, so this series is nevertheless: > > Tested-by: David Gow <davidgow@google.com> Thanks a lot for this! Really appreciate it. :) > > > > Also drop module support and remove kernel messages (i.e. through > > pr_info) as KUnit handles all debugging output. > > To clarify, are you actually dropping support for building this as a > module, or just letting KUnit handle it for you? Ideally, this will > still work as a module, even if it also works as a built-in test. > (Given that the Kconfig entry still is 'tristate', I assume this is the case.) This, actually, wasn't 100% clear to me at the time of writing this commit, so I apologize for the uninformed diff/message. Again, thanks for the heads up! > > > > Co-developed-by: Augusto Durães Camargo <augusto.duraes33@gmail.com> > > Signed-off-by: Augusto Durães Camargo <augusto.duraes33@gmail.com> > > Co-developed-by: Enzo Ferreira <ferreiraenzoa@gmail.com> > > Signed-off-by: Enzo Ferreira <ferreiraenzoa@gmail.com> > > Signed-off-by: Isabella Basso <isabellabdoamaral@usp.br> > > --- > > lib/Kconfig.debug | 28 ++++--- > > lib/Makefile | 2 +- > > lib/test_hash.c | 197 ++++++++++++++-------------------------------- > > 3 files changed, 79 insertions(+), 148 deletions(-) > > > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > > index 5e5894d98c50..adefb03a7e16 100644 > > --- a/lib/Kconfig.debug > > +++ b/lib/Kconfig.debug > > @@ -2221,15 +2221,6 @@ config TEST_RHASHTABLE > > > > If unsure, say N. > > > > -config TEST_HASH > > - tristate "Perform selftest on hash functions" > > - help > > - Enable this option to test the kernel's integer (<linux/hash.h>), and > > - string (<linux/stringhash.h>) hash functions on boot (or module load). > > - > > - This is intended to help people writing architecture-specific > > - optimized versions. If unsure, say N. > > - > > config TEST_SIPHASH > > tristate "Perform selftest on siphash functions" > > help > > @@ -2378,6 +2369,25 @@ config BITFIELD_KUNIT > > > > If unsure, say N. > > > > +config HASH_KUNIT_TEST > > + tristate "KUnit Test for integer hash functions" if !KUNIT_ALL_TESTS > > + depends on KUNIT > > + default KUNIT_ALL_TESTS > > + help > > + Enable this option to test the kernel's string (<linux/stringhash.h>), and > > + integer (<linux/hash.h>) hash functions on boot. > > + > > + KUnit tests run during boot and output the results to the debug log > > + in TAP format (https://testanything.org/). Only useful for kernel devs > > + running the KUnit test harness, and not intended for inclusion into a > > + production build. > > + > > + For more information on KUnit and unit tests in general please refer > > + to the KUnit documentation in Documentation/dev-tools/kunit/. > > + > > + This is intended to help people writing architecture-specific > > + optimized versions. If unsure, say N. > > + > > config RESOURCE_KUNIT_TEST > > tristate "KUnit test for resource API" > > depends on KUNIT > > diff --git a/lib/Makefile b/lib/Makefile > > index c2e81d0eb31c..0bc336d9d036 100644 > > --- a/lib/Makefile > > +++ b/lib/Makefile > > @@ -62,7 +62,7 @@ obj-$(CONFIG_TEST_BITOPS) += test_bitops.o > > CFLAGS_test_bitops.o += -Werror > > obj-$(CONFIG_TEST_SYSCTL) += test_sysctl.o > > obj-$(CONFIG_TEST_SIPHASH) += test_siphash.o > > -obj-$(CONFIG_TEST_HASH) += test_hash.o > > +obj-$(CONFIG_HASH_KUNIT_TEST) += test_hash.o > > obj-$(CONFIG_TEST_IDA) += test_ida.o > > obj-$(CONFIG_KASAN_KUNIT_TEST) += test_kasan.o > > CFLAGS_test_kasan.o += -fno-builtin > > diff --git a/lib/test_hash.c b/lib/test_hash.c > > index c168823b0963..84590bbf47dc 100644 > > --- a/lib/test_hash.c > > +++ b/lib/test_hash.c > > @@ -14,14 +14,10 @@ > > * and hash_64(). > > */ > > > > -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt "\n" > > - > > -#include <linux/compiler.h> > > #include <linux/types.h> > > -#include <linux/module.h> > > #include <linux/hash.h> > > #include <linux/stringhash.h> > > -#include <linux/printk.h> > > +#include <kunit/test.h> > > > > #define SIZE 256 /* Run time is cubic in SIZE */ > > > > @@ -29,7 +25,7 @@ static u32 string_or; /* stores or-ed string output */ > > static u32 hash_or[2][33] = { { 0, } }; /* stores or-ed hash output */ > > > > /* 32-bit XORSHIFT generator. Seed must not be zero. */ > > -static u32 __init __attribute_const__ > > +static u32 __attribute_const__ > > xorshift(u32 seed) > > { > > seed ^= seed << 13; > > @@ -39,7 +35,7 @@ xorshift(u32 seed) > > } > > > > /* Given a non-zero x, returns a non-zero byte. */ > > -static u8 __init __attribute_const__ > > +static u8 __attribute_const__ > > mod255(u32 x) > > { > > x = (x & 0xffff) + (x >> 16); /* 1 <= x <= 0x1fffe */ > > @@ -50,8 +46,7 @@ mod255(u32 x) > > } > > > > /* Fill the buffer with non-zero bytes. */ > > -static void __init > > -fill_buf(char *buf, size_t len, u32 seed) > > +static void fill_buf(char *buf, size_t len, u32 seed) > > { > > size_t i; > > > > @@ -62,41 +57,31 @@ fill_buf(char *buf, size_t len, u32 seed) > > } > > > > #ifdef HAVE_ARCH__HASH_32 > > -static bool __init > > -test_int_hash32(u32 *h0, u32 *h1, u32 *h2) > > +static bool test_int_hash32(struct kunit *test, u32 *h0, u32 *h1, u32 *h2) > > { > > hash_or[1][0] |= *h2 = __hash_32_generic(h0); > > #if HAVE_ARCH__HASH_32 == 1 > > - if (*h1 != *h2) { > > - pr_err("__hash_32(%#x) = %#x != __hash_32_generic() = %#x", > > - *h0, *h1, *h2); > > - return false; > > - } > > + KUNIT_ASSERT_EQ_MSG(test, *h1, *h2, > > + "__hash_32(%#x) = %#x != __hash_32_generic() = %#x", > > + *h0, *h1, *h2); > > Should this (and others) be EXPECTations rather than ASSERTions? I > imagine we'd want to continue the test even if this doesn't match. > > (I know that the existing function returns early here, but I'd argue > that __hash_32() and __hash_32_generic() producing different results > is a separate issue than the final ORed result turning out > differently, and we shouldn't change the latter by exiting out early > from the former. I was also a bit troubled by this question, as each operation performed is sequential and they all conspire for later test use. Would love to hear Spelvin on this matter but I'm equally as pleased by your guidance. :) > Equally, there's probably an argument for splitting > out the __hash_32() vs __hash_32_generic() tests completely from the > _or tests, but that would be more work.) > > > #endif > > - return true; > > } > > #endif > > > > #ifdef HAVE_ARCH_HASH_64 > > -static bool __init > > -test_int_hash64(unsigned long long h64, u32 *h0, u32 *h1, u32 *h2, u32 const *m, int k) > > +static bool test_int_hash64(struct kunit *test, unsigned long long h64, u32 *h0, u32 *h1, > > + u32 *h2, u32 const *m, int k) > > { > > *h2 = hash_64_generic(*h64, *k); > > #if HAVE_ARCH_HASH_64 == 1 > > - if (*h1 != *h2) { > > - pr_err("hash_64(%#llx, %d) = %#x != hash_64_generic() = %#x", > > - *h64, *k, *h1, *h2); > > - return false; > > - } > > + KUNIT_ASSERT_EQ_MSG(test, *h1, *h2, > > + "hash_64(%#llx, %d) = %#x != hash_64_generic() = %#x", > > + *h64, *k, *h1, *h2); > > #else > > - if (*h2 > *m) { > > - pr_err("hash_64_generic(%#llx, %d) = %#x > %#x", > > - *h64, *k, *h1, *m); > > - return false; > > - } > > + KUNIT_ASSERT_LE_MSG(test, *h1, *h2, > > + "hash_64_generic(%#llx, %d) = %#x > %#x", > > + *h64, *k, *h1, *m); > > #endif > > - return true; > > - > > } > > #endif > > > > @@ -109,8 +94,7 @@ test_int_hash64(unsigned long long h64, u32 *h0, u32 *h1, u32 *h2, u32 const *m, > > * inline, the code being tested is actually in the module, and you can > > * recompile and re-test the module without rebooting. > > */ > > -static bool __init > > -test_int_hash(unsigned long long h64) > > +static void test_int_hash(struct kunit *test, unsigned long long h64) > > { > > int k; > > u32 h0 = (u32)h64, h1; > > @@ -122,7 +106,7 @@ test_int_hash(unsigned long long h64) > > /* Test __hash32 */ > > hash_or[0][0] |= h1 = __hash_32(h0); > > #ifdef HAVE_ARCH__HASH_32 > > - if (!test_int_hash32(&h0, &h1, &h2)) > > + if (!test_int_hash32(test, &h0, &h1, &h2)) > > return false; > > #endif > > > > @@ -132,27 +116,22 @@ test_int_hash(unsigned long long h64) > > > > /* Test hash_32 */ > > hash_or[0][k] |= h1 = hash_32(h0, k); > > - if (h1 > m) { > > - pr_err("hash_32(%#x, %d) = %#x > %#x", h0, k, h1, m); > > - return false; > > - } > > + KUNIT_ASSERT_LE_MSG(test, h1, m, > > + "hash_32(%#x, %d) = %#x > %#x", > > + h0, k, h1, m); > > > > /* Test hash_64 */ > > hash_or[1][k] |= h1 = hash_64(h64, k); > > - if (h1 > m) { > > - pr_err("hash_64(%#llx, %d) = %#x > %#x", h64, k, h1, m); > > - return false; > > - } > > + KUNIT_ASSERT_LE_MSG(test, h1, m, > > + "hash_64(%#llx, %d) = %#x > %#x", > > + h64, k, h1, m); > > #ifdef HAVE_ARCH_HASH_64 > > - if (!test_int_hash64(&h64, &h0, &h1, &h2, &m, &k)) > > - return false; > > + test_int_hash64(test, &h64, &h0, &h1, &h2, &m, &k); > > #endif > > } > > - > > - return true; > > } > > > > -static int __init test_string_or(void) > > +static void test_string_or(struct kunit *test) > > { > > char buf[SIZE+1]; > > int i, j; > > @@ -173,19 +152,14 @@ static int __init test_string_or(void) > > } /* j */ > > > > /* The OR of all the hash values should cover all the bits */ > > - if (~string_or) { > > - pr_err("OR of all string hash results = %#x != %#x", > > - string_or, -1u); > > - return -EINVAL; > > - } > > - > > - return 0; > > + KUNIT_ASSERT_FALSE_MSG(test, ~string_or, > > + "OR of all string hash results = %#x != %#x", > > + string_or, -1u); > > } > > > > -static int __init test_hash_or(void) > > +static void test_hash_or(struct kunit *test) > > { > > char buf[SIZE+1]; > > - unsigned tests = 0; > > unsigned long long h64 = 0; > > int i, j; > > > > @@ -201,39 +175,27 @@ static int __init test_hash_or(void) > > u32 h0 = full_name_hash(buf+i, buf+i, j-i); > > > > /* Check that hashlen_string gets the length right */ > > - if (hashlen_len(hashlen) != j-i) { > > - pr_err("hashlen_string(%d..%d) returned length" > > - " %u, expected %d", > > - i, j, hashlen_len(hashlen), j-i); > > - return -EINVAL; > > - } > > + KUNIT_ASSERT_EQ_MSG(test, hashlen_len(hashlen), j-i, > > + "hashlen_string(%d..%d) returned length %u, expected %d", > > + i, j, hashlen_len(hashlen), j-i); > > /* Check that the hashes match */ > > - if (hashlen_hash(hashlen) != h0) { > > - pr_err("hashlen_string(%d..%d) = %08x != " > > - "full_name_hash() = %08x", > > - i, j, hashlen_hash(hashlen), h0); > > - return -EINVAL; > > - } > > + KUNIT_ASSERT_EQ_MSG(test, hashlen_hash(hashlen), h0, > > + "hashlen_string(%d..%d) = %08x != full_name_hash() = %08x", > > + i, j, hashlen_hash(hashlen), h0); > > > > h64 = h64 << 32 | h0; /* For use with hash_64 */ > > - if (!test_int_hash(h64)) > > - return -EINVAL; > > - tests++; > > + test_int_hash(test, h64); > > } /* i */ > > } /* j */ > > > > - if (~hash_or[0][0]) { > > - pr_err("OR of all __hash_32 results = %#x != %#x", > > - hash_or[0][0], -1u); > > - return -EINVAL; > > - } > > + KUNIT_ASSERT_FALSE_MSG(test, ~hash_or[0][0], > > + "OR of all __hash_32 results = %#x != %#x", > > + hash_or[0][0], -1u); > > #ifdef HAVE_ARCH__HASH_32 > > #if HAVE_ARCH__HASH_32 != 1 /* Test is pointless if results match */ > > - if (~hash_or[1][0]) { > > - pr_err("OR of all __hash_32_generic results = %#x != %#x", > > - hash_or[1][0], -1u); > > - return -EINVAL; > > - } > > + KUNIT_ASSERT_FALSE_MSG(test, ~hash_or[1][0], > > + "OR of all __hash_32_generic results = %#x != %#x", > > + hash_or[1][0], -1u); > > #endif > > #endif > > > > @@ -241,65 +203,24 @@ static int __init test_hash_or(void) > > for (i = 1; i <= 32; i++) { > > u32 const m = ((u32)2 << (i-1)) - 1; /* Low i bits set */ > > > > - if (hash_or[0][i] != m) { > > - pr_err("OR of all hash_32(%d) results = %#x " > > - "(%#x expected)", i, hash_or[0][i], m); > > - return -EINVAL; > > - } > > - if (hash_or[1][i] != m) { > > - pr_err("OR of all hash_64(%d) results = %#x " > > - "(%#x expected)", i, hash_or[1][i], m); > > - return -EINVAL; > > - } > > + KUNIT_ASSERT_EQ_MSG(test, hash_or[0][i], m, > > + "OR of all hash_32(%d) results = %#x (%#x expected)", > > + i, hash_or[0][i], m); > > + KUNIT_ASSERT_EQ_MSG(test, hash_or[1][i], m, > > + "OR of all hash_64(%d) results = %#x (%#x expected)", > > + i, hash_or[1][i], m); > > } > > - > > - pr_notice("%u tests passed.", tests); > > - > > - return 0; > > } > > > > -static void __init notice_skipped_tests(void) > > -{ > > - /* Issue notices about skipped tests. */ > > -#ifdef HAVE_ARCH__HASH_32 > > -#if HAVE_ARCH__HASH_32 != 1 > > - pr_info("__hash_32() is arch-specific; not compared to generic."); > > -#endif > > -#else > > - pr_info("__hash_32() has no arch implementation to test."); > > -#endif > > -#ifdef HAVE_ARCH_HASH_64 > > -#if HAVE_ARCH_HASH_64 != 1 > > - pr_info("hash_64() is arch-specific; not compared to generic."); > > -#endif > > -#else > > - pr_info("hash_64() has no arch implementation to test."); > > -#endif > > -} > > - > > -static int __init > > -test_hash_init(void) > > -{ > > - int ret; > > - > > - ret = test_string_or(); > > - if (ret < 0) > > - return ret; > > - > > - ret = test_hash_or(); > > - if (ret < 0) > > - return ret; > > - > > - notice_skipped_tests(); > > - > > - return ret; > > -} > > - > > -static void __exit test_hash_exit(void) > > -{ > > -} > > +static struct kunit_case hash_test_cases[] = { > > + KUNIT_CASE(test_string_or), > > + KUNIT_CASE(test_hash_or), > > Ideally, these could be split up further into separate __hash_32(), > hash_32(), and hash_64() tests. Maybe even with separate tests for > architecture-specific versus generic implementations as mentioned > above, if that made sense. It'd require enough reworking of the tests > and expected results though, that I wouldn't necessarily want to force > such a significant change at the same time as this more > straightforward port to KUnit. I agree such changes would be quite interesting and would be willing to continue working on this if I had some more knowledge of the hash functions. As of now, I'm really not sure how I'd rework those tests, unfortunately. :/ > > + {} > > +}; > > > > -module_init(test_hash_init); /* Does everything */ > > -module_exit(test_hash_exit); /* Does nothing */ > > +static struct kunit_suite hash_test_suite = { > > + .name = "hash_tests", > > It might be worth just naming this "hash", as the fact that it's a > KUnit suite already tells us it contains tests. > > > + .test_cases = hash_test_cases, > > +}; > > > > -MODULE_LICENSE("GPL"); > > It's probably worth keeping this in as it's still GPLed, and can be > built as a module. > > > +kunit_test_suite(hash_test_suite); > > > > -- > > 2.33.0 Thanks, -- Isabella Basso
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 5e5894d98c50..adefb03a7e16 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -2221,15 +2221,6 @@ config TEST_RHASHTABLE If unsure, say N. -config TEST_HASH - tristate "Perform selftest on hash functions" - help - Enable this option to test the kernel's integer (<linux/hash.h>), and - string (<linux/stringhash.h>) hash functions on boot (or module load). - - This is intended to help people writing architecture-specific - optimized versions. If unsure, say N. - config TEST_SIPHASH tristate "Perform selftest on siphash functions" help @@ -2378,6 +2369,25 @@ config BITFIELD_KUNIT If unsure, say N. +config HASH_KUNIT_TEST + tristate "KUnit Test for integer hash functions" if !KUNIT_ALL_TESTS + depends on KUNIT + default KUNIT_ALL_TESTS + help + Enable this option to test the kernel's string (<linux/stringhash.h>), and + integer (<linux/hash.h>) hash functions on boot. + + KUnit tests run during boot and output the results to the debug log + in TAP format (https://testanything.org/). Only useful for kernel devs + running the KUnit test harness, and not intended for inclusion into a + production build. + + For more information on KUnit and unit tests in general please refer + to the KUnit documentation in Documentation/dev-tools/kunit/. + + This is intended to help people writing architecture-specific + optimized versions. If unsure, say N. + config RESOURCE_KUNIT_TEST tristate "KUnit test for resource API" depends on KUNIT diff --git a/lib/Makefile b/lib/Makefile index c2e81d0eb31c..0bc336d9d036 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -62,7 +62,7 @@ obj-$(CONFIG_TEST_BITOPS) += test_bitops.o CFLAGS_test_bitops.o += -Werror obj-$(CONFIG_TEST_SYSCTL) += test_sysctl.o obj-$(CONFIG_TEST_SIPHASH) += test_siphash.o -obj-$(CONFIG_TEST_HASH) += test_hash.o +obj-$(CONFIG_HASH_KUNIT_TEST) += test_hash.o obj-$(CONFIG_TEST_IDA) += test_ida.o obj-$(CONFIG_KASAN_KUNIT_TEST) += test_kasan.o CFLAGS_test_kasan.o += -fno-builtin diff --git a/lib/test_hash.c b/lib/test_hash.c index c168823b0963..84590bbf47dc 100644 --- a/lib/test_hash.c +++ b/lib/test_hash.c @@ -14,14 +14,10 @@ * and hash_64(). */ -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt "\n" - -#include <linux/compiler.h> #include <linux/types.h> -#include <linux/module.h> #include <linux/hash.h> #include <linux/stringhash.h> -#include <linux/printk.h> +#include <kunit/test.h> #define SIZE 256 /* Run time is cubic in SIZE */ @@ -29,7 +25,7 @@ static u32 string_or; /* stores or-ed string output */ static u32 hash_or[2][33] = { { 0, } }; /* stores or-ed hash output */ /* 32-bit XORSHIFT generator. Seed must not be zero. */ -static u32 __init __attribute_const__ +static u32 __attribute_const__ xorshift(u32 seed) { seed ^= seed << 13; @@ -39,7 +35,7 @@ xorshift(u32 seed) } /* Given a non-zero x, returns a non-zero byte. */ -static u8 __init __attribute_const__ +static u8 __attribute_const__ mod255(u32 x) { x = (x & 0xffff) + (x >> 16); /* 1 <= x <= 0x1fffe */ @@ -50,8 +46,7 @@ mod255(u32 x) } /* Fill the buffer with non-zero bytes. */ -static void __init -fill_buf(char *buf, size_t len, u32 seed) +static void fill_buf(char *buf, size_t len, u32 seed) { size_t i; @@ -62,41 +57,31 @@ fill_buf(char *buf, size_t len, u32 seed) } #ifdef HAVE_ARCH__HASH_32 -static bool __init -test_int_hash32(u32 *h0, u32 *h1, u32 *h2) +static bool test_int_hash32(struct kunit *test, u32 *h0, u32 *h1, u32 *h2) { hash_or[1][0] |= *h2 = __hash_32_generic(h0); #if HAVE_ARCH__HASH_32 == 1 - if (*h1 != *h2) { - pr_err("__hash_32(%#x) = %#x != __hash_32_generic() = %#x", - *h0, *h1, *h2); - return false; - } + KUNIT_ASSERT_EQ_MSG(test, *h1, *h2, + "__hash_32(%#x) = %#x != __hash_32_generic() = %#x", + *h0, *h1, *h2); #endif - return true; } #endif #ifdef HAVE_ARCH_HASH_64 -static bool __init -test_int_hash64(unsigned long long h64, u32 *h0, u32 *h1, u32 *h2, u32 const *m, int k) +static bool test_int_hash64(struct kunit *test, unsigned long long h64, u32 *h0, u32 *h1, + u32 *h2, u32 const *m, int k) { *h2 = hash_64_generic(*h64, *k); #if HAVE_ARCH_HASH_64 == 1 - if (*h1 != *h2) { - pr_err("hash_64(%#llx, %d) = %#x != hash_64_generic() = %#x", - *h64, *k, *h1, *h2); - return false; - } + KUNIT_ASSERT_EQ_MSG(test, *h1, *h2, + "hash_64(%#llx, %d) = %#x != hash_64_generic() = %#x", + *h64, *k, *h1, *h2); #else - if (*h2 > *m) { - pr_err("hash_64_generic(%#llx, %d) = %#x > %#x", - *h64, *k, *h1, *m); - return false; - } + KUNIT_ASSERT_LE_MSG(test, *h1, *h2, + "hash_64_generic(%#llx, %d) = %#x > %#x", + *h64, *k, *h1, *m); #endif - return true; - } #endif @@ -109,8 +94,7 @@ test_int_hash64(unsigned long long h64, u32 *h0, u32 *h1, u32 *h2, u32 const *m, * inline, the code being tested is actually in the module, and you can * recompile and re-test the module without rebooting. */ -static bool __init -test_int_hash(unsigned long long h64) +static void test_int_hash(struct kunit *test, unsigned long long h64) { int k; u32 h0 = (u32)h64, h1; @@ -122,7 +106,7 @@ test_int_hash(unsigned long long h64) /* Test __hash32 */ hash_or[0][0] |= h1 = __hash_32(h0); #ifdef HAVE_ARCH__HASH_32 - if (!test_int_hash32(&h0, &h1, &h2)) + if (!test_int_hash32(test, &h0, &h1, &h2)) return false; #endif @@ -132,27 +116,22 @@ test_int_hash(unsigned long long h64) /* Test hash_32 */ hash_or[0][k] |= h1 = hash_32(h0, k); - if (h1 > m) { - pr_err("hash_32(%#x, %d) = %#x > %#x", h0, k, h1, m); - return false; - } + KUNIT_ASSERT_LE_MSG(test, h1, m, + "hash_32(%#x, %d) = %#x > %#x", + h0, k, h1, m); /* Test hash_64 */ hash_or[1][k] |= h1 = hash_64(h64, k); - if (h1 > m) { - pr_err("hash_64(%#llx, %d) = %#x > %#x", h64, k, h1, m); - return false; - } + KUNIT_ASSERT_LE_MSG(test, h1, m, + "hash_64(%#llx, %d) = %#x > %#x", + h64, k, h1, m); #ifdef HAVE_ARCH_HASH_64 - if (!test_int_hash64(&h64, &h0, &h1, &h2, &m, &k)) - return false; + test_int_hash64(test, &h64, &h0, &h1, &h2, &m, &k); #endif } - - return true; } -static int __init test_string_or(void) +static void test_string_or(struct kunit *test) { char buf[SIZE+1]; int i, j; @@ -173,19 +152,14 @@ static int __init test_string_or(void) } /* j */ /* The OR of all the hash values should cover all the bits */ - if (~string_or) { - pr_err("OR of all string hash results = %#x != %#x", - string_or, -1u); - return -EINVAL; - } - - return 0; + KUNIT_ASSERT_FALSE_MSG(test, ~string_or, + "OR of all string hash results = %#x != %#x", + string_or, -1u); } -static int __init test_hash_or(void) +static void test_hash_or(struct kunit *test) { char buf[SIZE+1]; - unsigned tests = 0; unsigned long long h64 = 0; int i, j; @@ -201,39 +175,27 @@ static int __init test_hash_or(void) u32 h0 = full_name_hash(buf+i, buf+i, j-i); /* Check that hashlen_string gets the length right */ - if (hashlen_len(hashlen) != j-i) { - pr_err("hashlen_string(%d..%d) returned length" - " %u, expected %d", - i, j, hashlen_len(hashlen), j-i); - return -EINVAL; - } + KUNIT_ASSERT_EQ_MSG(test, hashlen_len(hashlen), j-i, + "hashlen_string(%d..%d) returned length %u, expected %d", + i, j, hashlen_len(hashlen), j-i); /* Check that the hashes match */ - if (hashlen_hash(hashlen) != h0) { - pr_err("hashlen_string(%d..%d) = %08x != " - "full_name_hash() = %08x", - i, j, hashlen_hash(hashlen), h0); - return -EINVAL; - } + KUNIT_ASSERT_EQ_MSG(test, hashlen_hash(hashlen), h0, + "hashlen_string(%d..%d) = %08x != full_name_hash() = %08x", + i, j, hashlen_hash(hashlen), h0); h64 = h64 << 32 | h0; /* For use with hash_64 */ - if (!test_int_hash(h64)) - return -EINVAL; - tests++; + test_int_hash(test, h64); } /* i */ } /* j */ - if (~hash_or[0][0]) { - pr_err("OR of all __hash_32 results = %#x != %#x", - hash_or[0][0], -1u); - return -EINVAL; - } + KUNIT_ASSERT_FALSE_MSG(test, ~hash_or[0][0], + "OR of all __hash_32 results = %#x != %#x", + hash_or[0][0], -1u); #ifdef HAVE_ARCH__HASH_32 #if HAVE_ARCH__HASH_32 != 1 /* Test is pointless if results match */ - if (~hash_or[1][0]) { - pr_err("OR of all __hash_32_generic results = %#x != %#x", - hash_or[1][0], -1u); - return -EINVAL; - } + KUNIT_ASSERT_FALSE_MSG(test, ~hash_or[1][0], + "OR of all __hash_32_generic results = %#x != %#x", + hash_or[1][0], -1u); #endif #endif @@ -241,65 +203,24 @@ static int __init test_hash_or(void) for (i = 1; i <= 32; i++) { u32 const m = ((u32)2 << (i-1)) - 1; /* Low i bits set */ - if (hash_or[0][i] != m) { - pr_err("OR of all hash_32(%d) results = %#x " - "(%#x expected)", i, hash_or[0][i], m); - return -EINVAL; - } - if (hash_or[1][i] != m) { - pr_err("OR of all hash_64(%d) results = %#x " - "(%#x expected)", i, hash_or[1][i], m); - return -EINVAL; - } + KUNIT_ASSERT_EQ_MSG(test, hash_or[0][i], m, + "OR of all hash_32(%d) results = %#x (%#x expected)", + i, hash_or[0][i], m); + KUNIT_ASSERT_EQ_MSG(test, hash_or[1][i], m, + "OR of all hash_64(%d) results = %#x (%#x expected)", + i, hash_or[1][i], m); } - - pr_notice("%u tests passed.", tests); - - return 0; } -static void __init notice_skipped_tests(void) -{ - /* Issue notices about skipped tests. */ -#ifdef HAVE_ARCH__HASH_32 -#if HAVE_ARCH__HASH_32 != 1 - pr_info("__hash_32() is arch-specific; not compared to generic."); -#endif -#else - pr_info("__hash_32() has no arch implementation to test."); -#endif -#ifdef HAVE_ARCH_HASH_64 -#if HAVE_ARCH_HASH_64 != 1 - pr_info("hash_64() is arch-specific; not compared to generic."); -#endif -#else - pr_info("hash_64() has no arch implementation to test."); -#endif -} - -static int __init -test_hash_init(void) -{ - int ret; - - ret = test_string_or(); - if (ret < 0) - return ret; - - ret = test_hash_or(); - if (ret < 0) - return ret; - - notice_skipped_tests(); - - return ret; -} - -static void __exit test_hash_exit(void) -{ -} +static struct kunit_case hash_test_cases[] = { + KUNIT_CASE(test_string_or), + KUNIT_CASE(test_hash_or), + {} +}; -module_init(test_hash_init); /* Does everything */ -module_exit(test_hash_exit); /* Does nothing */ +static struct kunit_suite hash_test_suite = { + .name = "hash_tests", + .test_cases = hash_test_cases, +}; -MODULE_LICENSE("GPL"); +kunit_test_suite(hash_test_suite);