Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220823142456.3977086-1-joefradley@google.com>
 <20220823142456.3977086-2-joefradley@google.com> <CABVgOS=E44fQ5UY5b3np5ZSrkDeXkkEAPBBA5yDd3bE72gotww@mail.gmail.com>
 <CAF-60z1H77S-pVevKyZ7DQ9e09h4=Gnhbv9RA_SWTk=kmsq18g@mail.gmail.com> <CABVgOS=_pYqA6yEc5C4o+JpxSghVsWZV_rZ2TXHrgwjkvou28g@mail.gmail.com>
In-Reply-To: <CABVgOS=_pYqA6yEc5C4o+JpxSghVsWZV_rZ2TXHrgwjkvou28g@mail.gmail.com>
From:   Joe Fradley <joefradley@google.com>
Date:   Wed, 24 Aug 2022 07:13:42 -0700
Message-ID: <CAF-60z1xq3cMBO5xwRtFsm2upDT=je1H_QQG+AF4yKGZ30SjPQ@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] kunit: add kunit.enable to enable/disable KUnit test
To:     David Gow <davidgow@google.com>
Cc:     Jonathan Corbet <corbet@lwn.net>,
        Brendan Higgins <brendan.higgins@linux.dev>,
        kernel-team@android.com,
        "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        KUnit Development <kunit-dev@googlegroups.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Aug 23, 2022 at 11:33 PM David Gow <davidgow@google.com> wrote:
>
> On Wed, Aug 24, 2022 at 1:04 PM Joe Fradley <joefradley@google.com> wrote:
> >
> > On Tue, Aug 23, 2022 at 9:31 PM David Gow <davidgow@google.com> wrote:
> > >
> > > On Tue, Aug 23, 2022 at 10:25 PM 'Joe Fradley' via KUnit Development
> > > <kunit-dev@googlegroups.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_DEFAULT_ENABLED can be set to N requiring the tester to opt-in
> > > > by passing kunit.enable=1 to the kernel.
> > > >
> > > > Signed-off-by: Joe Fradley <joefradley@google.com>
> > > > ---
> > >
> > > Thanks very much. This looks good to me, and works on my machine.
> > >
> > > I've put a few comments/ideas below, but none of them feel necessary to me.
> >
> > Thank you for the review. I need to do one follow up revision to base this
> > off of the appropriate `linux-kselftest/kunit` branch.
> >
>
> This already applies cleanly to linux-kselftest/kunit -- it should be
> fine as-is.

Ok great, I'll leave as-is.


>
> (It also applies fine to kselftest/kunit-fixes, for what it's worth.)
>
> Cheers,
> -- David
>
> > >
> > > Regardless, this is
> > > Reviewed-by: David Gow <davidgow@google.com>
> > >
> > > Cheers,
> > > -- David
> > >
> > > > Changes since v1:
> > > > - Created a function to get kunit enable state
> > > > - Check kunit enable state in kunit_run_all_tests() in executor.c
> > > > - Load test module even if KUnit is disabled but still don't execute
> > > >   tests
> > > > - Simplified kunit disable message and kunit.enable parameter
> > > >   description
> > > > - Flipped around logic of new config to be KUNIT_DEFAULT_ENABLED
> > > > - kunit_tool.py now passes kunit.enable=1 to kernel
> > > >
> > > >  .../admin-guide/kernel-parameters.txt         |  6 +++++
> > > >  include/kunit/test.h                          |  2 ++
> > > >  lib/kunit/Kconfig                             | 11 +++++++++
> > > >  lib/kunit/executor.c                          |  4 ++++
> > > >  lib/kunit/test.c                              | 24 +++++++++++++++++++
> > > >  tools/testing/kunit/kunit_kernel.py           |  1 +
> > > >  6 files changed, 48 insertions(+)
> > > >
> > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > > index adfda56b2691..7aa3abd7f1c5 100644
> > > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > > @@ -2432,6 +2432,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 via
> > > > +                       KUNIT_DEFAULT_ENABLED.
> > > > +                       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/include/kunit/test.h b/include/kunit/test.h
> > > > index c958855681cc..ee6bf4ecbd89 100644
> > > > --- a/include/kunit/test.h
> > > > +++ b/include/kunit/test.h
> > > > @@ -228,6 +228,8 @@ static inline void kunit_set_failure(struct kunit *test)
> > > >         WRITE_ONCE(test->status, KUNIT_FAILURE);
> > > >  }
> > > >
> > > > +bool kunit_enabled(void);
> > > > +
> > >
> > > This probably isn't strictly necessary at this stage, given that it
> > > just checks one variable. That being said, I don't think it hurts (and
> > > personally, I quite like it), and find it more future-proof than
> > > exposing the variable more widely anyway.
> >
> > It also addressed it being a static variable.
> >
> > >
> > > >  void kunit_init_test(struct kunit *test, const char *name, char *log);
> > > >
> > > >  int kunit_run_tests(struct kunit_suite *suite);
> > > > diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig
> > > > index 0b5dfb001bac..626719b95bad 100644
> > > > --- a/lib/kunit/Kconfig
> > > > +++ b/lib/kunit/Kconfig
> > > > @@ -59,4 +59,15 @@ config KUNIT_ALL_TESTS
> > > >
> > > >           If unsure, say N.
> > > >
> > > > +config KUNIT_DEFAULT_ENABLED
> > > > +       bool "Default value of kunit.enable"
> > > > +       default y
> > > > +       help
> > > > +         Sets the default value of kunit.enable. If set to N then KUnit
> > > > +         tests will not execute unless kunit.enable=1 is passed to the
> > > > +         kernel command line.
> > > > +
> > > > +         In most cases this should be left as Y. Only if additional opt-in
> > > > +         behavior is needed should this be set to N.
> > > > +
> > > >  endif # KUNIT
> > > > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> > > > index 5e223327196a..9bbc422c284b 100644
> > > > --- a/lib/kunit/executor.c
> > > > +++ b/lib/kunit/executor.c
> > > > @@ -190,6 +190,10 @@ int kunit_run_all_tests(void)
> > > >  {
> > > >         struct suite_set suite_set = {__kunit_suites_start, __kunit_suites_end};
> > > >         int err = 0;
> > > > +       if (!kunit_enabled()) {
> > > > +               pr_info("kunit: disabled\n");
> > > > +               goto out;
> > > > +       }
> > > >
> > > >         if (filter_glob_param) {
> > > >                 suite_set = kunit_filter_suites(&suite_set, filter_glob_param, &err);
> > > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> > > > index b73d5bb5c473..1e54373309a4 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_DEFAULT_ENABLED
> > > > +static bool enable_param = true;
> > > > +#else
> > > > +static bool enable_param;
> > > > +#endif
> > > > +module_param_named(enable, enable_param, bool, 0);
> > > > +MODULE_PARM_DESC(enable, "Enable KUnit tests");
> > > > +
> > > >  /*
> > > >   * KUnit statistic mode:
> > > >   * 0 - disabled
> > > > @@ -586,10 +597,20 @@ static void kunit_init_suite(struct kunit_suite *suite)
> > > >         suite->suite_init_err = 0;
> > > >  }
> > > >
> > > > +bool kunit_enabled(void)
> > > > +{
> > > > +       return enable_param;
> > > > +}
> > > > +
> > > >  int __kunit_test_suites_init(struct kunit_suite * const * const suites, int num_suites)
> > > >  {
> > > >         unsigned int i;
> > > >
> > > > +       if (!kunit_enabled() && num_suites > 0) {
> > > > +               pr_info("kunit: disabled\n");
> > >
> > > _Maybe_ this could be pr_info_once(), if you were worried about spam
> > > (if a whole bunch of test modules were loaded at once). That being
> > > said, I prefer it as-is, as I don't think there are a lot of cases
> > > where large number of kunit test modules are loaded on a system with
> > > KUnit disable. And I'm liable to forget that KUnit is disabled if a
> > > system has been running for a while (and maybe one test module was
> > > loaded a boot), and end up wondering why my test isn't running.
> >
> > That's the same conclusion I came to after considering the one time
> > message used for the test taint message.
> >
> > >
> > > So, I'm all for leaving this as-is, personally.
> > >
> > > > +               return 0;
> > > > +       }
> > > > +
> > > >         for (i = 0; i < num_suites; i++) {
> > > >                 kunit_init_suite(suites[i]);
> > > >                 kunit_run_tests(suites[i]);
> > > > @@ -607,6 +628,9 @@ void __kunit_test_suites_exit(struct kunit_suite **suites, int num_suites)
> > > >  {
> > > >         unsigned int i;
> > > >
> > > > +       if (!kunit_enabled())
> > > > +               return;
> > > > +
> > > >         for (i = 0; i < num_suites; i++)
> > > >                 kunit_exit_suite(suites[i]);
> > > >
> > > > diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> > > > index f5c26ea89714..ef794da420d7 100644
> > > > --- a/tools/testing/kunit/kunit_kernel.py
> > > > +++ b/tools/testing/kunit/kunit_kernel.py
> > > > @@ -359,6 +359,7 @@ class LinuxSourceTree:
> > > >                         args = []
> > > >                 if filter_glob:
> > > >                         args.append('kunit.filter_glob='+filter_glob)
> > > > +               args.append('kunit.enable=1')
> > > >
> > > >                 process = self._ops.start(args, build_dir)
> > > >                 assert process.stdout is not None  # tell mypy it's set
> > > > --
> > > > 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@googlegroups.com.
> > > > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20220823142456.3977086-2-joefradley%40google.com.