[PATCH v2 linux-kselftest-test 0/3] kunit: support building core/tests as modules

[PATCH v2 linux-kselftest-test 0/3] kunit: support building core/tests as modules

[Alan Maguire]

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(-)

-- 
1.8.3.1

[PATCH v2 linux-kselftest-test 1/3] kunit: allow kunit tests to be loaded as a module

[Alan Maguire]

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,
 				    &params);
 }
+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);
-- 
1.8.3.1

[PATCH v2 linux-kselftest-test 2/3] kunit: allow kunit to be loaded as a module

[Alan Maguire]

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 */
-- 
1.8.3.1

[PATCH v2 linux-kselftest-test 3/3] kunit: update documentation to describe module-based build

[Alan Maguire]

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
 -----------------------------------------
 
-- 
1.8.3.1

Re: [PATCH v2 linux-kselftest-test 2/3] kunit: allow kunit to be loaded as a module

[Randy Dunlap]

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?

-- 
~Randy

Re: [PATCH v2 linux-kselftest-test 0/3] kunit: support building core/tests as modules

[Brendan Higgins]

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.

Re: [PATCH v2 linux-kselftest-test 1/3] kunit: allow kunit tests to be loaded as a module

[Brendan Higgins]

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.
>  	  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/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/.

Re: [PATCH v2 linux-kselftest-test 3/3] kunit: update documentation to describe module-based build

[Brendan Higgins]

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=m
> +        CONFIG_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
> 

Re: [PATCH v2 linux-kselftest-test 1/3] kunit: allow kunit tests to be loaded as a module

[Alan Maguire]

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.
> >  	  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/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

Re: [PATCH v2 linux-kselftest-test 1/3] kunit: allow kunit tests to be loaded as a module

[Brendan Higgins]

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?

Re: [PATCH v2 linux-kselftest-test 1/3] kunit: allow kunit tests to be loaded as a module

[Alan Maguire]

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 

Re: [PATCH v2 linux-kselftest-test 0/3] kunit: support building core/tests as modules

[Luis Chamberlain]

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

Re: [PATCH v2 linux-kselftest-test 0/3] kunit: support building core/tests as modules

[Alan Maguire]

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
> 

Re: [PATCH v2 linux-kselftest-test 0/3] kunit: support building core/tests as modules

[Luis Chamberlain]

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

Re: [PATCH v2 linux-kselftest-test 1/3] kunit: allow kunit tests to be loaded as a module

[Brendan Higgins]

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#L322
[2] https://elixir.bootlin.com/linux/v5.3.6/source/drivers/hwtracing/stm/policy.c#L40
[3] https://elixir.bootlin.com/linux/v5.3.6/source/drivers/hwtracing/stm/Makefile#L4
[4] https://elixir.bootlin.com/linux/v5.3.6/source/drivers/hwtracing/stm/Kconfig#L3

Re: [PATCH v2 linux-kselftest-test 1/3] kunit: allow kunit tests to be loaded as a module

[Alan Maguire]

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

[1] https://lkml.org/lkml/2019/10/17/801

Re: [PATCH v2 linux-kselftest-test 1/3] kunit: allow kunit tests to be loaded as a module

[Luis Chamberlain]

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

Re: [PATCH v2 linux-kselftest-test 1/3] kunit: allow kunit tests to be loaded as a module

[Brendan Higgins]

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.
>
> [0] https://lwn.net/Articles/798254/

Sounds good to me.