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.