On Thu, Oct 21, 2021 at 12:06 AM David Gow <davidgow@xxxxxxxxxx> wrote: > > 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. +1 > 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. +1 > Brendan: do you know of anyone who's actually building KUnit itself as > a module? Given that there are some nasty side effects (bloating No, some people asked for it early on, but I think we broke it once or twice and no one noticed which suggests that no one uses it. > '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. Oh yeah, I forgot that we put the test pointer in `current` when CONFIG_KUNIT is built-in OR a module. Yeah, I agree, that makes CONFIG_KUNIT=m kind of useless. > Thoughts? > > -- David
