Re: [PATCH v2 1/2] kunit: add kunit.enable to enable/disable KUnit test

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.

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