We refactored the lib/test_hash.c file into KUnit as part of the student
group LKCAMP [1] introductory hackathon for kernel development.
This test was pointed to our group by Daniel Latypov [2], so its full
conversion into a pure KUnit test was our goal in this patch series, but
we ran into many problems relating to it not being split as unit tests,
which complicated matters a bit, as the reasoning behind the original
tests is quite cryptic for those unfamiliar with hash implementations.
Some interesting developments we'd like to highlight are:
- In patch 1/6 we noticed that there was an unused define directive that
could be removed.
- In patch 5/6 we noticed how stringhash and hash tests are all under
the lib/test_hash.c file, which might cause some confusion, and we
also broke those kernel config entries up.
Overall KUnit developments have been made in the other patches in this
series:
In patches 2/6 through 4/6 and 6/6 we refactored the lib/test_hash.c
file so as to make it more compatible with the KUnit style, whilst
preserving the original idea of the maintainer who designed it (i.e.
George Spelvin), which might be undesirable for unit tests, but we
assume it is enough for a first patch.
This is our first patch series so we hope our contributions are
interesting and also hope to get some useful criticism from the
community :)
[1] - https://lkcamp.dev/
[2] - https://lore.kernel.org/linux-kselftest/CAGS_qxojszgM19u=3HLwFgKX5bm5KhywvsSunuBAt5RtR+GyxQ@mail.gmail.com/
Isabella Basso (6):
hash.h: remove unused define directive
test_hash.c: move common definitions to top of file
test_hash.c: split test_int_hash into arch-specific functions
test_hash.c: split test_hash_init
lib/Kconfig.debug: properly split hash test kernel entries
test_hash.c: refactor into kunit
include/linux/hash.h | 5 +-
lib/Kconfig.debug | 28 ++++-
lib/Makefile | 3 +-
lib/test_hash.c | 249 ++++++++++++++++---------------------
tools/include/linux/hash.h | 5 +-
5 files changed, 136 insertions(+), 154 deletions(-)
--
2.33.0
Split up test_hash_init so that it calls each test more explicitly
insofar it is possible without rewriting the entire file. This aims at
improving readability.
Split tests performed on string_or as they don't interfere with those
performed in hash_or. Also separate pr_info calls about skipped tests as
they're not part of the tests themselves, but only warn about
(un)defined arch-specific hash functions.
Signed-off-by: Isabella Basso <[email protected]>
---
lib/test_hash.c | 65 ++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 54 insertions(+), 11 deletions(-)
diff --git a/lib/test_hash.c b/lib/test_hash.c
index ed75c768c231..c168823b0963 100644
--- a/lib/test_hash.c
+++ b/lib/test_hash.c
@@ -152,8 +152,37 @@ test_int_hash(unsigned long long h64)
return true;
}
-static int __init
-test_hash_init(void)
+static int __init test_string_or(void)
+{
+ char buf[SIZE+1];
+ int i, j;
+ u32 h0;
+
+ string_or = 0;
+ fill_buf(buf, SIZE, 1);
+
+ /* Test every possible non-empty substring in the buffer. */
+ for (j = SIZE; j > 0; j--) {
+ buf[j] = '\0';
+
+ for (i = 0; i <= j; i++) {
+ h0 = full_name_hash(buf+i, buf+i, j-i);
+
+ string_or |= h0;
+ } /* i */
+ } /* 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;
+}
+
+static int __init test_hash_or(void)
{
char buf[SIZE+1];
unsigned tests = 0;
@@ -186,7 +215,6 @@ test_hash_init(void)
return -EINVAL;
}
- string_or |= h0;
h64 = h64 << 32 | h0; /* For use with hash_64 */
if (!test_int_hash(h64))
return -EINVAL;
@@ -194,12 +222,6 @@ test_hash_init(void)
} /* i */
} /* 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;
- }
if (~hash_or[0][0]) {
pr_err("OR of all __hash_32 results = %#x != %#x",
hash_or[0][0], -1u);
@@ -231,6 +253,13 @@ test_hash_init(void)
}
}
+ 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
@@ -246,10 +275,24 @@ test_hash_init(void)
#else
pr_info("hash_64() has no arch implementation to test.");
#endif
+}
- pr_notice("%u tests passed.", tests);
+static int __init
+test_hash_init(void)
+{
+ int ret;
- return 0;
+ 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)
--
2.33.0
Keep function signatures minimal by making common definitions static.
This does not change any behavior.
Signed-off-by: Isabella Basso <[email protected]>
---
lib/test_hash.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/lib/test_hash.c b/lib/test_hash.c
index d4b0cfdb0377..8bcc645a7294 100644
--- a/lib/test_hash.c
+++ b/lib/test_hash.c
@@ -23,6 +23,11 @@
#include <linux/stringhash.h>
#include <linux/printk.h>
+#define SIZE 256 /* Run time is cubic in SIZE */
+
+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__
xorshift(u32 seed)
@@ -66,7 +71,7 @@ fill_buf(char *buf, size_t len, u32 seed)
* recompile and re-test the module without rebooting.
*/
static bool __init
-test_int_hash(unsigned long long h64, u32 hash_or[2][33])
+test_int_hash(unsigned long long h64)
{
int k;
u32 h0 = (u32)h64, h1, h2;
@@ -123,17 +128,15 @@ test_int_hash(unsigned long long h64, u32 hash_or[2][33])
return true;
}
-#define SIZE 256 /* Run time is cubic in SIZE */
-
static int __init
test_hash_init(void)
{
char buf[SIZE+1];
- u32 string_or = 0, hash_or[2][33] = { { 0, } };
unsigned tests = 0;
unsigned long long h64 = 0;
int i, j;
+ string_or = 0;
fill_buf(buf, SIZE, 1);
/* Test every possible non-empty substring in the buffer. */
@@ -161,7 +164,7 @@ test_hash_init(void)
string_or |= h0;
h64 = h64 << 32 | h0; /* For use with hash_64 */
- if (!test_int_hash(h64, hash_or))
+ if (!test_int_hash(h64))
return -EINVAL;
tests++;
} /* i */
--
2.33.0
Split the The test_int_hash function to keep its mainloop separate from
arch-specific chunks, which are only compiled as needed. This aims at
improving readability.
Signed-off-by: Isabella Basso <[email protected]>
---
lib/test_hash.c | 84 +++++++++++++++++++++++++++++++------------------
1 file changed, 54 insertions(+), 30 deletions(-)
diff --git a/lib/test_hash.c b/lib/test_hash.c
index 8bcc645a7294..ed75c768c231 100644
--- a/lib/test_hash.c
+++ b/lib/test_hash.c
@@ -61,6 +61,45 @@ 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)
+{
+ 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;
+ }
+#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)
+{
+ *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;
+ }
+#else
+ if (*h2 > *m) {
+ pr_err("hash_64_generic(%#llx, %d) = %#x > %#x",
+ *h64, *k, *h1, *m);
+ return false;
+ }
+#endif
+ return true;
+
+}
+#endif
+
/*
* Test the various integer hash functions. h64 (or its low-order bits)
* is the integer to hash. hash_or accumulates the OR of the hash values,
@@ -74,19 +113,17 @@ static bool __init
test_int_hash(unsigned long long h64)
{
int k;
- u32 h0 = (u32)h64, h1, h2;
+ u32 h0 = (u32)h64, h1;
+
+#if defined HAVE_ARCH__HASH_32 || defined HAVE_ARCH_HASH_64
+ u32 h2;
+#endif
/* Test __hash32 */
hash_or[0][0] |= h1 = __hash_32(h0);
#ifdef HAVE_ARCH__HASH_32
- 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);
+ if (!test_int_hash32(&h0, &h1, &h2))
return false;
- }
-#endif
#endif
/* Test k = 1..32 bits */
@@ -107,24 +144,11 @@ test_int_hash(unsigned long long h64)
return false;
}
#ifdef HAVE_ARCH_HASH_64
- 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);
+ if (!test_int_hash64(&h64, &h0, &h1, &h2, &m, &k))
return false;
- }
-#else
- if (h2 > m) {
- pr_err("hash_64_generic(%#llx, %d) = %#x > %#x",
- h64, k, h1, m);
- return false;
- }
-#endif
#endif
}
- (void)h2; /* Suppress unused variable warning */
return true;
}
@@ -150,15 +174,15 @@ test_hash_init(void)
/* 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);
+ " %u, expected %d",
+ i, j, hashlen_len(hashlen), j-i);
return -EINVAL;
}
/* 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);
+ "full_name_hash() = %08x",
+ i, j, hashlen_hash(hashlen), h0);
return -EINVAL;
}
@@ -178,14 +202,14 @@ test_hash_init(void)
}
if (~hash_or[0][0]) {
pr_err("OR of all __hash_32 results = %#x != %#x",
- hash_or[0][0], -1u);
+ hash_or[0][0], -1u);
return -EINVAL;
}
#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);
+ hash_or[1][0], -1u);
return -EINVAL;
}
#endif
@@ -197,12 +221,12 @@ test_hash_init(void)
if (hash_or[0][i] != m) {
pr_err("OR of all hash_32(%d) results = %#x "
- "(%#x expected)", i, hash_or[0][i], m);
+ "(%#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);
+ "(%#x expected)", i, hash_or[1][i], m);
return -EINVAL;
}
}
--
2.33.0
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.
Also drop module support and remove kernel messages (i.e. through
pr_info) as KUnit handles all debugging output.
Co-developed-by: Augusto Durães Camargo <[email protected]>
Signed-off-by: Augusto Durães Camargo <[email protected]>
Co-developed-by: Enzo Ferreira <[email protected]>
Signed-off-by: Enzo Ferreira <[email protected]>
Signed-off-by: Isabella Basso <[email protected]>
---
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);
#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);
--
2.33.0
On Wed, Aug 25, 2021 at 6:26 PM Isabella Basso <[email protected]> wrote:
>
> We refactored the lib/test_hash.c file into KUnit as part of the student
> group LKCAMP [1] introductory hackathon for kernel development.
>
> This test was pointed to our group by Daniel Latypov [2], so its full
Oh, I hope I didn't lead you down a rabbit hole.
I just looked and saw that it used pr_info()'s to show that it was
skipping some stuff.
And I thought the new SKIP test support in 5.14 was something y'all
might not have been aware of, so I pointed to it as a motivating
example to rebase to get the latest KUnit patches.
I didn't do my homework and look into the test to see how suitable or
not it was for KUnit.
But I'll try and find some time soon to go over and review at the
KUnit parts of this patch series.
> conversion into a pure KUnit test was our goal in this patch series, but
> we ran into many problems relating to it not being split as unit tests,
> which complicated matters a bit, as the reasoning behind the original
> tests is quite cryptic for those unfamiliar with hash implementations.
>
> Some interesting developments we'd like to highlight are:
>
> - In patch 1/6 we noticed that there was an unused define directive that
> could be removed.
> - In patch 5/6 we noticed how stringhash and hash tests are all under
> the lib/test_hash.c file, which might cause some confusion, and we
> also broke those kernel config entries up.
>
> Overall KUnit developments have been made in the other patches in this
> series:
>
> In patches 2/6 through 4/6 and 6/6 we refactored the lib/test_hash.c
> file so as to make it more compatible with the KUnit style, whilst
> preserving the original idea of the maintainer who designed it (i.e.
> George Spelvin), which might be undesirable for unit tests, but we
> assume it is enough for a first patch.
>
> This is our first patch series so we hope our contributions are
> interesting and also hope to get some useful criticism from the
> community :)
>
> [1] - https://lkcamp.dev/
> [2] - https://lore.kernel.org/linux-kselftest/CAGS_qxojszgM19u=3HLwFgKX5bm5KhywvsSunuBAt5RtR+GyxQ@mail.gmail.com/
>
> Isabella Basso (6):
> hash.h: remove unused define directive
> test_hash.c: move common definitions to top of file
> test_hash.c: split test_int_hash into arch-specific functions
> test_hash.c: split test_hash_init
> lib/Kconfig.debug: properly split hash test kernel entries
> test_hash.c: refactor into kunit
>
> include/linux/hash.h | 5 +-
> lib/Kconfig.debug | 28 ++++-
> lib/Makefile | 3 +-
> lib/test_hash.c | 249 ++++++++++++++++---------------------
> tools/include/linux/hash.h | 5 +-
> 5 files changed, 136 insertions(+), 154 deletions(-)
>
> --
> 2.33.0
>
On Thu, Aug 26, 2021 at 9:26 AM Isabella Basso <[email protected]> wrote:
>
> Split the The test_int_hash function to keep its mainloop separate from
> arch-specific chunks, which are only compiled as needed. This aims at
> improving readability.
>
> Signed-off-by: Isabella Basso <[email protected]>
> ---
I like this, but have a note below. It _may_ be worth combining some
of these test refactoring patches with the KUnit port patch:
definitely a matter of taste rather than something I think is
necessary, but I personally think they're related enough they could go
together if you wanted.
Cheers,
-- David
> lib/test_hash.c | 84 +++++++++++++++++++++++++++++++------------------
> 1 file changed, 54 insertions(+), 30 deletions(-)
>
> diff --git a/lib/test_hash.c b/lib/test_hash.c
> index 8bcc645a7294..ed75c768c231 100644
> --- a/lib/test_hash.c
> +++ b/lib/test_hash.c
> @@ -61,6 +61,45 @@ 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)
I'm unsure about this name. Having test_int_hash32() test only
__hash_32(), where test_int_hash64() tests hash_64() feels a little
bit inconsistent. Maybe this is somewhere we should have the extra
underscore like in HAVE_ARCH__HASH_32.
I get that because the architecture-specific hash_32() is removed
earlier, there's no need for an extra function to test how that
compares against a generic function, so there's no conflict here, but
it did confuse me briefly.
The other option is, as mentioned in the earlier patch, to keep the
architecture-specific hash_32() (and _maybe_ get rid of __hash_32()
entirely), in which case this name would be perfect for testing that.
> +{
> + 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;
> + }
> +#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)
> +{
> + *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;
> + }
> +#else
> + if (*h2 > *m) {
> + pr_err("hash_64_generic(%#llx, %d) = %#x > %#x",
> + *h64, *k, *h1, *m);
> + return false;
> + }
> +#endif
> + return true;
> +
> +}
> +#endif
> +
> /*
> * Test the various integer hash functions. h64 (or its low-order bits)
> * is the integer to hash. hash_or accumulates the OR of the hash values,
> @@ -74,19 +113,17 @@ static bool __init
> test_int_hash(unsigned long long h64)
> {
> int k;
> - u32 h0 = (u32)h64, h1, h2;
> + u32 h0 = (u32)h64, h1;
> +
> +#if defined HAVE_ARCH__HASH_32 || defined HAVE_ARCH_HASH_64
> + u32 h2;
> +#endif
>
> /* Test __hash32 */
> hash_or[0][0] |= h1 = __hash_32(h0);
> #ifdef HAVE_ARCH__HASH_32
> - 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);
> + if (!test_int_hash32(&h0, &h1, &h2))
> return false;
> - }
> -#endif
> #endif
>
> /* Test k = 1..32 bits */
> @@ -107,24 +144,11 @@ test_int_hash(unsigned long long h64)
> return false;
> }
> #ifdef HAVE_ARCH_HASH_64
> - 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);
> + if (!test_int_hash64(&h64, &h0, &h1, &h2, &m, &k))
> return false;
> - }
> -#else
> - if (h2 > m) {
> - pr_err("hash_64_generic(%#llx, %d) = %#x > %#x",
> - h64, k, h1, m);
> - return false;
> - }
> -#endif
> #endif
> }
>
> - (void)h2; /* Suppress unused variable warning */
> return true;
> }
>
> @@ -150,15 +174,15 @@ test_hash_init(void)
> /* 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);
> + " %u, expected %d",
> + i, j, hashlen_len(hashlen), j-i);
These whitespace changes probably aren't necessary.
> return -EINVAL;
> }
> /* 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);
> + "full_name_hash() = %08x",
> + i, j, hashlen_hash(hashlen), h0);
These whitespace changes probably aren't necessary.
> return -EINVAL;
> }
>
> @@ -178,14 +202,14 @@ test_hash_init(void)
> }
> if (~hash_or[0][0]) {
> pr_err("OR of all __hash_32 results = %#x != %#x",
> - hash_or[0][0], -1u);
> + hash_or[0][0], -1u);
This whitespace change probably isn't necessary.
> return -EINVAL;
> }
> #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);
> + hash_or[1][0], -1u);
You get the idea...
> return -EINVAL;
> }
> #endif
> @@ -197,12 +221,12 @@ test_hash_init(void)
>
> if (hash_or[0][i] != m) {
> pr_err("OR of all hash_32(%d) results = %#x "
> - "(%#x expected)", i, hash_or[0][i], m);
> + "(%#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);
> + "(%#x expected)", i, hash_or[1][i], m);
> return -EINVAL;
> }
> }
> --
> 2.33.0
>
On Thu, Aug 26, 2021 at 9:26 AM Isabella Basso <[email protected]> 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 <[email protected]>
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 <[email protected]>
> Signed-off-by: Augusto Durães Camargo <[email protected]>
> Co-developed-by: Enzo Ferreira <[email protected]>
> Signed-off-by: Enzo Ferreira <[email protected]>
> Signed-off-by: Isabella Basso <[email protected]>
> ---
> 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 <[email protected]>
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/[email protected]
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 <[email protected]>
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/[email protected]
On Thu, 26 Aug 2021 at 03:26, 'Isabella Basso' via KUnit Development
<[email protected]> wrote:
> Keep function signatures minimal by making common definitions static.
> This does not change any behavior.
This seems like an odd change; if I read it right it's changing the
out-param passed to test_int_hash() to simply be static globals.
For one, it makes the code harder to read because now test_int_hash()
is no longer "pure" (no global side-effects ... modulo printfs), and
what was previously an out-param, is now a global.
Unfortunately this is poor style and likely to lead to hard-to-debug
problems. One such problem is if suddenly you have multiple threads
involved. While this is just a test and unlikely to be a problem, I
would recommend not introducing global state carelessly.
An alternative common idiom, where a set of variables are always
passed around to other functions, is to introduce a struct and pass a
pointer to it along.
> Signed-off-by: Isabella Basso <[email protected]>
> ---
> lib/test_hash.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/lib/test_hash.c b/lib/test_hash.c
> index d4b0cfdb0377..8bcc645a7294 100644
> --- a/lib/test_hash.c
> +++ b/lib/test_hash.c
> @@ -23,6 +23,11 @@
> #include <linux/stringhash.h>
> #include <linux/printk.h>
>
> +#define SIZE 256 /* Run time is cubic in SIZE */
> +
> +static u32 string_or; /* stores or-ed string output */
> +static u32 hash_or[2][33] = { { 0, } }; /* stores or-ed hash output */
These now use up memory for as long as this module is loaded, vs.
before where it would only use up stack space. (For a test that's not
a problem, but in non-test code it might.)
> /* 32-bit XORSHIFT generator. Seed must not be zero. */
> static u32 __init __attribute_const__
> xorshift(u32 seed)
> @@ -66,7 +71,7 @@ fill_buf(char *buf, size_t len, u32 seed)
> * recompile and re-test the module without rebooting.
> */
> static bool __init
> -test_int_hash(unsigned long long h64, u32 hash_or[2][33])
> +test_int_hash(unsigned long long h64)
> {
> int k;
> u32 h0 = (u32)h64, h1, h2;
> @@ -123,17 +128,15 @@ test_int_hash(unsigned long long h64, u32 hash_or[2][33])
> return true;
> }
>
> -#define SIZE 256 /* Run time is cubic in SIZE */
> -
> static int __init
> test_hash_init(void)
> {
> char buf[SIZE+1];
> - u32 string_or = 0, hash_or[2][33] = { { 0, } };
> unsigned tests = 0;
> unsigned long long h64 = 0;
> int i, j;
>
> + string_or = 0;
That's another problem with changes like this; now the compiler has no
chance to warn you in case the variable is not initialized correctly.
Also, I don't see string_or used anywhere else. Why make it global?
If a later change would require that, it should say so in the commit
message. But my guess is you can avoid all that by bundling everything
up in a struct.
> fill_buf(buf, SIZE, 1);
>
> /* Test every possible non-empty substring in the buffer. */
> @@ -161,7 +164,7 @@ test_hash_init(void)
>
> string_or |= h0;
> h64 = h64 << 32 | h0; /* For use with hash_64 */
> - if (!test_int_hash(h64, hash_or))
> + if (!test_int_hash(h64))
> return -EINVAL;
> tests++;
> } /* i */
> --
> 2.33.0
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20210826012626.1163705-3-isabellabdoamaral%40usp.br.
Hello, Marco,
On Thu, Aug 26, 2021 at 11:36 AM Marco Elver <[email protected]> wrote:
>
> On Thu, 26 Aug 2021 at 03:26, 'Isabella Basso' via KUnit Development
> <[email protected]> wrote:
> > Keep function signatures minimal by making common definitions static.
> > This does not change any behavior.
>
> This seems like an odd change; if I read it right it's changing the
> out-param passed to test_int_hash() to simply be static globals.
>
> For one, it makes the code harder to read because now test_int_hash()
> is no longer "pure" (no global side-effects ... modulo printfs), and
> what was previously an out-param, is now a global.
>
> Unfortunately this is poor style and likely to lead to hard-to-debug
> problems. One such problem is if suddenly you have multiple threads
> involved. While this is just a test and unlikely to be a problem, I
> would recommend not introducing global state carelessly.
I see. My peers at LKCamp and I talked over the thread-safety problems for a
while but we concluded it wasn't a big deal (precisely because this is a test).
Though being stylistically poor seems a huge heads up, so I'm really thankful
for your thorough explanation(, and review)! Noted! :)
> An alternative common idiom, where a set of variables are always
> passed around to other functions, is to introduce a struct and pass a
> pointer to it along.
>
> > Signed-off-by: Isabella Basso <[email protected]>
> > ---
> > lib/test_hash.c | 13 ++++++++-----
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/test_hash.c b/lib/test_hash.c
> > index d4b0cfdb0377..8bcc645a7294 100644
> > --- a/lib/test_hash.c
> > +++ b/lib/test_hash.c
> > @@ -23,6 +23,11 @@
> > #include <linux/stringhash.h>
> > #include <linux/printk.h>
> >
> > +#define SIZE 256 /* Run time is cubic in SIZE */
> > +
> > +static u32 string_or; /* stores or-ed string output */
> > +static u32 hash_or[2][33] = { { 0, } }; /* stores or-ed hash output */
>
> These now use up memory for as long as this module is loaded, vs.
> before where it would only use up stack space. (For a test that's not
> a problem, but in non-test code it might.)
>
> > /* 32-bit XORSHIFT generator. Seed must not be zero. */
> > static u32 __init __attribute_const__
> > xorshift(u32 seed)
> > @@ -66,7 +71,7 @@ fill_buf(char *buf, size_t len, u32 seed)
> > * recompile and re-test the module without rebooting.
> > */
> > static bool __init
> > -test_int_hash(unsigned long long h64, u32 hash_or[2][33])
> > +test_int_hash(unsigned long long h64)
> > {
> > int k;
> > u32 h0 = (u32)h64, h1, h2;
> > @@ -123,17 +128,15 @@ test_int_hash(unsigned long long h64, u32 hash_or[2][33])
> > return true;
> > }
> >
> > -#define SIZE 256 /* Run time is cubic in SIZE */
> > -
> > static int __init
> > test_hash_init(void)
> > {
> > char buf[SIZE+1];
> > - u32 string_or = 0, hash_or[2][33] = { { 0, } };
> > unsigned tests = 0;
> > unsigned long long h64 = 0;
> > int i, j;
> >
> > + string_or = 0;
>
> That's another problem with changes like this; now the compiler has no
> chance to warn you in case the variable is not initialized correctly.
>
> Also, I don't see string_or used anywhere else. Why make it global?
> If a later change would require that, it should say so in the commit
> message. But my guess is you can avoid all that by bundling everything
> up in a struct.
>
> > fill_buf(buf, SIZE, 1);
> >
> > /* Test every possible non-empty substring in the buffer. */
> > @@ -161,7 +164,7 @@ test_hash_init(void)
> >
> > string_or |= h0;
> > h64 = h64 << 32 | h0; /* For use with hash_64 */
> > - if (!test_int_hash(h64, hash_or))
> > + if (!test_int_hash(h64))
> > return -EINVAL;
> > tests++;
> > } /* i */
> > --
> > 2.33.0
Thanks,
--
Isabella Basso
Hi, David,
On Thu, Aug 26, 2021 at 1:21 AM David Gow <[email protected]> wrote:
>
> On Thu, Aug 26, 2021 at 9:26 AM Isabella Basso <[email protected]> wrote:
> >
> > Split the The test_int_hash function to keep its mainloop separate from
> > arch-specific chunks, which are only compiled as needed. This aims at
> > improving readability.
> >
> > Signed-off-by: Isabella Basso <[email protected]>
> > ---
>
> I like this, but have a note below. It _may_ be worth combining some
> of these test refactoring patches with the KUnit port patch:
> definitely a matter of taste rather than something I think is
> necessary, but I personally think they're related enough they could go
> together if you wanted.
I'm not really comfortable with such big diffs, to be honest, but I'll keep
this in mind!
> > lib/test_hash.c | 84 +++++++++++++++++++++++++++++++------------------
> > 1 file changed, 54 insertions(+), 30 deletions(-)
> >
> > diff --git a/lib/test_hash.c b/lib/test_hash.c
> > index 8bcc645a7294..ed75c768c231 100644
> > --- a/lib/test_hash.c
> > +++ b/lib/test_hash.c
> > @@ -61,6 +61,45 @@ 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)
>
> I'm unsure about this name. Having test_int_hash32() test only
> __hash_32(), where test_int_hash64() tests hash_64() feels a little
> bit inconsistent. Maybe this is somewhere we should have the extra
> underscore like in HAVE_ARCH__HASH_32.
>
> I get that because the architecture-specific hash_32() is removed
> earlier, there's no need for an extra function to test how that
> compares against a generic function, so there's no conflict here, but
> it did confuse me briefly.
I see your point. This actually hadn't occurred to me. Now I'm thinking
test_int__hash_32() (and, by extension, test_int_hash_64()) should make for a
clearer naming convention.
> The other option is, as mentioned in the earlier patch, to keep the
> architecture-specific hash_32() (and _maybe_ get rid of __hash_32()
> entirely), in which case this name would be perfect for testing that.
>
> > +{
> > + 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;
> > + }
> > +#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)
> > +{
> > + *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;
> > + }
> > +#else
> > + if (*h2 > *m) {
> > + pr_err("hash_64_generic(%#llx, %d) = %#x > %#x",
> > + *h64, *k, *h1, *m);
> > + return false;
> > + }
> > +#endif
> > + return true;
> > +
> > +}
> > +#endif
> > +
> > /*
> > * Test the various integer hash functions. h64 (or its low-order bits)
> > * is the integer to hash. hash_or accumulates the OR of the hash values,
> > @@ -74,19 +113,17 @@ static bool __init
> > test_int_hash(unsigned long long h64)
> > {
> > int k;
> > - u32 h0 = (u32)h64, h1, h2;
> > + u32 h0 = (u32)h64, h1;
> > +
> > +#if defined HAVE_ARCH__HASH_32 || defined HAVE_ARCH_HASH_64
> > + u32 h2;
> > +#endif
> >
> > /* Test __hash32 */
> > hash_or[0][0] |= h1 = __hash_32(h0);
> > #ifdef HAVE_ARCH__HASH_32
> > - 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);
> > + if (!test_int_hash32(&h0, &h1, &h2))
> > return false;
> > - }
> > -#endif
> > #endif
> >
> > /* Test k = 1..32 bits */
> > @@ -107,24 +144,11 @@ test_int_hash(unsigned long long h64)
> > return false;
> > }
> > #ifdef HAVE_ARCH_HASH_64
> > - 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);
> > + if (!test_int_hash64(&h64, &h0, &h1, &h2, &m, &k))
> > return false;
> > - }
> > -#else
> > - if (h2 > m) {
> > - pr_err("hash_64_generic(%#llx, %d) = %#x > %#x",
> > - h64, k, h1, m);
> > - return false;
> > - }
> > -#endif
> > #endif
> > }
> >
> > - (void)h2; /* Suppress unused variable warning */
> > return true;
> > }
> >
> > @@ -150,15 +174,15 @@ test_hash_init(void)
> > /* 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);
> > + " %u, expected %d",
> > + i, j, hashlen_len(hashlen), j-i);
>
> These whitespace changes probably aren't necessary.
Oops, that's my bad. Really unintended changes, thanks for the heads up!
> > return -EINVAL;
> > }
> > /* 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);
> > + "full_name_hash() = %08x",
> > + i, j, hashlen_hash(hashlen), h0);
>
> These whitespace changes probably aren't necessary.
>
> > return -EINVAL;
> > }
> >
> > @@ -178,14 +202,14 @@ test_hash_init(void)
> > }
> > if (~hash_or[0][0]) {
> > pr_err("OR of all __hash_32 results = %#x != %#x",
> > - hash_or[0][0], -1u);
> > + hash_or[0][0], -1u);
>
> This whitespace change probably isn't necessary.
>
> > return -EINVAL;
> > }
> > #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);
> > + hash_or[1][0], -1u);
>
> You get the idea...
>
> > return -EINVAL;
> > }
> > #endif
> > @@ -197,12 +221,12 @@ test_hash_init(void)
> >
> > if (hash_or[0][i] != m) {
> > pr_err("OR of all hash_32(%d) results = %#x "
> > - "(%#x expected)", i, hash_or[0][i], m);
> > + "(%#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);
> > + "(%#x expected)", i, hash_or[1][i], m);
> > return -EINVAL;
> > }
> > }
> > --
> > 2.33.0
Thanks,
--
Isabella Basso
Hi, David,
On Thu, Aug 26, 2021 at 1:26 AM David Gow <[email protected]> wrote:
>
> On Thu, Aug 26, 2021 at 9:26 AM Isabella Basso <[email protected]> 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 <[email protected]>
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 <[email protected]>
> > Signed-off-by: Augusto Durães Camargo <[email protected]>
> > Co-developed-by: Enzo Ferreira <[email protected]>
> > Signed-off-by: Enzo Ferreira <[email protected]>
> > Signed-off-by: Isabella Basso <[email protected]>
> > ---
> > 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