The current kunit infrastructure defines its own module_init() when built as a module, which conflicts with the mctp core's own.
So, only allow MCTP_TEST when both MCTP and KUNIT are built-in.
Reported-by: kernel test robot lkp@intel.com Signed-off-by: Jeremy Kerr jk@codeconstruct.com.au --- net/mctp/Kconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/mctp/Kconfig b/net/mctp/Kconfig index 15267a5043d9..868c92272cbd 100644 --- a/net/mctp/Kconfig +++ b/net/mctp/Kconfig @@ -13,6 +13,6 @@ menuconfig MCTP channel.
config MCTP_TEST - tristate "MCTP core tests" if !KUNIT_ALL_TESTS - depends on MCTP && KUNIT + bool "MCTP core tests" if !KUNIT_ALL_TESTS + depends on MCTP=y && KUNIT=y default KUNIT_ALL_TESTS
The MCTP device isn't available until we've registered the netdev, so defer storing our convenience pointer.
Signed-off-by: Jeremy Kerr jk@codeconstruct.com.au --- net/mctp/test/utils.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/net/mctp/test/utils.c b/net/mctp/test/utils.c index e2ab1f3da357..cc6b8803aa9d 100644 --- a/net/mctp/test/utils.c +++ b/net/mctp/test/utils.c @@ -46,17 +46,17 @@ struct mctp_test_dev *mctp_test_create_dev(void) dev = netdev_priv(ndev); dev->ndev = ndev;
- rcu_read_lock(); - dev->mdev = __mctp_dev_get(ndev); - mctp_dev_hold(dev->mdev); - rcu_read_unlock(); - rc = register_netdev(ndev); if (rc) { free_netdev(ndev); return NULL; }
+ rcu_read_lock(); + dev->mdev = __mctp_dev_get(ndev); + mctp_dev_hold(dev->mdev); + rcu_read_unlock(); + return dev; }
On Sat, Oct 2, 2021 at 10:27 AM Jeremy Kerr jk@codeconstruct.com.au wrote:
The current kunit infrastructure defines its own module_init() when built as a module, which conflicts with the mctp core's own.
So, only allow MCTP_TEST when both MCTP and KUNIT are built-in.
Reported-by: kernel test robot lkp@intel.com Signed-off-by: Jeremy Kerr jk@codeconstruct.com.au
This looks good to me. I don't think you'll be the only person to hit this issue, so -- while it's probably overall nicer if tests can sit in their own module -- we'll look into finding a way of supporting this with KUnit at some point. In the meantime, though, this is a reasonable workaround.
Reviewed-by: David Gow davidgow@google.com
-- David
net/mctp/Kconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/mctp/Kconfig b/net/mctp/Kconfig index 15267a5043d9..868c92272cbd 100644 --- a/net/mctp/Kconfig +++ b/net/mctp/Kconfig @@ -13,6 +13,6 @@ menuconfig MCTP channel.
config MCTP_TEST
tristate "MCTP core tests" if !KUNIT_ALL_TESTSdepends on MCTP && KUNIT
bool "MCTP core tests" if !KUNIT_ALL_TESTSdepends on MCTP=y && KUNIT=y default KUNIT_ALL_TESTS-- 2.30.2
On Sat, Oct 2, 2021 at 10:27 AM Jeremy Kerr jk@codeconstruct.com.au wrote:
The MCTP device isn't available until we've registered the netdev, so defer storing our convenience pointer.
Signed-off-by: Jeremy Kerr jk@codeconstruct.com.au
Haha -- you sent this just as I'd come up with the same patch here. :-)
With these changes, alongside the rt->dev == NULL in mctp_route_release() crash fix mentioned in [1], the tests all pass on my system. (They also pass under KASAN, which bodes well.)
This fix is, Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
[1]: https://lore.kernel.org/linux-kselftest/163309440949.24017.15314464452259318...
net/mctp/test/utils.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/net/mctp/test/utils.c b/net/mctp/test/utils.c index e2ab1f3da357..cc6b8803aa9d 100644 --- a/net/mctp/test/utils.c +++ b/net/mctp/test/utils.c @@ -46,17 +46,17 @@ struct mctp_test_dev *mctp_test_create_dev(void) dev = netdev_priv(ndev); dev->ndev = ndev;
rcu_read_lock();dev->mdev = __mctp_dev_get(ndev);mctp_dev_hold(dev->mdev);rcu_read_unlock();rc = register_netdev(ndev); if (rc) { free_netdev(ndev); return NULL; }
rcu_read_lock();dev->mdev = __mctp_dev_get(ndev);mctp_dev_hold(dev->mdev);rcu_read_unlock();return dev;}
-- 2.30.2
From: Jeremy Kerr jk@codeconstruct.com.au Date: Sat, 2 Oct 2021 10:26:55 +0800
The current kunit infrastructure defines its own module_init() when built as a module, which conflicts with the mctp core's own.
So, only allow MCTP_TEST when both MCTP and KUNIT are built-in.
Reported-by: kernel test robot lkp@intel.com Signed-off-by: Jeremy Kerr jk@codeconstruct.com.au
Jeremy I had to revert your entire series because of this.
You will need rseubmit the entire series with this build failure fixed.
Thasnk you.
Hi David,
Jeremy I had to revert your entire series because of this.
You will need rseubmit the entire series with this build failure fixed.
OK, thanks for letting me know, apologies for the breakage. Looks like my MCTP=m pre-send check didn't end up enabling MCTP_TEST...
v2 coming shortly.
Cheers,
Jeremy
Hi David,
Haha -- you sent this just as I'd come up with the same patch here. :-)
With these changes, alongside the rt->dev == NULL in mctp_route_release() crash fix mentioned in [1], the tests all pass on my system. (They also pass under KASAN, which bodes well.)
Awesome, thanks for checking these out. I've since sent a v2 with the fixes integrated, in order to not break davem's build.
I've refined the rt->dev == NULL case a little; rather than allowing ->dev == NULL in the core code (which should never happen), I've modified the test's route refcounting so that the route destroy path should only ever hit the test's own destructor instead (which allows !rt->dev cases). This means we can keep the ->dev != NULL assumption in the core, and still handle tests where our fake route->dev is unset.
Cheers,
Jeremy
Hi David,
[removing netdev, this is more kunit-related]
This looks good to me. I don't think you'll be the only person to hit this issue, so -- while it's probably overall nicer if tests can sit in their own module -- we'll look into finding a way of supporting this with KUnit at some point.
Just digging in to this a little: what's the intended structure here? Is it intended to have the tests as their own loadable modules?
For MCTP, I'd like to enable tests when we're built as a module; in that case, are you expecting we'd now have two modules: the core, and the kunit tests?
As you've seen, my test implementation here is to directly include the <tests>.c file; this means I don't need to EXPORT_SYMBOL all of the MCTP-internal functions that I want to test; they can remain private to the individual compilation unit. However, the current kunit_test_suites implementation prevents this.
I've been hacking around with the (very experimental) patch below, to allow tests in modules (without open-coding kunit_test_suites, which the aspeed mmc sdhci driver has done), but I don't have much background on kunit, so I'm not sure whether this is a useful direction to take.
However, this might allow a bunch of cleanups in future - we'd no longer need the array-of-arrays of suites, and can remove some of the custom suites definitions scattered around the tree.
Cheers,
Jeremy
--- From: Jeremy Kerr jk@codeconstruct.com.au Date: Sun, 3 Oct 2021 10:13:02 +0800 Subject: [RFC] kunit: Don't steal module_init
Currently, the MODULE version of kunit_test_suites defines its own module_init/exit_module functions, meaning that you can't define a module that has both an init and a kunit test suite.
This change removes the kunit-defined module inits, and instead parses the kunit tests from their own section in the module. After module init, we call __kunit_test_suites_init() on the contents of that section, which prepares and runs the suite.
This essentially unifies the module- and non-module kunit init formats.
Signed-off-by: Jeremy Kerr jk@codeconstruct.com.au --- include/kunit/test.h | 43 ++--------------------------------------- include/linux/module.h | 4 ++++ kernel/module.c | 6 ++++++ lib/kunit/test.c | 44 +++++++++++++++++++++++++++++++++++++++++- 4 files changed, 55 insertions(+), 42 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index 24b40e5c160b..8f34fc5c85ec 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -307,41 +307,8 @@ static inline int kunit_run_all_tests(void) } #endif /* IS_BUILTIN(CONFIG_KUNIT) */
-#ifdef MODULE -/** - * kunit_test_suites_for_module() - 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. - * - * 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_for_module(__suites) \ - static int __init kunit_test_suites_init(void) \ - { \ - return __kunit_test_suites_init(__suites); \ - } \ - module_init(kunit_test_suites_init); \ - \ - static void __exit kunit_test_suites_exit(void) \ - { \ - 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
@@ -354,14 +321,8 @@ static inline int kunit_run_all_tests(void) * 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. - * + * 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. */ #define kunit_test_suites(__suites...) \ __kunit_test_suites(__UNIQUE_ID(array), \ diff --git a/include/linux/module.h b/include/linux/module.h index c9f1200b2312..ff056cc667d4 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -502,6 +502,10 @@ struct module { int num_static_call_sites; struct static_call_site *static_call_sites; #endif +#ifdef CONFIG_KUNIT + struct kunit_suite ***kunit_suites_ptrs; + int num_kunit_suites_ptrs; +#endif
#ifdef CONFIG_LIVEPATCH bool klp; /* Is this a livepatch module? */ diff --git a/kernel/module.c b/kernel/module.c index 40ec9a030eec..4d90157a959d 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -3365,6 +3365,12 @@ static int find_module_sections(struct module *mod, struct load_info *info) sizeof(*mod->static_call_sites), &mod->num_static_call_sites); #endif +#ifdef CONFIG_KUNIT + mod->kunit_suites_ptrs = section_objs(info, ".kunit_test_suites", + sizeof(*mod->kunit_suites_ptrs), + &mod->num_kunit_suites_ptrs); +#endif + mod->extable = section_objs(info, "__ex_table", sizeof(*mod->extable), &mod->num_exentries);
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index f246b847024e..3b8dcb776030 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -586,6 +586,47 @@ void __kunit_test_suites_exit(struct kunit_suite **suites) } EXPORT_SYMBOL_GPL(__kunit_test_suites_exit);
+static void kunit_module_init(struct module *mod) +{ + unsigned int i; + + for (i = 0; i < mod->num_kunit_suites_ptrs; i++) + __kunit_test_suites_init(mod->kunit_suites_ptrs[i]); +} + +static void kunit_module_exit(struct module *mod) +{ + unsigned int i; + + for (i = 0; i < mod->num_kunit_suites_ptrs; i++) + __kunit_test_suites_exit(mod->kunit_suites_ptrs[i]); +} + +static int kunit_module_notify(struct notifier_block *nb, unsigned long val, + void *data) +{ + struct module *mod = data; + + switch (val) { + case MODULE_STATE_LIVE: + kunit_module_init(mod); + break; + case MODULE_STATE_GOING: + kunit_module_exit(mod); + break; + case MODULE_STATE_COMING: + case MODULE_STATE_UNFORMED: + break; + } + + return 0; +} + +static struct notifier_block kunit_mod_nb = { + .notifier_call = kunit_module_notify, + .priority = 0, +}; + /* * Used for static resources and when a kunit_resource * has been created by * kunit_alloc_resource(). When an init function is supplied, @data is passed @@ -795,12 +836,13 @@ static int __init kunit_init(void) { kunit_debugfs_init();
- return 0; + return register_module_notifier(&kunit_mod_nb); } late_initcall(kunit_init);
static void __exit kunit_exit(void) { + unregister_module_notifier(&kunit_mod_nb); kunit_debugfs_cleanup(); } module_exit(kunit_exit);
On Mon, Oct 4, 2021 at 10:21 AM Jeremy Kerr jk@codeconstruct.com.au wrote:
Hi David,
[removing netdev, this is more kunit-related]
This looks good to me. I don't think you'll be the only person to hit this issue, so -- while it's probably overall nicer if tests can sit in their own module -- we'll look into finding a way of supporting this with KUnit at some point.
Just digging in to this a little: what's the intended structure here? Is it intended to have the tests as their own loadable modules?
For MCTP, I'd like to enable tests when we're built as a module; in that case, are you expecting we'd now have two modules: the core, and the kunit tests?
That's what we've mostly done so far, just because it can be useful to load the core without the tests. There are also a number of test modules for things which themselves are built-in (e.g. KASAN), so having separate test modules was necessary for those.
We're still feeling out what the best way to do this is, though, and there's nothing wrong with including the KUnit tests in the MCTP module. It's just not as well explored, so does tend to hit a few issues. Better supporting tests embedded in a larger model (as well as making it easier to export symbols "for testing") are on our roadmap.
As you've seen, my test implementation here is to directly include the <tests>.c file; this means I don't need to EXPORT_SYMBOL all of the MCTP-internal functions that I want to test; they can remain private to the individual compilation unit. However, the current kunit_test_suites implementation prevents this.
We actually already have a test suite which already includes the "tests.c" file: the AppArmor suite. However AppArmor can't be built as a module, so we never hit any of the module issues with it.
We're still working out how best to test internal, static functions: the options basically are to include the "test.c" as you've found, or to find some way of exporting symbols "for testing", either with some macro magic (the functions are 'static' if CONFIG_KUNIT is undefined, or similar), or by trying to use module symbol namespaces. I expect we'll have a few tests doing each for a while, and then start to settle on whatever ends up being most convenient in most cases.
I've been hacking around with the (very experimental) patch below, to allow tests in modules (without open-coding kunit_test_suites, which the aspeed mmc sdhci driver has done), but I don't have much background on kunit, so I'm not sure whether this is a useful direction to take.
This looks really neat, and much better than the existing module support. There are a few other tests which open-code kunit_test_suites or something similar which have hit problems due to running too early if built-in, which this could help resolve as well. Getting rid of (or at least standardising) those custom KUnit suite initialisers is something I've wanted to do for a while.
I haven't had a chance to look through the patch in a lot of detail yet (and there are a few tweaks which would be needed, as it breaks the UML build at the moment), but this definitely looks a much better approach than what we've been doing. Module support has always been a little bit less stable than running built in tests, and this should at least go some way towards reconciling those two paths.
In any case, thanks a lot -- this is awesome.
Cheers, -- David
Hi David,
In any case, thanks a lot -- this is awesome.
Oh neat, glad it's useful!
I'm happy to keep hacking on this if it's in a direction that makes sense for kunit in general. As an approximate plan, I can fix the UML breakages, then work on some resulting simplifications for tree-wide initialisers (we'd no longer need the null-terminated arrays of suites everywhere, for example).
Cheers,
Jeremy
On Mon, Oct 11, 2021 at 10:10 AM Jeremy Kerr jk@codeconstruct.com.au wrote:
Hi David,
In any case, thanks a lot -- this is awesome.
Oh neat, glad it's useful!
I'm happy to keep hacking on this if it's in a direction that makes sense for kunit in general. As an approximate plan, I can fix the UML breakages, then work on some resulting simplifications for tree-wide initialisers (we'd no longer need the null-terminated arrays of suites everywhere, for example).
+dlatypov, +kunit-dev
Yeah, we think this would be a much better direction for KUnit to go for modules. If you're happy to work on it, that'd be great! Brendan, Daniel (CCed), and I would be more than willing to help out with questions, reviews, etc, as well.
On the other hand, if you're really busy and you'd rather we pick this up, improved module support has been on the to-do list for ages, so we could bump it up the list a bit and finish it off.
The UML breakages were mostly pretty simple: our default config doesn't require module support at all, so the various functions should just need to go behind the appropriate #ifdefs. A quick way to test it is just to run './tools/testing/kunit/kunit.py run' from the kernel source directory, which will configure, build, and run everything in the .kunit builddir.
Cheers, -- David
On Mon, Oct 11, 2021 at 9:38 PM David Gow davidgow@google.com wrote:
On Mon, Oct 11, 2021 at 10:10 AM Jeremy Kerr jk@codeconstruct.com.au wrote:
Hey Jeremy, small world! :-)
Hi David,
In any case, thanks a lot -- this is awesome.
Oh neat, glad it's useful!
I'm happy to keep hacking on this if it's in a direction that makes sense for kunit in general. As an approximate plan, I can fix the UML breakages, then work on some resulting simplifications for tree-wide initialisers (we'd no longer need the null-terminated arrays of suites everywhere, for example).
+dlatypov, +kunit-dev
Yeah, we think this would be a much better direction for KUnit to go for modules. If you're happy to work on it, that'd be great! Brendan, Daniel (CCed), and I would be more than willing to help out with questions, reviews, etc, as well.
+1 to David. My immediate reaction to looking at your diff is that it is the way that module support *should have* always worked, (No offense to those who worked on KUnit module support in the past: any module support was a big improvement over none.) your implementation is so much simpler and less obtrusive.
On the other hand, if you're really busy and you'd rather we pick this up, improved module support has been on the to-do list for ages, so we could bump it up the list a bit and finish it off.
The UML breakages were mostly pretty simple: our default config doesn't require module support at all, so the various functions should just need to go behind the appropriate #ifdefs. A quick way to test it is just to run './tools/testing/kunit/kunit.py run' from the kernel source directory, which will configure, build, and run everything in the .kunit builddir.
Cheers
On Sun, Oct 3, 2021 at 7:21 PM Jeremy Kerr jk@codeconstruct.com.au wrote:
Hi David,
[removing netdev, this is more kunit-related]
This looks good to me. I don't think you'll be the only person to hit this issue, so -- while it's probably overall nicer if tests can sit in their own module -- we'll look into finding a way of supporting this with KUnit at some point.
Just digging in to this a little: what's the intended structure here? Is it intended to have the tests as their own loadable modules?
For MCTP, I'd like to enable tests when we're built as a module; in that case, are you expecting we'd now have two modules: the core, and the kunit tests?
As you've seen, my test implementation here is to directly include the <tests>.c file; this means I don't need to EXPORT_SYMBOL all of the MCTP-internal functions that I want to test; they can remain private to the individual compilation unit. However, the current kunit_test_suites implementation prevents this.
I've been hacking around with the (very experimental) patch below, to allow tests in modules (without open-coding kunit_test_suites, which the aspeed mmc sdhci driver has done), but I don't have much background on kunit, so I'm not sure whether this is a useful direction to take.
However, this might allow a bunch of cleanups in future - we'd no longer need the array-of-arrays of suites, and can remove some of the custom suites definitions scattered around the tree.
Cheers,
Jeremy
From: Jeremy Kerr jk@codeconstruct.com.au Date: Sun, 3 Oct 2021 10:13:02 +0800 Subject: [RFC] kunit: Don't steal module_init
Currently, the MODULE version of kunit_test_suites defines its own module_init/exit_module functions, meaning that you can't define a module that has both an init and a kunit test suite.
This change removes the kunit-defined module inits, and instead parses the kunit tests from their own section in the module. After module init, we call __kunit_test_suites_init() on the contents of that section, which prepares and runs the suite.
This essentially unifies the module- and non-module kunit init formats.
Signed-off-by: Jeremy Kerr jk@codeconstruct.com.au
include/kunit/test.h | 43 ++--------------------------------------- include/linux/module.h | 4 ++++ kernel/module.c | 6 ++++++ lib/kunit/test.c | 44 +++++++++++++++++++++++++++++++++++++++++- 4 files changed, 55 insertions(+), 42 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index 24b40e5c160b..8f34fc5c85ec 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -307,41 +307,8 @@ static inline int kunit_run_all_tests(void) } #endif /* IS_BUILTIN(CONFIG_KUNIT) */
-#ifdef MODULE -/**
- kunit_test_suites_for_module() - 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.
- 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_for_module(__suites) \
static int __init kunit_test_suites_init(void) \{ \return __kunit_test_suites_init(__suites); \} \module_init(kunit_test_suites_init); \\static void __exit kunit_test_suites_exit(void) \{ \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@@ -354,14 +321,8 @@ static inline int kunit_run_all_tests(void)
- 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.
- 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.
#define kunit_test_suites(__suites...) \ __kunit_test_suites(__UNIQUE_ID(array), \ diff --git a/include/linux/module.h b/include/linux/module.h index c9f1200b2312..ff056cc667d4 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -502,6 +502,10 @@ struct module { int num_static_call_sites; struct static_call_site *static_call_sites; #endif +#ifdef CONFIG_KUNIT
struct kunit_suite ***kunit_suites_ptrs;int num_kunit_suites_ptrs;+#endif
#ifdef CONFIG_LIVEPATCH bool klp; /* Is this a livepatch module? */ diff --git a/kernel/module.c b/kernel/module.c index 40ec9a030eec..4d90157a959d 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -3365,6 +3365,12 @@ static int find_module_sections(struct module *mod, struct load_info *info) sizeof(*mod->static_call_sites), &mod->num_static_call_sites); #endif +#ifdef CONFIG_KUNIT
mod->kunit_suites_ptrs = section_objs(info, ".kunit_test_suites",sizeof(*mod->kunit_suites_ptrs),&mod->num_kunit_suites_ptrs);+#endif
mod->extable = section_objs(info, "__ex_table", sizeof(*mod->extable), &mod->num_exentries);diff --git a/lib/kunit/test.c b/lib/kunit/test.c index f246b847024e..3b8dcb776030 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c
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.
@@ -586,6 +586,47 @@ void __kunit_test_suites_exit(struct kunit_suite **suites) } EXPORT_SYMBOL_GPL(__kunit_test_suites_exit);
+static void kunit_module_init(struct module *mod) +{
unsigned int i;for (i = 0; i < mod->num_kunit_suites_ptrs; i++)__kunit_test_suites_init(mod->kunit_suites_ptrs[i]);+}
+static void kunit_module_exit(struct module *mod) +{
unsigned int i;for (i = 0; i < mod->num_kunit_suites_ptrs; i++)__kunit_test_suites_exit(mod->kunit_suites_ptrs[i]);+}
+static int kunit_module_notify(struct notifier_block *nb, unsigned long val,
void *data)+{
struct module *mod = data;switch (val) {case MODULE_STATE_LIVE:kunit_module_init(mod);break;case MODULE_STATE_GOING:kunit_module_exit(mod);break;case MODULE_STATE_COMING:case MODULE_STATE_UNFORMED:break;}return 0;+}
+static struct notifier_block kunit_mod_nb = {
.notifier_call = kunit_module_notify,.priority = 0,+};
/*
- Used for static resources and when a kunit_resource * has been created by
- kunit_alloc_resource(). When an init function is supplied, @data is passed
@@ -795,12 +836,13 @@ static int __init kunit_init(void) { kunit_debugfs_init();
return 0;
return register_module_notifier(&kunit_mod_nb);} late_initcall(kunit_init);
static void __exit kunit_exit(void) {
unregister_module_notifier(&kunit_mod_nb); kunit_debugfs_cleanup();} module_exit(kunit_exit);
Overall, this patch looks great to me!
Cheers!
On Mon, Oct 11, 2021 at 9:38 PM David Gow davidgow@google.com wrote:
On Mon, Oct 11, 2021 at 10:10 AM Jeremy Kerr jk@codeconstruct.com.au wrote:
Hi David,
In any case, thanks a lot -- this is awesome.
Oh neat, glad it's useful!
+1
I'm happy to keep hacking on this if it's in a direction that makes sense for kunit in general. As an approximate plan, I can fix the UML breakages, then work on some resulting simplifications for tree-wide initialisers (we'd no longer need the null-terminated arrays of suites everywhere, for example).
Getting rid of the arrays of arrays of suites sounds great to me.
I could also pick that bit up, since I'm the person who most recently added code that depends on it: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/co...
+dlatypov, +kunit-dev
Yeah, we think this would be a much better direction for KUnit to go for modules. If you're happy to work on it, that'd be great! Brendan, Daniel (CCed), and I would be more than willing to help out with questions, reviews, etc, as well.
On the other hand, if you're really busy and you'd rather we pick this up, improved module support has been on the to-do list for ages, so we could bump it up the list a bit and finish it off.
The UML breakages were mostly pretty simple: our default config doesn't require module support at all, so the various functions should just need to go behind the appropriate #ifdefs. A quick way to test it is just to run './tools/testing/kunit/kunit.py run' from the kernel source directory, which will configure, build, and run everything in the .kunit builddir.
Cheers, -- David
On Tue, Oct 12, 2021 at 1:27 PM Daniel Latypov dlatypov@google.com wrote:
On Mon, Oct 11, 2021 at 9:38 PM David Gow davidgow@google.com wrote:
On Mon, Oct 11, 2021 at 10:10 AM Jeremy Kerr jk@codeconstruct.com.au wrote:
Hi David,
In any case, thanks a lot -- this is awesome.
Oh neat, glad it's useful!
+1
I'm happy to keep hacking on this if it's in a direction that makes sense for kunit in general. As an approximate plan, I can fix the UML breakages, then work on some resulting simplifications for tree-wide initialisers (we'd no longer need the null-terminated arrays of suites everywhere, for example).
Getting rid of the arrays of arrays of suites sounds great to me.
I could also pick that bit up, since I'm the person who most recently added code that depends on it: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/co...
I made the executor internally flatten the array-of-arrays into just an array to flush out the changes we'd need to make. I've sent that here: https://lore.kernel.org/linux-kselftest/20211013191320.2490913-1-dlatypov@go...
After doing it and seeing the diffstat (2 files changed, 85 insertions(+), 178 deletions(-)), I'm actually tempted to have that go in as an actual patch.
When we rework the kunit_test_suites section, we could then just drop the flattening helper function.
+dlatypov, +kunit-dev
Yeah, we think this would be a much better direction for KUnit to go for modules. If you're happy to work on it, that'd be great! Brendan, Daniel (CCed), and I would be more than willing to help out with questions, reviews, etc, as well.
On the other hand, if you're really busy and you'd rather we pick this up, improved module support has been on the to-do list for ages, so we could bump it up the list a bit and finish it off.
The UML breakages were mostly pretty simple: our default config doesn't require module support at all, so the various functions should just need to go behind the appropriate #ifdefs. A quick way to test it is just to run './tools/testing/kunit/kunit.py run' from the kernel source directory, which will configure, build, and run everything in the .kunit builddir.
Cheers, -- David
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)
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.
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?).
(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).
So - any preferences for the options there?
Cheers,
Jeremy
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.
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
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
Hi all,
Happy new year! I'm just picking up this thread again, after having a bunch of other things come up at the end of December. I've since implemented some of the direct feedback on the patch, but wanted to clarify some overall direction too:
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.
I had a look at implementing this, but it doesn't seem to win us much with the current structure: since we kunit_run_all_tests() before a filesystem is available, kunit will always be "ready" (and the tests run) before we've had a chance to load modules, which may contain further tests.
One option would be to defer kunit_run_all_tests() until we think we have the full set of tests, but there's no specific point at which we know that all required modules are loaded. We could defer this to an explicit user-triggered "run the tests now" interface (debugfs?), but that might break expectations of the tests automatically executing on init.
Alternatively, I could properly split the TAP output, and just run tests whenever they're probed - either from the built-in set or as modules are loaded at arbitrary points in the future. However, I'm not sure of what the expectations on the consumer-side of the TAP output might be.
Are there any preferences on the approach here?
Cheers,
Jeremy
On Thu, Jan 6, 2022 at 6:53 PM Jeremy Kerr jk@codeconstruct.com.au wrote:
Hi all,
Happy new year! I'm just picking up this thread again, after having a bunch of other things come up at the end of December. I've since implemented some of the direct feedback on the patch, but wanted to clarify some overall direction too:
Thanks for reaching back out -- and sorry for the delay. I've dusted everything off post-LCA now, and had another look over this.
I'm CCing both the KUnit mailing list and Daniel Latypov on this thread so we can avoid (or track) any potential conflicts with things like: https://lore.kernel.org/linux-kselftest/20211013191320.2490913-1-dlatypov@go...
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.
I had a look at implementing this, but it doesn't seem to win us much with the current structure: since we kunit_run_all_tests() before a filesystem is available, kunit will always be "ready" (and the tests run) before we've had a chance to load modules, which may contain further tests.
I think the benefit of something like this isn't really with test modules so much as with built-in tests which are run manually. The thunderbolt test, for instance, currently initialises and runs tests itself[1,2], rather than via the KUnit executor, so that it can ensure some proportion of the stack is properly initialised. If there's a way of coalescing those into the same TAP output as other built-in tests, that'd be useful.
Thinking about it, though, I think it's a separate problem from module support, so not worth shoehorning in at this stage.
One option would be to defer kunit_run_all_tests() until we think we have the full set of tests, but there's no specific point at which we know that all required modules are loaded. We could defer this to an explicit user-triggered "run the tests now" interface (debugfs?), but that might break expectations of the tests automatically executing on init.
Yeah, while I do think it'd be neat to have an interface to explicitly run (or re-run) tests, I think having tests run on module load is still the most sensible thing, particularly since that's what people are expecting at the moment (especially with tests which were ported from standalone modules to KUnit).
There was a plan to allow test suites to be triggered from debugfs individually at some point, but I think it got derailed as tests weren't working if run twice, or some builtin-only tests having problems if run after a userland was brought up.
In any case, I think we should get test suites in modules running on module load, and leave any debugfs-related shenanigans for a future patchset.
Alternatively, I could properly split the TAP output, and just run tests whenever they're probed - either from the built-in set or as modules are loaded at arbitrary points in the future. However, I'm not sure of what the expectations on the consumer-side of the TAP output might be.
At the moment, the KUnit tooling will stop parsing after the first full TAP output, so if suites are outputting TAP separately, only the first one will be handled properly by kunit_tool. Of course, kunit_tool doesn't really handle modules by itself at the moment, so as long as the output is provided to kunit_tool one at a time, it's still possible to use it. (And the possibility of adding a way for kunit_tool to handle multiple TAP outputs in the same file is something we plan to look into.)
So, I think this isn't a problem for modules at the moment, though again it could be a bit painful for things like the thunderbolt test where there are multiple TAP documents from built-ins.
Note that the updated KTAP[3] spec does actually leave the option open for TAP output to effectively be "streaming" and to not state the total number of tests in advance, but I don't think that's the ideal way of handling it here.
Are there any preferences on the approach here?
So, after all that, I think the original plan makes the most sense, and if we find any cases which are particularly annoying we can either change things then, or (better still) adapt the tooling to handle them better.
Thanks again for looking into this! -- David
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv... [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv... [3]: https://www.kernel.org/doc/html/latest/dev-tools/ktap.html
On Fri, Jan 28, 2022 at 3:41 PM David Gow davidgow@google.com wrote:
On Thu, Jan 6, 2022 at 6:53 PM Jeremy Kerr jk@codeconstruct.com.au wrote:
Hi all,
Happy new year! I'm just picking up this thread again, after having a bunch of other things come up at the end of December. I've since implemented some of the direct feedback on the patch, but wanted to clarify some overall direction too:
Thanks for reaching back out -- and sorry for the delay. I've dusted everything off post-LCA now, and had another look over this.
I'm CCing both the KUnit mailing list and Daniel Latypov on this thread so we can avoid (or track) any potential conflicts with things like: https://lore.kernel.org/linux-kselftest/20211013191320.2490913-1-dlatypov@go...
[Whoops, I'm smart. Actually CCing them this time!]
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.
I had a look at implementing this, but it doesn't seem to win us much with the current structure: since we kunit_run_all_tests() before a filesystem is available, kunit will always be "ready" (and the tests run) before we've had a chance to load modules, which may contain further tests.
I think the benefit of something like this isn't really with test modules so much as with built-in tests which are run manually. The thunderbolt test, for instance, currently initialises and runs tests itself[1,2], rather than via the KUnit executor, so that it can ensure some proportion of the stack is properly initialised. If there's a way of coalescing those into the same TAP output as other built-in tests, that'd be useful.
Thinking about it, though, I think it's a separate problem from module support, so not worth shoehorning in at this stage.
One option would be to defer kunit_run_all_tests() until we think we have the full set of tests, but there's no specific point at which we know that all required modules are loaded. We could defer this to an explicit user-triggered "run the tests now" interface (debugfs?), but that might break expectations of the tests automatically executing on init.
Yeah, while I do think it'd be neat to have an interface to explicitly run (or re-run) tests, I think having tests run on module load is still the most sensible thing, particularly since that's what people are expecting at the moment (especially with tests which were ported from standalone modules to KUnit).
There was a plan to allow test suites to be triggered from debugfs individually at some point, but I think it got derailed as tests weren't working if run twice, or some builtin-only tests having problems if run after a userland was brought up.
In any case, I think we should get test suites in modules running on module load, and leave any debugfs-related shenanigans for a future patchset.
Alternatively, I could properly split the TAP output, and just run tests whenever they're probed - either from the built-in set or as modules are loaded at arbitrary points in the future. However, I'm not sure of what the expectations on the consumer-side of the TAP output might be.
At the moment, the KUnit tooling will stop parsing after the first full TAP output, so if suites are outputting TAP separately, only the first one will be handled properly by kunit_tool. Of course, kunit_tool doesn't really handle modules by itself at the moment, so as long as the output is provided to kunit_tool one at a time, it's still possible to use it. (And the possibility of adding a way for kunit_tool to handle multiple TAP outputs in the same file is something we plan to look into.)
So, I think this isn't a problem for modules at the moment, though again it could be a bit painful for things like the thunderbolt test where there are multiple TAP documents from built-ins.
Note that the updated KTAP[3] spec does actually leave the option open for TAP output to effectively be "streaming" and to not state the total number of tests in advance, but I don't think that's the ideal way of handling it here.
Are there any preferences on the approach here?
So, after all that, I think the original plan makes the most sense, and if we find any cases which are particularly annoying we can either change things then, or (better still) adapt the tooling to handle them better.
Thanks again for looking into this! -- David
linux-kselftest-mirror@lists.linaro.org
-
Brendan Higgins -
Daniel Latypov -
David Gow -
David Miller -
Jeremy Kerr