| Message ID | 20200626210917.358969-1-brendanhiggins@google.com |
|---|---|
| Headers | show |
| Series | kunit: create a centralized executor to dispatch all KUnit tests | expand |
On Fri, Jun 26, 2020 at 02:22:11PM -0700, Brendan Higgins wrote: > On Fri, Jun 26, 2020 at 2:20 PM Kees Cook <keescook@chromium.org> wrote: > > > > On Fri, Jun 26, 2020 at 02:09:06PM -0700, Brendan Higgins wrote: > > > 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. > > > > > > Co-developed-by: Iurii Zaikin <yzaikin@google.com> > > > Signed-off-by: Iurii Zaikin <yzaikin@google.com> > > > Signed-off-by: Brendan Higgins <brendanhiggins@google.com> > > > Reviewed-by: Stephen Boyd <sboyd@kernel.org> > > > --- > > > 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 db600ef218d7d..4f9b036fc9616 100644 > > > --- a/include/asm-generic/vmlinux.lds.h > > > +++ b/include/asm-generic/vmlinux.lds.h > > > @@ -881,6 +881,13 @@ > > > KEEP(*(.con_initcall.init)) \ > > > __con_initcall_end = .; > > > > > > +/* Alignment must be consistent with (kunit_suite *) in include/kunit/test.h */ > > > > Nit on naming: > > > > > +#define KUNIT_TEST_SUITES \ > > > > I would call this KUNIT_TABLE to maintain the same names as other things > > of this nature. > > > > > + . = ALIGN(8); \ > > > + __kunit_suites_start = .; \ > > > + KEEP(*(.kunit_test_suites)) \ > > > + __kunit_suites_end = .; > > > + > > > #ifdef CONFIG_BLK_DEV_INITRD > > > #define INIT_RAM_FS \ > > > . = ALIGN(4); \ > > > @@ -1056,6 +1063,7 @@ > > > INIT_CALLS \ > > > CON_INITCALL \ > > > INIT_RAM_FS \ > > > + KUNIT_TEST_SUITES \ > > > } > > > > Nack: this must be in INIT_DATA, not in INIT_DATA_SECTION. Not all > > architectures use the INIT_DATA_SECTION macro (e.g. arm64), but everything > > uses INIT_DATA. > > Oh, maybe that would eliminate the need for the other linkerscript > patches? That would be nice. Curious, did changing it as Kees suggest fix it for m68k? Luis
On Fri, Jun 26, 2020 at 02:09:13PM -0700, Brendan Higgins wrote: > Remove KUnit from init calls entirely, instead call directly from > kernel_init(). The commit log does not explain *why*. > Co-developed-by: Alan Maguire <alan.maguire@oracle.com> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > Signed-off-by: Brendan Higgins <brendanhiggins@google.com> > Reviewed-by: Stephen Boyd <sboyd@kernel.org> Other than that: Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> Luis
On Fri, Jun 26, 2020 at 2:52 PM Kees Cook <keescook@chromium.org> wrote: > > On Fri, Jun 26, 2020 at 02:09:05PM -0700, Brendan Higgins wrote: > > 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. Sorry it took so long to reply. I got sucked into some other stuff again. > So, the new section looks fine to me (modulo the INIT_DATA change). The > plumbing to start the tests, though, I think is redundant. Why not just > add a sysctl that starts all known tests? We already have that; however, we use debugfs to start the tests - same difference. I just find it convenient to not have to build and then maintain a userland for each architecture. It's also really nice that KUnit "just works out of the box" - you don't have to download anything other than the kernel source, and you don't need to do any steps outside of just run "kuit.py run". That seems like a big advantage to me. > That way you don't need the plumbing into init/main.c, and you can have > a mode where builtin tests can be started on a fully booted system too. > > i.e. boot with "sysctl.kernel.kunit=start" or when fully booted with > "echo start > /proc/sys/kernel/kunit" > > And instead of the kunit-specific halt/reboot stuff, how about moving > /proc/sysrq-trigger into /proc/sys instead? Then you (or anything) could > do: > > sysctl.kernel.kunit=start sysctl.kernel.sysrq-trigger=b I think it might be harder to make a case for the reboot stuff without the stuff I am working on outside of this patchset. I think I will probably drop that patch from this patchset and reintroduce it later.
On Tue, Jul 7, 2020 at 9:31 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > On Fri, Jun 26, 2020 at 02:22:11PM -0700, Brendan Higgins wrote: > > On Fri, Jun 26, 2020 at 2:20 PM Kees Cook <keescook@chromium.org> wrote: > > > > > > On Fri, Jun 26, 2020 at 02:09:06PM -0700, Brendan Higgins wrote: > > > > 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. > > > > > > > > Co-developed-by: Iurii Zaikin <yzaikin@google.com> > > > > Signed-off-by: Iurii Zaikin <yzaikin@google.com> > > > > Signed-off-by: Brendan Higgins <brendanhiggins@google.com> > > > > Reviewed-by: Stephen Boyd <sboyd@kernel.org> > > > > --- > > > > 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 db600ef218d7d..4f9b036fc9616 100644 > > > > --- a/include/asm-generic/vmlinux.lds.h > > > > +++ b/include/asm-generic/vmlinux.lds.h > > > > @@ -881,6 +881,13 @@ > > > > KEEP(*(.con_initcall.init)) \ > > > > __con_initcall_end = .; > > > > > > > > +/* Alignment must be consistent with (kunit_suite *) in include/kunit/test.h */ > > > > > > Nit on naming: > > > > > > > +#define KUNIT_TEST_SUITES \ > > > > > > I would call this KUNIT_TABLE to maintain the same names as other things > > > of this nature. > > > > > > > + . = ALIGN(8); \ > > > > + __kunit_suites_start = .; \ > > > > + KEEP(*(.kunit_test_suites)) \ > > > > + __kunit_suites_end = .; > > > > + > > > > #ifdef CONFIG_BLK_DEV_INITRD > > > > #define INIT_RAM_FS \ > > > > . = ALIGN(4); \ > > > > @@ -1056,6 +1063,7 @@ > > > > INIT_CALLS \ > > > > CON_INITCALL \ > > > > INIT_RAM_FS \ > > > > + KUNIT_TEST_SUITES \ > > > > } > > > > > > Nack: this must be in INIT_DATA, not in INIT_DATA_SECTION. Not all > > > architectures use the INIT_DATA_SECTION macro (e.g. arm64), but everything > > > uses INIT_DATA. > > > > Oh, maybe that would eliminate the need for the other linkerscript > > patches? That would be nice. Sorry for the delayed response. I got pulled into some other things. > Curious, did changing it as Kees suggest fix it for m68k? It did! There are still some architectures I cannot test due to a lack of GCC or QEMU support, but it seems to work on everything else now.
On Fri, Jun 26, 2020 at 2:29 PM Kees Cook <keescook@chromium.org> wrote: > > On Fri, Jun 26, 2020 at 02:09:12PM -0700, Brendan Higgins wrote: > > From: Alan Maguire <alan.maguire@oracle.com> > > > > Add a centralized executor to dispatch tests rather than relying on > > late_initcall to schedule each test suite separately. Centralized > > execution is for built-in tests only; modules will execute tests when > > loaded. > > > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > > Co-developed-by: Iurii Zaikin <yzaikin@google.com> > > Signed-off-by: Iurii Zaikin <yzaikin@google.com> > > Co-developed-by: Brendan Higgins <brendanhiggins@google.com> > > Signed-off-by: Brendan Higgins <brendanhiggins@google.com> > > Reviewed-by: Stephen Boyd <sboyd@kernel.org> > > --- > > include/kunit/test.h | 67 +++++++++++++++++++++++++++++--------------- > > lib/kunit/Makefile | 3 +- > > lib/kunit/executor.c | 28 ++++++++++++++++++ > > lib/kunit/test.c | 2 +- > > 4 files changed, 76 insertions(+), 24 deletions(-) > > create mode 100644 lib/kunit/executor.c > > > > diff --git a/include/kunit/test.h b/include/kunit/test.h > > index 47e61e1d53370..f3e86c3953a2b 100644 > > --- a/include/kunit/test.h > > +++ b/include/kunit/test.h > > @@ -224,7 +224,7 @@ size_t kunit_suite_num_test_cases(struct kunit_suite *suite); > > unsigned int kunit_test_case_num(struct kunit_suite *suite, > > struct kunit_case *test_case); > > > > -int __kunit_test_suites_init(struct kunit_suite **suites); > > +int __kunit_test_suites_init(struct kunit_suite * const * const suites); > > > > void __kunit_test_suites_exit(struct kunit_suite **suites); > > > > @@ -237,34 +237,57 @@ void __kunit_test_suites_exit(struct kunit_suite **suites); > > * Registers @suites_list with the test framework. See &struct kunit_suite for > > * more information. > > * > > - * When builtin, KUnit tests are all run as late_initcalls; this means > > - * that they cannot test anything where tests must run at a different init > > - * phase. One significant restriction resulting from this is that KUnit > > - * cannot reliably test anything that is initialize in the late_init phase; > > - * another is that KUnit is useless to test things that need to be run in > > - * an earlier init phase. > > - * > > - * An alternative is to build the tests as a module. Because modules > > - * do not support multiple late_initcall()s, we need to initialize an > > - * array of suites for a module. > > - * > > - * TODO(brendanhiggins@google.com): Don't run all KUnit tests as > > - * late_initcalls. I have some future work planned to dispatch all KUnit > > - * tests from the same place, and at the very least to do so after > > - * everything else is definitely initialized. > > + * If a test suite is built-in, module_init() gets translated into > > + * an initcall which we don't want as the idea is that for builtins > > + * the executor will manage execution. So ensure we do not define > > + * module_{init|exit} functions for the builtin case when registering > > + * suites via kunit_test_suites() below. > > */ > > -#define kunit_test_suites(suites_list...) \ > > - static struct kunit_suite *suites[] = {suites_list, NULL}; \ > > - static int kunit_test_suites_init(void) \ > > +#ifdef MODULE > > +#define kunit_test_suites_for_module(__suites) \ > > + static int __init kunit_test_suites_init(void) \ > > { \ > > - return __kunit_test_suites_init(suites); \ > > + return __kunit_test_suites_init(__suites); \ > > } \ > > - late_initcall(kunit_test_suites_init); \ > > + module_init(kunit_test_suites_init); \ > > + \ > > static void __exit kunit_test_suites_exit(void) \ > > { \ > > - return __kunit_test_suites_exit(suites); \ > > + return __kunit_test_suites_exit(__suites); \ > > } \ > > module_exit(kunit_test_suites_exit) > > +#else > > +#define kunit_test_suites_for_module(__suites) > > +#endif /* MODULE */ > > + > > +#define __kunit_test_suites(unique_array, unique_suites, ...) \ > > + static struct kunit_suite *unique_array[] = { __VA_ARGS__, NULL }; \ > > + kunit_test_suites_for_module(unique_array); \ > > + static struct kunit_suite **unique_suites \ > > + __used __section(.kunit_test_suites) = unique_array > > + > > +/** > > + * kunit_test_suites() - used to register one or more &struct kunit_suite > > + * with KUnit. > > + * > > + * @suites: a statically allocated list of &struct kunit_suite. > > + * > > + * Registers @suites with the test framework. See &struct kunit_suite for > > + * more information. > > + * > > + * When builtin, KUnit tests are all run via executor; this is done > > + * by placing the array of struct kunit_suite * in the .kunit_test_suites > > + * ELF section. > > + * > > + * An alternative is to build the tests as a module. Because modules do not > > + * support multiple initcall()s, we need to initialize an array of suites for a > > + * module. > > + * > > + */ > > +#define kunit_test_suites(...) \ > > + __kunit_test_suites(__UNIQUE_ID(array), \ > > + __UNIQUE_ID(suites), \ > > + __VA_ARGS__) > > > > #define kunit_test_suite(suite) kunit_test_suites(&suite) > > > > diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile > > index 724b94311ca36..c49f4ffb6273a 100644 > > --- a/lib/kunit/Makefile > > +++ b/lib/kunit/Makefile > > @@ -3,7 +3,8 @@ obj-$(CONFIG_KUNIT) += kunit.o > > kunit-objs += test.o \ > > string-stream.o \ > > assert.o \ > > - try-catch.o > > + try-catch.o \ > > + executor.o > > > > ifeq ($(CONFIG_KUNIT_DEBUGFS),y) > > kunit-objs += debugfs.o > > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c > > new file mode 100644 > > index 0000000000000..7015e7328dce7 > > --- /dev/null > > +++ b/lib/kunit/executor.c > > @@ -0,0 +1,28 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +#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 * const * const __kunit_suites_start[]; > > +extern struct kunit_suite * const * const __kunit_suites_end[]; > > I would expect these to be in include/asm-generic/sections.h but I guess > it's not required. I don't have strong opinions either way, but I think this is less clutter since KUnit is the only one that uses it. > Reviewed-by: Kees Cook <keescook@chromium.org> Thanks!
On Fri, Jun 26, 2020 at 2:35 PM Kees Cook <keescook@chromium.org> wrote: > > On Fri, Jun 26, 2020 at 02:09:14PM -0700, Brendan Higgins wrote: > > 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> > > Look good, except... > > > 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 62ebc0288355c4b122ccc18ae2505f971efa57bc..bc0dc8fe35b760b1feb74ec419818dbfae1adb5c 100644 > > GIT binary patch > > delta 28 > > jcmbQmGoME|#4$jjEVZaOGe1wk(1goSPtRy09}gP<dC~`u > > > > delta 23 > > ecmbQwGmD2W#4$jjEVZaOGe1wk&}5@94;uhhkp{*9 > > > > 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 0b249870c8be417a5865bd40a24c8597bb7f5ab1..4d97f6708c4a5ad5bb2ac879e12afca6e816d83d 100644 > > GIT binary patch > > delta 15 > > WcmX>hepY;fFN>j`p3z318g2k9Uj*m? > > > > delta 10 > > RcmX>renNbL@5Z2NZU7lr1S$Xk > > > > 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 9e89d32d5667a59d137f8adacf3a88fdb7f88baf..7a416497e3bec044eefc1535f7d84ee85703ba97 100644 > > GIT binary patch > > delta 28 > > jcmZ3&yOLKp#4$jjEVZaOGe1wk(1goSPtRy0-!wJ=eKrU$ > > > > delta 23 > > ecmZ3<yM&i7#4$jjEVZaOGe1wk&}5_VG&TTPhX-Z= > > What is happening here?? Those logs appear as text to me. Why did git > freak out? That's because this is all test data; it's all plaintext, but out of necessity some of the test data is kind of munged up and causes checkpatch to complain, so Shuah asked us to mark it as binary since it isn't actually code and so checkpatch will stop flagging it.
## 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 of three patches in this series add features which depend on this: PATCH 09/12 Prints out a test plan[1] right before KUnit tests are run; 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. The test plan includes a count of tests that will run. With the centralized executor, the tests are located in a single data structure and thus can be counted. PATCH 10/12 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 centralized executor provides a definitive point when all tests have completed and the poweroff, halt, or reboot could occur. 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. ## Changes since last revision: - Fixed a compilation error in the centralized executor patch (07/12). I had forgotten to test the patches when building as modules. I verified that works now. - I accidentally merged patches 09/12 and 10/12 in the previous revision (v4), and made them separate patches again. ## Changes since v3: - On the last revision I got some messages from 0day that showed that this patchset didn't work on several architectures, one issue that this patchset addresses is that we were aligning both memory segments as well as structures in the segments to specific byte boundaries which was incorrect. - The issue mentioned above also caused me to test on additional architectures which revealed that some architectures other than UML do not use the default init linker section macro that most architectures use. There are now several new patches (2, 3, 4, and 6). - Fixed a formatting consistency issue in the kernel params documentation patch (11/12). - Add a brief blurb on how and when the kunit_test_suite macro works. ## Remaining work to be done: The only architecture for which I was able to get a compiler, but was apparently unable to get KUnit into a section that the executor to see was m68k - not sure why. Alan Maguire (1): kunit: test: create a single centralized executor for all tests Brendan Higgins (10): vmlinux.lds.h: add linker section for KUnit test suites arch: arm64: add linker section for KUnit test suites arch: microblaze: add linker section for KUnit test suites arch: powerpc: add linker section for KUnit test suites arch: um: add linker section for KUnit test suites arch: xtensa: add linker section for KUnit test suites init: main: add KUnit to kernel init kunit: test: add test plan to KUnit TAP format Documentation: Add kunit_shutdown to kernel-parameters.txt Documentation: kunit: add a brief blurb about kunit_test_suite David Gow (1): kunit: Add 'kunit_shutdown' option .../admin-guide/kernel-parameters.txt | 8 ++ Documentation/dev-tools/kunit/usage.rst | 5 ++ arch/arm64/kernel/vmlinux.lds.S | 3 + arch/microblaze/kernel/vmlinux.lds.S | 4 + arch/powerpc/kernel/vmlinux.lds.S | 4 + arch/um/include/asm/common.lds.S | 4 + arch/xtensa/kernel/vmlinux.lds.S | 4 + include/asm-generic/vmlinux.lds.h | 8 ++ include/kunit/test.h | 76 +++++++++++++----- init/main.c | 4 + lib/kunit/Makefile | 3 +- lib/kunit/executor.c | 63 +++++++++++++++ lib/kunit/test.c | 13 +-- tools/testing/kunit/kunit_kernel.py | 2 +- tools/testing/kunit/kunit_parser.py | 74 ++++++++++++++--- .../test_is_test_passed-all_passed.log | Bin 1562 -> 1567 bytes .../test_data/test_is_test_passed-crash.log | Bin 3016 -> 3021 bytes .../test_data/test_is_test_passed-failure.log | Bin 1700 -> 1705 bytes 18 files changed, 227 insertions(+), 48 deletions(-) create mode 100644 lib/kunit/executor.c These patches are available for download with dependencies here: https://kunit-review.googlesource.com/c/linux/+/3829 [1] https://github.com/isaacs/testanything.github.io/blob/tap14/tap-version-14-specification.md#the-plan [2] https://patchwork.kernel.org/patch/11383635/ base-commit: 4333a9b0b67bb4e8bcd91bdd80da80b0ec151162 prerequisite-patch-id: 2d4b5aa9fa8ada9ae04c8584b47c299a822b9455 prerequisite-patch-id: 582b6d9d28ce4b71628890ec832df6522ca68de0