The current kunit execution model is to provide base kunit functionality and tests built-in to the kernel. The aim of this series is to allow building kunit itself and tests as modules. This in turn allows a simple form of selective execution; load the module you wish to test. In doing so, kunit itself (if also built as a module) will be loaded as an implicit dependency.
Because this requires a core API modification - if a module delivers multiple suites, they must be declared with the kunit_test_suites() macro - we're proposing this patch as a candidate to be applied to the test tree before too many kunit consumers appear. We attempt to deal with existing consumers in patch 1.
Changes since v1:
- sent correct patch set; apologies, previous patch set was built prior to kunit move to lib/ and should be ignored.
Patch 1 consists changes needed to support loading tests as modules. Patch 2 allows kunit itself to be loaded as a module. Patch 3 documents module support.
Alan Maguire (3): kunit: allow kunit tests to be loaded as a module kunit: allow kunit to be loaded as a module kunit: update documentation to describe module-based build
Documentation/dev-tools/kunit/faq.rst | 3 ++- Documentation/dev-tools/kunit/index.rst | 3 +++ Documentation/dev-tools/kunit/usage.rst | 16 ++++++++++++++++ include/kunit/test.h | 30 +++++++++++++++++++++++------- kernel/sysctl-test.c | 6 +++++- lib/Kconfig.debug | 4 ++-- lib/kunit/Kconfig | 6 +++--- lib/kunit/Makefile | 4 +++- lib/kunit/assert.c | 8 ++++++++ lib/kunit/example-test.c | 6 +++++- lib/kunit/string-stream-test.c | 9 +++++++-- lib/kunit/string-stream.c | 7 +++++++ lib/kunit/test-test.c | 8 ++++++-- lib/kunit/test.c | 12 ++++++++++++ lib/kunit/try-catch.c | 8 ++++++-- 15 files changed, 108 insertions(+), 22 deletions(-)
as tests are added to kunit, it will become less feasible to execute all built tests together. By supporting modular tests we provide a simple way to do selective execution on a running system; specifying
CONFIG_KUNIT=y CONFIG_KUNIT_EXAMPLE_TEST=m
...means we can simply "insmod example-test.ko" to run the tests.
To achieve this we need to
o export the required symbols in kunit o support a new way of declaring test suites. Because a module cannot do multiple late_initcall()s, we provide a kunit_test_suites() macro to declare multiple suites within the same module at once.
Signed-off-by: Alan Maguire alan.maguire@oracle.com Signed-off-by: Knut Omang knut.omang@oracle.com
--- include/kunit/test.h | 30 +++++++++++++++++++++++------- kernel/sysctl-test.c | 6 +++++- lib/Kconfig.debug | 4 ++-- lib/kunit/Kconfig | 4 ++-- lib/kunit/assert.c | 8 ++++++++ lib/kunit/example-test.c | 6 +++++- lib/kunit/string-stream-test.c | 9 +++++++-- lib/kunit/string-stream.c | 7 +++++++ lib/kunit/test-test.c | 8 ++++++-- lib/kunit/test.c | 8 ++++++++ lib/kunit/try-catch.c | 8 ++++++-- 11 files changed, 79 insertions(+), 19 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index dba4830..9fc6c1b 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -12,6 +12,7 @@ #include <kunit/assert.h> #include <kunit/try-catch.h> #include <linux/kernel.h> +#include <linux/module.h> #include <linux/slab.h> #include <linux/types.h>
@@ -204,24 +205,39 @@ struct kunit { * Registers @suite with the test framework. See &struct kunit_suite for * more information. * - * NOTE: Currently KUnit tests are all run as late_initcalls; this means + * When builtin, KUnit tests are all run as late_initcalls; this means * that they cannot test anything where tests must run at a different init * phase. One significant restriction resulting from this is that KUnit * cannot reliably test anything that is initialize in the late_init phase; * another is that KUnit is useless to test things that need to be run in * an earlier init phase. * + * An alternative is to build the tests as a module. Because modules + * do not support multiple late_initcall()s, we need to initialize an + * array of suites for a module. + * * TODO(brendanhiggins@google.com): Don't run all KUnit tests as * late_initcalls. I have some future work planned to dispatch all KUnit * tests from the same place, and at the very least to do so after * everything else is definitely initialized. */ -#define kunit_test_suite(suite) \ - static int kunit_suite_init##suite(void) \ - { \ - return kunit_run_tests(&suite); \ - } \ - late_initcall(kunit_suite_init##suite) +#define kunit_test_suites(...) \ + static struct kunit_suite *suites[] = { __VA_ARGS__, NULL}; \ + static int kunit_test_suites_init(void) \ + { \ + unsigned int i; \ + for (i = 0; suites[i] != NULL; i++) \ + kunit_run_tests(suites[i]); \ + return 0; \ + } \ + late_initcall(kunit_test_suites_init); \ + static void __exit kunit_test_suites_exit(void) \ + { \ + return; \ + } \ + module_exit(kunit_test_suites_exit) + +#define kunit_test_suite(suite) kunit_test_suites(suite)
/* * Like kunit_alloc_resource() below, but returns the struct kunit_resource diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c index 2a63241..15161c5 100644 --- a/kernel/sysctl-test.c +++ b/kernel/sysctl-test.c @@ -389,4 +389,8 @@ static void sysctl_test_api_dointvec_write_single_greater_int_max( .test_cases = sysctl_test_cases, };
-kunit_test_suite(sysctl_test_suite); +kunit_test_suite(&sysctl_test_suite); + +#ifdef MODULE +MODULE_LICENSE("GPL"); +#endif /* MODULE */ diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index a3017a5..f9f411a6 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1951,10 +1951,10 @@ config TEST_SYSCTL If unsure, say N.
config SYSCTL_KUNIT_TEST - bool "KUnit test for sysctl" + tristate "KUnit test for sysctl" depends on KUNIT help - This builds the proc sysctl unit test, which runs on boot. + This builds the proc sysctl unit test, which runs on boot/module load. Tests the API contract and implementation correctness of sysctl. For more information on KUnit and unit tests in general please refer to the KUnit documentation in Documentation/dev-tools/kunit/. diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig index af37016..9ebd5e6 100644 --- a/lib/kunit/Kconfig +++ b/lib/kunit/Kconfig @@ -15,7 +15,7 @@ menuconfig KUNIT if KUNIT
config KUNIT_TEST - bool "KUnit test for KUnit" + tristate "KUnit test for KUnit" help Enables the unit tests for the KUnit test framework. These tests test the KUnit test framework itself; the tests are both written using @@ -24,7 +24,7 @@ config KUNIT_TEST expected.
config KUNIT_EXAMPLE_TEST - bool "Example test for KUnit" + tristate "Example test for KUnit" help Enables an example unit test that illustrates some of the basic features of KUnit. This test only exists to help new users understand diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c index 86013d4..92eba2a 100644 --- a/lib/kunit/assert.c +++ b/lib/kunit/assert.c @@ -24,6 +24,7 @@ void kunit_base_assert_format(const struct kunit_assert *assert, string_stream_add(stream, "%s FAILED at %s:%d\n", expect_or_assert, assert->file, assert->line); } +EXPORT_SYMBOL_GPL(kunit_base_assert_format);
void kunit_assert_print_msg(const struct kunit_assert *assert, struct string_stream *stream) @@ -31,6 +32,7 @@ void kunit_assert_print_msg(const struct kunit_assert *assert, if (assert->message.fmt) string_stream_add(stream, "\n%pV", &assert->message); } +EXPORT_SYMBOL_GPL(kunit_assert_print_msg);
void kunit_fail_assert_format(const struct kunit_assert *assert, struct string_stream *stream) @@ -38,6 +40,7 @@ void kunit_fail_assert_format(const struct kunit_assert *assert, kunit_base_assert_format(assert, stream); string_stream_add(stream, "%pV", &assert->message); } +EXPORT_SYMBOL_GPL(kunit_fail_assert_format);
void kunit_unary_assert_format(const struct kunit_assert *assert, struct string_stream *stream) @@ -56,6 +59,7 @@ void kunit_unary_assert_format(const struct kunit_assert *assert, unary_assert->condition); kunit_assert_print_msg(assert, stream); } +EXPORT_SYMBOL_GPL(kunit_unary_assert_format);
void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, struct string_stream *stream) @@ -76,6 +80,7 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, } kunit_assert_print_msg(assert, stream); } +EXPORT_SYMBOL_GPL(kunit_ptr_not_err_assert_format);
void kunit_binary_assert_format(const struct kunit_assert *assert, struct string_stream *stream) @@ -97,6 +102,7 @@ void kunit_binary_assert_format(const struct kunit_assert *assert, binary_assert->right_value); kunit_assert_print_msg(assert, stream); } +EXPORT_SYMBOL_GPL(kunit_binary_assert_format);
void kunit_binary_ptr_assert_format(const struct kunit_assert *assert, struct string_stream *stream) @@ -118,6 +124,7 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert, binary_assert->right_value); kunit_assert_print_msg(assert, stream); } +EXPORT_SYMBOL_GPL(kunit_binary_ptr_assert_format);
void kunit_binary_str_assert_format(const struct kunit_assert *assert, struct string_stream *stream) @@ -139,3 +146,4 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert, binary_assert->right_value); kunit_assert_print_msg(assert, stream); } +EXPORT_SYMBOL_GPL(kunit_binary_str_assert_format); diff --git a/lib/kunit/example-test.c b/lib/kunit/example-test.c index f64a829..6c6a408 100644 --- a/lib/kunit/example-test.c +++ b/lib/kunit/example-test.c @@ -85,4 +85,8 @@ static int example_test_init(struct kunit *test) * This registers the above test suite telling KUnit that this is a suite of * tests that need to be run. */ -kunit_test_suite(example_test_suite); +kunit_test_suite(&example_test_suite); + +#ifdef MODULE +MODULE_LICENSE("GPL"); +#endif /* MODULE */ diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c index 76cc05e..7a3e7a0 100644 --- a/lib/kunit/string-stream-test.c +++ b/lib/kunit/string-stream-test.c @@ -45,8 +45,13 @@ static void string_stream_test_get_string(struct kunit *test) {} };
-static struct kunit_suite string_stream_test_suite = { +struct kunit_suite string_stream_test_suite = { .name = "string-stream-test", .test_cases = string_stream_test_cases }; -kunit_test_suite(string_stream_test_suite); + +kunit_test_suite(&string_stream_test_suite); + +#ifdef MODULE +MODULE_LICENSE("GPL"); +#endif /* MODULE */ diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c index e6d17aa..e4f3a97 100644 --- a/lib/kunit/string-stream.c +++ b/lib/kunit/string-stream.c @@ -100,6 +100,7 @@ int string_stream_vadd(struct string_stream *stream,
return 0; } +EXPORT_SYMBOL_GPL(string_stream_vadd);
int string_stream_add(struct string_stream *stream, const char *fmt, ...) { @@ -112,6 +113,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...)
return result; } +EXPORT_SYMBOL_GPL(string_stream_add);
static void string_stream_clear(struct string_stream *stream) { @@ -145,6 +147,7 @@ char *string_stream_get_string(struct string_stream *stream)
return buf; } +EXPORT_SYMBOL_GPL(string_stream_get_string);
int string_stream_append(struct string_stream *stream, struct string_stream *other) @@ -158,11 +161,13 @@ int string_stream_append(struct string_stream *stream,
return string_stream_add(stream, other_content); } +EXPORT_SYMBOL_GPL(string_stream_append);
bool string_stream_is_empty(struct string_stream *stream) { return list_empty(&stream->fragments); } +EXPORT_SYMBOL_GPL(string_stream_is_empty);
struct string_stream_alloc_context { struct kunit *test; @@ -207,6 +212,7 @@ struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp) gfp, &context); } +EXPORT_SYMBOL_GPL(alloc_string_stream);
int string_stream_destroy(struct string_stream *stream) { @@ -215,3 +221,4 @@ int string_stream_destroy(struct string_stream *stream) string_stream_free, stream); } +EXPORT_SYMBOL_GPL(string_stream_destroy); diff --git a/lib/kunit/test-test.c b/lib/kunit/test-test.c index 5ebe059..b5fdbe3 100644 --- a/lib/kunit/test-test.c +++ b/lib/kunit/test-test.c @@ -100,7 +100,6 @@ static int kunit_try_catch_test_init(struct kunit *test) .init = kunit_try_catch_test_init, .test_cases = kunit_try_catch_test_cases, }; -kunit_test_suite(kunit_try_catch_test_suite);
/* * Context for testing test managed resources @@ -328,4 +327,9 @@ static void kunit_resource_test_exit(struct kunit *test) .exit = kunit_resource_test_exit, .test_cases = kunit_resource_test_cases, }; -kunit_test_suite(kunit_resource_test_suite); +kunit_test_suites(&kunit_resource_test_suite, + &kunit_try_catch_test_suite); + +#ifdef MODULE +MODULE_LICENSE("GPL"); +#endif /* MODULE */ diff --git a/lib/kunit/test.c b/lib/kunit/test.c index c83c0fa..e7896f1 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -171,6 +171,7 @@ void kunit_do_assertion(struct kunit *test, if (assert->type == KUNIT_ASSERTION) kunit_abort(test); } +EXPORT_SYMBOL_GPL(kunit_do_assertion);
void kunit_init_test(struct kunit *test, const char *name) { @@ -179,6 +180,7 @@ void kunit_init_test(struct kunit *test, const char *name) test->name = name; test->success = true; } +EXPORT_SYMBOL_GPL(kunit_init_test);
/* * Initializes and runs test case. Does not clean up or do post validations. @@ -317,6 +319,7 @@ int kunit_run_tests(struct kunit_suite *suite)
return 0; } +EXPORT_SYMBOL_GPL(kunit_run_tests);
struct kunit_resource *kunit_alloc_and_get_resource(struct kunit *test, kunit_resource_init_t init, @@ -342,6 +345,7 @@ struct kunit_resource *kunit_alloc_and_get_resource(struct kunit *test,
return res; } +EXPORT_SYMBOL_GPL(kunit_alloc_and_get_resource);
static void kunit_resource_free(struct kunit *test, struct kunit_resource *res) { @@ -400,6 +404,7 @@ int kunit_resource_destroy(struct kunit *test, kunit_resource_free(test, resource); return 0; } +EXPORT_SYMBOL_GPL(kunit_resource_destroy);
struct kunit_kmalloc_params { size_t size; @@ -435,6 +440,7 @@ void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp) gfp, ¶ms); } +EXPORT_SYMBOL_GPL(kunit_kmalloc);
void kunit_kfree(struct kunit *test, const void *ptr) { @@ -447,6 +453,7 @@ void kunit_kfree(struct kunit *test, const void *ptr)
WARN_ON(rc); } +EXPORT_SYMBOL_GPL(kunit_kfree);
void kunit_cleanup(struct kunit *test) { @@ -476,3 +483,4 @@ void kunit_cleanup(struct kunit *test) kunit_resource_free(test, resource); } } +EXPORT_SYMBOL_GPL(kunit_cleanup); diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c index 55686839..a288012 100644 --- a/lib/kunit/try-catch.c +++ b/lib/kunit/try-catch.c @@ -19,6 +19,7 @@ void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch) try_catch->try_result = -EFAULT; complete_and_exit(try_catch->try_completion, -EFAULT); } +EXPORT_SYMBOL_GPL(kunit_try_catch_throw);
static int kunit_generic_run_threadfn_adapter(void *data) { @@ -50,6 +51,7 @@ static unsigned long kunit_test_timeout(void) * For more background on this topic, see: * https://mike-bland.com/2011/11/01/small-medium-large.html */ +#ifndef MODULE if (sysctl_hung_task_timeout_secs) { /* * If sysctl_hung_task is active, just set the timeout to some @@ -60,9 +62,9 @@ static unsigned long kunit_test_timeout(void) */ timeout_msecs = (sysctl_hung_task_timeout_secs - 1) * MSEC_PER_SEC; - } else { + } else +#endif timeout_msecs = 300 * MSEC_PER_SEC; /* 5 min */ - }
return timeout_msecs; } @@ -106,6 +108,7 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context)
try_catch->catch(try_catch->context); } +EXPORT_SYMBOL_GPL(kunit_try_catch_run);
void kunit_try_catch_init(struct kunit_try_catch *try_catch, struct kunit *test, @@ -116,3 +119,4 @@ void kunit_try_catch_init(struct kunit_try_catch *try_catch, try_catch->try = try; try_catch->catch = catch; } +EXPORT_SYMBOL_GPL(kunit_try_catch_init);
On Tue, Oct 08, 2019 at 03:55:44PM +0100, Alan Maguire wrote:
as tests are added to kunit, it will become less feasible to execute all built tests together. By supporting modular tests we provide a simple way to do selective execution on a running system; specifying
CONFIG_KUNIT=y CONFIG_KUNIT_EXAMPLE_TEST=m
...means we can simply "insmod example-test.ko" to run the tests.
To achieve this we need to
o export the required symbols in kunit o support a new way of declaring test suites. Because a module cannot do multiple late_initcall()s, we provide a kunit_test_suites() macro to declare multiple suites within the same module at once.
Signed-off-by: Alan Maguire alan.maguire@oracle.com Signed-off-by: Knut Omang knut.omang@oracle.com
include/kunit/test.h | 30 +++++++++++++++++++++++------- kernel/sysctl-test.c | 6 +++++- lib/Kconfig.debug | 4 ++-- lib/kunit/Kconfig | 4 ++-- lib/kunit/assert.c | 8 ++++++++ lib/kunit/example-test.c | 6 +++++- lib/kunit/string-stream-test.c | 9 +++++++-- lib/kunit/string-stream.c | 7 +++++++ lib/kunit/test-test.c | 8 ++++++-- lib/kunit/test.c | 8 ++++++++ lib/kunit/try-catch.c | 8 ++++++-- 11 files changed, 79 insertions(+), 19 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index dba4830..9fc6c1b 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -12,6 +12,7 @@ #include <kunit/assert.h> #include <kunit/try-catch.h> #include <linux/kernel.h> +#include <linux/module.h> #include <linux/slab.h> #include <linux/types.h> @@ -204,24 +205,39 @@ struct kunit {
- Registers @suite with the test framework. See &struct kunit_suite for
- more information.
- NOTE: Currently KUnit tests are all run as late_initcalls; this means
- When builtin, KUnit tests are all run as late_initcalls; this means
- that they cannot test anything where tests must run at a different init
- phase. One significant restriction resulting from this is that KUnit
- cannot reliably test anything that is initialize in the late_init phase;
- another is that KUnit is useless to test things that need to be run in
- an earlier init phase.
- An alternative is to build the tests as a module. Because modules
- do not support multiple late_initcall()s, we need to initialize an
- array of suites for a module.
*/
- TODO(brendanhiggins@google.com): Don't run all KUnit tests as
- late_initcalls. I have some future work planned to dispatch all KUnit
- tests from the same place, and at the very least to do so after
- everything else is definitely initialized.
-#define kunit_test_suite(suite) \
- static int kunit_suite_init##suite(void) \
- { \
return kunit_run_tests(&suite); \- } \
- late_initcall(kunit_suite_init##suite)
+#define kunit_test_suites(...) \
- static struct kunit_suite *suites[] = { __VA_ARGS__, NULL}; \
- static int kunit_test_suites_init(void) \
- { \
unsigned int i; \for (i = 0; suites[i] != NULL; i++) \kunit_run_tests(suites[i]); \return 0; \- } \
- late_initcall(kunit_test_suites_init); \
- static void __exit kunit_test_suites_exit(void) \
- { \
return; \- } \
- module_exit(kunit_test_suites_exit)
+#define kunit_test_suite(suite) kunit_test_suites(suite)
I think it is fine to just rename this kunit_test_suites.
/*
- Like kunit_alloc_resource() below, but returns the struct kunit_resource
diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c index 2a63241..15161c5 100644 --- a/kernel/sysctl-test.c +++ b/kernel/sysctl-test.c @@ -389,4 +389,8 @@ static void sysctl_test_api_dointvec_write_single_greater_int_max( .test_cases = sysctl_test_cases, }; -kunit_test_suite(sysctl_test_suite); +kunit_test_suite(&sysctl_test_suite);
+#ifdef MODULE +MODULE_LICENSE("GPL"); +#endif /* MODULE */
Here and elsewhere: the "ifdef/endif MODULE" should not be necessary.
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index a3017a5..f9f411a6 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1951,10 +1951,10 @@ config TEST_SYSCTL If unsure, say N. config SYSCTL_KUNIT_TEST
- bool "KUnit test for sysctl"
- tristate "KUnit test for sysctl" depends on KUNIT help
This builds the proc sysctl unit test, which runs on boot.
This builds the proc sysctl unit test, which runs on boot/module load.
[...]
diff --git a/lib/kunit/example-test.c b/lib/kunit/example-test.c index f64a829..6c6a408 100644 --- a/lib/kunit/example-test.c +++ b/lib/kunit/example-test.c @@ -85,4 +85,8 @@ static int example_test_init(struct kunit *test)
- This registers the above test suite telling KUnit that this is a suite of
- tests that need to be run.
*/ -kunit_test_suite(example_test_suite); +kunit_test_suite(&example_test_suite);
+#ifdef MODULE +MODULE_LICENSE("GPL"); +#endif /* MODULE */
nit: The "ifdef/endif MODULE" should not be necessary.
diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c index 76cc05e..7a3e7a0 100644 --- a/lib/kunit/string-stream-test.c +++ b/lib/kunit/string-stream-test.c @@ -45,8 +45,13 @@ static void string_stream_test_get_string(struct kunit *test) {} }; -static struct kunit_suite string_stream_test_suite = { +struct kunit_suite string_stream_test_suite = { .name = "string-stream-test", .test_cases = string_stream_test_cases }; -kunit_test_suite(string_stream_test_suite);
+kunit_test_suite(&string_stream_test_suite);
+#ifdef MODULE +MODULE_LICENSE("GPL"); +#endif /* MODULE */ diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c index e6d17aa..e4f3a97 100644 --- a/lib/kunit/string-stream.c +++ b/lib/kunit/string-stream.c @@ -100,6 +100,7 @@ int string_stream_vadd(struct string_stream *stream, return 0; } +EXPORT_SYMBOL_GPL(string_stream_vadd);
Is this actually needed by anything other than lib/kunit/test.c right now? Maybe we should move the include file into the kunit/ directory to hide these so no one else can use them.
int string_stream_add(struct string_stream *stream, const char *fmt, ...) { @@ -112,6 +113,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...) return result; } +EXPORT_SYMBOL_GPL(string_stream_add);
[...]
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index c83c0fa..e7896f1 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c
[...]
@@ -50,6 +51,7 @@ static unsigned long kunit_test_timeout(void) * For more background on this topic, see: * https://mike-bland.com/2011/11/01/small-medium-large.html */ +#ifndef MODULE
Why is this block of code "ifndef MODULE"?
if (sysctl_hung_task_timeout_secs) { /* * If sysctl_hung_task is active, just set the timeout to some @@ -60,9 +62,9 @@ static unsigned long kunit_test_timeout(void) */ timeout_msecs = (sysctl_hung_task_timeout_secs - 1) * MSEC_PER_SEC;
- } else {
- } else
+#endif timeout_msecs = 300 * MSEC_PER_SEC; /* 5 min */
- }
return timeout_msecs; } @@ -106,6 +108,7 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context) try_catch->catch(try_catch->context); } +EXPORT_SYMBOL_GPL(kunit_try_catch_run); void kunit_try_catch_init(struct kunit_try_catch *try_catch, struct kunit *test, @@ -116,3 +119,4 @@ void kunit_try_catch_init(struct kunit_try_catch *try_catch, try_catch->try = try; try_catch->catch = catch; } +EXPORT_SYMBOL_GPL(kunit_try_catch_init);
This code should also probably be hidden from outside of kunit/.
On Tue, 8 Oct 2019, Brendan Higgins wrote:
On Tue, Oct 08, 2019 at 03:55:44PM +0100, Alan Maguire wrote:
as tests are added to kunit, it will become less feasible to execute all built tests together. By supporting modular tests we provide a simple way to do selective execution on a running system; specifying
CONFIG_KUNIT=y CONFIG_KUNIT_EXAMPLE_TEST=m
...means we can simply "insmod example-test.ko" to run the tests.
To achieve this we need to
o export the required symbols in kunit o support a new way of declaring test suites. Because a module cannot do multiple late_initcall()s, we provide a kunit_test_suites() macro to declare multiple suites within the same module at once.
Signed-off-by: Alan Maguire alan.maguire@oracle.com Signed-off-by: Knut Omang knut.omang@oracle.com
include/kunit/test.h | 30 +++++++++++++++++++++++------- kernel/sysctl-test.c | 6 +++++- lib/Kconfig.debug | 4 ++-- lib/kunit/Kconfig | 4 ++-- lib/kunit/assert.c | 8 ++++++++ lib/kunit/example-test.c | 6 +++++- lib/kunit/string-stream-test.c | 9 +++++++-- lib/kunit/string-stream.c | 7 +++++++ lib/kunit/test-test.c | 8 ++++++-- lib/kunit/test.c | 8 ++++++++ lib/kunit/try-catch.c | 8 ++++++-- 11 files changed, 79 insertions(+), 19 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index dba4830..9fc6c1b 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -12,6 +12,7 @@ #include <kunit/assert.h> #include <kunit/try-catch.h> #include <linux/kernel.h> +#include <linux/module.h> #include <linux/slab.h> #include <linux/types.h> @@ -204,24 +205,39 @@ struct kunit {
- Registers @suite with the test framework. See &struct kunit_suite for
- more information.
- NOTE: Currently KUnit tests are all run as late_initcalls; this means
- When builtin, KUnit tests are all run as late_initcalls; this means
- that they cannot test anything where tests must run at a different init
- phase. One significant restriction resulting from this is that KUnit
- cannot reliably test anything that is initialize in the late_init phase;
- another is that KUnit is useless to test things that need to be run in
- an earlier init phase.
- An alternative is to build the tests as a module. Because modules
- do not support multiple late_initcall()s, we need to initialize an
- array of suites for a module.
*/
- TODO(brendanhiggins@google.com): Don't run all KUnit tests as
- late_initcalls. I have some future work planned to dispatch all KUnit
- tests from the same place, and at the very least to do so after
- everything else is definitely initialized.
-#define kunit_test_suite(suite) \
- static int kunit_suite_init##suite(void) \
- { \
return kunit_run_tests(&suite); \- } \
- late_initcall(kunit_suite_init##suite)
+#define kunit_test_suites(...) \
- static struct kunit_suite *suites[] = { __VA_ARGS__, NULL}; \
- static int kunit_test_suites_init(void) \
- { \
unsigned int i; \for (i = 0; suites[i] != NULL; i++) \kunit_run_tests(suites[i]); \return 0; \- } \
- late_initcall(kunit_test_suites_init); \
- static void __exit kunit_test_suites_exit(void) \
- { \
return; \- } \
- module_exit(kunit_test_suites_exit)
+#define kunit_test_suite(suite) kunit_test_suites(suite)
I think it is fine to just rename this kunit_test_suites.
Will do.
/*
- Like kunit_alloc_resource() below, but returns the struct kunit_resource
diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c index 2a63241..15161c5 100644 --- a/kernel/sysctl-test.c +++ b/kernel/sysctl-test.c @@ -389,4 +389,8 @@ static void sysctl_test_api_dointvec_write_single_greater_int_max( .test_cases = sysctl_test_cases, }; -kunit_test_suite(sysctl_test_suite); +kunit_test_suite(&sysctl_test_suite);
+#ifdef MODULE +MODULE_LICENSE("GPL"); +#endif /* MODULE */
Here and elsewhere: the "ifdef/endif MODULE" should not be necessary.
Will fix, thanks!
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index a3017a5..f9f411a6 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1951,10 +1951,10 @@ config TEST_SYSCTL If unsure, say N. config SYSCTL_KUNIT_TEST
- bool "KUnit test for sysctl"
- tristate "KUnit test for sysctl" depends on KUNIT help
This builds the proc sysctl unit test, which runs on boot.
This builds the proc sysctl unit test, which runs on boot/module load.[...]
diff --git a/lib/kunit/example-test.c b/lib/kunit/example-test.c index f64a829..6c6a408 100644 --- a/lib/kunit/example-test.c +++ b/lib/kunit/example-test.c @@ -85,4 +85,8 @@ static int example_test_init(struct kunit *test)
- This registers the above test suite telling KUnit that this is a suite of
- tests that need to be run.
*/ -kunit_test_suite(example_test_suite); +kunit_test_suite(&example_test_suite);
+#ifdef MODULE +MODULE_LICENSE("GPL"); +#endif /* MODULE */
nit: The "ifdef/endif MODULE" should not be necessary.
diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c index 76cc05e..7a3e7a0 100644 --- a/lib/kunit/string-stream-test.c +++ b/lib/kunit/string-stream-test.c @@ -45,8 +45,13 @@ static void string_stream_test_get_string(struct kunit *test) {} }; -static struct kunit_suite string_stream_test_suite = { +struct kunit_suite string_stream_test_suite = { .name = "string-stream-test", .test_cases = string_stream_test_cases }; -kunit_test_suite(string_stream_test_suite);
+kunit_test_suite(&string_stream_test_suite);
+#ifdef MODULE +MODULE_LICENSE("GPL"); +#endif /* MODULE */ diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c index e6d17aa..e4f3a97 100644 --- a/lib/kunit/string-stream.c +++ b/lib/kunit/string-stream.c @@ -100,6 +100,7 @@ int string_stream_vadd(struct string_stream *stream, return 0; } +EXPORT_SYMBOL_GPL(string_stream_vadd);
Is this actually needed by anything other than lib/kunit/test.c right now? Maybe we should move the include file into the kunit/ directory to hide these so no one else can use them.
I tried this, and it's the right answer I think but it exposes a problem with symbol visibility when kunit is compiled as a module. More on this below...
int string_stream_add(struct string_stream *stream, const char *fmt, ...) { @@ -112,6 +113,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...) return result; } +EXPORT_SYMBOL_GPL(string_stream_add);
[...]
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index c83c0fa..e7896f1 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c
[...]
@@ -50,6 +51,7 @@ static unsigned long kunit_test_timeout(void) * For more background on this topic, see: * https://mike-bland.com/2011/11/01/small-medium-large.html */ +#ifndef MODULE
Why is this block of code "ifndef MODULE"?
Symbol visibility is the problem again; sysctl_hung_task_timeout_secs isn't exported so when kunit is a module it can't find the symbol.
I think I saw Kees mentioned something about symbol lookup too; in KTF Knut solved this by defining ktf_find_symbol(). I'd suggest we may need a kunit_find_symbol() with a function signature
void *kunit_find_symbol(const char *modname, const char *symbol_name);
...which does a [module_]kallsyms_lookup_sym().
If the above makes sense I can look at adding it as a patch (and adding a test of it of course!). What do you think?
if (sysctl_hung_task_timeout_secs) { /* * If sysctl_hung_task is active, just set the timeout to some @@ -60,9 +62,9 @@ static unsigned long kunit_test_timeout(void) */ timeout_msecs = (sysctl_hung_task_timeout_secs - 1) * MSEC_PER_SEC;
- } else {
- } else
+#endif timeout_msecs = 300 * MSEC_PER_SEC; /* 5 min */
- }
return timeout_msecs; } @@ -106,6 +108,7 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context) try_catch->catch(try_catch->context); } +EXPORT_SYMBOL_GPL(kunit_try_catch_run); void kunit_try_catch_init(struct kunit_try_catch *try_catch, struct kunit *test, @@ -116,3 +119,4 @@ void kunit_try_catch_init(struct kunit_try_catch *try_catch, try_catch->try = try; try_catch->catch = catch; } +EXPORT_SYMBOL_GPL(kunit_try_catch_init);
This code should also probably be hidden from outside of kunit/.
Sure, will do.
Thanks again for the review!
Alan
Sorry for the delayed reply. I will be on vacation until Wednesday, October 16th.
On Wed, Oct 9, 2019 at 9:36 AM Alan Maguire alan.maguire@oracle.com wrote:
On Tue, 8 Oct 2019, Brendan Higgins wrote:
On Tue, Oct 08, 2019 at 03:55:44PM +0100, Alan Maguire wrote:
[...]
diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c index e6d17aa..e4f3a97 100644 --- a/lib/kunit/string-stream.c +++ b/lib/kunit/string-stream.c @@ -100,6 +100,7 @@ int string_stream_vadd(struct string_stream *stream,
return 0;} +EXPORT_SYMBOL_GPL(string_stream_vadd);
Is this actually needed by anything other than lib/kunit/test.c right now? Maybe we should move the include file into the kunit/ directory to hide these so no one else can use them.
I tried this, and it's the right answer I think but it exposes a problem with symbol visibility when kunit is compiled as a module. More on this below...
int string_stream_add(struct string_stream *stream, const char *fmt, ...) { @@ -112,6 +113,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...)
return result;} +EXPORT_SYMBOL_GPL(string_stream_add);
[...]
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index c83c0fa..e7896f1 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c
[...]
@@ -50,6 +51,7 @@ static unsigned long kunit_test_timeout(void) * For more background on this topic, see: * https://mike-bland.com/2011/11/01/small-medium-large.html */ +#ifndef MODULE
Why is this block of code "ifndef MODULE"?
Symbol visibility is the problem again; sysctl_hung_task_timeout_secs isn't exported so when kunit is a module it can't find the symbol.
I think I saw Kees mentioned something about symbol lookup too; in KTF Knut solved this by defining ktf_find_symbol(). I'd suggest we may need a kunit_find_symbol() with a function signature
I thought we were just talking about exposing symbols for linking outside of a compilation unit (static vs. not static); nevertheless, I think you are right that it is relevant here. Kees, thoughts?
void *kunit_find_symbol(const char *modname, const char *symbol_name);
...which does a [module_]kallsyms_lookup_sym().
If the above makes sense I can look at adding it as a patch (and adding a test of it of course!). What do you think?
So that won't work if you are trying to link against a symbol not in a module, right? Also, it won't work against a static symbol, right?
Even so, I think it is pretty wonky to expect users to either a) export any symbol name to be tested, or b) have to access them via kunit_find_symbol. I think it is fine to have some tests that cannot be compiled as modules, if there is no other user friendly way to make this work in those cases.
Thoughts?
On Fri, 11 Oct 2019, Brendan Higgins wrote:
Sorry for the delayed reply. I will be on vacation until Wednesday, October 16th.
On Wed, Oct 9, 2019 at 9:36 AM Alan Maguire alan.maguire@oracle.com wrote:
On Tue, 8 Oct 2019, Brendan Higgins wrote:
On Tue, Oct 08, 2019 at 03:55:44PM +0100, Alan Maguire wrote:
[...]
diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c index e6d17aa..e4f3a97 100644 --- a/lib/kunit/string-stream.c +++ b/lib/kunit/string-stream.c @@ -100,6 +100,7 @@ int string_stream_vadd(struct string_stream *stream,
return 0;} +EXPORT_SYMBOL_GPL(string_stream_vadd);
Is this actually needed by anything other than lib/kunit/test.c right now? Maybe we should move the include file into the kunit/ directory to hide these so no one else can use them.
I tried this, and it's the right answer I think but it exposes a problem with symbol visibility when kunit is compiled as a module. More on this below...
int string_stream_add(struct string_stream *stream, const char *fmt, ...) { @@ -112,6 +113,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...)
return result;} +EXPORT_SYMBOL_GPL(string_stream_add);
[...]
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index c83c0fa..e7896f1 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c
[...]
@@ -50,6 +51,7 @@ static unsigned long kunit_test_timeout(void) * For more background on this topic, see: * https://mike-bland.com/2011/11/01/small-medium-large.html */ +#ifndef MODULE
Why is this block of code "ifndef MODULE"?
Symbol visibility is the problem again; sysctl_hung_task_timeout_secs isn't exported so when kunit is a module it can't find the symbol.
I think I saw Kees mentioned something about symbol lookup too; in KTF Knut solved this by defining ktf_find_symbol(). I'd suggest we may need a kunit_find_symbol() with a function signature
I thought we were just talking about exposing symbols for linking outside of a compilation unit (static vs. not static); nevertheless, I think you are right that it is relevant here. Kees, thoughts?
void *kunit_find_symbol(const char *modname, const char *symbol_name);
...which does a [module_]kallsyms_lookup_sym().
If the above makes sense I can look at adding it as a patch (and adding a test of it of course!). What do you think?
So that won't work if you are trying to link against a symbol not in a module, right? Also, it won't work against a static symbol, right?
Nope, works in both cases with the proviso that we need to use an alternative name for symbols when compiling built-in. For example in the case of the string-stream tests, we'd use a test init callback to initialize used symbols:
static int string_stream_test_init(struct kunit *test) { _alloc_string_stream = kunit_find_symbol("alloc_string_stream"); _string_stream_add = kunit_find_symbol("string_stream_add"); _string_stream_get_string = kunit_find_symbol("string_stream_get_string"); _string_stream_is_empty = kunit_find_symbol("string_stream_is_empty"); if (IS_ERR(_alloc_string_stream) || IS_ERR(_string_stream_add) || IS_ERR(_string_stream_get_string) || IS_ERR(_string_stream_is_empty)) return EINVAL; return 0; }
I've tested this when string-stream-test is compiled built-in and as a module. We can of course create a wrapper macro to handle these assignments.
To illustrate further here's the test cases I'd propose adding to test-test.c with the changes.
In the first case we're grabbing the "modules" variable from the kernel, and in the second we're grabbing a static symbol from the test-test.ko module (when it is compiled as a module):
/* * Find non-exported kernel symbol; we use the modules list as a safe * choice that should always be present. */ static void kunit_find_symbol_kernel(struct kunit *test) { KUNIT_ASSERT_NOT_ERR_OR_NULL(test, kunit_find_symbol("modules")); }
#ifdef MODULE /* * If we are compiled as a module, use this module for lookup. */ static void kunit_find_symbol_module(struct kunit *test) { KUNIT_ASSERT_NOT_ERR_OR_NULL(test, kunit_find_symbol("kunit_find_symbol_kernel")); } #endif
Even so, I think it is pretty wonky to expect users to either a) export any symbol name to be tested,
Absolutely not, I'd never advocate that. Nothing should need to change in the component under test simply to facilitate testing, especially if there's a way the test framework can work around it.
or b) have to access them via kunit_find_symbol. I think it is fine to have some tests that cannot be compiled as modules, if there is no other user friendly way to make this work in those cases.
That's fine, and I agree in some cases it's unworkable, but there are going to be a lot of tristate componenets we'd like to test, and restricting testing of those by requiring CONFIG_FOO=y seems like a limitation too. In practice I've found symbol lookup isn't needed extensively for test development. For cases where the weight of symbol lookup is too heavy the tests can simply stay built-in - the non-exported nature of the symbols is probably suggesting something about the nature of the interface that makes that a more natural choice anyway. However for other cases I think there's value to having something like this feature. Of course there may be better ways to realize the functionality than what I'm proposing.
Thanks!
Alan
On Fri, Oct 11, 2019 at 11:25:33AM +0100, Alan Maguire wrote:
On Fri, 11 Oct 2019, Brendan Higgins wrote:
Sorry for the delayed reply. I will be on vacation until Wednesday, October 16th.
On Wed, Oct 9, 2019 at 9:36 AM Alan Maguire alan.maguire@oracle.com wrote:
On Tue, 8 Oct 2019, Brendan Higgins wrote:
On Tue, Oct 08, 2019 at 03:55:44PM +0100, Alan Maguire wrote:
[...]
diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c index e6d17aa..e4f3a97 100644 --- a/lib/kunit/string-stream.c +++ b/lib/kunit/string-stream.c @@ -100,6 +100,7 @@ int string_stream_vadd(struct string_stream *stream,
return 0;} +EXPORT_SYMBOL_GPL(string_stream_vadd);
Is this actually needed by anything other than lib/kunit/test.c right now? Maybe we should move the include file into the kunit/ directory to hide these so no one else can use them.
I tried this, and it's the right answer I think but it exposes a problem with symbol visibility when kunit is compiled as a module. More on this below...
int string_stream_add(struct string_stream *stream, const char *fmt, ...) { @@ -112,6 +113,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...)
return result;} +EXPORT_SYMBOL_GPL(string_stream_add);
[...]
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index c83c0fa..e7896f1 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c
[...]
@@ -50,6 +51,7 @@ static unsigned long kunit_test_timeout(void) * For more background on this topic, see: * https://mike-bland.com/2011/11/01/small-medium-large.html */ +#ifndef MODULE
Why is this block of code "ifndef MODULE"?
Symbol visibility is the problem again; sysctl_hung_task_timeout_secs isn't exported so when kunit is a module it can't find the symbol.
I think I saw Kees mentioned something about symbol lookup too; in KTF Knut solved this by defining ktf_find_symbol(). I'd suggest we may need a kunit_find_symbol() with a function signature
Based on what you said below, I think the kunit_find_symbol() may have value for writing tests; however, I do not think it is the right way to handle resources needed by test.c. I think exporting the symbols in this case is the lesser of the two evils.
I am still suprised that you need to export a symbol that is getting compiled into and is only used by the kunit module. In fact, I think I found an example in the kernel where someone else managed this. Checkout stp_policy_node_priv(). Looks like the symbol is used here[1] and is defined here[2]. You can see here[3] and here[4] that the files end up in the same module. Do you mind taking a look why it works for stm, but not here?
I thought we were just talking about exposing symbols for linking outside of a compilation unit (static vs. not static); nevertheless, I think you are right that it is relevant here. Kees, thoughts?
void *kunit_find_symbol(const char *modname, const char *symbol_name);
...which does a [module_]kallsyms_lookup_sym().
If the above makes sense I can look at adding it as a patch (and adding a test of it of course!). What do you think?
So that won't work if you are trying to link against a symbol not in a module, right? Also, it won't work against a static symbol, right?
Nope, works in both cases with the proviso that we need to use an
Nifty! That sounds great!
alternative name for symbols when compiling built-in. For example
Can you elaborate on "need[ing] to use an alternative name"?
in the case of the string-stream tests, we'd use a test init callback to initialize used symbols:
static int string_stream_test_init(struct kunit *test) { _alloc_string_stream = kunit_find_symbol("alloc_string_stream"); _string_stream_add = kunit_find_symbol("string_stream_add"); _string_stream_get_string = kunit_find_symbol("string_stream_get_string"); _string_stream_is_empty = kunit_find_symbol("string_stream_is_empty"); if (IS_ERR(_alloc_string_stream) || IS_ERR(_string_stream_add) || IS_ERR(_string_stream_get_string) || IS_ERR(_string_stream_is_empty)) return EINVAL; return 0; }
I've tested this when string-stream-test is compiled built-in and as a module. We can of course create a wrapper macro to handle these assignments.
I've got mixed feelings on this. On one hand, that has the potential to solve a lot of problems with visibility and modules in a way that doesn't immediately cause code under test to change in undesirable ways. On the other hand, I feel that this has the potential to be really prone to breakage. It would be much nicer if the compiler could tell you that your symbol changed rather than having to wait until you run the test. Just having the test tell you that a symbol doesn't exist anymore would be mildly annoying, but having the signature of the symbol change could get downright frustrating using this method.
To illustrate further here's the test cases I'd propose adding to test-test.c with the changes.
In the first case we're grabbing the "modules" variable from the kernel, and in the second we're grabbing a static symbol from the test-test.ko module (when it is compiled as a module):
/*
- Find non-exported kernel symbol; we use the modules list as a safe
- choice that should always be present.
*/ static void kunit_find_symbol_kernel(struct kunit *test) { KUNIT_ASSERT_NOT_ERR_OR_NULL(test, kunit_find_symbol("modules")); }
#ifdef MODULE /*
- If we are compiled as a module, use this module for lookup.
*/ static void kunit_find_symbol_module(struct kunit *test) { KUNIT_ASSERT_NOT_ERR_OR_NULL(test, kunit_find_symbol("kunit_find_symbol_kernel")); } #endif
Even so, I think it is pretty wonky to expect users to either a) export any symbol name to be tested,
Absolutely not, I'd never advocate that. Nothing should need to change in
Cool. Looks like we are on the same page then :-)
the component under test simply to facilitate testing, especially if there's a way the test framework can work around it.
I would say it depends. I think it is fine to ask people to write code in such a manner that makes it more testable. From my experience, this generally leads to better quality code.
Nevertheless, I agree from the perspective that suddenly having to export symbol names for no good reason is not something we should make users do.
or b) have to access them via kunit_find_symbol. I think it is fine to have some tests that cannot be compiled as modules, if there is no other user friendly way to make this work in those cases.
That's fine, and I agree in some cases it's unworkable, but there are going to be a lot of tristate componenets we'd like to test, and restricting testing of those by requiring CONFIG_FOO=y seems like a limitation too. In practice I've found symbol lookup isn't needed extensively for test development. For cases where the weight of symbol lookup is too heavy the tests can simply stay built-in - the non-exported nature of the symbols is probably suggesting something about the nature of the interface that makes that a more natural choice anyway. However for other cases I think there's value to having something like this feature.
I think that makes sense.
So, I don't think the symbol lookup is needed for this patchset. I think all the symbols that you use should *probably* be exported at worse since they are used by the core kunit library stuff.
Nevertheless, I have a test coming down the pipeline for which this could be a potential solution. I will CC you on the test when I send it.
Of course there may be better ways to realize the functionality than what I'm proposing.
Cheers!
[1] https://elixir.bootlin.com/linux/v5.3.6/source/drivers/hwtracing/stm/core.c#... [2] https://elixir.bootlin.com/linux/v5.3.6/source/drivers/hwtracing/stm/policy.... [3] https://elixir.bootlin.com/linux/v5.3.6/source/drivers/hwtracing/stm/Makefil... [4] https://elixir.bootlin.com/linux/v5.3.6/source/drivers/hwtracing/stm/Kconfig...
On Wed, 16 Oct 2019, Brendan Higgins wrote:
On Fri, Oct 11, 2019 at 11:25:33AM +0100, Alan Maguire wrote:
On Fri, 11 Oct 2019, Brendan Higgins wrote:
Sorry for the delayed reply. I will be on vacation until Wednesday, October 16th.
On Wed, Oct 9, 2019 at 9:36 AM Alan Maguire alan.maguire@oracle.com wrote:
On Tue, 8 Oct 2019, Brendan Higgins wrote:
On Tue, Oct 08, 2019 at 03:55:44PM +0100, Alan Maguire wrote:
[...]
diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c index e6d17aa..e4f3a97 100644 --- a/lib/kunit/string-stream.c +++ b/lib/kunit/string-stream.c @@ -100,6 +100,7 @@ int string_stream_vadd(struct string_stream *stream,
return 0;} +EXPORT_SYMBOL_GPL(string_stream_vadd);
Is this actually needed by anything other than lib/kunit/test.c right now? Maybe we should move the include file into the kunit/ directory to hide these so no one else can use them.
I tried this, and it's the right answer I think but it exposes a problem with symbol visibility when kunit is compiled as a module. More on this below...
int string_stream_add(struct string_stream *stream, const char *fmt, ...) { @@ -112,6 +113,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...)
return result;} +EXPORT_SYMBOL_GPL(string_stream_add);
[...]
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index c83c0fa..e7896f1 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c
[...]
@@ -50,6 +51,7 @@ static unsigned long kunit_test_timeout(void) * For more background on this topic, see: * https://mike-bland.com/2011/11/01/small-medium-large.html */ +#ifndef MODULE
Why is this block of code "ifndef MODULE"?
Symbol visibility is the problem again; sysctl_hung_task_timeout_secs isn't exported so when kunit is a module it can't find the symbol.
I think I saw Kees mentioned something about symbol lookup too; in KTF Knut solved this by defining ktf_find_symbol(). I'd suggest we may need a kunit_find_symbol() with a function signature
Based on what you said below, I think the kunit_find_symbol() may have value for writing tests; however, I do not think it is the right way to handle resources needed by test.c. I think exporting the symbols in this case is the lesser of the two evils.
The only symbol we need in core kunit from the kernel when compiling kunit as a module is sysctl_hung_task_timeout_secs; it's a core kernel symbol. There's no issue with symbols within kunit in this case.
I've come up with a new way to handle variables and functions in the v3 patch set 3 [1] I've sent out. While not being perfect, it attempts to satisfy some of the requirements you describe below. It will generate compiler errors if there is a mismatch between local symbol definition and the target symbol type.
Symbol variable definitions are handled such that the same symbol name can be used; see try-catch.c in patch 5 where we assign sysctl_hung_task_timeout_secs.
Unfortunately the same scheme won't work for functions. The reason for this is we've already #included a definiton of the function, so if we attempt to redefine that same name as a function pointer we get a compile-time that we are redefining the symbol. As a consequence the approach I took is for us to define a local function pointer and it gets assigned either to
- the results of kunit_find_symbol() (module case) - the function itself (builtin case)
The latter will trigger a compile-time error if our local definition is out of sync.
I am still suprised that you need to export a symbol that is
getting
compiled into and is only used by the kunit module.
see above - kunit needs a non-exported global kernel symbol (sysctl_hung_task_timeout_secs).
In fact, I think I found an example in the kernel where someone else managed this. Checkout stp_policy_node_priv(). Looks like the symbol is used here[1] and is defined here[2]. You can see here[3] and here[4] that the files end up in the same module. Do you mind taking a look why it works for stm, but not here?
I thought we were just talking about exposing symbols for linking outside of a compilation unit (static vs. not static); nevertheless, I think you are right that it is relevant here. Kees, thoughts?
void *kunit_find_symbol(const char *modname, const char *symbol_name);
...which does a [module_]kallsyms_lookup_sym().
If the above makes sense I can look at adding it as a patch (and adding a test of it of course!). What do you think?
So that won't work if you are trying to link against a symbol not in a module, right? Also, it won't work against a static symbol, right?
Nope, works in both cases with the proviso that we need to use an
Nifty! That sounds great!
alternative name for symbols when compiling built-in. For example
Can you elaborate on "need[ing] to use an alternative name"?
See above and patch 4 in the v3 patchset.
in the case of the string-stream tests, we'd use a test init callback to initialize used symbols:
static int string_stream_test_init(struct kunit *test) { _alloc_string_stream = kunit_find_symbol("alloc_string_stream"); _string_stream_add = kunit_find_symbol("string_stream_add"); _string_stream_get_string = kunit_find_symbol("string_stream_get_string"); _string_stream_is_empty = kunit_find_symbol("string_stream_is_empty"); if (IS_ERR(_alloc_string_stream) || IS_ERR(_string_stream_add) || IS_ERR(_string_stream_get_string) || IS_ERR(_string_stream_is_empty)) return EINVAL; return 0; }
I've tested this when string-stream-test is compiled built-in and as a module. We can of course create a wrapper macro to handle these assignments.
I've got mixed feelings on this. On one hand, that has the potential to solve a lot of problems with visibility and modules in a way that doesn't immediately cause code under test to change in undesirable ways. On the other hand, I feel that this has the potential to be really prone to breakage. It would be much nicer if the compiler could tell you that your symbol changed rather than having to wait until you run the test. Just having the test tell you that a symbol doesn't exist anymore would be mildly annoying, but having the signature of the symbol change could get downright frustrating using this method.
See above; if compiled as builtin compiler errors will be generated.
Thanks!
Alan
On Thu, Oct 17, 2019 at 07:32:18PM +0100, Alan Maguire wrote:
kunit needs a non-exported global kernel symbol (sysctl_hung_task_timeout_secs).
Sounds like a perfect use case for the new symbol namespaces [0]. We wouldn't want random drivers importing this namespace, but for kunit it would seem reasonable.
[0] https://lwn.net/Articles/798254/
Luis
On Fri, Oct 18, 2019 at 5:21 AM Luis Chamberlain mcgrof@kernel.org wrote:
On Thu, Oct 17, 2019 at 07:32:18PM +0100, Alan Maguire wrote:
kunit needs a non-exported global kernel symbol (sysctl_hung_task_timeout_secs).
Sounds like a perfect use case for the new symbol namespaces [0]. We wouldn't want random drivers importing this namespace, but for kunit it would seem reasonable.
Sounds good to me.
Making kunit itself buildable as a module allows for "always-on" kunit configuration; specifying CONFIG_KUNIT=m means the module is built but only used when loaded. Kunit test modules will load kunit.ko as an implicit dependency, so simply running "modprobe my-kunit-tests" will load the tests along with the kunit module and run them.
Signed-off-by: Alan Maguire alan.maguire@oracle.com Signed-off-by: Knut Omang knut.omang@oracle.com
--- lib/kunit/Kconfig | 2 +- lib/kunit/Makefile | 4 +++- lib/kunit/test.c | 4 ++++ 3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig index 9ebd5e6..065aa16 100644 --- a/lib/kunit/Kconfig +++ b/lib/kunit/Kconfig @@ -3,7 +3,7 @@ #
menuconfig KUNIT - bool "KUnit - Enable support for unit tests" + tristate "KUnit - Enable support for unit tests" help Enables support for kernel unit tests (KUnit), a lightweight unit testing and mocking framework for the Linux kernel. These tests are diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile index 769d940..8e2635a 100644 --- a/lib/kunit/Makefile +++ b/lib/kunit/Makefile @@ -1,4 +1,6 @@ -obj-$(CONFIG_KUNIT) += test.o \ +obj-$(CONFIG_KUNIT) += kunit.o + +kunit-objs += test.o \ string-stream.o \ assert.o \ try-catch.o diff --git a/lib/kunit/test.c b/lib/kunit/test.c index e7896f1..6024627 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -484,3 +484,7 @@ void kunit_cleanup(struct kunit *test) } } EXPORT_SYMBOL_GPL(kunit_cleanup); + +#ifdef MODULE +MODULE_LICENSE("GPL"); +#endif /* MODULE */
On 10/8/19 7:55 AM, Alan Maguire wrote:
Making kunit itself buildable as a module allows for "always-on" kunit configuration; specifying CONFIG_KUNIT=m means the module is built but only used when loaded. Kunit test modules will load kunit.ko as an implicit dependency, so simply running "modprobe my-kunit-tests" will load the tests along with the kunit module and run them.
Signed-off-by: Alan Maguire alan.maguire@oracle.com Signed-off-by: Knut Omang knut.omang@oracle.com
lib/kunit/Kconfig | 2 +- lib/kunit/Makefile | 4 +++- lib/kunit/test.c | 4 ++++ 3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index e7896f1..6024627 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -484,3 +484,7 @@ void kunit_cleanup(struct kunit *test) } } EXPORT_SYMBOL_GPL(kunit_cleanup);
+#ifdef MODULE +MODULE_LICENSE("GPL"); +#endif /* MODULE */
That ifdef/endif should not be necessary. Did you try a modular build without them?
Documentation should describe how to build kunit and tests as modules.
Signed-off-by: Alan Maguire alan.maguire@oracle.com Signed-off-by: Knut Omang knut.omang@oracle.com
--- Documentation/dev-tools/kunit/faq.rst | 3 ++- Documentation/dev-tools/kunit/index.rst | 3 +++ Documentation/dev-tools/kunit/usage.rst | 16 ++++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/Documentation/dev-tools/kunit/faq.rst b/Documentation/dev-tools/kunit/faq.rst index bf20951..ea55b24 100644 --- a/Documentation/dev-tools/kunit/faq.rst +++ b/Documentation/dev-tools/kunit/faq.rst @@ -29,7 +29,8 @@ Yes, well, mostly.
For the most part, the KUnit core framework (what you use to write the tests) can compile to any architecture; it compiles like just another part of the -kernel and runs when the kernel boots. However, there is some infrastructure, +kernel and runs when the kernel boots, or when built as a module, when the +module is loaded. However, there is some infrastructure, like the KUnit Wrapper (``tools/testing/kunit/kunit.py``) that does not support other architectures.
diff --git a/Documentation/dev-tools/kunit/index.rst b/Documentation/dev-tools/kunit/index.rst index 26ffb46..7ddc385 100644 --- a/Documentation/dev-tools/kunit/index.rst +++ b/Documentation/dev-tools/kunit/index.rst @@ -48,6 +48,9 @@ to a standalone program that can be run like any other program directly inside of a host operating system; to be clear, it does not require any virtualization support; it is just a regular program.
+Alternatively, kunit and kunit tests can be built as modules and tests will +run when the test module is loaded. + KUnit is fast. Excluding build time, from invocation to completion KUnit can run several dozen tests in only 10 to 20 seconds; this might not sound like a big deal to some people, but having such fast and easy to run tests fundamentally diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst index c6e6963..fa0f03f 100644 --- a/Documentation/dev-tools/kunit/usage.rst +++ b/Documentation/dev-tools/kunit/usage.rst @@ -539,6 +539,22 @@ Interspersed in the kernel logs you might see the following:
Congratulations, you just ran a KUnit test on the x86 architecture!
+In a similar manner, kunit and kunit tests can also be built as modules, +so if you wanted to run tests in this way you might add the following config +options to your ``.config``: + +.. code-block:: none + + CONFIG_KUNIT=m + CONFIG_KUNIT_EXAMPLE_TEST=m + +Once the kernel is built and installed, a simple + +.. code-block:: bash + modprobe example-test + +...will run the tests. + Writing new tests for other architectures -----------------------------------------
On Tue, Oct 08, 2019 at 03:55:46PM +0100, Alan Maguire wrote:
Documentation should describe how to build kunit and tests as modules.
Signed-off-by: Alan Maguire alan.maguire@oracle.com Signed-off-by: Knut Omang knut.omang@oracle.com
Documentation/dev-tools/kunit/faq.rst | 3 ++- Documentation/dev-tools/kunit/index.rst | 3 +++ Documentation/dev-tools/kunit/usage.rst | 16 ++++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-)
[...]
diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst index c6e6963..fa0f03f 100644 --- a/Documentation/dev-tools/kunit/usage.rst +++ b/Documentation/dev-tools/kunit/usage.rst @@ -539,6 +539,22 @@ Interspersed in the kernel logs you might see the following: Congratulations, you just ran a KUnit test on the x86 architecture! +In a similar manner, kunit and kunit tests can also be built as modules, +so if you wanted to run tests in this way you might add the following config +options to your ``.config``:
+.. code-block:: none
CONFIG_KUNIT=mCONFIG_KUNIT_EXAMPLE_TEST=m
This doesn't appear to be properly tabbed.
+Once the kernel is built and installed, a simple
+.. code-block:: bash
- modprobe example-test
+...will run the tests.
Writing new tests for other architectures
1.8.3.1
On Tue, Oct 08, 2019 at 03:55:43PM +0100, Alan Maguire wrote:
The current kunit execution model is to provide base kunit functionality and tests built-in to the kernel. The aim of this series is to allow building kunit itself and tests as modules. This in turn allows a
Cool! I had plans for supporting this eventually, so I am more than happy to accept support for this!
simple form of selective execution; load the module you wish to test. In doing so, kunit itself (if also built as a module) will be loaded as an implicit dependency.
Seems like a reasonable initial approach. I had some plans for a centralized test executor, but I don't think that this should be a problem.
Because this requires a core API modification - if a module delivers multiple suites, they must be declared with the kunit_test_suites() macro - we're proposing this patch as a candidate to be applied to the test tree before too many kunit consumers appear. We attempt to deal with existing consumers in patch 1.
Makese sense.
On Tue, Oct 08, 2019 at 03:55:43PM +0100, Alan Maguire wrote:
The current kunit execution model is to provide base kunit functionality and tests built-in to the kernel. The aim of this series is to allow building kunit itself and tests as modules. This in turn allows a simple form of selective execution; load the module you wish to test. In doing so, kunit itself (if also built as a module) will be loaded as an implicit dependency.
Because this requires a core API modification - if a module delivers multiple suites, they must be declared with the kunit_test_suites() macro - we're proposing this patch as a candidate to be applied to the test tree before too many kunit consumers appear. We attempt to deal with existing consumers in patch 1.
This is neat and makes sense to me. However the ordering of the patches seems odd. If modules depend on kunit module, then shouldn't that go first? Ie, we want this to be bisectable in proper order.
Luis
On Mon, 14 Oct 2019, Luis Chamberlain wrote:
On Tue, Oct 08, 2019 at 03:55:43PM +0100, Alan Maguire wrote:
The current kunit execution model is to provide base kunit functionality and tests built-in to the kernel. The aim of this series is to allow building kunit itself and tests as modules. This in turn allows a simple form of selective execution; load the module you wish to test. In doing so, kunit itself (if also built as a module) will be loaded as an implicit dependency.
Because this requires a core API modification - if a module delivers multiple suites, they must be declared with the kunit_test_suites() macro - we're proposing this patch as a candidate to be applied to the test tree before too many kunit consumers appear. We attempt to deal with existing consumers in patch 1.
This is neat and makes sense to me.
Thanks for taking a look!
However the ordering of the patches seems odd. If modules depend on kunit module, then shouldn't that go first? Ie, we want this to be bisectable in proper order.
The reasoning here is it seemed a more likely scenario that users mught build kunit built-in (CONFIG_KUNIT=y) along with test suites built as modules (CONFIG_KUNIT_TEST=m). So the intermediate state after patch 2 - tests buildable as modules while kunit is still built-in-only - made more sense to me as something users might do in practice so that's why I ordered things that way. I'm working on a new revision of the patchset though, so if you feel strongly about this shout and I'll try and accommodate the alternative ordering.
Thanks!
Alan
Luis
On Mon, Oct 14, 2019 at 03:02:03PM +0100, Alan Maguire wrote:
On Mon, 14 Oct 2019, Luis Chamberlain wrote:
On Tue, Oct 08, 2019 at 03:55:43PM +0100, Alan Maguire wrote:
The current kunit execution model is to provide base kunit functionality and tests built-in to the kernel. The aim of this series is to allow building kunit itself and tests as modules. This in turn allows a simple form of selective execution; load the module you wish to test. In doing so, kunit itself (if also built as a module) will be loaded as an implicit dependency.
Because this requires a core API modification - if a module delivers multiple suites, they must be declared with the kunit_test_suites() macro - we're proposing this patch as a candidate to be applied to the test tree before too many kunit consumers appear. We attempt to deal with existing consumers in patch 1.
This is neat and makes sense to me.
Thanks for taking a look!
However the ordering of the patches seems odd. If modules depend on kunit module, then shouldn't that go first? Ie, we want this to be bisectable in proper order.
The reasoning here is it seemed a more likely scenario that users mught build kunit built-in (CONFIG_KUNIT=y) along with test suites built as modules (CONFIG_KUNIT_TEST=m). So the intermediate state after patch 2 - tests buildable as modules while kunit is still built-in-only - made more sense to me as something users might do in practice so that's why I ordered things that way. I'm working on a new revision of the patchset though, so if you feel strongly about this shout and I'll try and accommodate the alternative ordering.
No, that makes sense. All good.
Luis
linux-kselftest-mirror@lists.linaro.org
-
Alan Maguire -
Brendan Higgins -
Luis Chamberlain -
Randy Dunlap