On Thu, Oct 21, 2021 at 2:17 PM Jeremy Kerr <jk@xxxxxxxxxxxxxxxxxxxx> wrote: > > Hi Brendan, > > > I think this change here should mostly go into lib/kunit/executor.c: > > > > https://elixir.bootlin.com/linux/latest/source/lib/kunit/executor.c > > > > Not totally sure how this should interact with printing the TAP header > > and some other functionality, but we already have some module params > > in there that we may want to pick up when KUnit is loaded as a module. > > So I had a go at doing this as part of the executor, but that just > raised more questions about how we want this to work for the various > configurations of built-in/modules, where we have five options, assuming > two example kunit consumers: > > - CONFIG_example1_TEST=y - our built-in suite: suites end up in the > vmlinux kunit_test_suites section > > - CONFIG_example2_TEST=m - our modular suite: suites end up in the > modules' kunit_test_suites section, to be iterated on module load. > > Currently, it looks like we have: > > CONFIG_KUNIT=y > > 1) example1's tests are run through the built-in init path: > kernel_init_freeable() > -> kunit_run_all_tests, which iterates through the built-in > kunit_test_suites section > > 2) example2's tests are run through: > > the module's own module_init(), > -> __kunit_test_suites_init() > - passing the suite to be init-ed and immediately run. > > CONFIG_KUNIT=m - is where this gets tricky: > > 3) example1's tests are never run; we don't iterate the > kunit_test_suites section when KUNIT=m (as kunit_run_all_tests() is > empty) > Thus far, this hasn't been a much of a problem (at least for me), as the kunit test modules all depend on kunit.ko, so modprobing one of them will pull kunit.ko in as well. And I think Kconfig will discourage you from building kunit tests into a kernel if CONFIG_KUNIT=m. > 4) loading example2.ko after kunit.ko will get example2's test run > through example2's module_init -> __kunit_test_suites_init() > > 5) loading example2.ko before kunit.ko would result in an unresolved > symbol, as __kunit_test_suites_init() doesn't exist yet. Again, this shouldn't happen if people use modprobe, but manual insmodding could trigger it. > > Is that all correct? > > With the proposed change to use a section for module's test suites: > > (1) would stay as-is > > (2) is similar, but the suites are loaded from the module's > kuint_test_suites section rather than an explicit call from > module_init(). > > (3) would stay as-is (but we could export the __kuint_test_suites > section details, allowing kunit.ko to iterate the built-in suites - is > this desirable?). I'd be okay with exporting __kunit_test_suites to support this. I do feel that this is the most niche case, though, so I'd equally be okay with not supporting it unless someone actually has a need for it. > > (4) is now the same as (2); once kunit.ko is present, it will be > notified on subsequent module loads, and will extract tests from the > module's own kuint_test_suites section > > (5) does not result in any dependencies between example2.ko and > kunit.ko. exmaple2.ko is able to be loaded before kunit.ko without > symbol dep issues, but (of course) its tests will not be run. We have an > option here to store the suites of loaded modules into a small built-in > list, for kunit.ko to consume when it starts, similar to the changes > possible in (3). One idea I've had in the past is to keep such a list around of "test suites to be run when KUnit is ready". This is partly because it's much nicer to have all the test suites run together as part of a single (K)TAP output, so this could be a way of implementing (at least part of) that. In any case, this plan looks pretty good for me: I'm not sure if cases (3) and (5) show up often enough in the real world to be worth complicating things too much to get them working, but the other cases are important, and your plan handles those the way I'd expect. Brendan: do you know of anyone who's actually building KUnit itself as a module? Given that there are some nasty side effects (bloating 'current' with the test pointer, the KASLR address leak) which affects the whole build, I'm not sure it's actually ever useful to do so. Thoughts? -- David
