[PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[Jeremy Kerr]

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
-- 
2.30.2

[PATCH net-next 2/2] mctp: test: defer mdev setup until we've registered

[Jeremy Kerr]

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;
 }
 
-- 
2.30.2

Re: [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[David Gow]

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_TESTS
> -        depends on MCTP && KUNIT
> +        bool "MCTP core tests" if !KUNIT_ALL_TESTS
> +        depends on MCTP=y && KUNIT=y
>          default KUNIT_ALL_TESTS
> --
> 2.30.2
>

Re: [PATCH net-next 2/2] mctp: test: defer mdev setup until we've registered

[David Gow]

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.15314464452259318665.git-patchwork-notify@kernel.org/T/#m1a319b6932dd2bffaf78ab2d3b4c399282f7bda2



>  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
>

Re: [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[David Miller]

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.

Re: [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[Jeremy Kerr]

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

Re: [PATCH net-next 2/2] mctp: test: defer mdev setup until we've registered

[Jeremy Kerr]

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

Re: [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[Jeremy Kerr]

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);
-- 
2.30.2

Re: [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[David Gow]

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

Re: [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[Jeremy Kerr]

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

Re: [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[David Gow]

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

Re: [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[Brendan Higgins]

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

Re: [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[Brendan Higgins]

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!

Re: [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[Daniel Latypov]

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/commit/?h=kunit&id=3b29021ddd10cfb6b2565c623595bd3b02036f33

> >
> +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

Re: [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[Daniel Latypov]

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/commit/?h=kunit&id=3b29021ddd10cfb6b2565c623595bd3b02036f33

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@google.com

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

Re: [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[Jeremy Kerr]

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

Re: [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[David Gow]

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
>
> 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

Re: [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[Brendan Higgins]

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
> >
> > 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

Re: [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[Jeremy Kerr]

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

Re: [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[David Gow]

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@google.com/


>
> > 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/drivers/thunderbolt/test.c#n2730
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thunderbolt/domain.c#n881
[3]: https://www.kernel.org/doc/html/latest/dev-tools/ktap.html

Re: [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[David Gow]

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@google.com/

[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
>
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thunderbolt/test.c#n2730
> [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thunderbolt/domain.c#n881
> [3]: https://www.kernel.org/doc/html/latest/dev-tools/ktap.html