On Thu, Oct 21, 2021 at 12:06 AM David Gow davidgow@google.com wrote:
On Thu, Oct 21, 2021 at 2:17 PM Jeremy Kerr jk@codeconstruct.com.au 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
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
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:
- 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.
loading example2.ko after kunit.ko will get example2's test run through example2's module_init -> __kunit_test_suites_init()
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