[RFC v1 0/6] kunit: create a centralized executor to dispatch all KUnit tests

[RFC v1 0/6] kunit: create a centralized executor to dispatch all KUnit tests

[Brendan Higgins]

## TL;DR

This patchset adds a centralized executor to dispatch tests rather than
relying on late_initcall to schedule each test suite separately along
with a couple of new features that depend on it.

## What am I trying to do?

Conceptually, I am trying to provide a mechanism by which test suites
can be grouped together so that they can be reasoned about collectively.
The last two patches in this series add features which depend on this:

RFC 5/6 Prints out a test plan right before KUnit tests are run[1]; this
        is valuable because it makes it possible for a test harness to
        detect whether the number of tests run matches the number of
        tests expected to be run, ensuring that no tests silently
        failed.

RFC 6/6 Add a new kernel command-line option which allows the user to
        specify that the kernel poweroff, halt, or reboot after
        completing all KUnit tests; this is very handy for running KUnit
        tests on UML or a VM so that the UML/VM process exits cleanly
        immediately after running all tests without needing a special
        initramfs.

In addition, by dispatching tests from a single location, we can
guarantee that all KUnit tests run after late_init is complete, which
was a concern during the initial KUnit patchset review (this has not
been a problem in practice, but resolving with certainty is nevertheless
desirable).

Other use cases for this exist, but the above features should provide an
idea of the value that this could provide.

## What work remains to be done?

These patches were based on patches in our non-upstream branch[2], so we
have a pretty good idea that they are useable as presented;
nevertheless, some of the changes done in this patchset could
*definitely* use some review by subsystem experts (linker scripts, init,
etc), and will likely change a lot after getting feedback.

The biggest thing that I know will require additional attention is
integrating this patchset with the KUnit module support patchset[3]. I
have not even attempted to build these patches on top of the module
support patches as I would like to get people's initial thoughts first
(especially Alan's :-) ). I think that making these patches work with
module support should be fairly straight forward, nevertheless.

Brendan Higgins (5):
  vmlinux.lds.h: add linker section for KUnit test suites
  arch: um: add linker section for KUnit test suites
  kunit: test: create a single centralized executor for all tests
  init: main: add KUnit to kernel init
  kunit: test: add test plan to KUnit TAP format

David Gow (1):
  kunit: Add 'kunit_shutdown' option

 arch/um/include/asm/common.lds.S              |  4 +
 include/asm-generic/vmlinux.lds.h             |  8 ++
 include/kunit/test.h                          | 16 ++--
 init/main.c                                   |  4 +
 lib/kunit/Makefile                            |  3 +-
 lib/kunit/executor.c                          | 74 ++++++++++++++++++
 lib/kunit/test.c                              | 11 ---
 tools/testing/kunit/kunit_kernel.py           |  2 +-
 tools/testing/kunit/kunit_parser.py           | 76 +++++++++++++++----
 .../test_is_test_passed-all_passed.log        |  1 +
 .../test_data/test_is_test_passed-crash.log   |  1 +
 .../test_data/test_is_test_passed-failure.log |  1 +
 12 files changed, 170 insertions(+), 31 deletions(-)
 create mode 100644 lib/kunit/executor.c

[1]: https://github.com/isaacs/testanything.github.io/blob/tap14/tap-version-14-specification.md#the-plan
[2]: https://kunit-review.googlesource.com/c/linux/+/1037
[3]: https://patchwork.kernel.org/project/linux-kselftest/list/?series=211727

-- 
2.24.1.735.g03f4e72817-goog

[RFC v1 1/6] vmlinux.lds.h: add linker section for KUnit test suites

[Brendan Higgins]

Add a linker section where KUnit can put references to its test suites.
This patch is the first step in transitioning to dispatching all KUnit
tests from a centralized executor rather than having each as its own
separate late_initcall.

Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Co-developed-by: Iurii Zaikin <yzaikin@google.com>
Signed-off-by: Iurii Zaikin <yzaikin@google.com>
---
 include/asm-generic/vmlinux.lds.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index e00f41aa8ec4f..99a866f49cb3d 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -856,6 +856,13 @@
 		KEEP(*(.con_initcall.init))				\
 		__con_initcall_end = .;
 
+/* Alignment must be consistent with (kunit_suite *) in include/kunit/test.h */
+#define KUNIT_TEST_SUITES						\
+		. = ALIGN(8);						\
+		__kunit_suites_start = .;				\
+		KEEP(*(.kunit_test_suites))				\
+		__kunit_suites_end = .;
+
 #ifdef CONFIG_BLK_DEV_INITRD
 #define INIT_RAM_FS							\
 	. = ALIGN(4);							\
@@ -1024,6 +1031,7 @@
 		INIT_CALLS						\
 		CON_INITCALL						\
 		INIT_RAM_FS						\
+		KUNIT_TEST_SUITES					\
 	}
 
 #define BSS_SECTION(sbss_align, bss_align, stop_align)			\
-- 
2.24.1.735.g03f4e72817-goog

Re: [RFC v1 1/6] vmlinux.lds.h: add linker section for KUnit test suites

[Stephen Boyd]

Quoting Brendan Higgins (2019-12-16 14:05:50)
> Add a linker section where KUnit can put references to its test suites.
> This patch is the first step in transitioning to dispatching all KUnit
> tests from a centralized executor rather than having each as its own
> separate late_initcall.
> 
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> Co-developed-by: Iurii Zaikin <yzaikin@google.com>
> Signed-off-by: Iurii Zaikin <yzaikin@google.com>

I thought your SoB should come last given you're sending the patch.

Otherwise,

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

[RFC v1 2/6] arch: um: add linker section for KUnit test suites

[Brendan Higgins]

Add a linker section to UML where KUnit can put references to its test
suites. This patch is an early step in transitioning to dispatching all
KUnit tests from a centralized executor rather than having each as its
own separate late_initcall.

Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
 arch/um/include/asm/common.lds.S | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/um/include/asm/common.lds.S b/arch/um/include/asm/common.lds.S
index 7145ce6999822..eab9ceb450efd 100644
--- a/arch/um/include/asm/common.lds.S
+++ b/arch/um/include/asm/common.lds.S
@@ -52,6 +52,10 @@
 	CON_INITCALL
   }
 
+  .kunit_test_suites : {
+	KUNIT_TEST_SUITES
+  }
+
   .exitcall : {
 	__exitcall_begin = .;
 	*(.exitcall.exit)
-- 
2.24.1.735.g03f4e72817-goog

Re: [RFC v1 2/6] arch: um: add linker section for KUnit test suites

[Stephen Boyd]

Quoting Brendan Higgins (2019-12-16 14:05:51)
> Add a linker section to UML where KUnit can put references to its test
> suites. This patch is an early step in transitioning to dispatching all
> KUnit tests from a centralized executor rather than having each as its
> own separate late_initcall.
> 
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

[RFC v1 3/6] kunit: test: create a single centralized executor for all tests

[Brendan Higgins]

Add a centralized executor to dispatch tests rather than relying on
late_initcall to schedule each test suite separately.

Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Co-developed-by: Iurii Zaikin <yzaikin@google.com>
Signed-off-by: Iurii Zaikin <yzaikin@google.com>
---
 include/kunit/test.h |  7 ++-----
 lib/kunit/Makefile   |  3 ++-
 lib/kunit/executor.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 6 deletions(-)
 create mode 100644 lib/kunit/executor.c

diff --git a/include/kunit/test.h b/include/kunit/test.h
index dba48304b3bd3..c070798ebb765 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -217,11 +217,8 @@ int kunit_run_tests(struct kunit_suite *suite);
  * 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)
+	static struct kunit_suite *__kunit_suite_##suite		       \
+	__used __aligned(8) __section(.kunit_test_suites) = &suite
 
 /*
  * Like kunit_alloc_resource() below, but returns the struct kunit_resource
diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
index 769d9402b5d3a..893df8a685880 100644
--- a/lib/kunit/Makefile
+++ b/lib/kunit/Makefile
@@ -1,7 +1,8 @@
 obj-$(CONFIG_KUNIT) +=			test.o \
 					string-stream.o \
 					assert.o \
-					try-catch.o
+					try-catch.o \
+					executor.o
 
 obj-$(CONFIG_KUNIT_TEST) +=		test-test.o \
 					string-stream-test.o
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
new file mode 100644
index 0000000000000..978086cfd257d
--- /dev/null
+++ b/lib/kunit/executor.c
@@ -0,0 +1,43 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Base unit test (KUnit) API.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#include <linux/init.h>
+#include <linux/printk.h>
+#include <kunit/test.h>
+
+/*
+ * These symbols point to the .kunit_test_suites section and are defined in
+ * include/asm-generic/vmlinux.lds.h, and consequently must be extern.
+ */
+extern struct kunit_suite *__kunit_suites_start[];
+extern struct kunit_suite *__kunit_suites_end[];
+
+static bool kunit_run_all_tests(void)
+{
+	struct kunit_suite **suite;
+	bool has_test_failed = false;
+
+	for (suite = __kunit_suites_start;
+	     suite < __kunit_suites_end;
+	     ++suite) {
+		if (kunit_run_tests(*suite))
+			has_test_failed = true;
+	}
+
+	return !has_test_failed;
+}
+
+static int kunit_executor_init(void)
+{
+	if (kunit_run_all_tests())
+		return 0;
+	else
+		return -EFAULT;
+}
+
+late_initcall(kunit_executor_init);
-- 
2.24.1.735.g03f4e72817-goog

Re: [RFC v1 3/6] kunit: test: create a single centralized executor for all tests

[Stephen Boyd]

Quoting Brendan Higgins (2019-12-16 14:05:52)
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index dba48304b3bd3..c070798ebb765 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -217,11 +217,8 @@ int kunit_run_tests(struct kunit_suite *suite);
>   * everything else is definitely initialized.
>   */
>  #define kunit_test_suite(suite)                                                       \
> -       static int kunit_suite_init##suite(void)                               \

Oh this should have been __init before.

> -       {                                                                      \
> -               return kunit_run_tests(&suite);                                \
> -       }                                                                      \
> -       late_initcall(kunit_suite_init##suite)
> +       static struct kunit_suite *__kunit_suite_##suite                       \
> +       __used __aligned(8) __section(.kunit_test_suites) = &suite
>  
>  /*
>   * Like kunit_alloc_resource() below, but returns the struct kunit_resource
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> new file mode 100644
> index 0000000000000..978086cfd257d
> --- /dev/null
> +++ b/lib/kunit/executor.c
> @@ -0,0 +1,43 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Base unit test (KUnit) API.
> + *
> + * Copyright (C) 2019, Google LLC.
> + * Author: Brendan Higgins <brendanhiggins@google.com>
> + */
> +
> +#include <linux/init.h>
> +#include <linux/printk.h>
> +#include <kunit/test.h>
> +
> +/*
> + * These symbols point to the .kunit_test_suites section and are defined in
> + * include/asm-generic/vmlinux.lds.h, and consequently must be extern.
> + */
> +extern struct kunit_suite *__kunit_suites_start[];
> +extern struct kunit_suite *__kunit_suites_end[];
> +
> +static bool kunit_run_all_tests(void)

Should be __init?

> +{
> +       struct kunit_suite **suite;

Can this be const? And the linker references above too?

> +       bool has_test_failed = false;
> +
> +       for (suite = __kunit_suites_start;
> +            suite < __kunit_suites_end;
> +            ++suite) {
> +               if (kunit_run_tests(*suite))
> +                       has_test_failed = true;
> +       }
> +
> +       return !has_test_failed;
> +}
> +
> +static int kunit_executor_init(void)

Should be __init?

> +{
> +       if (kunit_run_all_tests())
> +               return 0;
> +       else
> +               return -EFAULT;

Why two functions instead of just one that is the target of the
late_initcall? Nitpick: deindent that last return and take it out of the
else.

> +}
> +
> +late_initcall(kunit_executor_init);

Re: [RFC v1 3/6] kunit: test: create a single centralized executor for all tests

[Brendan Higgins]

On Tue, Dec 17, 2019 at 12:04 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-12-16 14:05:52)
> > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > index dba48304b3bd3..c070798ebb765 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -217,11 +217,8 @@ int kunit_run_tests(struct kunit_suite *suite);
> >   * everything else is definitely initialized.
> >   */
> >  #define kunit_test_suite(suite)                                                       \
> > -       static int kunit_suite_init##suite(void)                               \
>
> Oh this should have been __init before.

No, the stuff in this patch shouldn't be init. With the work that Alan
has been doing (adding support for modules, debugfs); the test code
can run after booting, so init in any of this code is incorrect.

> > -       {                                                                      \
> > -               return kunit_run_tests(&suite);                                \
> > -       }                                                                      \
> > -       late_initcall(kunit_suite_init##suite)
> > +       static struct kunit_suite *__kunit_suite_##suite                       \
> > +       __used __aligned(8) __section(.kunit_test_suites) = &suite
> >
> >  /*
> >   * Like kunit_alloc_resource() below, but returns the struct kunit_resource
> > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> > new file mode 100644
> > index 0000000000000..978086cfd257d
> > --- /dev/null
> > +++ b/lib/kunit/executor.c
> > @@ -0,0 +1,43 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Base unit test (KUnit) API.
> > + *
> > + * Copyright (C) 2019, Google LLC.
> > + * Author: Brendan Higgins <brendanhiggins@google.com>
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/printk.h>
> > +#include <kunit/test.h>
> > +
> > +/*
> > + * These symbols point to the .kunit_test_suites section and are defined in
> > + * include/asm-generic/vmlinux.lds.h, and consequently must be extern.
> > + */
> > +extern struct kunit_suite *__kunit_suites_start[];
> > +extern struct kunit_suite *__kunit_suites_end[];
> > +
> > +static bool kunit_run_all_tests(void)
>
> Should be __init?

It could be, I think. Alan's code doesn't call this, so for now we
might as well make it __init.

> > +{
> > +       struct kunit_suite **suite;
>
> Can this be const? And the linker references above too?

Good catch. Will fix.

> > +       bool has_test_failed = false;
> > +
> > +       for (suite = __kunit_suites_start;
> > +            suite < __kunit_suites_end;
> > +            ++suite) {
> > +               if (kunit_run_tests(*suite))
> > +                       has_test_failed = true;
> > +       }
> > +
> > +       return !has_test_failed;
> > +}
> > +
> > +static int kunit_executor_init(void)
>
> Should be __init?

Will do.

> > +{
> > +       if (kunit_run_all_tests())
> > +               return 0;
> > +       else
> > +               return -EFAULT;
>
> Why two functions instead of just one that is the target of the
> late_initcall? Nitpick: deindent that last return and take it out of the
> else.

Yeah, it probably makes more sense to just call kunit_run_all_tests
and have it return an int.

> > +}
> > +
> > +late_initcall(kunit_executor_init);

[RFC v1 4/6] init: main: add KUnit to kernel init

[Brendan Higgins]

Remove KUnit from init calls entirely, instead call directly from
kernel_init().

Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
 include/kunit/test.h | 9 +++++++++
 init/main.c          | 4 ++++
 lib/kunit/executor.c | 4 +---
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index c070798ebb765..9da4f2cc1a3fc 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -196,6 +196,15 @@ void kunit_init_test(struct kunit *test, const char *name);
 
 int kunit_run_tests(struct kunit_suite *suite);
 
+#if IS_ENABLED(CONFIG_KUNIT)
+int kunit_executor_init(void);
+#else
+static inline int kunit_executor_init(void)
+{
+	return 0;
+}
+#endif /* IS_ENABLED(CONFIG_KUNIT) */
+
 /**
  * kunit_test_suite() - used to register a &struct kunit_suite with KUnit.
  *
diff --git a/init/main.c b/init/main.c
index 91f6ebb30ef04..b299396a5466b 100644
--- a/init/main.c
+++ b/init/main.c
@@ -103,6 +103,8 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/initcall.h>
 
+#include <kunit/test.h>
+
 static int kernel_init(void *);
 
 extern void init_IRQ(void);
@@ -1190,6 +1192,8 @@ static noinline void __init kernel_init_freeable(void)
 
 	do_basic_setup();
 
+	kunit_executor_init();
+
 	/* Open the /dev/console on the rootfs, this should never fail */
 	if (ksys_open((const char __user *) "/dev/console", O_RDWR, 0) < 0)
 		pr_err("Warning: unable to open an initial console.\n");
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index 978086cfd257d..ca880224c0bab 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c
@@ -32,12 +32,10 @@ static bool kunit_run_all_tests(void)
 	return !has_test_failed;
 }
 
-static int kunit_executor_init(void)
+int kunit_executor_init(void)
 {
 	if (kunit_run_all_tests())
 		return 0;
 	else
 		return -EFAULT;
 }
-
-late_initcall(kunit_executor_init);
-- 
2.24.1.735.g03f4e72817-goog

Re: [RFC v1 4/6] init: main: add KUnit to kernel init

[Stephen Boyd]

Quoting Brendan Higgins (2019-12-16 14:05:53)
> Remove KUnit from init calls entirely, instead call directly from
> kernel_init().

Yes, but why? Is it desired to run the unit tests earlier than opening
the console or something?

> 
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index 978086cfd257d..ca880224c0bab 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -32,12 +32,10 @@ static bool kunit_run_all_tests(void)
>         return !has_test_failed;
>  }
>  
> -static int kunit_executor_init(void)
> +int kunit_executor_init(void)

Should be marked __init? Even before this patch presumably.

>  {
>         if (kunit_run_all_tests())
>                 return 0;
>         else
>                 return -EFAULT;
>  }
> -
> -late_initcall(kunit_executor_init);

Re: [RFC v1 4/6] init: main: add KUnit to kernel init

[Brendan Higgins]

Sorry for the late reply. I sent this thinking I would check in over
vacation, and then didn't.

On Mon, Dec 16, 2019 at 11:58 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-12-16 14:05:53)
> > Remove KUnit from init calls entirely, instead call directly from
> > kernel_init().
>
> Yes, but why? Is it desired to run the unit tests earlier than opening
> the console or something?

I want to make sure it is called after late_init is done (so that you
can test things initialized in late_init). And I want to make sure it
runs before init*fs is loaded so that there is a mechanism to run
tests without having to put a userland together.

> > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> > index 978086cfd257d..ca880224c0bab 100644
> > --- a/lib/kunit/executor.c
> > +++ b/lib/kunit/executor.c
> > @@ -32,12 +32,10 @@ static bool kunit_run_all_tests(void)
> >         return !has_test_failed;
> >  }
> >
> > -static int kunit_executor_init(void)
> > +int kunit_executor_init(void)
>
> Should be marked __init? Even before this patch presumably.

Just this function? No strong opinion.

If by "before this patch" you mean other stuff in this patchset?

> >  {
> >         if (kunit_run_all_tests())
> >                 return 0;
> >         else
> >                 return -EFAULT;
> >  }
> > -
> > -late_initcall(kunit_executor_init);

[RFC v1 5/6] kunit: test: add test plan to KUnit TAP format

[Brendan Higgins]

TAP 14 allows an optional test plan to be emitted before the start of
the start of testing[1]; this is valuable because it makes it possible
for a test harness to detect whether the number of tests run matches the
number of tests expected to be run, ensuring that no tests silently
failed.

Link[1]: https://github.com/isaacs/testanything.github.io/blob/tap14/tap-version-14-specification.md#the-plan
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
 lib/kunit/executor.c                          | 15 ++++
 lib/kunit/test.c                              | 11 ---
 tools/testing/kunit/kunit_parser.py           | 74 ++++++++++++++++---
 .../test_is_test_passed-all_passed.log        |  1 +
 .../test_data/test_is_test_passed-crash.log   |  1 +
 .../test_data/test_is_test_passed-failure.log |  1 +
 6 files changed, 80 insertions(+), 23 deletions(-)

diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index ca880224c0bab..d5f1d07f2f817 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c
@@ -17,11 +17,26 @@
 extern struct kunit_suite *__kunit_suites_start[];
 extern struct kunit_suite *__kunit_suites_end[];
 
+static void kunit_print_tap_header(void)
+{
+	size_t num_of_suites;
+
+	for (num_of_suites = 0;
+	     num_of_suites + __kunit_suites_start < __kunit_suites_end;
+	     ++num_of_suites)
+		;
+
+	pr_info("TAP version 14\n");
+	pr_info("1..%zd\n", num_of_suites);
+}
+
 static bool kunit_run_all_tests(void)
 {
 	struct kunit_suite **suite;
 	bool has_test_failed = false;
 
+	kunit_print_tap_header();
+
 	for (suite = __kunit_suites_start;
 	     suite < __kunit_suites_end;
 	     ++suite) {
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index c83c0fa59cbd7..f8f235f9e466a 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -16,16 +16,6 @@ static void kunit_set_failure(struct kunit *test)
 	WRITE_ONCE(test->success, false);
 }
 
-static void kunit_print_tap_version(void)
-{
-	static bool kunit_has_printed_tap_version;
-
-	if (!kunit_has_printed_tap_version) {
-		pr_info("TAP version 14\n");
-		kunit_has_printed_tap_version = true;
-	}
-}
-
 static size_t kunit_test_cases_len(struct kunit_case *test_cases)
 {
 	struct kunit_case *test_case;
@@ -39,7 +29,6 @@ static size_t kunit_test_cases_len(struct kunit_case *test_cases)
 
 static void kunit_print_subtest_start(struct kunit_suite *suite)
 {
-	kunit_print_tap_version();
 	pr_info("\t# Subtest: %s\n", suite->name);
 	pr_info("\t1..%zd\n", kunit_test_cases_len(suite->test_cases));
 }
diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
index 4ffbae0f67325..78b3bdd03b1e4 100644
--- a/tools/testing/kunit/kunit_parser.py
+++ b/tools/testing/kunit/kunit_parser.py
@@ -45,6 +45,7 @@ class TestStatus(Enum):
 	FAILURE = auto()
 	TEST_CRASHED = auto()
 	NO_TESTS = auto()
+	FAILURE_TO_PARSE_TESTS = auto()
 
 kunit_start_re = re.compile(r'^TAP version [0-9]+$')
 kunit_end_re = re.compile('List of all partitions:')
@@ -106,7 +107,7 @@ OkNotOkResult = namedtuple('OkNotOkResult', ['is_ok','description', 'text'])
 
 OK_NOT_OK_SUBTEST = re.compile(r'^\t(ok|not ok) [0-9]+ - (.*)$')
 
-OK_NOT_OK_MODULE = re.compile(r'^(ok|not ok) [0-9]+ - (.*)$')
+OK_NOT_OK_MODULE = re.compile(r'^(ok|not ok) ([0-9]+) - (.*)$')
 
 def parse_ok_not_ok_test_case(lines: List[str],
 			      test_case: TestCase,
@@ -196,7 +197,9 @@ def max_status(left: TestStatus, right: TestStatus) -> TestStatus:
 	else:
 		return TestStatus.SUCCESS
 
-def parse_ok_not_ok_test_suite(lines: List[str], test_suite: TestSuite) -> bool:
+def parse_ok_not_ok_test_suite(lines: List[str],
+			       test_suite: TestSuite,
+			       expected_suite_index: int) -> bool:
 	consume_non_diagnositic(lines)
 	if not lines:
 		test_suite.status = TestStatus.TEST_CRASHED
@@ -209,6 +212,12 @@ def parse_ok_not_ok_test_suite(lines: List[str], test_suite: TestSuite) -> bool:
 			test_suite.status = TestStatus.SUCCESS
 		else:
 			test_suite.status = TestStatus.FAILURE
+		suite_index = int(match.group(2))
+		if suite_index != expected_suite_index:
+			print_with_timestamp(
+				red('[ERROR] ') + 'expected_suite_index ' +
+				str(expected_suite_index) + ', but got ' +
+				str(suite_index))
 		return True
 	else:
 		return False
@@ -221,7 +230,7 @@ def bubble_up_test_case_errors(test_suite: TestSuite) -> TestStatus:
 	max_test_case_status = bubble_up_errors(lambda x: x.status, test_suite.cases)
 	return max_status(max_test_case_status, test_suite.status)
 
-def parse_test_suite(lines: List[str]) -> TestSuite:
+def parse_test_suite(lines: List[str], expected_suite_index: int) -> TestSuite:
 	if not lines:
 		return None
 	consume_non_diagnositic(lines)
@@ -240,7 +249,7 @@ def parse_test_suite(lines: List[str]) -> TestSuite:
 		test_suite.cases.append(test_case)
 		test_case = parse_test_case(lines, expected_test_case_num > 0)
 		expected_test_case_num -= 1
-	if parse_ok_not_ok_test_suite(lines, test_suite):
+	if parse_ok_not_ok_test_suite(lines, test_suite, expected_suite_index):
 		test_suite.status = bubble_up_test_case_errors(test_suite)
 		return test_suite
 	elif not lines:
@@ -260,6 +269,17 @@ def parse_tap_header(lines: List[str]) -> bool:
 	else:
 		return False
 
+TEST_PLAN = re.compile(r'[0-9]+\.\.([0-9]+)')
+
+def parse_test_plan(lines: List[str]) -> int:
+	consume_non_diagnositic(lines)
+	match = TEST_PLAN.match(lines[0])
+	if match:
+		lines.pop(0)
+		return int(match.group(1))
+	else:
+		return None
+
 def bubble_up_suite_errors(test_suite_list: List[TestSuite]) -> TestStatus:
 	return bubble_up_errors(lambda x: x.status, test_suite_list)
 
@@ -268,19 +288,34 @@ def parse_test_result(lines: List[str]) -> TestResult:
 		return TestResult(TestStatus.NO_TESTS, [], lines)
 	consume_non_diagnositic(lines)
 	if not parse_tap_header(lines):
-		return None
+		return TestResult(TestStatus.NO_TESTS, [], lines)
+	expected_test_suite_num = parse_test_plan(lines)
+	if not expected_test_suite_num:
+		return TestResult(TestStatus.FAILURE_TO_PARSE_TESTS, [], lines)
 	test_suites = []
-	test_suite = parse_test_suite(lines)
-	while test_suite:
-		test_suites.append(test_suite)
-		test_suite = parse_test_suite(lines)
-	return TestResult(bubble_up_suite_errors(test_suites), test_suites, lines)
+	for i in range(1, expected_test_suite_num + 1):
+		test_suite = parse_test_suite(lines, i)
+		if test_suite:
+			test_suites.append(test_suite)
+		else:
+			print_with_timestamp(
+				red('[ERROR] ') + ' expected ' +
+				str(expected_test_suite_num) +
+				' test suites, but got ' + str(i - 2))
+			break
+	test_suite = parse_test_suite(lines, -1)
+	if test_suite:
+		print_with_timestamp(red('[ERROR] ') +
+			'got unexpected test suite: ' + test_suite.name)
+	if test_suites:
+		return TestResult(bubble_up_suite_errors(test_suites), test_suites, lines)
+	else:
+		return TestResult(TestStatus.NO_TESTS, [], lines)
 
-def parse_run_tests(kernel_output) -> TestResult:
+def print_and_count_results(test_result: TestResult) -> None:
 	total_tests = 0
 	failed_tests = 0
 	crashed_tests = 0
-	test_result = parse_test_result(list(isolate_kunit_output(kernel_output)))
 	for test_suite in test_result.suites:
 		if test_suite.status == TestStatus.SUCCESS:
 			print_suite_divider(green('[PASSED] ') + test_suite.name)
@@ -302,6 +337,21 @@ def parse_run_tests(kernel_output) -> TestResult:
 				print_with_timestamp(red('[FAILED] ') + test_case.name)
 				print_log(map(yellow, test_case.log))
 				print_with_timestamp('')
+	return total_tests, failed_tests, crashed_tests
+
+def parse_run_tests(kernel_output) -> TestResult:
+	total_tests = 0
+	failed_tests = 0
+	crashed_tests = 0
+	test_result = parse_test_result(list(isolate_kunit_output(kernel_output)))
+	if test_result.status == TestStatus.NO_TESTS:
+		print(red('[ERROR] ') + yellow('no tests run!'))
+	elif test_result.status == TestStatus.FAILURE_TO_PARSE_TESTS:
+		print(red('[ERROR] ') + yellow('could not parse test results!'))
+	else:
+		(total_tests,
+		 failed_tests,
+		 crashed_tests) = print_and_count_results(test_result)
 	print_with_timestamp(DIVIDER)
 	fmt = green if test_result.status == TestStatus.SUCCESS else red
 	print_with_timestamp(
diff --git a/tools/testing/kunit/test_data/test_is_test_passed-all_passed.log b/tools/testing/kunit/test_data/test_is_test_passed-all_passed.log
index 62ebc0288355c..bc0dc8fe35b76 100644
--- a/tools/testing/kunit/test_data/test_is_test_passed-all_passed.log
+++ b/tools/testing/kunit/test_data/test_is_test_passed-all_passed.log
@@ -1,4 +1,5 @@
 TAP version 14
+1..2
 	# Subtest: sysctl_test
 	1..8
 	# sysctl_test_dointvec_null_tbl_data: sysctl_test_dointvec_null_tbl_data passed
diff --git a/tools/testing/kunit/test_data/test_is_test_passed-crash.log b/tools/testing/kunit/test_data/test_is_test_passed-crash.log
index 0b249870c8be4..4d97f6708c4a5 100644
--- a/tools/testing/kunit/test_data/test_is_test_passed-crash.log
+++ b/tools/testing/kunit/test_data/test_is_test_passed-crash.log
@@ -1,6 +1,7 @@
 printk: console [tty0] enabled
 printk: console [mc-1] enabled
 TAP version 14
+1..2
 	# Subtest: sysctl_test
 	1..8
 	# sysctl_test_dointvec_null_tbl_data: sysctl_test_dointvec_null_tbl_data passed
diff --git a/tools/testing/kunit/test_data/test_is_test_passed-failure.log b/tools/testing/kunit/test_data/test_is_test_passed-failure.log
index 9e89d32d5667a..7a416497e3bec 100644
--- a/tools/testing/kunit/test_data/test_is_test_passed-failure.log
+++ b/tools/testing/kunit/test_data/test_is_test_passed-failure.log
@@ -1,4 +1,5 @@
 TAP version 14
+1..2
 	# Subtest: sysctl_test
 	1..8
 	# sysctl_test_dointvec_null_tbl_data: sysctl_test_dointvec_null_tbl_data passed
-- 
2.24.1.735.g03f4e72817-goog

Re: [RFC v1 5/6] kunit: test: add test plan to KUnit TAP format

[Stephen Boyd]

Quoting Brendan Higgins (2019-12-16 14:05:54)
> TAP 14 allows an optional test plan to be emitted before the start of
> the start of testing[1]; this is valuable because it makes it possible
> for a test harness to detect whether the number of tests run matches the
> number of tests expected to be run, ensuring that no tests silently
> failed.
> 
> Link[1]: https://github.com/isaacs/testanything.github.io/blob/tap14/tap-version-14-specification.md#the-plan
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

[RFC v1 6/6] kunit: Add 'kunit_shutdown' option

[Brendan Higgins]

From: David Gow <davidgow@google.com>

Add a new kernel command-line option, 'kunit_shutdown', which allows the
user to specify that the kernel poweroff, halt, or reboot after
completing all KUnit tests; this is very handy for running KUnit tests
on UML or a VM so that the UML/VM process exits cleanly immediately
after running all tests without needing a special initramfs.

Signed-off-by: David Gow <davidgow@google.com>
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
 lib/kunit/executor.c                | 18 ++++++++++++++++++
 tools/testing/kunit/kunit_kernel.py |  2 +-
 tools/testing/kunit/kunit_parser.py |  2 +-
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index d5f1d07f2f817..32462ecb94eb6 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c
@@ -7,7 +7,9 @@
  */
 
 #include <linux/init.h>
+#include <linux/moduleparam.h>
 #include <linux/printk.h>
+#include <linux/reboot.h>
 #include <kunit/test.h>
 
 /*
@@ -17,6 +19,9 @@
 extern struct kunit_suite *__kunit_suites_start[];
 extern struct kunit_suite *__kunit_suites_end[];
 
+static char *kunit_shutdown;
+core_param(kunit_shutdown, kunit_shutdown, charp, 0644);
+
 static void kunit_print_tap_header(void)
 {
 	size_t num_of_suites;
@@ -30,6 +35,17 @@ static void kunit_print_tap_header(void)
 	pr_info("1..%zd\n", num_of_suites);
 }
 
+static void kunit_handle_shutdown(void)
+{
+	if (!strcmp(kunit_shutdown, "poweroff")) {
+		kernel_power_off();
+	} else if (!strcmp(kunit_shutdown, "halt")) {
+		kernel_halt();
+	} else if (!strcmp(kunit_shutdown, "reboot")) {
+		kernel_restart(NULL);
+	}
+}
+
 static bool kunit_run_all_tests(void)
 {
 	struct kunit_suite **suite;
@@ -44,6 +60,8 @@ static bool kunit_run_all_tests(void)
 			has_test_failed = true;
 	}
 
+	kunit_handle_shutdown();
+
 	return !has_test_failed;
 }
 
diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
index bf38768353313..0070c6b807d2a 100644
--- a/tools/testing/kunit/kunit_kernel.py
+++ b/tools/testing/kunit/kunit_kernel.py
@@ -141,7 +141,7 @@ class LinuxSourceTree(object):
 		return True
 
 	def run_kernel(self, args=[], timeout=None, build_dir=None):
-		args.extend(['mem=256M'])
+		args.extend(['mem=256M', 'kunit_shutdown=halt'])
 		process = self._ops.linux_bin(args, timeout, build_dir)
 		with open('test.log', 'w') as f:
 			for line in process.stdout:
diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
index 78b3bdd03b1e4..633811dd9bce8 100644
--- a/tools/testing/kunit/kunit_parser.py
+++ b/tools/testing/kunit/kunit_parser.py
@@ -48,7 +48,7 @@ class TestStatus(Enum):
 	FAILURE_TO_PARSE_TESTS = auto()
 
 kunit_start_re = re.compile(r'^TAP version [0-9]+$')
-kunit_end_re = re.compile('List of all partitions:')
+kunit_end_re = re.compile(r'reboot: System halted')
 
 def isolate_kunit_output(kernel_output):
 	started = False
-- 
2.24.1.735.g03f4e72817-goog

Re: [RFC v1 6/6] kunit: Add 'kunit_shutdown' option

[Stephen Boyd]

Quoting Brendan Higgins (2019-12-16 14:05:55)
> From: David Gow <davidgow@google.com>
> 
> Add a new kernel command-line option, 'kunit_shutdown', which allows the
> user to specify that the kernel poweroff, halt, or reboot after
> completing all KUnit tests; this is very handy for running KUnit tests
> on UML or a VM so that the UML/VM process exits cleanly immediately
> after running all tests without needing a special initramfs.
> 
> Signed-off-by: David Gow <davidgow@google.com>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---
>  lib/kunit/executor.c                | 18 ++++++++++++++++++
>  tools/testing/kunit/kunit_kernel.py |  2 +-
>  tools/testing/kunit/kunit_parser.py |  2 +-
>  3 files changed, 20 insertions(+), 2 deletions(-)

Can you document it in Documentation/admin-guide/kernel-parameters.txt ?

Re: [RFC v1 6/6] kunit: Add 'kunit_shutdown' option

[Brendan Higgins]

On Tue, Dec 17, 2019 at 12:06 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-12-16 14:05:55)
> > From: David Gow <davidgow@google.com>
> >
> > Add a new kernel command-line option, 'kunit_shutdown', which allows the
> > user to specify that the kernel poweroff, halt, or reboot after
> > completing all KUnit tests; this is very handy for running KUnit tests
> > on UML or a VM so that the UML/VM process exits cleanly immediately
> > after running all tests without needing a special initramfs.
> >
> > Signed-off-by: David Gow <davidgow@google.com>
> > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > ---
> >  lib/kunit/executor.c                | 18 ++++++++++++++++++
> >  tools/testing/kunit/kunit_kernel.py |  2 +-
> >  tools/testing/kunit/kunit_parser.py |  2 +-
> >  3 files changed, 20 insertions(+), 2 deletions(-)
>
> Can you document it in Documentation/admin-guide/kernel-parameters.txt ?

Ah, yes. That would be a good idea. Sorry, I just expected to be
shouted at loudly for doing this, and didn't want to expend the effort
until some people told me that they didn't hate the idea.

Re: [RFC v1 0/6] kunit: create a centralized executor to dispatch all KUnit tests

[Luis Chamberlain]

On Mon, Dec 16, 2019 at 02:05:49PM -0800, Brendan Higgins wrote:
> ## TL;DR
> 
> This patchset adds a centralized executor to dispatch tests rather than
> relying on late_initcall to schedule each test suite separately along
> with a couple of new features that depend on it.
> 
> ## What am I trying to do?
> 
> Conceptually, I am trying to provide a mechanism by which test suites
> can be grouped together so that they can be reasoned about collectively.
> The last two patches in this series add features which depend on this:
> 
> RFC 5/6 Prints out a test plan right before KUnit tests are run[1]; this
>         is valuable because it makes it possible for a test harness to
>         detect whether the number of tests run matches the number of
>         tests expected to be run, ensuring that no tests silently
>         failed.
> 
> RFC 6/6 Add a new kernel command-line option which allows the user to
>         specify that the kernel poweroff, halt, or reboot after
>         completing all KUnit tests; this is very handy for running KUnit
>         tests on UML or a VM so that the UML/VM process exits cleanly
>         immediately after running all tests without needing a special
>         initramfs.

The approach seems sensible to me given that it separates from a
semantics perspective kernel subsystem init work from *testing*, and
so we are sure we'd run the *test* stuff *after* all subsystem init
stuff.

Dispatching, however is still immediate, and with a bit of work, this
dispatcher could be configurable to run at an arbirary time after boot.
If there are not immediate use cases for that though, then I suppose
this is not a requirement for the dispatcher. But since there exists
another modular test framework with its own dispatcher and it seems the
goal is to merge the work long term, this might preempt the requirement
to define how and when we can dispatch tests post boot.

And, if we're going to do that, I can suggest that a data structure
instead of just a function init call be used to describe tests to be
placed into an ELF section. With my linker table work this would be
easy, I define section ranges for code describing only executable
routines, but it defines linker tables for when a component in the
kernel would define a data structure, part of which can be a callback.
Such data structure stuffed into an ELF section could allow dynamic
configuration of the dipsatching, even post boot.

I think this is a good stepping stone forward then, and to allow
dynamic configuration of the dispatcher could mean eventual extensions
to kunit's init stuff to stuff init calls into a data structure which
can then allow configuration of the dispatching. One benefit that the
linker table work *may* be able to help here with is that it allows
an easy way to create kunit specific ordering, at linker time.
There is also an example of addressing / generalizing dynamic / run time
changes of ordering, by using the x86 IOMMU initialization as an
example case. We don't have an easy way to do this today, but if kunit
could benefit from such framework, it'd be another use case for
the linker table work. That is, the ability to easilly allow
dynamically modifying run time ordering of code through ELF sections.

> In addition, by dispatching tests from a single location, we can
> guarantee that all KUnit tests run after late_init is complete, which
> was a concern during the initial KUnit patchset review (this has not
> been a problem in practice, but resolving with certainty is nevertheless
> desirable).

Indeed, the concern is just a real semantics limitations. With the tests
*always* running after all subsystem init stuff, we know we'd have a
real full kernel ready.

It does beg the question if this means kunit is happy to not be a tool
to test pre basic setup stuff (terminology used in init.c, meaning prior
to running all init levels). I suspect this is the case.

> Other use cases for this exist, but the above features should provide an
> idea of the value that this could provide.
> 
> ## What work remains to be done?
> 
> These patches were based on patches in our non-upstream branch[2], so we
> have a pretty good idea that they are useable as presented;
> nevertheless, some of the changes done in this patchset could
> *definitely* use some review by subsystem experts (linker scripts, init,
> etc), and will likely change a lot after getting feedback.
> 
> The biggest thing that I know will require additional attention is
> integrating this patchset with the KUnit module support patchset[3]. I
> have not even attempted to build these patches on top of the module
> support patches as I would like to get people's initial thoughts first
> (especially Alan's :-) ). I think that making these patches work with
> module support should be fairly straight forward, nevertheless.

Modules just have their own sections too. That's all. So it'd be a
matter of extending the linker script for modules too. But a module's
init is different than the core kernel's for vmlinux.

  Luis

Re: [RFC v1 0/6] kunit: create a centralized executor to dispatch all KUnit tests

[Brendan Higgins]

Sorry for the late reply. I am still catching up from being on vacation.

On Mon, Jan 6, 2020 at 2:40 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Mon, Dec 16, 2019 at 02:05:49PM -0800, Brendan Higgins wrote:
> > ## TL;DR
> >
> > This patchset adds a centralized executor to dispatch tests rather than
> > relying on late_initcall to schedule each test suite separately along
> > with a couple of new features that depend on it.
> >
> > ## What am I trying to do?
> >
> > Conceptually, I am trying to provide a mechanism by which test suites
> > can be grouped together so that they can be reasoned about collectively.
> > The last two patches in this series add features which depend on this:
> >
> > RFC 5/6 Prints out a test plan right before KUnit tests are run[1]; this
> >         is valuable because it makes it possible for a test harness to
> >         detect whether the number of tests run matches the number of
> >         tests expected to be run, ensuring that no tests silently
> >         failed.
> >
> > RFC 6/6 Add a new kernel command-line option which allows the user to
> >         specify that the kernel poweroff, halt, or reboot after
> >         completing all KUnit tests; this is very handy for running KUnit
> >         tests on UML or a VM so that the UML/VM process exits cleanly
> >         immediately after running all tests without needing a special
> >         initramfs.
>
> The approach seems sensible to me given that it separates from a
> semantics perspective kernel subsystem init work from *testing*, and
> so we are sure we'd run the *test* stuff *after* all subsystem init
> stuff.

Cool, I thought you would find this interesting.

> Dispatching, however is still immediate, and with a bit of work, this
> dispatcher could be configurable to run at an arbirary time after boot.
> If there are not immediate use cases for that though, then I suppose
> this is not a requirement for the dispatcher. But since there exists
> another modular test framework with its own dispatcher and it seems the
> goal is to merge the work long term, this might preempt the requirement
> to define how and when we can dispatch tests post boot.
>
> And, if we're going to do that, I can suggest that a data structure
> instead of just a function init call be used to describe tests to be
> placed into an ELF section. With my linker table work this would be
> easy, I define section ranges for code describing only executable
> routines, but it defines linker tables for when a component in the
> kernel would define a data structure, part of which can be a callback.
> Such data structure stuffed into an ELF section could allow dynamic
> configuration of the dipsatching, even post boot.

The linker table work does sound interesting. Do you have a link?

I was thinking about dynamic dispatching, actually. I thought it would
be handy to be able to build all tests into a single kernel and then
run different tests on different invocations.

Also, for post boot dynamic dispatching, you should check out Alan's
debugfs patches:

https://lore.kernel.org/linux-kselftest/CAFd5g46657gZ36PaP8Pi999hPPgBU2Kz94nrMspS-AzGwdBF+g@mail.gmail.com/T/#m210cadbeee267e5c5a9253d83b7b7ca723d1f871

They look pretty handy!

> I think this is a good stepping stone forward then, and to allow
> dynamic configuration of the dispatcher could mean eventual extensions
> to kunit's init stuff to stuff init calls into a data structure which
> can then allow configuration of the dispatching. One benefit that the
> linker table work *may* be able to help here with is that it allows
> an easy way to create kunit specific ordering, at linker time.
> There is also an example of addressing / generalizing dynamic / run time
> changes of ordering, by using the x86 IOMMU initialization as an
> example case. We don't have an easy way to do this today, but if kunit
> could benefit from such framework, it'd be another use case for
> the linker table work. That is, the ability to easilly allow
> dynamically modifying run time ordering of code through ELF sections.
>
> > In addition, by dispatching tests from a single location, we can
> > guarantee that all KUnit tests run after late_init is complete, which
> > was a concern during the initial KUnit patchset review (this has not
> > been a problem in practice, but resolving with certainty is nevertheless
> > desirable).
>
> Indeed, the concern is just a real semantics limitations. With the tests
> *always* running after all subsystem init stuff, we know we'd have a
> real full kernel ready.

Yep.

> It does beg the question if this means kunit is happy to not be a tool
> to test pre basic setup stuff (terminology used in init.c, meaning prior
> to running all init levels). I suspect this is the case.

Not sure. I still haven't seen any cases where this is necessary, so I
am not super worried about it. Regardless, I don't think this patchset
really changes anything in that regard, we are moving from late_init
to after late_init, so it isn't that big of a change for most use
cases.

Please share if you can think of some things that need to be tested in
early init.

> > Other use cases for this exist, but the above features should provide an
> > idea of the value that this could provide.
> >
> > ## What work remains to be done?
> >
> > These patches were based on patches in our non-upstream branch[2], so we
> > have a pretty good idea that they are useable as presented;
> > nevertheless, some of the changes done in this patchset could
> > *definitely* use some review by subsystem experts (linker scripts, init,
> > etc), and will likely change a lot after getting feedback.
> >
> > The biggest thing that I know will require additional attention is
> > integrating this patchset with the KUnit module support patchset[3]. I
> > have not even attempted to build these patches on top of the module
> > support patches as I would like to get people's initial thoughts first
> > (especially Alan's :-) ). I think that making these patches work with
> > module support should be fairly straight forward, nevertheless.
>
> Modules just have their own sections too. That's all. So it'd be a
> matter of extending the linker script for modules too. But a module's
> init is different than the core kernel's for vmlinux.

Truth. It seems as though Alan has already fixed this for me, however.

Re: [RFC v1 0/6] kunit: create a centralized executor to dispatch all KUnit tests

[Frank Rowand]

On 1/23/20 4:40 PM, Brendan Higgins wrote:
> Sorry for the late reply. I am still catching up from being on vacation.
> 
> On Mon, Jan 6, 2020 at 2:40 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>>
>> On Mon, Dec 16, 2019 at 02:05:49PM -0800, Brendan Higgins wrote:
>>> ## TL;DR
>>>
>>> This patchset adds a centralized executor to dispatch tests rather than
>>> relying on late_initcall to schedule each test suite separately along
>>> with a couple of new features that depend on it.
>>>
>>> ## What am I trying to do?
>>>
>>> Conceptually, I am trying to provide a mechanism by which test suites
>>> can be grouped together so that they can be reasoned about collectively.
>>> The last two patches in this series add features which depend on this:
>>>
>>> RFC 5/6 Prints out a test plan right before KUnit tests are run[1]; this
>>>         is valuable because it makes it possible for a test harness to
>>>         detect whether the number of tests run matches the number of
>>>         tests expected to be run, ensuring that no tests silently
>>>         failed.
>>>
>>> RFC 6/6 Add a new kernel command-line option which allows the user to
>>>         specify that the kernel poweroff, halt, or reboot after
>>>         completing all KUnit tests; this is very handy for running KUnit
>>>         tests on UML or a VM so that the UML/VM process exits cleanly
>>>         immediately after running all tests without needing a special
>>>         initramfs.
>>
>> The approach seems sensible to me given that it separates from a
>> semantics perspective kernel subsystem init work from *testing*, and
>> so we are sure we'd run the *test* stuff *after* all subsystem init
>> stuff.
> 
> Cool, I thought you would find this interesting.
> 
>> Dispatching, however is still immediate, and with a bit of work, this
>> dispatcher could be configurable to run at an arbirary time after boot.
>> If there are not immediate use cases for that though, then I suppose
>> this is not a requirement for the dispatcher. But since there exists
>> another modular test framework with its own dispatcher and it seems the
>> goal is to merge the work long term, this might preempt the requirement
>> to define how and when we can dispatch tests post boot.
>>
>> And, if we're going to do that, I can suggest that a data structure
>> instead of just a function init call be used to describe tests to be
>> placed into an ELF section. With my linker table work this would be
>> easy, I define section ranges for code describing only executable
>> routines, but it defines linker tables for when a component in the
>> kernel would define a data structure, part of which can be a callback.
>> Such data structure stuffed into an ELF section could allow dynamic
>> configuration of the dipsatching, even post boot.
> 
> The linker table work does sound interesting. Do you have a link?
> 
> I was thinking about dynamic dispatching, actually. I thought it would
> be handy to be able to build all tests into a single kernel and then
> run different tests on different invocations.
> 
> Also, for post boot dynamic dispatching, you should check out Alan's
> debugfs patches:
> 
> https://lore.kernel.org/linux-kselftest/CAFd5g46657gZ36PaP8Pi999hPPgBU2Kz94nrMspS-AzGwdBF+g@mail.gmail.com/T/#m210cadbeee267e5c5a9253d83b7b7ca723d1f871
> 
> They look pretty handy!
> 
>> I think this is a good stepping stone forward then, and to allow
>> dynamic configuration of the dispatcher could mean eventual extensions
>> to kunit's init stuff to stuff init calls into a data structure which
>> can then allow configuration of the dispatching. One benefit that the
>> linker table work *may* be able to help here with is that it allows
>> an easy way to create kunit specific ordering, at linker time.
>> There is also an example of addressing / generalizing dynamic / run time
>> changes of ordering, by using the x86 IOMMU initialization as an
>> example case. We don't have an easy way to do this today, but if kunit
>> could benefit from such framework, it'd be another use case for
>> the linker table work. That is, the ability to easilly allow
>> dynamically modifying run time ordering of code through ELF sections.
>>
>>> In addition, by dispatching tests from a single location, we can
>>> guarantee that all KUnit tests run after late_init is complete, which
>>> was a concern during the initial KUnit patchset review (this has not
>>> been a problem in practice, but resolving with certainty is nevertheless
>>> desirable).
>>
>> Indeed, the concern is just a real semantics limitations. With the tests
>> *always* running after all subsystem init stuff, we know we'd have a
>> real full kernel ready.
> 
> Yep.
> 
>> It does beg the question if this means kunit is happy to not be a tool
>> to test pre basic setup stuff (terminology used in init.c, meaning prior
>> to running all init levels). I suspect this is the case.
> 
> Not sure. I still haven't seen any cases where this is necessary, so I
> am not super worried about it. Regardless, I don't think this patchset
> really changes anything in that regard, we are moving from late_init
> to after late_init, so it isn't that big of a change for most use
> cases.
> 
> Please share if you can think of some things that need to be tested in
> early init.

I don't have a specific need for this right now.  I had not thought about
how the current kunit implementation forces all kunit tests to run at a
specific initcall level before reading this email thread.

I can see the value of being able to have some tests run at different
initcall levels to verify what functionality is available and working
at different points in the boot sequence.

But more important than early initcall levels, I do not want the
framework to prevent using or testing code and data that are marked
as '__init'.  So it is important to retain a way to invoke the tests
while __init code and data are available, if there is also a change
to generally invoke the tests later.

-Frank

> 
>>> Other use cases for this exist, but the above features should provide an
>>> idea of the value that this could provide.
>>>
>>> ## What work remains to be done?
>>>
>>> These patches were based on patches in our non-upstream branch[2], so we
>>> have a pretty good idea that they are useable as presented;
>>> nevertheless, some of the changes done in this patchset could
>>> *definitely* use some review by subsystem experts (linker scripts, init,
>>> etc), and will likely change a lot after getting feedback.
>>>
>>> The biggest thing that I know will require additional attention is
>>> integrating this patchset with the KUnit module support patchset[3]. I
>>> have not even attempted to build these patches on top of the module
>>> support patches as I would like to get people's initial thoughts first
>>> (especially Alan's :-) ). I think that making these patches work with
>>> module support should be fairly straight forward, nevertheless.
>>
>> Modules just have their own sections too. That's all. So it'd be a
>> matter of extending the linker script for modules too. But a module's
>> init is different than the core kernel's for vmlinux.
> 
> Truth. It seems as though Alan has already fixed this for me, however.
> 

Re: [RFC v1 0/6] kunit: create a centralized executor to dispatch all KUnit tests

[Brendan Higgins]

On Mon, Jan 27, 2020 at 9:40 AM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 1/23/20 4:40 PM, Brendan Higgins wrote:
> > Sorry for the late reply. I am still catching up from being on vacation.
> >> > On Mon, Jan 6, 2020 at 2:40 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >> It does beg the question if this means kunit is happy to not be a tool
> >> to test pre basic setup stuff (terminology used in init.c, meaning prior
> >> to running all init levels). I suspect this is the case.
> >
> > Not sure. I still haven't seen any cases where this is necessary, so I
> > am not super worried about it. Regardless, I don't think this patchset
> > really changes anything in that regard, we are moving from late_init
> > to after late_init, so it isn't that big of a change for most use
> > cases.
> >
> > Please share if you can think of some things that need to be tested in
> > early init.
>
> I don't have a specific need for this right now.  I had not thought about
> how the current kunit implementation forces all kunit tests to run at a
> specific initcall level before reading this email thread.
>
> I can see the value of being able to have some tests run at different
> initcall levels to verify what functionality is available and working
> at different points in the boot sequence.

Let's cross that bridge when we get there. It should be fairly easy to
add that functionality.

> But more important than early initcall levels, I do not want the
> framework to prevent using or testing code and data that are marked
> as '__init'.  So it is important to retain a way to invoke the tests
> while __init code and data are available, if there is also a change
> to generally invoke the tests later.

Definitely. For now that still works as long as you don't build KUnit
as a module, but I think Alan's new patches which allow KUnit to be
run at runtime via debugfs could cause some difficulty there. Again,
we could add Kconfigs to control this, but the compiler nevertheless
complains because it doesn't know what phase KUnit runs in.

Is there any way to tell the compiler that it is okay for non __init
code to call __init code? I would prefer not to have a duplicate
version of all the KUnit libraries with all the symbols marked __init.
Thoughts?

Re: [RFC v1 0/6] kunit: create a centralized executor to dispatch all KUnit tests

[Frank Rowand]

On 1/28/20 1:19 AM, Brendan Higgins wrote:
> On Mon, Jan 27, 2020 at 9:40 AM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> On 1/23/20 4:40 PM, Brendan Higgins wrote:
>>> Sorry for the late reply. I am still catching up from being on vacation.
>>>>> On Mon, Jan 6, 2020 at 2:40 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>>>> It does beg the question if this means kunit is happy to not be a tool
>>>> to test pre basic setup stuff (terminology used in init.c, meaning prior
>>>> to running all init levels). I suspect this is the case.
>>>
>>> Not sure. I still haven't seen any cases where this is necessary, so I
>>> am not super worried about it. Regardless, I don't think this patchset
>>> really changes anything in that regard, we are moving from late_init
>>> to after late_init, so it isn't that big of a change for most use
>>> cases.
>>>
>>> Please share if you can think of some things that need to be tested in
>>> early init.
>>
>> I don't have a specific need for this right now.  I had not thought about
>> how the current kunit implementation forces all kunit tests to run at a
>> specific initcall level before reading this email thread.
>>
>> I can see the value of being able to have some tests run at different
>> initcall levels to verify what functionality is available and working
>> at different points in the boot sequence.
> 
> Let's cross that bridge when we get there. It should be fairly easy to
> add that functionality.

Yes. I just wanted to add the thought to the back of your mind so that
it does not get precluded by future changes to the kunit architecture.

> 
>> But more important than early initcall levels, I do not want the
>> framework to prevent using or testing code and data that are marked
>> as '__init'.  So it is important to retain a way to invoke the tests
>> while __init code and data are available, if there is also a change
>> to generally invoke the tests later.
> 
> Definitely. For now that still works as long as you don't build KUnit
> as a module, but I think Alan's new patches which allow KUnit to be
> run at runtime via debugfs could cause some difficulty there. Again,

Yes, Alan's patches are part of what triggered me thinking about the
issues I raised.


> we could add Kconfigs to control this, but the compiler nevertheless
> complains because it doesn't know what phase KUnit runs in.
> 
> Is there any way to tell the compiler that it is okay for non __init
> code to call __init code? I would prefer not to have a duplicate
> version of all the KUnit libraries with all the symbols marked __init.

I'm not sure.  The build messages have always been useful and valid in
my context, so I never thought to consider that possibility.

> Thoughts?
> .
> 

RE: [RFC v1 0/6] kunit: create a centralized executor to dispatch all KUnit tests

[Tim.Bird]

> -----Original Message-----
> From:  Frank Rowand on January 28, 2020 11:37 AM
> 
> On 1/28/20 1:19 AM, Brendan Higgins wrote:
> > On Mon, Jan 27, 2020 at 9:40 AM Frank Rowand <frowand.list@gmail.com> wrote:
...
> > we could add Kconfigs to control this, but the compiler nevertheless
> > complains because it doesn't know what phase KUnit runs in.
> >
> > Is there any way to tell the compiler that it is okay for non __init
> > code to call __init code? I would prefer not to have a duplicate
> > version of all the KUnit libraries with all the symbols marked __init.
> 
> I'm not sure.  The build messages have always been useful and valid in
> my context, so I never thought to consider that possibility.
> 
> > Thoughts?

I'm not sure there's a restriction on non __init code calling __init
code.  In init/main.c arch_call_reset_init() is in __init, and it calls
rest_init which is non __init, without any special handling.

Is the compiler complaint mentioned above related to  calling
into __init code, or with some other issue?
 -- Tim

Re: [RFC v1 0/6] kunit: create a centralized executor to dispatch all KUnit tests

[Brendan Higgins]

On Tue, Jan 28, 2020 at 11:35 AM <Tim.Bird@sony.com> wrote:
>
> > -----Original Message-----
> > From:  Frank Rowand on January 28, 2020 11:37 AM
> >
> > On 1/28/20 1:19 AM, Brendan Higgins wrote:
> > > On Mon, Jan 27, 2020 at 9:40 AM Frank Rowand <frowand.list@gmail.com> wrote:
> ...
> > > we could add Kconfigs to control this, but the compiler nevertheless
> > > complains because it doesn't know what phase KUnit runs in.
> > >
> > > Is there any way to tell the compiler that it is okay for non __init
> > > code to call __init code? I would prefer not to have a duplicate
> > > version of all the KUnit libraries with all the symbols marked __init.
> >
> > I'm not sure.  The build messages have always been useful and valid in
> > my context, so I never thought to consider that possibility.
> >
> > > Thoughts?
>
> I'm not sure there's a restriction on non __init code calling __init
> code.  In init/main.c arch_call_reset_init() is in __init, and it calls
> rest_init which is non __init, without any special handling.
>
> Is the compiler complaint mentioned above related to  calling
> into __init code, or with some other issue?

I distinctly remember having the compiler complain at me when I was
messing around with the device tree unit tests because of KUnit
calling code marked as __init. Maybe it's time to start converting
those to KUnit to force the issue? Frank, does that work for you?

Re: [RFC v1 0/6] kunit: create a centralized executor to dispatch all KUnit tests

[Frank Rowand]

On 1/28/20 1:53 PM, Brendan Higgins wrote:
> On Tue, Jan 28, 2020 at 11:35 AM <Tim.Bird@sony.com> wrote:
>>
>>> -----Original Message-----
>>> From:  Frank Rowand on January 28, 2020 11:37 AM
>>>
>>> On 1/28/20 1:19 AM, Brendan Higgins wrote:
>>>> On Mon, Jan 27, 2020 at 9:40 AM Frank Rowand <frowand.list@gmail.com> wrote:
>> ...
>>>> we could add Kconfigs to control this, but the compiler nevertheless
>>>> complains because it doesn't know what phase KUnit runs in.
>>>>
>>>> Is there any way to tell the compiler that it is okay for non __init
>>>> code to call __init code? I would prefer not to have a duplicate
>>>> version of all the KUnit libraries with all the symbols marked __init.
>>>
>>> I'm not sure.  The build messages have always been useful and valid in
>>> my context, so I never thought to consider that possibility.
>>>
>>>> Thoughts?
>>
>> I'm not sure there's a restriction on non __init code calling __init
>> code.  In init/main.c arch_call_reset_init() is in __init, and it calls
>> rest_init which is non __init, without any special handling.
>>
>> Is the compiler complaint mentioned above related to  calling
>> into __init code, or with some other issue?
> 
> I distinctly remember having the compiler complain at me when I was
> messing around with the device tree unit tests because of KUnit
> calling code marked as __init. Maybe it's time to start converting
> those to KUnit to force the issue? Frank, does that work for you?

I have agreed to try converting the devicetree unittest to KUnit.

Now that KUnit is in 5.5, I think there is a solid foundation for
me to proceed.

-Frank

Re: [RFC v1 0/6] kunit: create a centralized executor to dispatch all KUnit tests

[Brendan Higgins]

On Tue, Jan 28, 2020 at 8:24 PM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 1/28/20 1:53 PM, Brendan Higgins wrote:
> > On Tue, Jan 28, 2020 at 11:35 AM <Tim.Bird@sony.com> wrote:
> >>
> >>> -----Original Message-----
> >>> From:  Frank Rowand on January 28, 2020 11:37 AM
> >>>
> >>> On 1/28/20 1:19 AM, Brendan Higgins wrote:
> >>>> On Mon, Jan 27, 2020 at 9:40 AM Frank Rowand <frowand.list@gmail.com> wrote:
> >> ...
> >>>> we could add Kconfigs to control this, but the compiler nevertheless
> >>>> complains because it doesn't know what phase KUnit runs in.
> >>>>
> >>>> Is there any way to tell the compiler that it is okay for non __init
> >>>> code to call __init code? I would prefer not to have a duplicate
> >>>> version of all the KUnit libraries with all the symbols marked __init.
> >>>
> >>> I'm not sure.  The build messages have always been useful and valid in
> >>> my context, so I never thought to consider that possibility.
> >>>
> >>>> Thoughts?
> >>
> >> I'm not sure there's a restriction on non __init code calling __init
> >> code.  In init/main.c arch_call_reset_init() is in __init, and it calls
> >> rest_init which is non __init, without any special handling.
> >>
> >> Is the compiler complaint mentioned above related to  calling
> >> into __init code, or with some other issue?
> >
> > I distinctly remember having the compiler complain at me when I was
> > messing around with the device tree unit tests because of KUnit
> > calling code marked as __init. Maybe it's time to start converting
> > those to KUnit to force the issue? Frank, does that work for you?
>
> I have agreed to try converting the devicetree unittest to KUnit.
>
> Now that KUnit is in 5.5, I think there is a solid foundation for
> me to proceed.

Awesome! Last time we talked (offline), it sounded like you had a
clear idea of what you wanted to do; nevertheless, feel free to reuse
anything from my attempt at it, if you find anything useful, or
otherwise rope me in if you have any questions, comments, or
complaints.

Re: [RFC v1 0/6] kunit: create a centralized executor to dispatch all KUnit tests

[Alan Maguire]

On Tue, 28 Jan 2020, Frank Rowand wrote:

> On 1/28/20 1:19 AM, Brendan Higgins wrote:
> > On Mon, Jan 27, 2020 at 9:40 AM Frank Rowand <frowand.list@gmail.com> wrote:
> >>
> >> On 1/23/20 4:40 PM, Brendan Higgins wrote:
> >>> Sorry for the late reply. I am still catching up from being on vacation.
> >>>>> On Mon, Jan 6, 2020 at 2:40 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >>>> It does beg the question if this means kunit is happy to not be a tool
> >>>> to test pre basic setup stuff (terminology used in init.c, meaning prior
> >>>> to running all init levels). I suspect this is the case.
> >>>
> >>> Not sure. I still haven't seen any cases where this is necessary, so I
> >>> am not super worried about it. Regardless, I don't think this patchset
> >>> really changes anything in that regard, we are moving from late_init
> >>> to after late_init, so it isn't that big of a change for most use
> >>> cases.
> >>>
> >>> Please share if you can think of some things that need to be tested in
> >>> early init.
> >>
> >> I don't have a specific need for this right now.  I had not thought about
> >> how the current kunit implementation forces all kunit tests to run at a
> >> specific initcall level before reading this email thread.
> >>
> >> I can see the value of being able to have some tests run at different
> >> initcall levels to verify what functionality is available and working
> >> at different points in the boot sequence.
> > 
> > Let's cross that bridge when we get there. It should be fairly easy to
> > add that functionality.
> 
> Yes. I just wanted to add the thought to the back of your mind so that
> it does not get precluded by future changes to the kunit architecture.
> 
> > 
> >> But more important than early initcall levels, I do not want the
> >> framework to prevent using or testing code and data that are marked
> >> as '__init'.  So it is important to retain a way to invoke the tests
> >> while __init code and data are available, if there is also a change
> >> to generally invoke the tests later.
> > 
> > Definitely. For now that still works as long as you don't build KUnit
> > as a module, but I think Alan's new patches which allow KUnit to be
> > run at runtime via debugfs could cause some difficulty there. Again,
> 
> Yes, Alan's patches are part of what triggered me thinking about the
> issues I raised.
> 
>

As Brendan says, any such tests probably shouldn't be buildable
as modules, but I wonder if we need to add some sort of way
to ensure execution from debugfs is not allowed for such cases?
Even if a test suite is builtin, it can be executed via debugfs
in the patches I sent out, allowing suites to be re-run.  Sounds
like we need a way to control that behaviour based on the
desired test suite execution environment.

Say, for example, the "struct kunit_suite" definitions associated
with the tests was marked as __initdata; are there any handy macros to 
identify it as being in the __init section? If so, we could simply
avoid adding a "run" file to the debugfs representation for such
suites.  Failing that, perhaps we need some sort of flags field
in "struct kunit_suite" to specify execution environment constraints?

Thanks!

Alan

Re: [RFC v1 0/6] kunit: create a centralized executor to dispatch all KUnit tests

[Brendan Higgins]

On Wed, Jan 29, 2020 at 5:07 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> On Tue, 28 Jan 2020, Frank Rowand wrote:
>
> > On 1/28/20 1:19 AM, Brendan Higgins wrote:
> > > On Mon, Jan 27, 2020 at 9:40 AM Frank Rowand <frowand.list@gmail.com> wrote:
> > >>
> > >> On 1/23/20 4:40 PM, Brendan Higgins wrote:
> > >>> Sorry for the late reply. I am still catching up from being on vacation.
> > >>>>> On Mon, Jan 6, 2020 at 2:40 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > >>>> It does beg the question if this means kunit is happy to not be a tool
> > >>>> to test pre basic setup stuff (terminology used in init.c, meaning prior
> > >>>> to running all init levels). I suspect this is the case.
> > >>>
> > >>> Not sure. I still haven't seen any cases where this is necessary, so I
> > >>> am not super worried about it. Regardless, I don't think this patchset
> > >>> really changes anything in that regard, we are moving from late_init
> > >>> to after late_init, so it isn't that big of a change for most use
> > >>> cases.
> > >>>
> > >>> Please share if you can think of some things that need to be tested in
> > >>> early init.
> > >>
> > >> I don't have a specific need for this right now.  I had not thought about
> > >> how the current kunit implementation forces all kunit tests to run at a
> > >> specific initcall level before reading this email thread.
> > >>
> > >> I can see the value of being able to have some tests run at different
> > >> initcall levels to verify what functionality is available and working
> > >> at different points in the boot sequence.
> > >
> > > Let's cross that bridge when we get there. It should be fairly easy to
> > > add that functionality.
> >
> > Yes. I just wanted to add the thought to the back of your mind so that
> > it does not get precluded by future changes to the kunit architecture.
> >
> > >
> > >> But more important than early initcall levels, I do not want the
> > >> framework to prevent using or testing code and data that are marked
> > >> as '__init'.  So it is important to retain a way to invoke the tests
> > >> while __init code and data are available, if there is also a change
> > >> to generally invoke the tests later.
> > >
> > > Definitely. For now that still works as long as you don't build KUnit
> > > as a module, but I think Alan's new patches which allow KUnit to be
> > > run at runtime via debugfs could cause some difficulty there. Again,
> >
> > Yes, Alan's patches are part of what triggered me thinking about the
> > issues I raised.
> >
> >
>
> As Brendan says, any such tests probably shouldn't be buildable
> as modules, but I wonder if we need to add some sort of way
> to ensure execution from debugfs is not allowed for such cases?
> Even if a test suite is builtin, it can be executed via debugfs
> in the patches I sent out, allowing suites to be re-run.  Sounds
> like we need a way to control that behaviour based on the
> desired test suite execution environment.

I think that's true.

> Say, for example, the "struct kunit_suite" definitions associated
> with the tests was marked as __initdata; are there any handy macros to
> identify it as being in the __init section? If so, we could simply
> avoid adding a "run" file to the debugfs representation for such
> suites.  Failing that, perhaps we need some sort of flags field
> in "struct kunit_suite" to specify execution environment constraints?

I think the former would be ideal, but the latter is acceptable as
well, assuming neither results in complaints from the compiler (I
guess we will find out for sure once we get a hold of the device tree
KUnit test).

Luis, you mentioned your linker table work might be applicable for
dynamic post boot configuring of dispatching. Do you think this work
could help solve this problem?

For reference, Alan's debugfs code can be found here:

https://lore.kernel.org/linux-kselftest/CAFd5g46657gZ36PaP8Pi999hPPgBU2Kz94nrMspS-AzGwdBF+g@mail.gmail.com/T/#m210cadbeee267e5c5a9253d83b7b7ca723d1f871

Re: [RFC v1 0/6] kunit: create a centralized executor to dispatch all KUnit tests

[Luis Chamberlain]

On Wed, Jan 29, 2020 at 01:28:19PM -0800, Brendan Higgins wrote:
> On Wed, Jan 29, 2020 at 5:07 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> >
> > On Tue, 28 Jan 2020, Frank Rowand wrote:
> >
> > > On 1/28/20 1:19 AM, Brendan Higgins wrote:
> > > > On Mon, Jan 27, 2020 at 9:40 AM Frank Rowand <frowand.list@gmail.com> wrote:
> > > >>
> > > >> On 1/23/20 4:40 PM, Brendan Higgins wrote:
> > > >>> Sorry for the late reply. I am still catching up from being on vacation.
> > > >>>>> On Mon, Jan 6, 2020 at 2:40 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > > >>>> It does beg the question if this means kunit is happy to not be a tool
> > > >>>> to test pre basic setup stuff (terminology used in init.c, meaning prior
> > > >>>> to running all init levels). I suspect this is the case.
> > > >>>
> > > >>> Not sure. I still haven't seen any cases where this is necessary, so I
> > > >>> am not super worried about it. Regardless, I don't think this patchset
> > > >>> really changes anything in that regard, we are moving from late_init
> > > >>> to after late_init, so it isn't that big of a change for most use
> > > >>> cases.
> > > >>>
> > > >>> Please share if you can think of some things that need to be tested in
> > > >>> early init.
> > > >>
> > > >> I don't have a specific need for this right now.  I had not thought about
> > > >> how the current kunit implementation forces all kunit tests to run at a
> > > >> specific initcall level before reading this email thread.
> > > >>
> > > >> I can see the value of being able to have some tests run at different
> > > >> initcall levels to verify what functionality is available and working
> > > >> at different points in the boot sequence.
> > > >
> > > > Let's cross that bridge when we get there. It should be fairly easy to
> > > > add that functionality.
> > >
> > > Yes. I just wanted to add the thought to the back of your mind so that
> > > it does not get precluded by future changes to the kunit architecture.
> > >
> > > >
> > > >> But more important than early initcall levels, I do not want the
> > > >> framework to prevent using or testing code and data that are marked
> > > >> as '__init'.  So it is important to retain a way to invoke the tests
> > > >> while __init code and data are available, if there is also a change
> > > >> to generally invoke the tests later.
> > > >
> > > > Definitely. For now that still works as long as you don't build KUnit
> > > > as a module, but I think Alan's new patches which allow KUnit to be
> > > > run at runtime via debugfs could cause some difficulty there. Again,
> > >
> > > Yes, Alan's patches are part of what triggered me thinking about the
> > > issues I raised.
> > >
> > >
> >
> > As Brendan says, any such tests probably shouldn't be buildable
> > as modules, but I wonder if we need to add some sort of way
> > to ensure execution from debugfs is not allowed for such cases?

The kernel's linker will ensure this doesn't happen by default, ie
__init data called from non __init code gets a complaint at linker
time today.

*Iff* you are sure the code is proper, you *whitelist* it by adding the
__ref tag to it.

> > Even if a test suite is builtin, it can be executed via debugfs
> > in the patches I sent out, allowing suites to be re-run.  Sounds
> > like we need a way to control that behaviour based on the
> > desired test suite execution environment.
> 
> I think that's true.
> 
> > Say, for example, the "struct kunit_suite" definitions associated
> > with the tests was marked as __initdata; are there any handy macros to
> > identify it as being in the __init section? If so, we could simply
> > avoid adding a "run" file to the debugfs representation for such
> > suites.

> > Failing that, perhaps we need some sort of flags field
> > in "struct kunit_suite" to specify execution environment constraints?
> 
> I think the former would be ideal, but the latter is acceptable as
> well, assuming neither results in complaints from the compiler (I
> guess we will find out for sure once we get a hold of the device tree
> KUnit test).

I'd split out tests in two different arrays, one with __init or
__initdata  one without. Likewise two dispatches, one for init and
one for non-init data.

> Luis, you mentioned your linker table work might be applicable for
> dynamic post boot configuring of dispatching. Do you think this work
> could help solve this problem?

The Linux kernel table / section ranges code helps aggregate data into
ELF sections in a generic way, that is, hacks we have been doing over
years into a generic way.

So it would be easier to read and implement. For instance see how in
this commit the intent/goal of kprobe blacklists is a bit easier to
read:

https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/commit/?h=20170620-linker-tables-v8&id=b2662efa7c6a3c436961c07fa3082e8640f0e352

In particular DEFINE_LINKTABLE_INIT_DATA() use. I think Youd' want to
use DEFINE_LINKTABLE_INIT_DATA() for code which you want to use to
dispatch on init and and a DEFINE_LINKTABLE_DATA() for non-init code.

If a dynamic dispatcher is used you'd opt out of the using for instance
linktable_for_each() and instead use the data structure defined for
however you want to disaptch your run time.

  Luis

Re: [RFC v1 0/6] kunit: create a centralized executor to dispatch all KUnit tests

[Luis Chamberlain]

On Thu, Jan 23, 2020 at 02:40:31PM -0800, Brendan Higgins wrote:
> On Mon, Jan 6, 2020 at 2:40 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > On Mon, Dec 16, 2019 at 02:05:49PM -0800, Brendan Higgins wrote:
> > dispatcher could be configurable to run at an arbirary time after boot.
> > If there are not immediate use cases for that though, then I suppose
> > this is not a requirement for the dispatcher. But since there exists
> > another modular test framework with its own dispatcher and it seems the
> > goal is to merge the work long term, this might preempt the requirement
> > to define how and when we can dispatch tests post boot.
> >
> > And, if we're going to do that, I can suggest that a data structure
> > instead of just a function init call be used to describe tests to be
> > placed into an ELF section. With my linker table work this would be
> > easy, I define section ranges for code describing only executable
> > routines, but it defines linker tables for when a component in the
> > kernel would define a data structure, part of which can be a callback.
> > Such data structure stuffed into an ELF section could allow dynamic
> > configuration of the dipsatching, even post boot.
> 
> The linker table work does sound interesting. Do you have a link?

Sure

https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170620-linker-tables-v8

I had dropped this long ago mostly due to the fact that my use case
(removal of dead code) was more long term, and not immediate so the
use case wasn't there yet.

I have been meaning to pick this work up again.

> I was thinking about dynamic dispatching, actually. I thought it would
> be handy to be able to build all tests into a single kernel and then
> run different tests on different invocations.

For built-in code it would be up to it to manage that. The linker table
stuff would just allow a way for you to systematically aggregate
data into an ELF section in a generic way. It does have a built in
light weight sort mechanism, if you opt out of that and say wanted
to do your own order it'd be up to how you program it in on the data
structure and dispatching after that.

> Also, for post boot dynamic dispatching, you should check out Alan's
> debugfs patches:
> 
> https://lore.kernel.org/linux-kselftest/CAFd5g46657gZ36PaP8Pi999hPPgBU2Kz94nrMspS-AzGwdBF+g@mail.gmail.com/T/#m210cadbeee267e5c5a9253d83b7b7ca723d1f871
> 
> They look pretty handy!

Sure.

> > I think this is a good stepping stone forward then, and to allow
> > dynamic configuration of the dispatcher could mean eventual extensions
> > to kunit's init stuff to stuff init calls into a data structure which
> > can then allow configuration of the dispatching. One benefit that the
> > linker table work *may* be able to help here with is that it allows
> > an easy way to create kunit specific ordering, at linker time.
> > There is also an example of addressing / generalizing dynamic / run time
> > changes of ordering, by using the x86 IOMMU initialization as an
> > example case. We don't have an easy way to do this today, but if kunit
> > could benefit from such framework, it'd be another use case for
> > the linker table work. That is, the ability to easilly allow
> > dynamically modifying run time ordering of code through ELF sections.
> >
> > > In addition, by dispatching tests from a single location, we can
> > > guarantee that all KUnit tests run after late_init is complete, which
> > > was a concern during the initial KUnit patchset review (this has not
> > > been a problem in practice, but resolving with certainty is nevertheless
> > > desirable).
> >
> > Indeed, the concern is just a real semantics limitations. With the tests
> > *always* running after all subsystem init stuff, we know we'd have a
> > real full kernel ready.
> 
> Yep.
> 
> > It does beg the question if this means kunit is happy to not be a tool
> > to test pre basic setup stuff (terminology used in init.c, meaning prior
> > to running all init levels). I suspect this is the case.
> 
> Not sure. I still haven't seen any cases where this is necessary, so I
> am not super worried about it. Regardless, I don't think this patchset
> really changes anything in that regard, we are moving from late_init
> to after late_init, so it isn't that big of a change for most use
> cases.
> 
> Please share if you can think of some things that need to be tested in
> early init.

If and when we get to that point we can deal with it then. My instincts
tell me that for early init code we should probably be specially crafted
tests and have they should have their own hand crafted dispatchers.

  Luis