Message-ID: <CAF-60z3qm+a-POv-db8y8tsQn_qxxT_Jk+4_zHQ4xt2xd9ud8w@mail.gmail.com>
Date:   Mon, 22 Aug 2022 13:37:54 -0700
From:   Joe Fradley <joefradley@...gle.com>
To:     David Gow <davidgow@...gle.com>
Cc:     Nico Pache <npache@...hat.com>, Jonathan Corbet <corbet@....net>,
        Brendan Higgins <brendan.higgins@...ux.dev>,
        kernel-team@...roid.com,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        KUnit Development <kunit-dev@...glegroups.com>
Subject: Re: [PATCH 1/2] kunit: add kunit.enable to enable/disable KUnit test

On Fri, Aug 19, 2022 at 1:16 AM David Gow <davidgow@...gle.com> wrote:
>
> +Nico Pache in case this could be useful with kernel/kunit packaging.
>
> On Thu, Aug 18, 2022 at 12:49 AM 'Joe Fradley' via KUnit Development
> <kunit-dev@...glegroups.com> wrote:
> >
> > This patch adds the kunit.enable module parameter that will need to be
> > set to true in addition to KUNIT being enabled for KUnit tests to run.
> > The default value is true giving backwards compatibility. However, for
> > the production+testing use case the new config option
> > KUNIT_ENABLE_DEFAULT_DISABLED can be enabled to default kunit.enable to
> > false requiring the tester to opt-in by passing kunit.enable=1 to
> > the kernel.
> >
> > Signed-off-by: Joe Fradley <joefradley@...gle.com>
> > ---
>
> Thanks for sending this out.
>
> I'm generally in support of the idea, and the implementation is okay,
> but there are a few small usability issues or bits of futureproofing
> we could do.
>
> On the first front, this doesn't integrate well with kunit_tool: if
> built-in tests are disabled, it will print the test header and test
> plan, but no results, which confuses the kunit_tool parser.

Thanks for pointing this out and reviewing this patch. I neglected
to test it with the wrapper script. I had only tested manually with
local VMs.

> In addition, maybe it'd be nice to have kunit_tool automatically pass
> kunit.enable=1 to any kernels it boots. Equally, a few minor
> naming/description suggestions.

Makes sense to me, I’ll add it. This would technically make the missing
TAP header and results a moot point. (But we can still safeguard that
scenario)

>
> More details in comments below.
>
> On the second topic, I think we need to work out exactly how we can
> evolve this to make it as useful as possible upstream going forward.
> In general, while there's nothing fundamentally wrong with having
> tests disabled at runtime, it is going to be a very niche feature, as
> for most users the correct solution here is to build a new kernel,
> without KUnit.
>
> My feeling is that the real distinction worth making here is that
> tests can be compiled in and loaded (e.g., if tests are embedded in a
> non-test module, like apparmor, or thunderbolt, or (soon) amdgpu), but
> won't execute automatically. Now, at the moment there's no way to
> manually trigger a test. so this distinction isn't yet important, but
> we may want to add something down the line, such as the ability to
> trigger a test via debugfs (this was proposed as part of the original
> debugfs support packages), or the ability to change the value of this
> 'enable' flag at runtime, and then load a specific test.
>
> Maybe that involves some further changes to the implementation (at its
> most extreme, it could involve moving the checks out into the module
> loader and the kernel_init_freeable() function, though I don't think
> that's _necessary_), but for the most part, it probably just involves
> describing and documenting this change as such.
>
> Would something like that still serve Android's purposes? Or is it
> critically important that the idea behind this is "if this is set to
> false, there should be no way of running KUnit tests", and having a
> manual way to trigger them down the line would defeat the point?
>

As of right now, we wouldn’t want someone to enable KUnit after boot.
However, because we lock out debugfs for these instances maybe that
would be an acceptable path to enable it after boot.

I’m wondering why these ‘mixed’ modules would need that. Is it that
those tests shouldn’t/can’t run at startup?


> Cheers,
> -- David
>
> >  .../admin-guide/kernel-parameters.txt         |  6 ++++++
> >  lib/kunit/Kconfig                             |  8 ++++++++
> >  lib/kunit/test.c                              | 20 +++++++++++++++++++
> >  3 files changed, 34 insertions(+)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index d7f30902fda0..ab4c7d133c38 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -2436,6 +2436,12 @@
> >                         0: force disabled
> >                         1: force enabled
> >
> > +       kunit.enable=   [KUNIT] Enable executing KUnit tests. Requires
> > +                       CONFIG_KUNIT to be set to be fully enabled. The
> > +                       default value can be overridden to disabled via
> > +                       KUNIT_ENABLE_DEFAULT_DISABLED.
> > +                       Default is 1 (enabled)
> > +
> >         kvm.ignore_msrs=[KVM] Ignore guest accesses to unhandled MSRs.
> >                         Default is 0 (don't ignore, but inject #GP)
> >
> > diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig
> > index 0b5dfb001bac..5d6db58dbe9b 100644
> > --- a/lib/kunit/Kconfig
> > +++ b/lib/kunit/Kconfig
> > @@ -59,4 +59,12 @@ config KUNIT_ALL_TESTS
> >
> >           If unsure, say N.
> >
> > +config KUNIT_ENABLE_DEFAULT_DISABLED
> > +       bool "Require boot parameter to enable KUnit test execution"
> > +       help
> > +         Sets the default value of the kernel parameter kunit.enable to 0
> > +         (disabled). This means to fully enable kunit, config KUNIT needs
> > +         to be enabled along with `kunit.enable=1` passed to the kernel. This
> > +         allows KUnit to be opt-in at boot time.
> > +
>
> Hmm... would it make more sense to have this be DEFAULT_ENABLED and
> have the default value of the config option be y instead?
> Personally, I think that'd avoid the double-negative, and so might be clearer.

That would definitely be more clear. We just want to make sure it's backwards
compatible, which it should be if we default it in the config to y.

>
> >  endif # KUNIT
> > diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> > index b73d5bb5c473..e6f98e16e8ae 100644
> > --- a/lib/kunit/test.c
> > +++ b/lib/kunit/test.c
> > @@ -54,6 +54,17 @@ void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...)
> >  EXPORT_SYMBOL_GPL(__kunit_fail_current_test);
> >  #endif
> >
> > +/*
> > + * Enable KUnit tests to run.
> > + */
> > +#ifdef CONFIG_KUNIT_ENABLE_DEFAULT_DISABLED
> > +static bool enable_param;
> > +#else
> > +static bool enable_param = true;
> > +#endif
> > +module_param_named(enable, enable_param, bool, 0);
> > +MODULE_PARM_DESC(enable, "Enable KUnit tests to run");
>
> Maybe "Enable KUnit tests" is simpler than adding "to run", which
> reads a bit awkwardly.
>
> If we were to treat this variable as specifically enabling the "run
> tests on boot" and/or "module load", then that could be worked into
> the description, too.

Going the “Enable KUnit tests” route is definitely less awkward, thanks.

>
> > +
> >  /*
> >   * KUnit statistic mode:
> >   * 0 - disabled
> > @@ -590,6 +601,12 @@ int __kunit_test_suites_init(struct kunit_suite * const * const suites, int num_
> >  {
> >         unsigned int i;
> >
> > +       if (!enable_param && num_suites > 0) {
> > +               pr_info("kunit: deactivated, cannot load %s\n",
> > +                       suites != NULL && suites[0] != NULL ? suites[0]->name : "NULL");
> > +               return -EPERM;
> > +       }
> > +
>
> This mostly works, but has a few small issues:
> - KUnit will still print the header and a test plan, so kunit_tool
> will report a large number of "crashed" tests when no results are
> forthcoming.
>   It should be posible to add a similar check in kunit_run_all_tests()
> to handle that case:
> https://elixir.bootlin.com/linux/latest/source/lib/kunit/executor.c#L246
>   You can test this with:
> ./tools/testing/kunit/kunit.py run --kconfig_add
> CONFIG_KUNIT_ENABLE_DEFAULT_DISABLED=y

Thank you, I can easily repro that output now and adding a check in
`kunit_run_all_tests()` as you suggested cleans it up. However, it
still shows an error if the TAP header is missing and if I add the
header, it errors on having 0 tests. But maybe either of these are
OK, as it conveys no tests have run even though you're running
the `knuit_tool` assumably expecting test output. But we're not
falsely stating particular tests have crashed.

Also, now that we’re going to be passing in kunit.enable=1 it will be
very difficult, if not impossible, to get in this situation.

>
> - The message is not ideal: it only refers to the first suite in the
> module (or built into the kernel). and "cannot load" is not really
> applicable to built-in tests.

If I move this check up one call into `kunit_modue_init` I could use
the module name. (There won't be a built-in path through here any
more after the `kunit_run_all_tests() addition.) However,
`__kunit_test_suites_init()` is exported so theoretically someone else
could call it bypassing the check. So, I think I'll leave it where it's at
now and simplify the message to something like:
"kunit: disabled, not executing tests\n"

>   Maybe the goal should be less to "not load test modules" but more to
> "allow test modules to load, but don't run the tests in them".
>   Thoughts?

Originally the feeling was, if we aren't going to run the tests, why
load the test modules? However, with the “mixed” modules you
mentioned in mind we would want to load them just not run the tests.

>
> - If we were to treat this as "tests load, but don't run
> automatically", then we probably don't want this to be an EPERM...

Agreed.

>
> >         for (i = 0; i < num_suites; i++) {
> >                 kunit_init_suite(suites[i]);
> >                 kunit_run_tests(suites[i]);
> > @@ -607,6 +624,9 @@ void __kunit_test_suites_exit(struct kunit_suite **suites, int num_suites)
> >  {
> >         unsigned int i;
> >
> > +       if (!enable_param)
> > +               return;
> > +
> >         for (i = 0; i < num_suites; i++)
> >                 kunit_exit_suite(suites[i]);
> >
> > --
> > 2.37.1.595.g718a3a8f04-goog
> >
> > --
> > You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@...glegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20220817164851.3574140-2-joefradley%40google.com.

Powered by blists - more mailing lists