Re: [PATCH v5 3/3] kunit: Allow kunit test modules to use test filtering

[David Gow]

On Mon, 7 Aug 2023 at 18:28, Janusz Krzysztofik
<janusz.krzysztofik@xxxxxxxxxxxxxxx> wrote:
>
> External tools, e.g., Intel GPU tools (IGT), support execution of
> individual selftests provided by kernel modules.  That could be also
> applicable to kunit test modules if they provided test filtering.  But
> test filtering is now possible only when kunit code is built into the
> kernel.  Moreover, a filter can be specified only at boot time, then
> reboot is required each time a different filter is needed.
>
> Build the test filtering code also when kunit is configured as a module,
> expose test filtering functions to other kunit source files, and use them
> in kunit module notifier callback functions.  Userspace can then reload
> the kunit module with a value of the filter_glob parameter tuned to a
> specific kunit test module every time it wants to limit the scope of tests
> executed on that module load.  Make the kunit.filter* parameters visible
> in sysfs for user convenience.
>
> v5: Refresh on tpp of attributes filtering fix
> v4: Refresh on top of newly applied attributes patches and changes
>     introdced by new versions of other patches submitted in series with
>     this one.
> v3: Fix CONFIG_GLOB, required by filtering functions, not selected when
>     building as a module (lkp@xxxxxxxxx).
> v2: Fix new name of a structure moved to kunit namespace not updated
>     across all uses (lkp@xxxxxxxxx).
>
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@xxxxxxxxxxxxxxx>
> ---

This works a treat here. It's a bit annoying to have to unload /
reload the module to change the filter, which might be something for
us to consider in the future (maybe via a debugfs entry?), but this is
nevertheless an improvement.

Reviewed-by: David Gow <davidgow@xxxxxxxxxx>

Cheers,
-- David

>  include/kunit/test.h | 11 ++++++++
>  lib/kunit/Kconfig    |  2 +-
>  lib/kunit/executor.c | 63 ++++++++++++++++++++++++++------------------
>  lib/kunit/test.c     | 17 ++++++++++++
>  4 files changed, 66 insertions(+), 27 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 6a338a3267ed5..d33114097d0d0 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -310,6 +310,9 @@ static inline void kunit_set_failure(struct kunit *test)
>
>  bool kunit_enabled(void);
>  const char *kunit_action(void);
> +const char *kunit_filter_glob(void);
> +char *kunit_filter(void);
> +char *kunit_filter_action(void);
>
>  void kunit_init_test(struct kunit *test, const char *name, char *log);
>
> @@ -320,6 +323,14 @@ size_t kunit_suite_num_test_cases(struct kunit_suite *suite);
>  unsigned int kunit_test_case_num(struct kunit_suite *suite,
>                                  struct kunit_case *test_case);
>
> +struct kunit_suite_set
> +kunit_filter_suites(const struct kunit_suite_set *suite_set,
> +                   const char *filter_glob,
> +                   char *filters,
> +                   char *filter_action,
> +                   int *err);
> +void kunit_free_suite_set(struct kunit_suite_set suite_set);
> +
>  int __kunit_test_suites_init(struct kunit_suite * const * const suites, int num_suites);
>
>  void __kunit_test_suites_exit(struct kunit_suite **suites, int num_suites);
> diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig
> index 626719b95badd..68a6daec0aef1 100644
> --- a/lib/kunit/Kconfig
> +++ b/lib/kunit/Kconfig
> @@ -4,7 +4,7 @@
>
>  menuconfig KUNIT
>         tristate "KUnit - Enable support for unit tests"
> -       select GLOB if KUNIT=y
> +       select GLOB
>         help
>           Enables support for kernel unit tests (KUnit), a lightweight unit
>           testing and mocking framework for the Linux kernel. These tests are
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index e877c1f1e75c8..5181aa2e760b6 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -27,24 +27,37 @@ const char *kunit_action(void)
>         return action_param;
>  }
>
> -#if IS_BUILTIN(CONFIG_KUNIT)
> -
>  static char *filter_glob_param;
>  static char *filter_param;
>  static char *filter_action_param;
>
> -module_param_named(filter_glob, filter_glob_param, charp, 0);
> +module_param_named(filter_glob, filter_glob_param, charp, 0400);
>  MODULE_PARM_DESC(filter_glob,
>                 "Filter which KUnit test suites/tests run at boot-time, e.g. list* or list*.*del_test");
> -module_param_named(filter, filter_param, charp, 0);
> +module_param_named(filter, filter_param, charp, 0400);
>  MODULE_PARM_DESC(filter,
>                 "Filter which KUnit test suites/tests run at boot-time using attributes, e.g. speed>slow");
> -module_param_named(filter_action, filter_action_param, charp, 0);
> +module_param_named(filter_action, filter_action_param, charp, 0400);
>  MODULE_PARM_DESC(filter_action,
>                 "Changes behavior of filtered tests using attributes, valid values are:\n"
>                 "<none>: do not run filtered tests as normal\n"
>                 "'skip': skip all filtered tests instead so tests will appear in output\n");
>
> +const char *kunit_filter_glob(void)
> +{
> +       return filter_glob_param;
> +}
> +
> +char *kunit_filter(void)
> +{
> +       return filter_param;
> +}
> +
> +char *kunit_filter_action(void)
> +{
> +       return filter_action_param;
> +}
> +
>  /* glob_match() needs NULL terminated strings, so we need a copy of filter_glob_param. */
>  struct kunit_glob_filter {
>         char *suite_glob;
> @@ -108,10 +121,7 @@ kunit_filter_glob_tests(const struct kunit_suite *const suite, const char *test_
>         return copy;
>  }
>
> -static char *kunit_shutdown;
> -core_param(kunit_shutdown, kunit_shutdown, charp, 0644);
> -
> -static void kunit_free_suite_set(struct kunit_suite_set suite_set)
> +void kunit_free_suite_set(struct kunit_suite_set suite_set)
>  {
>         struct kunit_suite * const *suites;
>
> @@ -120,7 +130,7 @@ static void kunit_free_suite_set(struct kunit_suite_set suite_set)
>         kfree(suite_set.start);
>  }
>
> -static struct kunit_suite_set
> +struct kunit_suite_set
>  kunit_filter_suites(const struct kunit_suite_set *suite_set,
>                     const char *filter_glob,
>                     char *filters,
> @@ -218,22 +228,6 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,
>         return filtered;
>  }
>
> -static void kunit_handle_shutdown(void)
> -{
> -       if (!kunit_shutdown)
> -               return;
> -
> -       if (!strcmp(kunit_shutdown, "poweroff"))
> -               kernel_power_off();
> -       else if (!strcmp(kunit_shutdown, "halt"))
> -               kernel_halt();
> -       else if (!strcmp(kunit_shutdown, "reboot"))
> -               kernel_restart(NULL);
> -
> -}
> -
> -#endif
> -
>  void kunit_exec_run_tests(struct kunit_suite_set *suite_set, bool builtin)
>  {
>         size_t num_suites = suite_set->end - suite_set->start;
> @@ -271,6 +265,23 @@ void kunit_exec_list_tests(struct kunit_suite_set *suite_set, bool include_attr)
>
>  #if IS_BUILTIN(CONFIG_KUNIT)
>
> +static char *kunit_shutdown;
> +core_param(kunit_shutdown, kunit_shutdown, charp, 0644);
> +
> +static void kunit_handle_shutdown(void)
> +{
> +       if (!kunit_shutdown)
> +               return;
> +
> +       if (!strcmp(kunit_shutdown, "poweroff"))
> +               kernel_power_off();
> +       else if (!strcmp(kunit_shutdown, "halt"))
> +               kernel_halt();
> +       else if (!strcmp(kunit_shutdown, "reboot"))
> +               kernel_restart(NULL);
> +
> +}
> +
>  int kunit_run_all_tests(void)
>  {
>         struct kunit_suite_set suite_set = {
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 5232a43737826..49698a168437a 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -740,6 +740,17 @@ static void kunit_module_init(struct module *mod)
>                 mod->kunit_suites, mod->kunit_suites + mod->num_kunit_suites,
>         };
>         const char *action = kunit_action();
> +       int err = 0;
> +
> +       suite_set = kunit_filter_suites(&suite_set,
> +                                       kunit_filter_glob() ?: "*.*",

I've just learnt a new gcc extension! A bit scary, but it works on
clang as well, so I'm fine with it.


> +                                       kunit_filter(), kunit_filter_action(),
> +                                       &err);
> +       if (err)
> +               pr_err("kunit module: error filtering suites: %d\n", err);
> +
> +       mod->kunit_suites = (struct kunit_suite **)suite_set.start;
> +       mod->num_kunit_suites = suite_set.end - suite_set.start;
>
>         if (!action)
>                 kunit_exec_run_tests(&suite_set, false);
> @@ -753,11 +764,17 @@ static void kunit_module_init(struct module *mod)
>
>  static void kunit_module_exit(struct module *mod)
>  {
> +       struct kunit_suite_set suite_set = {
> +               mod->kunit_suites, mod->kunit_suites + mod->num_kunit_suites,
> +       };
>         const char *action = kunit_action();
>
>         if (!action)
>                 __kunit_test_suites_exit(mod->kunit_suites,
>                                          mod->num_kunit_suites);
> +
> +       if (suite_set.start)
> +               kunit_free_suite_set(suite_set);
>  }
>
>  static int kunit_module_notify(struct notifier_block *nb, unsigned long val,
> --
> 2.41.0
>

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature