## 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-s... [2]: https://kunit-review.googlesource.com/c/linux/+/1037 [3]: https://patchwork.kernel.org/project/linux-kselftest/list/?series=211727
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) \
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
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)
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
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);
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;elsereturn -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);
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;elsereturn -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);
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);
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);
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);
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-s... 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
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-s... Signed-off-by: Brendan Higgins brendanhiggins@google.com
Reviewed-by: Stephen Boyd sboyd@kernel.org
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
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 ?
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.
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
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/CAFd5g46657gZ36PaP8Pi999hPPgBU2Kz94n...
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.
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/CAFd5g46657gZ36PaP8Pi999hPPgBU2Kz94n...
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.
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?
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? .
-----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
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?
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
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.
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
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/CAFd5g46657gZ36PaP8Pi999hPPgBU2Kz94n...
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...
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
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...
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/CAFd5g46657gZ36PaP8Pi999hPPgBU2Kz94n...
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
linux-kselftest-mirror@lists.linaro.org
-
Alan Maguire -
Brendan Higgins -
Frank Rowand -
Luis Chamberlain -
Stephen Boyd -
Tim.Bird＠sony.com