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

[PATCH v3 linux-kselftest-test 0/6] 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 set 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 4.

Changes since v2:
 - moved string-stream.h header to lib/kunit/string-stream-impl.h (Brendan)
   (patch 1)
 - split out non-exported interfaces in try-catch-impl.h (Brendan)
   (patch 2)
 - added kunit_find_symbol() and KUNIT_INIT_*SYMBOL to lookup non-exported
   symbols. KUNIT_INIT_*SYMBOL() is defined so that a mismatch between
   local symbol definition and definition of symbol in target will trigger
   a compilation error when the object is compiled built-in (Brendan)
   (patches 3, 4)
 - removed #ifdef MODULE around module licenses (Randy, Brendan, Andy)
   (patch 4)
 - replaced kunit_test_suite() with kunit_test_suites() rather than
   supporting both (Brendan) (patch 4)
 - lookup sysctl_hung_task_timeout_secs as kunit may be built as a module
   and the symbol may not be available (patch 5)
 - fixed whitespace issues in doc (patch 6)

Alan Maguire (6):
  kunit: move string-stream.h to lib/kunit/string-stream-impl.h
  kunit: hide unexported try-catch interface in try-catch-impl.h
  kunit: add kunit_find_symbol() function for symbol lookup
  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/assert.h                  |   3 +-
 include/kunit/string-stream.h           |  51 -------------
 include/kunit/test.h                    | 123 +++++++++++++++++++++++++++++---
 include/kunit/try-catch.h               |  10 ---
 kernel/sysctl-test.c                    |   4 +-
 lib/Kconfig.debug                       |   2 +-
 lib/kunit/Kconfig                       |   6 +-
 lib/kunit/Makefile                      |   4 +-
 lib/kunit/assert.c                      |   9 +++
 lib/kunit/example-test.c                |   4 +-
 lib/kunit/string-stream-impl.h          |  51 +++++++++++++
 lib/kunit/string-stream-test.c          |  46 ++++++++----
 lib/kunit/string-stream.c               |   3 +-
 lib/kunit/test-test.c                   |  50 ++++++++++---
 lib/kunit/test.c                        |  49 +++++++++++++
 lib/kunit/try-catch-impl.h              |  23 ++++++
 lib/kunit/try-catch.c                   |   6 ++
 20 files changed, 363 insertions(+), 103 deletions(-)
 delete mode 100644 include/kunit/string-stream.h
 create mode 100644 lib/kunit/string-stream-impl.h
 create mode 100644 lib/kunit/try-catch-impl.h

-- 
1.8.3.1

[PATCH v3 linux-kselftest-test 1/6] kunit: move string-stream.h to lib/kunit/string-stream-impl.h

[Alan Maguire]

string stream interfaces are not intended for external use;
move them from include/kunit to lib/kunit/string-stream-impl.h accordingly.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 include/kunit/assert.h         |  3 ++-
 include/kunit/string-stream.h  | 51 ------------------------------------------
 lib/kunit/assert.c             |  1 +
 lib/kunit/string-stream-impl.h | 51 ++++++++++++++++++++++++++++++++++++++++++
 lib/kunit/string-stream-test.c |  2 +-
 lib/kunit/string-stream.c      |  3 ++-
 lib/kunit/test.c               |  1 +
 7 files changed, 58 insertions(+), 54 deletions(-)
 delete mode 100644 include/kunit/string-stream.h
 create mode 100644 lib/kunit/string-stream-impl.h

diff --git a/include/kunit/assert.h b/include/kunit/assert.h
index db6a0fc..ad889b5 100644
--- a/include/kunit/assert.h
+++ b/include/kunit/assert.h
@@ -9,10 +9,11 @@
 #ifndef _KUNIT_ASSERT_H
 #define _KUNIT_ASSERT_H
 
-#include <kunit/string-stream.h>
 #include <linux/err.h>
+#include <linux/kernel.h>
 
 struct kunit;
+struct string_stream;
 
 /**
  * enum kunit_assert_type - Type of expectation/assertion.
diff --git a/include/kunit/string-stream.h b/include/kunit/string-stream.h
deleted file mode 100644
index fe98a00..0000000
--- a/include/kunit/string-stream.h
+++ /dev/null
@@ -1,51 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * C++ stream style string builder used in KUnit for building messages.
- *
- * Copyright (C) 2019, Google LLC.
- * Author: Brendan Higgins <brendanhiggins@google.com>
- */
-
-#ifndef _KUNIT_STRING_STREAM_H
-#define _KUNIT_STRING_STREAM_H
-
-#include <linux/spinlock.h>
-#include <linux/types.h>
-#include <stdarg.h>
-
-struct string_stream_fragment {
-	struct kunit *test;
-	struct list_head node;
-	char *fragment;
-};
-
-struct string_stream {
-	size_t length;
-	struct list_head fragments;
-	/* length and fragments are protected by this lock */
-	spinlock_t lock;
-	struct kunit *test;
-	gfp_t gfp;
-};
-
-struct kunit;
-
-struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp);
-
-int __printf(2, 3) string_stream_add(struct string_stream *stream,
-				     const char *fmt, ...);
-
-int string_stream_vadd(struct string_stream *stream,
-		       const char *fmt,
-		       va_list args);
-
-char *string_stream_get_string(struct string_stream *stream);
-
-int string_stream_append(struct string_stream *stream,
-			 struct string_stream *other);
-
-bool string_stream_is_empty(struct string_stream *stream);
-
-int string_stream_destroy(struct string_stream *stream);
-
-#endif /* _KUNIT_STRING_STREAM_H */
diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
index 86013d4..d8ae94e 100644
--- a/lib/kunit/assert.c
+++ b/lib/kunit/assert.c
@@ -6,6 +6,7 @@
  * Author: Brendan Higgins <brendanhiggins@google.com>
  */
 #include <kunit/assert.h>
+#include "string-stream-impl.h"
 
 void kunit_base_assert_format(const struct kunit_assert *assert,
 			      struct string_stream *stream)
diff --git a/lib/kunit/string-stream-impl.h b/lib/kunit/string-stream-impl.h
new file mode 100644
index 0000000..0fe4100
--- /dev/null
+++ b/lib/kunit/string-stream-impl.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * C++ stream style string builder used in KUnit for building messages.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#ifndef _KUNIT_STRING_STREAM_IMPL_H
+#define _KUNIT_STRING_STREAM_IMPL_H
+
+#include <linux/spinlock.h>
+#include <linux/types.h>
+#include <stdarg.h>
+
+struct string_stream_fragment {
+	struct kunit *test;
+	struct list_head node;
+	char *fragment;
+};
+
+struct string_stream {
+	size_t length;
+	struct list_head fragments;
+	/* length and fragments are protected by this lock */
+	spinlock_t lock;
+	struct kunit *test;
+	gfp_t gfp;
+};
+
+struct kunit;
+
+struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp);
+
+int __printf(2, 3) string_stream_add(struct string_stream *stream,
+				     const char *fmt, ...);
+
+int string_stream_vadd(struct string_stream *stream,
+		       const char *fmt,
+		       va_list args);
+
+char *string_stream_get_string(struct string_stream *stream);
+
+int string_stream_append(struct string_stream *stream,
+			 struct string_stream *other);
+
+bool string_stream_is_empty(struct string_stream *stream);
+
+int string_stream_destroy(struct string_stream *stream);
+
+#endif /* _KUNIT_STRING_STREAM_IMPL_H */
diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
index 76cc05e..25b2cf3 100644
--- a/lib/kunit/string-stream-test.c
+++ b/lib/kunit/string-stream-test.c
@@ -6,9 +6,9 @@
  * Author: Brendan Higgins <brendanhiggins@google.com>
  */
 
-#include <kunit/string-stream.h>
 #include <kunit/test.h>
 #include <linux/slab.h>
+#include "string-stream-impl.h"
 
 static void string_stream_test_empty_on_creation(struct kunit *test)
 {
diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
index e6d17aa..a1e005d 100644
--- a/lib/kunit/string-stream.c
+++ b/lib/kunit/string-stream.c
@@ -6,11 +6,12 @@
  * Author: Brendan Higgins <brendanhiggins@google.com>
  */
 
-#include <kunit/string-stream.h>
 #include <kunit/test.h>
 #include <linux/list.h>
 #include <linux/slab.h>
 
+#include "string-stream-impl.h"
+
 struct string_stream_fragment_alloc_context {
 	struct kunit *test;
 	int len;
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index c83c0fa..017d4fb 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -10,6 +10,7 @@
 #include <kunit/try-catch.h>
 #include <linux/kernel.h>
 #include <linux/sched/debug.h>
+#include "string-stream-impl.h"
 
 static void kunit_set_failure(struct kunit *test)
 {
-- 
1.8.3.1

Re: [PATCH v3 linux-kselftest-test 1/6] kunit: move string-stream.h to lib/kunit/string-stream-impl.h

[Brendan Higgins]

On Thu, Oct 17, 2019 at 11:07 AM Alan Maguire <alan.maguire@oracle.com> wrote:

Sorry for taking so long to get to this.

> string stream interfaces are not intended for external use;
> move them from include/kunit to lib/kunit/string-stream-impl.h accordingly.
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> ---
>  include/kunit/assert.h         |  3 ++-
>  include/kunit/string-stream.h  | 51 ------------------------------------------
>  lib/kunit/assert.c             |  1 +
>  lib/kunit/string-stream-impl.h | 51 ++++++++++++++++++++++++++++++++++++++++++

I agree with the move of string-stream.h from include/kunit/ to
lib/kunit/, but can you keep the name string-stream.h?
string-stream-impl.h seems a little wonky to me. Otherwise, this patch
looks good to me.

>  lib/kunit/string-stream-test.c |  2 +-
>  lib/kunit/string-stream.c      |  3 ++-
>  lib/kunit/test.c               |  1 +
>  7 files changed, 58 insertions(+), 54 deletions(-)
>  delete mode 100644 include/kunit/string-stream.h
>  create mode 100644 lib/kunit/string-stream-impl.h
>
> diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> index db6a0fc..ad889b5 100644
> --- a/include/kunit/assert.h
> +++ b/include/kunit/assert.h
> @@ -9,10 +9,11 @@
>  #ifndef _KUNIT_ASSERT_H
>  #define _KUNIT_ASSERT_H
>
> -#include <kunit/string-stream.h>
>  #include <linux/err.h>
> +#include <linux/kernel.h>
>
>  struct kunit;
> +struct string_stream;
>
>  /**
>   * enum kunit_assert_type - Type of expectation/assertion.
[...]
> diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
> index 86013d4..d8ae94e 100644
> --- a/lib/kunit/assert.c
> +++ b/lib/kunit/assert.c
> @@ -6,6 +6,7 @@
>   * Author: Brendan Higgins <brendanhiggins@google.com>
>   */
>  #include <kunit/assert.h>
> +#include "string-stream-impl.h"
>
>  void kunit_base_assert_format(const struct kunit_assert *assert,
>                               struct string_stream *stream)
[...]
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index 76cc05e..25b2cf3 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -6,9 +6,9 @@
>   * Author: Brendan Higgins <brendanhiggins@google.com>
>   */
>
> -#include <kunit/string-stream.h>
>  #include <kunit/test.h>
>  #include <linux/slab.h>
> +#include "string-stream-impl.h"
>
>  static void string_stream_test_empty_on_creation(struct kunit *test)
>  {
> diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
> index e6d17aa..a1e005d 100644
> --- a/lib/kunit/string-stream.c
> +++ b/lib/kunit/string-stream.c
> @@ -6,11 +6,12 @@
>   * Author: Brendan Higgins <brendanhiggins@google.com>
>   */
>
> -#include <kunit/string-stream.h>
>  #include <kunit/test.h>
>  #include <linux/list.h>
>  #include <linux/slab.h>
>

very minor nit: Here you put a space in the include list, elsewhere
you did not. I don't really care whether you do put the space in or
you don't (I have a slight preference for the extra space as you have
done here), but I do think it would be better if we remain consistent.

> +#include "string-stream-impl.h"
> +
>  struct string_stream_fragment_alloc_context {
>         struct kunit *test;
>         int len;
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index c83c0fa..017d4fb 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -10,6 +10,7 @@
>  #include <kunit/try-catch.h>
>  #include <linux/kernel.h>
>  #include <linux/sched/debug.h>
> +#include "string-stream-impl.h"
>
>  static void kunit_set_failure(struct kunit *test)
>  {
> --
> 1.8.3.1
>

[PATCH v3 linux-kselftest-test 2/6] kunit: hide unexported try-catch interface in try-catch-impl.h

[Alan Maguire]

also remove unused kunit_generic_try_catch

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 include/kunit/try-catch.h  | 10 ----------
 lib/kunit/test-test.c      |  1 +
 lib/kunit/test.c           |  1 +
 lib/kunit/try-catch-impl.h | 23 +++++++++++++++++++++++
 lib/kunit/try-catch.c      |  1 +
 5 files changed, 26 insertions(+), 10 deletions(-)
 create mode 100644 lib/kunit/try-catch-impl.h

diff --git a/include/kunit/try-catch.h b/include/kunit/try-catch.h
index 404f336..c507dd4 100644
--- a/include/kunit/try-catch.h
+++ b/include/kunit/try-catch.h
@@ -53,11 +53,6 @@ struct kunit_try_catch {
 	void *context;
 };
 
-void kunit_try_catch_init(struct kunit_try_catch *try_catch,
-			  struct kunit *test,
-			  kunit_try_catch_func_t try,
-			  kunit_try_catch_func_t catch);
-
 void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context);
 
 void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch);
@@ -67,9 +62,4 @@ static inline int kunit_try_catch_get_result(struct kunit_try_catch *try_catch)
 	return try_catch->try_result;
 }
 
-/*
- * Exposed for testing only.
- */
-void kunit_generic_try_catch_init(struct kunit_try_catch *try_catch);
-
 #endif /* _KUNIT_TRY_CATCH_H */
diff --git a/lib/kunit/test-test.c b/lib/kunit/test-test.c
index 5ebe059..c4162a9 100644
--- a/lib/kunit/test-test.c
+++ b/lib/kunit/test-test.c
@@ -6,6 +6,7 @@
  * Author: Brendan Higgins <brendanhiggins@google.com>
  */
 #include <kunit/test.h>
+#include "try-catch-impl.h"
 
 struct kunit_try_catch_test_context {
 	struct kunit_try_catch *try_catch;
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 017d4fb..49ac5fe 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/sched/debug.h>
 #include "string-stream-impl.h"
+#include "try-catch-impl.h"
 
 static void kunit_set_failure(struct kunit *test)
 {
diff --git a/lib/kunit/try-catch-impl.h b/lib/kunit/try-catch-impl.h
new file mode 100644
index 0000000..aa11e46
--- /dev/null
+++ b/lib/kunit/try-catch-impl.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * An API to allow a function, that may fail, to be executed, and recover in a
+ * controlled manner.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#ifndef _KUNIT_TRY_CATCH_IMPL_H
+#define _KUNIT_TRY_CATCH_IMPL_H
+
+#include <kunit/try-catch.h>
+#include <linux/types.h>
+
+struct kunit;
+
+void kunit_try_catch_init(struct kunit_try_catch *try_catch,
+			  struct kunit *test,
+			  kunit_try_catch_func_t try,
+			  kunit_try_catch_func_t catch);
+
+#endif /* _KUNIT_TRY_CATCH_IMPL_H */
diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
index 55686839..0c4c90c 100644
--- a/lib/kunit/try-catch.c
+++ b/lib/kunit/try-catch.c
@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/kthread.h>
 #include <linux/sched/sysctl.h>
+#include "try-catch-impl.h"
 
 void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch)
 {
-- 
1.8.3.1

Re: [PATCH v3 linux-kselftest-test 2/6] kunit: hide unexported try-catch interface in try-catch-impl.h

[Brendan Higgins]

On Thu, Oct 17, 2019 at 11:08 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> also remove unused kunit_generic_try_catch
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

> ---
>  include/kunit/try-catch.h  | 10 ----------
>  lib/kunit/test-test.c      |  1 +
>  lib/kunit/test.c           |  1 +
>  lib/kunit/try-catch-impl.h | 23 +++++++++++++++++++++++

Just wanted to say that I *am* happy with the *-impl.h naming scheme
here since there is a public header file with almost the same name. So
everything looks good to me with this patch.

>  lib/kunit/try-catch.c      |  1 +
>  5 files changed, 26 insertions(+), 10 deletions(-)
>  create mode 100644 lib/kunit/try-catch-impl.h

Thanks for the patch!

[PATCH v3 linux-kselftest-test 3/6] kunit: add kunit_find_symbol() function for symbol lookup

[Alan Maguire]

In preparation for module support for kunit and kunit tests,
we need a way of retrieving non-exported symbols from the
core kernel and modules.  kunit_find_symbol() supports this.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 include/kunit/test.h  |  8 ++++++++
 lib/kunit/test-test.c | 19 +++++++++++++++++++
 lib/kunit/test.c      | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index dba4830..c645d18 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -339,6 +339,14 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
 
 void kunit_cleanup(struct kunit *test);
 
+/**
+ * kunit_find_symbol() - lookup un-exported symbol in kernel or modules.
+ * @sym: symbol name.
+ *
+ * Returns symbol or ERR_PTR value on error.
+ */
+void *kunit_find_symbol(const char *sym);
+
 #define kunit_printk(lvl, test, fmt, ...) \
 	printk(lvl "\t# %s: " fmt, (test)->name, ##__VA_ARGS__)
 
diff --git a/lib/kunit/test-test.c b/lib/kunit/test-test.c
index c4162a9..7f09dd0 100644
--- a/lib/kunit/test-test.c
+++ b/lib/kunit/test-test.c
@@ -330,3 +330,22 @@ static void kunit_resource_test_exit(struct kunit *test)
 	.test_cases = kunit_resource_test_cases,
 };
 kunit_test_suite(kunit_resource_test_suite);
+
+/*
+ * 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"));
+}
+
+static struct kunit_case kunit_find_symbol_test_cases[] = {
+	KUNIT_CASE(kunit_find_symbol_kernel),
+};
+
+static struct kunit_suite kunit_find_symbol_test_suite = {
+	.name = "kunit-find-symbol",
+	.test_cases = kunit_find_symbol_test_cases,
+};
+kunit_test_suite(kunit_find_symbol_test_suite);
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 49ac5fe..a2b1b46 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -8,6 +8,7 @@
 
 #include <kunit/test.h>
 #include <kunit/try-catch.h>
+#include <linux/kallsyms.h>
 #include <linux/kernel.h>
 #include <linux/sched/debug.h>
 #include "string-stream-impl.h"
@@ -478,3 +479,38 @@ void kunit_cleanup(struct kunit *test)
 		kunit_resource_free(test, resource);
 	}
 }
+
+/*
+ * Support for looking up kernel/module internal symbols to enable testing.
+ */
+void *kunit_find_symbol(const char *sym)
+{
+	unsigned long (*modlookup)(const char *name);
+	unsigned long addr = 0;
+
+	if (!sym || strlen(sym) > KSYM_NAME_LEN)
+		return ERR_PTR(-EINVAL);
+
+	/*
+	 * Try for kernel-internal symbol first; fall back to modules
+	 * if that fails.
+	 */
+	addr = kallsyms_lookup_name(sym);
+	if (addr)
+		return (void *)addr;
+	modlookup = (void *)kallsyms_lookup_name("module_kallsyms_lookup_name");
+	if (modlookup)
+		addr = modlookup(sym);
+	if (addr)
+		return (void *)addr;
+
+#ifndef CONFIG_KALLSYMS_ALL
+	WARN_ONCE(true,
+		  "CONFIG_KALLSYMS_ALL is not set, so unexported symbols like '%s' are not available\n",
+		  sym);
+	return ERR_PTR(-ENOTSUPP);
+#else
+	WARN_ONCE(true, "symbol '%s' is not available\n", sym);
+#endif
+	return ERR_PTR(-ENOENT);
+}
-- 
1.8.3.1

Re: [PATCH v3 linux-kselftest-test 3/6] kunit: add kunit_find_symbol() function for symbol lookup

[Brendan Higgins]

On Thu, Oct 17, 2019 at 11:08 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> In preparation for module support for kunit and kunit tests,
> we need a way of retrieving non-exported symbols from the
> core kernel and modules.  kunit_find_symbol() supports this.
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> Signed-off-by: Knut Omang <knut.omang@oracle.com>

I think you suggested on another thread that splitting this patch out
of this patchset might be a good idea. I agree with that. Can you send
this patch separately?

> ---
>  include/kunit/test.h  |  8 ++++++++
>  lib/kunit/test-test.c | 19 +++++++++++++++++++
>  lib/kunit/test.c      | 36 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index dba4830..c645d18 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -339,6 +339,14 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
>
>  void kunit_cleanup(struct kunit *test);
>
> +/**
> + * kunit_find_symbol() - lookup un-exported symbol in kernel or modules.
> + * @sym: symbol name.
> + *
> + * Returns symbol or ERR_PTR value on error.

Can you document which ERR_PTRs it returns?

> + */
> +void *kunit_find_symbol(const char *sym);
> +
>  #define kunit_printk(lvl, test, fmt, ...) \
>         printk(lvl "\t# %s: " fmt, (test)->name, ##__VA_ARGS__)
>
> diff --git a/lib/kunit/test-test.c b/lib/kunit/test-test.c
> index c4162a9..7f09dd0 100644
> --- a/lib/kunit/test-test.c
> +++ b/lib/kunit/test-test.c
> @@ -330,3 +330,22 @@ static void kunit_resource_test_exit(struct kunit *test)
>         .test_cases = kunit_resource_test_cases,
>  };
>  kunit_test_suite(kunit_resource_test_suite);
> +
> +/*
> + * 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"));

I think this should be a KUNIT_EXPECT_... here since nothing in this
test case depends on this check passing.

> +}
> +
> +static struct kunit_case kunit_find_symbol_test_cases[] = {
> +       KUNIT_CASE(kunit_find_symbol_kernel),
> +};
> +
> +static struct kunit_suite kunit_find_symbol_test_suite = {
> +       .name = "kunit-find-symbol",
> +       .test_cases = kunit_find_symbol_test_cases,
> +};
> +kunit_test_suite(kunit_find_symbol_test_suite);
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 49ac5fe..a2b1b46 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -8,6 +8,7 @@
>
>  #include <kunit/test.h>
>  #include <kunit/try-catch.h>
> +#include <linux/kallsyms.h>
>  #include <linux/kernel.h>
>  #include <linux/sched/debug.h>
>  #include "string-stream-impl.h"
> @@ -478,3 +479,38 @@ void kunit_cleanup(struct kunit *test)
>                 kunit_resource_free(test, resource);
>         }
>  }
> +
> +/*
> + * Support for looking up kernel/module internal symbols to enable testing.
> + */
> +void *kunit_find_symbol(const char *sym)
> +{
> +       unsigned long (*modlookup)(const char *name);
> +       unsigned long addr = 0;
> +
> +       if (!sym || strlen(sym) > KSYM_NAME_LEN)
> +               return ERR_PTR(-EINVAL);
> +
> +       /*
> +        * Try for kernel-internal symbol first; fall back to modules
> +        * if that fails.
> +        */
> +       addr = kallsyms_lookup_name(sym);
> +       if (addr)
> +               return (void *)addr;

nit: please add a newline here.

> +       modlookup = (void *)kallsyms_lookup_name("module_kallsyms_lookup_name");

Can you add a comment here explaining what module_kallsyms_lookup_name
is and why you need to look it up this way?

> +       if (modlookup)
> +               addr = modlookup(sym);
> +       if (addr)
> +               return (void *)addr;
> +
> +#ifndef CONFIG_KALLSYMS_ALL
> +       WARN_ONCE(true,
> +                 "CONFIG_KALLSYMS_ALL is not set, so unexported symbols like '%s' are not available\n",
> +                 sym);
> +       return ERR_PTR(-ENOTSUPP);
> +#else
> +       WARN_ONCE(true, "symbol '%s' is not available\n", sym);
> +#endif
> +       return ERR_PTR(-ENOENT);
> +}
> --
> 1.8.3.1
>

[PATCH v3 linux-kselftest-test 4/6] 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 for non-exported symbols, we need to utilize kunit_find_symbol;
  the simplest way is for the test suite init to call
  KUNIT_INIT_[FN|VAR]_SYMBOL() for each non-exported symbol.
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.

When compiled as a module, use of KUNIT_INIT_[FN|VAR]_symbol() will
retrieve the symbol address via kunit_find_symbol() and assign a local
variable with the same symbol name appropriately.  When compiled builtin,
these definitions are used to verify that the types we specify match
the type of the symbol we are looking for.  Compiler errors will be
generated if not.

One wrinkle here is that we cannot use the same names for local function
pointer definitions; the reason for this is we have likely #included
a definition for the function in question already, so an attempt to
redefine it as a function pointer variable fails.  As a result the
KUNIT_INIT_FN_SYMBOL() macro requires a name for a local symbol we
have defined as a function pointer (with a signature matching the
desired function).

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 include/kunit/test.h           | 115 +++++++++++++++++++++++++++++++++++++----
 kernel/sysctl-test.c           |   4 +-
 lib/Kconfig.debug              |   2 +-
 lib/kunit/Kconfig              |   4 +-
 lib/kunit/assert.c             |   8 +++
 lib/kunit/example-test.c       |   4 +-
 lib/kunit/string-stream-test.c |  44 ++++++++++++----
 lib/kunit/test-test.c          |  32 ++++++++----
 lib/kunit/test.c               |   9 ++++
 lib/kunit/try-catch.c          |   2 +
 10 files changed, 187 insertions(+), 37 deletions(-)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index c645d18..9a3835a 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>
 
@@ -78,6 +79,86 @@ struct kunit_resource {
 	struct list_head node;
 };
 
+/**
+ * KUNIT_VAR_SYMBOL - A helper for defining non-exported variable symbols
+ *
+ * @name: name of the symbol.
+ * @type: type of symbol.
+ *
+ * In the module case, we define the pointer to the symbol type where
+ * we will store the symbol address; KUNIT_INIT_VAR_SYMBOL() will assign
+ * the symbol name to the dereferenced kunit_<symbol_name>.  Note that
+ * in the builtin case we still define kunit_<symbol_name>; the reason
+ * for this is it allows us to verify that the type value is correct
+ * in the builtin case and has not fallen out-of-sync with its original
+ * definition.
+ */
+#ifdef MODULE
+#define KUNIT_VAR_SYMBOL(symbol, type)					\
+	type * kunit_##symbol;						\
+	type symbol
+#else
+#define KUNIT_VAR_SYMBOL(symbol, type)					\
+	type * kunit_##symbol
+#endif
+
+/**
+ * KUNIT_INIT_VAR_SYMBOL - A helper for initializing non-exported variable
+ *			   symbols
+ * @test: optional pointer to test context
+ * @name: name of symbol
+ *
+ * In the module case, initialization consists of using kunit_find_symbol()
+ * to find the address of the symbol, and if found, we set the variable
+ * to the dereferenced address value.  As mentioned above, in the builtin
+ * case we simply assing kunit_<symbol_name> to &<symbol_name> ; this will
+ * generate a compilation warning if the type we specified in KUNIT_VAR_SYMBOL
+ * and the type of the symbol itself do not match.
+ */
+#ifdef MODULE
+#define KUNIT_INIT_VAR_SYMBOL(test, symbol)				\
+	do {								\
+		if (!(kunit_##symbol)) {				\
+			kunit_##symbol = kunit_find_symbol(#symbol);	\
+			if (!IS_ERR((kunit_##symbol)))			\
+				symbol = *(kunit_##symbol);		\
+		}							\
+		if (test)						\
+			KUNIT_ASSERT_NOT_ERR_OR_NULL(test,		\
+						     kunit_##symbol);	\
+	} while (0)
+#else
+#define KUNIT_INIT_VAR_SYMBOL(test, symbol)				\
+	kunit_##symbol = &(symbol)
+#endif
+
+/**
+ * KUNIT_INIT_FN_SYMBOL - A helper for initializing non-exported function
+ *			  symbols
+ * @test: optional pointer to test context
+ * @symbol: name of symbol
+ * @name: local name of function used to store function pointer to symbol
+ *
+ * In the module case, initialization consists of using kunit_find_symbol()
+ * to find the address of the symbol, and if found, we set function pointer
+ * name to the function address value.  In the non-module case, we simply
+ * assign name to symbol; this will generate a compilation error if the
+ * type we specified for function pointer @name does not match the symbol
+ * function type.
+ */
+#ifdef MODULE
+#define KUNIT_INIT_FN_SYMBOL(test, symbol, name)			\
+	do {								\
+		if (!name)						\
+			name = kunit_find_symbol(#symbol);		\
+		if (test)                                               \
+			KUNIT_ASSERT_NOT_ERR_OR_NULL(test, name);	\
+	} while (0)
+#else
+#define KUNIT_INIT_FN_SYMBOL(test, symbol, name)			\
+	name = symbol
+#endif
+
 struct kunit;
 
 /**
@@ -197,31 +278,45 @@ struct kunit {
 int kunit_run_tests(struct kunit_suite *suite);
 
 /**
- * kunit_test_suite() - used to register a &struct kunit_suite with KUnit.
+ * kunit_test_suites() - used to register one or more &struct kunit_suite
+ *			 with KUnit.
  *
- * @suite: a statically allocated &struct kunit_suite.
+ * @suites: a statically allocated list of &struct kunit_suite.
  *
- * Registers @suite with the test framework. See &struct kunit_suite for
+ * Registers @suites 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)
 
 /*
  * 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..98d3e4e 100644
--- a/kernel/sysctl-test.c
+++ b/kernel/sysctl-test.c
@@ -389,4 +389,6 @@ static void sysctl_test_api_dointvec_write_single_greater_int_max(
 	.test_cases = sysctl_test_cases,
 };
 
-kunit_test_suite(sysctl_test_suite);
+kunit_test_suites(&sysctl_test_suite);
+
+MODULE_LICENSE("GPL");
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index a3017a5..cdf14ce 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1951,7 +1951,7 @@ 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.
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 d8ae94e..2d57302 100644
--- a/lib/kunit/assert.c
+++ b/lib/kunit/assert.c
@@ -25,6 +25,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)
@@ -32,6 +33,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)
@@ -39,6 +41,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)
@@ -57,6 +60,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)
@@ -77,6 +81,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)
@@ -98,6 +103,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)
@@ -119,6 +125,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)
@@ -140,3 +147,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..ff930e0 100644
--- a/lib/kunit/example-test.c
+++ b/lib/kunit/example-test.c
@@ -85,4 +85,6 @@ 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_suites(&example_test_suite);
+
+MODULE_LICENSE("GPL");
diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
index 25b2cf3..7d6055a 100644
--- a/lib/kunit/string-stream-test.c
+++ b/lib/kunit/string-stream-test.c
@@ -10,34 +10,53 @@
 #include <linux/slab.h>
 #include "string-stream-impl.h"
 
+/* Non-exported string stream functions which we will use in testing. */
+struct string_stream * (*_alloc_string_stream)(struct kunit *test, gfp_t gfp);
+int __printf(2, 3) (*_string_stream_add)(struct string_stream *stream,
+					 const char *fmt, ...);
+char * (*_string_stream_get_string)(struct string_stream *stream);
+bool (*_string_stream_is_empty)(struct string_stream *stream);
+
 static void string_stream_test_empty_on_creation(struct kunit *test)
 {
-	struct string_stream *stream = alloc_string_stream(test, GFP_KERNEL);
+	struct string_stream *stream = _alloc_string_stream(test, GFP_KERNEL);
 
-	KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
+	KUNIT_EXPECT_TRUE(test, _string_stream_is_empty(stream));
 }
 
 static void string_stream_test_not_empty_after_add(struct kunit *test)
 {
-	struct string_stream *stream = alloc_string_stream(test, GFP_KERNEL);
+	struct string_stream *stream = _alloc_string_stream(test, GFP_KERNEL);
 
-	string_stream_add(stream, "Foo");
+	_string_stream_add(stream, "Foo");
 
-	KUNIT_EXPECT_FALSE(test, string_stream_is_empty(stream));
+	KUNIT_EXPECT_FALSE(test, _string_stream_is_empty(stream));
 }
 
 static void string_stream_test_get_string(struct kunit *test)
 {
-	struct string_stream *stream = alloc_string_stream(test, GFP_KERNEL);
+	struct string_stream *stream = _alloc_string_stream(test, GFP_KERNEL);
 	char *output;
 
-	string_stream_add(stream, "Foo");
-	string_stream_add(stream, " %s", "bar");
+	_string_stream_add(stream, "Foo");
+	_string_stream_add(stream, " %s", "bar");
 
-	output = string_stream_get_string(stream);
+	output = _string_stream_get_string(stream);
 	KUNIT_ASSERT_STREQ(test, output, "Foo bar");
 }
 
+static int string_stream_test_init(struct kunit *test)
+{
+	KUNIT_INIT_FN_SYMBOL(test, alloc_string_stream, _alloc_string_stream);
+	KUNIT_INIT_FN_SYMBOL(test, string_stream_add, _string_stream_add);
+	KUNIT_INIT_FN_SYMBOL(test, string_stream_get_string,
+			     _string_stream_get_string);
+	KUNIT_INIT_FN_SYMBOL(test, string_stream_is_empty,
+			     _string_stream_is_empty);
+
+	return 0;
+}
+
 static struct kunit_case string_stream_test_cases[] = {
 	KUNIT_CASE(string_stream_test_empty_on_creation),
 	KUNIT_CASE(string_stream_test_not_empty_after_add),
@@ -47,6 +66,9 @@ static void string_stream_test_get_string(struct kunit *test)
 
 static struct kunit_suite string_stream_test_suite = {
 	.name = "string-stream-test",
-	.test_cases = string_stream_test_cases
+	.test_cases = string_stream_test_cases,
+	.init = string_stream_test_init
 };
-kunit_test_suite(string_stream_test_suite);
+kunit_test_suites(&string_stream_test_suite);
+
+MODULE_LICENSE("GPL");
diff --git a/lib/kunit/test-test.c b/lib/kunit/test-test.c
index 7f09dd0..b4c1216 100644
--- a/lib/kunit/test-test.c
+++ b/lib/kunit/test-test.c
@@ -8,6 +8,11 @@
 #include <kunit/test.h>
 #include "try-catch-impl.h"
 
+void (*_kunit_try_catch_init)(struct kunit_try_catch *try_catch,
+			      struct kunit *test,
+			      kunit_try_catch_func_t try,
+			      kunit_try_catch_func_t catch);
+
 struct kunit_try_catch_test_context {
 	struct kunit_try_catch *try_catch;
 	bool function_called;
@@ -33,10 +38,10 @@ static void kunit_test_try_catch_successful_try_no_catch(struct kunit *test)
 	struct kunit_try_catch_test_context *ctx = test->priv;
 	struct kunit_try_catch *try_catch = ctx->try_catch;
 
-	kunit_try_catch_init(try_catch,
-			     test,
-			     kunit_test_successful_try,
-			     kunit_test_no_catch);
+	_kunit_try_catch_init(try_catch,
+			      test,
+			      kunit_test_successful_try,
+			      kunit_test_no_catch);
 	kunit_try_catch_run(try_catch, test);
 
 	KUNIT_EXPECT_TRUE(test, ctx->function_called);
@@ -65,10 +70,10 @@ static void kunit_test_try_catch_unsuccessful_try_does_catch(struct kunit *test)
 	struct kunit_try_catch_test_context *ctx = test->priv;
 	struct kunit_try_catch *try_catch = ctx->try_catch;
 
-	kunit_try_catch_init(try_catch,
-			     test,
-			     kunit_test_unsuccessful_try,
-			     kunit_test_catch);
+	_kunit_try_catch_init(try_catch,
+			      test,
+			      kunit_test_unsuccessful_try,
+			      kunit_test_catch);
 	kunit_try_catch_run(try_catch, test);
 
 	KUNIT_EXPECT_TRUE(test, ctx->function_called);
@@ -78,6 +83,8 @@ static int kunit_try_catch_test_init(struct kunit *test)
 {
 	struct kunit_try_catch_test_context *ctx;
 
+	KUNIT_INIT_FN_SYMBOL(test, kunit_try_catch_init, _kunit_try_catch_init);
+
 	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
 	test->priv = ctx;
@@ -101,7 +108,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
@@ -329,7 +335,6 @@ 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);
 
 /*
  * Find non-exported kernel symbol; we use the modules list as a safe
@@ -348,4 +353,9 @@ static void kunit_find_symbol_kernel(struct kunit *test)
 	.name = "kunit-find-symbol",
 	.test_cases = kunit_find_symbol_test_cases,
 };
-kunit_test_suite(kunit_find_symbol_test_suite);
+
+kunit_test_suites(&kunit_resource_test_suite,
+		  &kunit_try_catch_test_suite,
+		  &kunit_find_symbol_test_suite);
+
+MODULE_LICENSE("GPL");
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index a2b1b46..e8b2443 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -174,6 +174,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)
 {
@@ -182,6 +183,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.
@@ -320,6 +322,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,
@@ -345,6 +348,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)
 {
@@ -403,6 +407,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;
@@ -438,6 +443,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)
 {
@@ -450,6 +456,7 @@ void kunit_kfree(struct kunit *test, const void *ptr)
 
 	WARN_ON(rc);
 }
+EXPORT_SYMBOL_GPL(kunit_kfree);
 
 void kunit_cleanup(struct kunit *test)
 {
@@ -479,6 +486,7 @@ void kunit_cleanup(struct kunit *test)
 		kunit_resource_free(test, resource);
 	}
 }
+EXPORT_SYMBOL_GPL(kunit_cleanup);
 
 /*
  * Support for looking up kernel/module internal symbols to enable testing.
@@ -514,3 +522,4 @@ void *kunit_find_symbol(const char *sym)
 #endif
 	return ERR_PTR(-ENOENT);
 }
+EXPORT_SYMBOL(kunit_find_symbol);
diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
index 0c4c90c..1c1e9af 100644
--- a/lib/kunit/try-catch.c
+++ b/lib/kunit/try-catch.c
@@ -20,6 +20,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)
 {
@@ -107,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,
-- 
1.8.3.1

Re: [PATCH v3 linux-kselftest-test 4/6] kunit: allow kunit tests to be loaded as a module

[Brendan Higgins]

On Thu, Oct 17, 2019 at 07:07:17PM +0100, Alan Maguire wrote:
> as tests are added to kunit, it will become less feasible to execute
  ^
nit: please capitalize "as".

> 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 for non-exported symbols, we need to utilize kunit_find_symbol;
>   the simplest way is for the test suite init to call
>   KUNIT_INIT_[FN|VAR]_SYMBOL() for each non-exported symbol.
> 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.
> 
> When compiled as a module, use of KUNIT_INIT_[FN|VAR]_symbol() will
> retrieve the symbol address via kunit_find_symbol() and assign a local
> variable with the same symbol name appropriately.  When compiled builtin,
> these definitions are used to verify that the types we specify match
> the type of the symbol we are looking for.  Compiler errors will be
> generated if not.
> 
> One wrinkle here is that we cannot use the same names for local function
> pointer definitions; the reason for this is we have likely #included
> a definition for the function in question already, so an attempt to
> redefine it as a function pointer variable fails.  As a result the
> KUNIT_INIT_FN_SYMBOL() macro requires a name for a local symbol we
> have defined as a function pointer (with a signature matching the
> desired function).
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> ---
>  include/kunit/test.h           | 115 +++++++++++++++++++++++++++++++++++++----
>  kernel/sysctl-test.c           |   4 +-
>  lib/Kconfig.debug              |   2 +-
>  lib/kunit/Kconfig              |   4 +-
>  lib/kunit/assert.c             |   8 +++
>  lib/kunit/example-test.c       |   4 +-
>  lib/kunit/string-stream-test.c |  44 ++++++++++++----
>  lib/kunit/test-test.c          |  32 ++++++++----
>  lib/kunit/test.c               |   9 ++++
>  lib/kunit/try-catch.c          |   2 +
>  10 files changed, 187 insertions(+), 37 deletions(-)
> 
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index c645d18..9a3835a 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>
>  
> @@ -78,6 +79,86 @@ struct kunit_resource {
>  	struct list_head node;
>  };
>  
> +/**
> + * KUNIT_VAR_SYMBOL - A helper for defining non-exported variable symbols
> + *
> + * @name: name of the symbol.
> + * @type: type of symbol.
> + *
> + * In the module case, we define the pointer to the symbol type where
> + * we will store the symbol address; KUNIT_INIT_VAR_SYMBOL() will assign
> + * the symbol name to the dereferenced kunit_<symbol_name>.  Note that
> + * in the builtin case we still define kunit_<symbol_name>; the reason
> + * for this is it allows us to verify that the type value is correct
> + * in the builtin case and has not fallen out-of-sync with its original

Very clever! Can you maybe elaborate on how? I didn't understand until I
looked at the initialization code. It would probably be sufficient to
just tell the reader to look at the initialization code.

> + * definition.
> + */
> +#ifdef MODULE
> +#define KUNIT_VAR_SYMBOL(symbol, type)					\
> +	type * kunit_##symbol;						\
> +	type symbol
> +#else
> +#define KUNIT_VAR_SYMBOL(symbol, type)					\
> +	type * kunit_##symbol
> +#endif
> +
> +/**
> + * KUNIT_INIT_VAR_SYMBOL - A helper for initializing non-exported variable
> + *			   symbols
> + * @test: optional pointer to test context
> + * @name: name of symbol
> + *
> + * In the module case, initialization consists of using kunit_find_symbol()
> + * to find the address of the symbol, and if found, we set the variable
> + * to the dereferenced address value.  As mentioned above, in the builtin
> + * case we simply assing kunit_<symbol_name> to &<symbol_name> ; this will
> + * generate a compilation warning if the type we specified in KUNIT_VAR_SYMBOL
> + * and the type of the symbol itself do not match.
> + */
> +#ifdef MODULE
> +#define KUNIT_INIT_VAR_SYMBOL(test, symbol)				\
> +	do {								\
> +		if (!(kunit_##symbol)) {				\
> +			kunit_##symbol = kunit_find_symbol(#symbol);	\
> +			if (!IS_ERR((kunit_##symbol)))			\
> +				symbol = *(kunit_##symbol);		\
> +		}							\
> +		if (test)						\
> +			KUNIT_ASSERT_NOT_ERR_OR_NULL(test,		\
> +						     kunit_##symbol);	\
> +	} while (0)
> +#else
> +#define KUNIT_INIT_VAR_SYMBOL(test, symbol)				\
> +	kunit_##symbol = &(symbol)
> +#endif
> +
> +/**
> + * KUNIT_INIT_FN_SYMBOL - A helper for initializing non-exported function
> + *			  symbols
> + * @test: optional pointer to test context
> + * @symbol: name of symbol
> + * @name: local name of function used to store function pointer to symbol
> + *
> + * In the module case, initialization consists of using kunit_find_symbol()
> + * to find the address of the symbol, and if found, we set function pointer
> + * name to the function address value.  In the non-module case, we simply
> + * assign name to symbol; this will generate a compilation error if the
> + * type we specified for function pointer @name does not match the symbol
> + * function type.
> + */
> +#ifdef MODULE
> +#define KUNIT_INIT_FN_SYMBOL(test, symbol, name)			\
> +	do {								\
> +		if (!name)						\
> +			name = kunit_find_symbol(#symbol);		\
> +		if (test)                                               \
> +			KUNIT_ASSERT_NOT_ERR_OR_NULL(test, name);	\
> +	} while (0)
> +#else
> +#define KUNIT_INIT_FN_SYMBOL(test, symbol, name)			\
> +	name = symbol
> +#endif
> +

Can you put all the KUNIT_*_SYMBOL stuff in another patchset along with
the kunit_find_symbol?

>  struct kunit;
>  
>  /**
> @@ -197,31 +278,45 @@ struct kunit {
>  int kunit_run_tests(struct kunit_suite *suite);
>  
>  /**
> - * kunit_test_suite() - used to register a &struct kunit_suite with KUnit.
> + * kunit_test_suites() - used to register one or more &struct kunit_suite
> + *			 with KUnit.
>   *
> - * @suite: a statically allocated &struct kunit_suite.
> + * @suites: a statically allocated list of &struct kunit_suite.
>   *
> - * Registers @suite with the test framework. See &struct kunit_suite for
> + * Registers @suites 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)
>  
>  /*
>   * Like kunit_alloc_resource() below, but returns the struct kunit_resource

[PATCH v3 linux-kselftest-test 5/6] 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      | 2 ++
 lib/kunit/try-catch.c | 3 +++
 4 files changed, 9 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 e8b2443..c0ace36 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -523,3 +523,5 @@ void *kunit_find_symbol(const char *sym)
 	return ERR_PTR(-ENOENT);
 }
 EXPORT_SYMBOL(kunit_find_symbol);
+
+MODULE_LICENSE("GPL");
diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
index 1c1e9af..72fc8ed 100644
--- a/lib/kunit/try-catch.c
+++ b/lib/kunit/try-catch.c
@@ -31,6 +31,8 @@ static int kunit_generic_run_threadfn_adapter(void *data)
 	complete_and_exit(try_catch->try_completion, 0);
 }
 
+KUNIT_VAR_SYMBOL(sysctl_hung_task_timeout_secs, unsigned long);
+
 static unsigned long kunit_test_timeout(void)
 {
 	unsigned long timeout_msecs;
@@ -52,6 +54,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
 	 */
+	KUNIT_INIT_VAR_SYMBOL(NULL, sysctl_hung_task_timeout_secs);
 	if (sysctl_hung_task_timeout_secs) {
 		/*
 		 * If sysctl_hung_task is active, just set the timeout to some
-- 
1.8.3.1

Re: [PATCH v3 linux-kselftest-test 5/6] kunit: allow kunit to be loaded as a module

[Brendan Higgins]

On Thu, Oct 17, 2019 at 11:09 AM Alan Maguire <alan.maguire@oracle.com> 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      | 2 ++
>  lib/kunit/try-catch.c | 3 +++
>  4 files changed, 9 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 e8b2443..c0ace36 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -523,3 +523,5 @@ void *kunit_find_symbol(const char *sym)
>         return ERR_PTR(-ENOENT);
>  }
>  EXPORT_SYMBOL(kunit_find_symbol);
> +
> +MODULE_LICENSE("GPL");
> diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> index 1c1e9af..72fc8ed 100644
> --- a/lib/kunit/try-catch.c
> +++ b/lib/kunit/try-catch.c
> @@ -31,6 +31,8 @@ static int kunit_generic_run_threadfn_adapter(void *data)
>         complete_and_exit(try_catch->try_completion, 0);
>  }
>
> +KUNIT_VAR_SYMBOL(sysctl_hung_task_timeout_secs, unsigned long);

Can you just export sysctl_hung_task_timeout_secs?

I don't mean to make you redo all this work for one symbol twice, but
I thought we agreed on just exposing this symbol, but in a namespace.
It seemed like a good use case for that namespaced exporting thing
that Luis was talking about. As I understood it, you would have to
export it in the module that defines it, and then use the new
MODULE_IMPORT_NS() macro here.

> +
>  static unsigned long kunit_test_timeout(void)
>  {
>         unsigned long timeout_msecs;
> @@ -52,6 +54,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
>          */
> +       KUNIT_INIT_VAR_SYMBOL(NULL, sysctl_hung_task_timeout_secs);
>         if (sysctl_hung_task_timeout_secs) {
>                 /*
>                  * If sysctl_hung_task is active, just set the timeout to some
> --
> 1.8.3.1
>

Re: [PATCH v3 linux-kselftest-test 5/6] kunit: allow kunit to be loaded as a module

[Brendan Higgins]

On Thu, Nov 07, 2019 at 06:43:11PM -0800, Brendan Higgins wrote:
> On Thu, Oct 17, 2019 at 11:09 AM Alan Maguire <alan.maguire@oracle.com> 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      | 2 ++
> >  lib/kunit/try-catch.c | 3 +++
> >  4 files changed, 9 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 e8b2443..c0ace36 100644
> > --- a/lib/kunit/test.c
> > +++ b/lib/kunit/test.c
> > @@ -523,3 +523,5 @@ void *kunit_find_symbol(const char *sym)
> >         return ERR_PTR(-ENOENT);
> >  }
> >  EXPORT_SYMBOL(kunit_find_symbol);
> > +
> > +MODULE_LICENSE("GPL");
> > diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> > index 1c1e9af..72fc8ed 100644
> > --- a/lib/kunit/try-catch.c
> > +++ b/lib/kunit/try-catch.c
> > @@ -31,6 +31,8 @@ static int kunit_generic_run_threadfn_adapter(void *data)
> >         complete_and_exit(try_catch->try_completion, 0);
> >  }
> >
> > +KUNIT_VAR_SYMBOL(sysctl_hung_task_timeout_secs, unsigned long);
> 
> Can you just export sysctl_hung_task_timeout_secs?
> 
> I don't mean to make you redo all this work for one symbol twice, but
> I thought we agreed on just exposing this symbol, but in a namespace.
> It seemed like a good use case for that namespaced exporting thing
> that Luis was talking about. As I understood it, you would have to
> export it in the module that defines it, and then use the new
> MODULE_IMPORT_NS() macro here.

Also, I tried applying this and running this on kselftest/test and got
the following build error:

In file included from lib/kunit/try-catch.c:10:
lib/kunit/try-catch.c: In function ‘kunit_test_timeout’:
./include/kunit/test.h:132:19: error: lvalue required as unary ‘&’ operand
  kunit_##symbol = &(symbol)
                   ^
lib/kunit/try-catch.c:57:2: note: in expansion of macro ‘KUNIT_INIT_VAR_SYMBOL’
  KUNIT_INIT_VAR_SYMBOL(NULL, sysctl_hung_task_timeout_secs);
  ^~~~~~~~~~~~~~~~~~~~~

> > +
> >  static unsigned long kunit_test_timeout(void)
> >  {
> >         unsigned long timeout_msecs;
> > @@ -52,6 +54,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
> >          */
> > +       KUNIT_INIT_VAR_SYMBOL(NULL, sysctl_hung_task_timeout_secs);
> >         if (sysctl_hung_task_timeout_secs) {
> >                 /*
> >                  * If sysctl_hung_task is active, just set the timeout to some
> > --
> > 1.8.3.1
> >

Re: [PATCH v3 linux-kselftest-test 5/6] kunit: allow kunit to be loaded as a module

[Alan Maguire]

On Thu, 7 Nov 2019, Brendan Higgins wrote:

> On Thu, Oct 17, 2019 at 11:09 AM Alan Maguire <alan.maguire@oracle.com> 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      | 2 ++
> >  lib/kunit/try-catch.c | 3 +++
> >  4 files changed, 9 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 e8b2443..c0ace36 100644
> > --- a/lib/kunit/test.c
> > +++ b/lib/kunit/test.c
> > @@ -523,3 +523,5 @@ void *kunit_find_symbol(const char *sym)
> >         return ERR_PTR(-ENOENT);
> >  }
> >  EXPORT_SYMBOL(kunit_find_symbol);
> > +
> > +MODULE_LICENSE("GPL");
> > diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> > index 1c1e9af..72fc8ed 100644
> > --- a/lib/kunit/try-catch.c
> > +++ b/lib/kunit/try-catch.c
> > @@ -31,6 +31,8 @@ static int kunit_generic_run_threadfn_adapter(void *data)
> >         complete_and_exit(try_catch->try_completion, 0);
> >  }
> >
> > +KUNIT_VAR_SYMBOL(sysctl_hung_task_timeout_secs, unsigned long);
> 
> Can you just export sysctl_hung_task_timeout_secs?
> 
> I don't mean to make you redo all this work for one symbol twice, but
> I thought we agreed on just exposing this symbol, but in a namespace.
> It seemed like a good use case for that namespaced exporting thing
> that Luis was talking about. As I understood it, you would have to
> export it in the module that defines it, and then use the new
> MODULE_IMPORT_NS() macro here.
>

Sure, I can certainly look into that, though I wonder if we should 
consider another possibility - should kunit have its own sysctl table for 
things like configuring timeouts? I can look at adding a patch for that 
prior to the module patch so the issues with exporting the hung task 
timeout would go away. Now the reason I suggest this isn't as much a hack 
to solve this specific problem, rather it seems to fit better with the 
longer-term intent expressed by the comment around use of the field (at 
least as I read it, I may be wrong).

Exporting the symbol does allow us to piggy-back on an existing value, but 
maybe we should support out our own tunable "kunit_timeout_secs" here?
Doing so would also lay the groundwork for supporting other kunit 
tunables in the future if needed. What do you think?

Many thanks for the review! I've got an updated patchset almost 
ready with the symbol lookup stuff removed; the above is the last issue 
outstanding from my side.

Thanks!

Alan

> > +
> >  static unsigned long kunit_test_timeout(void)
> >  {
> >         unsigned long timeout_msecs;
> > @@ -52,6 +54,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
> >          */
> > +       KUNIT_INIT_VAR_SYMBOL(NULL, sysctl_hung_task_timeout_secs);
> >         if (sysctl_hung_task_timeout_secs) {
> >                 /*
> >                  * If sysctl_hung_task is active, just set the timeout to some
> > --
> > 1.8.3.1
> >
> 

Re: [PATCH v3 linux-kselftest-test 5/6] kunit: allow kunit to be loaded as a module

[Brendan Higgins]

+Stephen Boyd - since he is more of an expert on the hung task timer than I am.

On Fri, Nov 8, 2019 at 7:30 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> On Thu, 7 Nov 2019, Brendan Higgins wrote:
>
> > On Thu, Oct 17, 2019 at 11:09 AM Alan Maguire <alan.maguire@oracle.com> 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      | 2 ++
> > >  lib/kunit/try-catch.c | 3 +++
> > >  4 files changed, 9 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 e8b2443..c0ace36 100644
> > > --- a/lib/kunit/test.c
> > > +++ b/lib/kunit/test.c
> > > @@ -523,3 +523,5 @@ void *kunit_find_symbol(const char *sym)
> > >         return ERR_PTR(-ENOENT);
> > >  }
> > >  EXPORT_SYMBOL(kunit_find_symbol);
> > > +
> > > +MODULE_LICENSE("GPL");
> > > diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> > > index 1c1e9af..72fc8ed 100644
> > > --- a/lib/kunit/try-catch.c
> > > +++ b/lib/kunit/try-catch.c
> > > @@ -31,6 +31,8 @@ static int kunit_generic_run_threadfn_adapter(void *data)
> > >         complete_and_exit(try_catch->try_completion, 0);
> > >  }
> > >
> > > +KUNIT_VAR_SYMBOL(sysctl_hung_task_timeout_secs, unsigned long);
> >
> > Can you just export sysctl_hung_task_timeout_secs?
> >
> > I don't mean to make you redo all this work for one symbol twice, but
> > I thought we agreed on just exposing this symbol, but in a namespace.
> > It seemed like a good use case for that namespaced exporting thing
> > that Luis was talking about. As I understood it, you would have to
> > export it in the module that defines it, and then use the new
> > MODULE_IMPORT_NS() macro here.
> >
>
> Sure, I can certainly look into that, though I wonder if we should
> consider another possibility - should kunit have its own sysctl table for
> things like configuring timeouts? I can look at adding a patch for that

So on the one hand, yes, I would like to have configurable test
timeouts for KUnit, but that is not what the parameter check is for
here. This is to make sure KUnit times a test case out before the hung
task timer does.

> prior to the module patch so the issues with exporting the hung task
> timeout would go away. Now the reason I suggest this isn't as much a hack
> to solve this specific problem, rather it seems to fit better with the
> longer-term intent expressed by the comment around use of the field (at
> least as I read it, I may be wrong).

Not really. Although I do agree that adding configurability here might
be a good idea, I believe we would need to clamp such a value by
sysctl_hung_task_timeout_secs regardless since we don't want to be
killed by the hung task timer; thus, we still need access to
sysctl_hung_task_timeout_secs either way, and so doing what you are
proposing would be off topic.

> Exporting the symbol does allow us to piggy-back on an existing value, but
> maybe we should support out our own tunable "kunit_timeout_secs" here?
> Doing so would also lay the groundwork for supporting other kunit
> tunables in the future if needed. What do you think?

The goal is not to piggy back on the value as I mentioned above.
Stephen, do you have any thoughts on this? Do you see any other
preferable solution to what Alan is trying to do?

> Many thanks for the review! I've got an updated patchset almost
> ready with the symbol lookup stuff removed; the above is the last issue
> outstanding from my side.

Awesome! No thanks necessary, I appreciate the work you are doing!
There were some other people who mentioned that they wanted this in
the past, so it is a really big help having you do this. I feel bad
that I couldn't get the review back to you faster. :-)

>
> > > +
> > >  static unsigned long kunit_test_timeout(void)
> > >  {
> > >         unsigned long timeout_msecs;
> > > @@ -52,6 +54,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
> > >          */
> > > +       KUNIT_INIT_VAR_SYMBOL(NULL, sysctl_hung_task_timeout_secs);
> > >         if (sysctl_hung_task_timeout_secs) {
> > >                 /*
> > >                  * If sysctl_hung_task is active, just set the timeout to some
> > > --
> > > 1.8.3.1
> > >
> >

Re: [PATCH v3 linux-kselftest-test 5/6] kunit: allow kunit to be loaded as a module

[Brendan Higgins]

On Thu, Nov 14, 2019 at 1:31 PM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> +kselftest and kunit lists to document this decision.

Sorry for the spam. I accidentally CC'ed the doc list instead of the
kselftest list in my previous email.

> On Wed, Nov 13, 2019 at 11:54 PM Alan Maguire <alan.maguire@oracle.com> wrote:
> >
> > On Wed, 13 Nov 2019, Stephen Boyd wrote:
> >
> > > Quoting Brendan Higgins (2019-11-11 13:41:38)
> > > > +Stephen Boyd - since he is more of an expert on the hung task timer than I am.
> > > >
> > > > On Fri, Nov 8, 2019 at 7:30 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> > > > >
> > > > > On Thu, 7 Nov 2019, Brendan Higgins wrote:
> > > > >
> > > > > > On Thu, Oct 17, 2019 at 11:09 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> > > > > > > +MODULE_LICENSE("GPL");
> > > > > > > diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> > > > > > > index 1c1e9af..72fc8ed 100644
> > > > > > > --- a/lib/kunit/try-catch.c
> > > > > > > +++ b/lib/kunit/try-catch.c
> > > > > > > @@ -31,6 +31,8 @@ static int kunit_generic_run_threadfn_adapter(void *data)
> > > > > > >         complete_and_exit(try_catch->try_completion, 0);
> > > > > > >  }
> > > > > > >
> > > > > > > +KUNIT_VAR_SYMBOL(sysctl_hung_task_timeout_secs, unsigned long);
> > > > > >
> > > > > > Can you just export sysctl_hung_task_timeout_secs?
> > > > > >
> > > > > > I don't mean to make you redo all this work for one symbol twice, but
> > > > > > I thought we agreed on just exposing this symbol, but in a namespace.
> > > > > > It seemed like a good use case for that namespaced exporting thing
> > > > > > that Luis was talking about. As I understood it, you would have to
> > > > > > export it in the module that defines it, and then use the new
> > > > > > MODULE_IMPORT_NS() macro here.
> > > > > >
> > > > >
> > > > > Sure, I can certainly look into that, though I wonder if we should
> > > > > consider another possibility - should kunit have its own sysctl table for
> > > > > things like configuring timeouts? I can look at adding a patch for that
> > > >
> > > > So on the one hand, yes, I would like to have configurable test
> > > > timeouts for KUnit, but that is not what the parameter check is for
> > > > here. This is to make sure KUnit times a test case out before the hung
> > > > task timer does.
> > > >
> > > > > prior to the module patch so the issues with exporting the hung task
> > > > > timeout would go away. Now the reason I suggest this isn't as much a hack
> > > > > to solve this specific problem, rather it seems to fit better with the
> > > > > longer-term intent expressed by the comment around use of the field (at
> > > > > least as I read it, I may be wrong).
> > > >
> > > > Not really. Although I do agree that adding configurability here might
> > > > be a good idea, I believe we would need to clamp such a value by
> > > > sysctl_hung_task_timeout_secs regardless since we don't want to be
> > > > killed by the hung task timer; thus, we still need access to
> > > > sysctl_hung_task_timeout_secs either way, and so doing what you are
> > > > proposing would be off topic.
> > > >
> > > > > Exporting the symbol does allow us to piggy-back on an existing value, but
> > > > > maybe we should support out our own tunable "kunit_timeout_secs" here?
> > > > > Doing so would also lay the groundwork for supporting other kunit
> > > > > tunables in the future if needed. What do you think?
> > > >
> > > > The goal is not to piggy back on the value as I mentioned above.
> > > > Stephen, do you have any thoughts on this? Do you see any other
> > > > preferable solution to what Alan is trying to do?
> > >
> > > One idea would be to make some sort of process flag that says "this is a
> > > kunit task, ignore me with regards to the hung task timeout". Then we
> > > can hardcode the 5 minute kunit timeout. I'm not sure we have any more
> > > flags though.
> > >
> > > Or drop the whole timeout clamping logic, let the hung task timeout kick
> > > in and potentially oops the kernel, but then continue to let the test
> > > run and maybe sometimes get the kunit timeout here. This last option
> > > doesn't sound so bad to me given that this is all a corner case anyway
> > > where we don't expect to actually ever hit this problem so letting the
> > > hung task detector do its job is probably fine. This nicely avoids
> > > having to export this symbol to modules too.
> > >
> >
> > Thanks for suggestions! This latter approach seems fine to me; presumably
> > something has gone wrong if we are tripping the hung task timeout anyway,
> > so having an oops to document that seems fine. Brendan, what do you think?
>
> If Stephen thinks it is fine to drop the clamping logic, I think it is
> fine too. I think it would probably be good to replace it with a
> comment under the TODO that explains that a hung test *can* cause an
> oops if the hung task timeout is less than the kunit timeout value. It
> would probably be good to also select a timeout value that is less
> than the default hung task timeout. We might also want to link to this
> discussion. I fully expect that the timeout logic will get more
> attention at some point in the future.
>
> One more thing: Alan, can you submit the commit that drops the
> clamping logic in its own commit? I would prefer to make sure that it
> is easy to spot in the commit history.
>
> Cheers!

[PATCH v3 linux-kselftest-test 6/6] 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..82f9213 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