[v3,2/6] kunit: add KUNIT_INIT_TABLE to init linker section

Add KUNIT_INIT_TABLE to the INIT_DATA linker section.

Alter the KUnit macros to create init tests:
kunit_test_init_section_suites

Update lib/kunit/executor.c to run both the suites in KUNIT_TABLE and
KUNIT_INIT_TABLE.

Signed-off-by: Rae Moar <rmoar@google.com>
---
 include/asm-generic/vmlinux.lds.h |  9 ++++-
 include/kunit/test.h              | 10 ++++-
 include/linux/module.h            |  2 +
 kernel/module/main.c              |  3 ++
 lib/kunit/executor.c              | 64 ++++++++++++++++++++++++++++---
 lib/kunit/test.c                  | 26 +++++++++----
 6 files changed, 99 insertions(+), 15 deletions(-)

On Tue, 5 Dec 2023 at 06:19, Rae Moar <rmoar@google.com> wrote:
>
> Add KUNIT_INIT_TABLE to the INIT_DATA linker section.
>
> Alter the KUnit macros to create init tests:
> kunit_test_init_section_suites
>
> Update lib/kunit/executor.c to run both the suites in KUNIT_TABLE and
> KUNIT_INIT_TABLE.
>
> Signed-off-by: Rae Moar <rmoar@google.com>
> ---

This works well here.

I'm still a little bit conflicted around the idea of merging suite
sets at runtime -- I think there could be more efficient ways of
handling that -- though the more I think about it, the less worried
I'm getting (since we'll need to keep init suites around somewhere for
debugfs, anyway, right?).

In fact, that's something we probably need to work out -- is it legal
for the actual kunit_test_suite struct to be __initdata? I'd thought
so, but if we need to loop over these later in debugfs to keep their
logs, then probably not. Unless you wanted to make a copy of the
kunit_suite itself, not just the pointers to it (though that seems
excessive).

If we're settled on that (the suite itself can't be __initdata), then this is:
Reviewed-by: David Gow <davidgow@google.com>

-- David

>  include/asm-generic/vmlinux.lds.h |  9 ++++-
>  include/kunit/test.h              | 10 ++++-
>  include/linux/module.h            |  2 +
>  kernel/module/main.c              |  3 ++
>  lib/kunit/executor.c              | 64 ++++++++++++++++++++++++++++---
>  lib/kunit/test.c                  | 26 +++++++++----
>  6 files changed, 99 insertions(+), 15 deletions(-)
>
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 1107905d37fc..5dd3a61d673d 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -700,7 +700,8 @@
>         THERMAL_TABLE(governor)                                         \
>         EARLYCON_TABLE()                                                \
>         LSM_TABLE()                                                     \
> -       EARLY_LSM_TABLE()
> +       EARLY_LSM_TABLE()                                               \
> +       KUNIT_INIT_TABLE()
>
>  #define INIT_TEXT                                                      \
>         *(.init.text .init.text.*)                                      \
> @@ -926,6 +927,12 @@
>                 . = ALIGN(8);                                           \
>                 BOUNDED_SECTION_POST_LABEL(.kunit_test_suites, __kunit_suites, _start, _end)
>
> +/* Alignment must be consistent with (kunit_suite *) in include/kunit/test.h */
> +#define KUNIT_INIT_TABLE()                                             \
> +               . = ALIGN(8);                                           \

I still hate that we hardcode '8' here, but I guess we've got no
choice in a linker script.


> +               BOUNDED_SECTION_POST_LABEL(.kunit_init_test_suites, \
> +                               __kunit_init_suites, _start, _end)
> +
>  #ifdef CONFIG_BLK_DEV_INITRD
>  #define INIT_RAM_FS                                                    \
>         . = ALIGN(4);                                                   \
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 20ed9f9275c9..06e826a0b894 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -337,6 +337,9 @@ void __kunit_test_suites_exit(struct kunit_suite **suites, int num_suites);
>  void kunit_exec_run_tests(struct kunit_suite_set *suite_set, bool builtin);
>  void kunit_exec_list_tests(struct kunit_suite_set *suite_set, bool include_attr);
>
> +struct kunit_suite_set kunit_merge_suite_sets(struct kunit_suite_set init_suite_set,
> +               struct kunit_suite_set suite_set);
> +
>  #if IS_BUILTIN(CONFIG_KUNIT)
>  int kunit_run_all_tests(void);
>  #else
> @@ -371,6 +374,11 @@ static inline int kunit_run_all_tests(void)
>
>  #define kunit_test_suite(suite)        kunit_test_suites(&suite)
>
> +#define __kunit_init_test_suites(unique_array, ...)                           \
> +       static struct kunit_suite *unique_array[]                              \
> +       __aligned(sizeof(struct kunit_suite *))                                \
> +       __used __section(".kunit_init_test_suites") = { __VA_ARGS__ }
> +
>  /**
>   * kunit_test_init_section_suites() - used to register one or more &struct
>   *                                   kunit_suite containing init functions or
> @@ -392,7 +400,7 @@ static inline int kunit_run_all_tests(void)
>   * this manner.
>   */
>  #define kunit_test_init_section_suites(__suites...)                    \
> -       __kunit_test_suites(CONCATENATE(__UNIQUE_ID(array), _probe),    \
> +       __kunit_init_test_suites(__UNIQUE_ID(array),                    \
>                             ##__suites)
>
>  #define kunit_test_init_section_suite(suite)   \
> diff --git a/include/linux/module.h b/include/linux/module.h
> index a98e188cf37b..9cd0009bd050 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -540,6 +540,8 @@ struct module {
>         struct static_call_site *static_call_sites;
>  #endif
>  #if IS_ENABLED(CONFIG_KUNIT)
> +       int num_kunit_init_suites;
> +       struct kunit_suite **kunit_init_suites;
>         int num_kunit_suites;
>         struct kunit_suite **kunit_suites;
>  #endif
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 98fedfdb8db5..36681911c05a 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2199,6 +2199,9 @@ static int find_module_sections(struct module *mod, struct load_info *info)
>         mod->kunit_suites = section_objs(info, ".kunit_test_suites",
>                                               sizeof(*mod->kunit_suites),
>                                               &mod->num_kunit_suites);
> +       mod->kunit_init_suites = section_objs(info, ".kunit_init_test_suites",
> +                                             sizeof(*mod->kunit_init_suites),
> +                                             &mod->num_kunit_init_suites);
>  #endif
>
>         mod->extable = section_objs(info, "__ex_table",
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index 1236b3cd2fbb..847329c51e91 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -12,6 +12,8 @@
>   */
>  extern struct kunit_suite * const __kunit_suites_start[];
>  extern struct kunit_suite * const __kunit_suites_end[];
> +extern struct kunit_suite * const __kunit_init_suites_start[];
> +extern struct kunit_suite * const __kunit_init_suites_end[];
>
>  static char *action_param;
>
> @@ -292,6 +294,33 @@ void kunit_exec_list_tests(struct kunit_suite_set *suite_set, bool include_attr)
>         }
>  }
>
> +struct kunit_suite_set kunit_merge_suite_sets(struct kunit_suite_set init_suite_set,
> +               struct kunit_suite_set suite_set)
> +{
> +       struct kunit_suite_set total_suite_set = {NULL, NULL};
> +       struct kunit_suite **total_suite_start = NULL;
> +       size_t init_num_suites, num_suites, suite_size;
> +
> +       init_num_suites = init_suite_set.end - init_suite_set.start;
> +       num_suites = suite_set.end - suite_set.start;
> +       suite_size = sizeof(suite_set.start);
> +
> +       /* Allocate memory for array of all kunit suites */
> +       total_suite_start = kmalloc_array(init_num_suites + num_suites, suite_size, GFP_KERNEL);
> +       if (!total_suite_start)
> +               return total_suite_set;
> +
> +       /* Append init suites and then all other kunit suites */
> +       memcpy(total_suite_start, init_suite_set.start, init_num_suites * suite_size);
> +       memcpy(total_suite_start + init_num_suites, suite_set.start, num_suites * suite_size);
> +
> +       /* Set kunit suite set start and end */
> +       total_suite_set.start = total_suite_start;
> +       total_suite_set.end = total_suite_start + (init_num_suites + num_suites);
> +
> +       return total_suite_set;
> +}
> +
>  #if IS_BUILTIN(CONFIG_KUNIT)
>
>  static char *kunit_shutdown;
> @@ -313,21 +342,41 @@ static void kunit_handle_shutdown(void)
>
>  int kunit_run_all_tests(void)
>  {
> -       struct kunit_suite_set suite_set = {
> +       struct kunit_suite_set suite_set = {NULL, NULL};
> +       struct kunit_suite_set filtered_suite_set = {NULL, NULL};
> +       struct kunit_suite_set init_suite_set = {
> +               __kunit_init_suites_start, __kunit_init_suites_end,
> +       };
> +       struct kunit_suite_set normal_suite_set = {
>                 __kunit_suites_start, __kunit_suites_end,
>         };
> +       size_t init_num_suites = init_suite_set.end - init_suite_set.start;
>         int err = 0;
> +
> +       if (init_num_suites > 0) {
> +               suite_set = kunit_merge_suite_sets(init_suite_set, normal_suite_set);
> +               if (!suite_set.start)
> +                       goto out;
> +       } else
> +               suite_set = normal_suite_set;
> +
>         if (!kunit_enabled()) {
>                 pr_info("kunit: disabled\n");
> -               goto out;
> +               goto free_out;
>         }
>
>         if (filter_glob_param || filter_param) {
> -               suite_set = kunit_filter_suites(&suite_set, filter_glob_param,
> +               filtered_suite_set = kunit_filter_suites(&suite_set, filter_glob_param,
>                                 filter_param, filter_action_param, &err);
> +
> +               /* Free original suite set before using filtered suite set */
> +               if (init_num_suites > 0)
> +                       kfree(suite_set.start);
> +               suite_set = filtered_suite_set;
> +
>                 if (err) {
>                         pr_err("kunit executor: error filtering suites: %d\n", err);
> -                       goto out;
> +                       goto free_out;
>                 }
>         }
>
> @@ -340,9 +389,12 @@ int kunit_run_all_tests(void)
>         else
>                 pr_err("kunit executor: unknown action '%s'\n", action_param);
>
> -       if (filter_glob_param || filter_param) { /* a copy was made of each suite */
> +free_out:
> +       if (filter_glob_param || filter_param)
>                 kunit_free_suite_set(suite_set);
> -       }
> +       else if (init_num_suites > 0)
> +               /* Don't use kunit_free_suite_set because suites aren't individually allocated */
> +               kfree(suite_set.start);
>
>  out:
>         kunit_handle_shutdown();
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index f2eb71f1a66c..8bae6e2bc6a0 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -704,28 +704,40 @@ EXPORT_SYMBOL_GPL(__kunit_test_suites_exit);
>  #ifdef CONFIG_MODULES
>  static void kunit_module_init(struct module *mod)
>  {
> -       struct kunit_suite_set suite_set = {
> +       struct kunit_suite_set suite_set, filtered_set;
> +       struct kunit_suite_set normal_suite_set = {
>                 mod->kunit_suites, mod->kunit_suites + mod->num_kunit_suites,
>         };
> +       struct kunit_suite_set init_suite_set = {
> +               mod->kunit_init_suites, mod->kunit_init_suites + mod->num_kunit_init_suites,
> +       };
>         const char *action = kunit_action();
>         int err = 0;
>
> -       suite_set = kunit_filter_suites(&suite_set,
> +       if (mod->num_kunit_init_suites > 0)
> +               suite_set = kunit_merge_suite_sets(init_suite_set, normal_suite_set);
> +       else
> +               suite_set = normal_suite_set;
> +
> +       filtered_set = kunit_filter_suites(&suite_set,
>                                         kunit_filter_glob() ?: "*.*",
>                                         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;
> +       mod->kunit_suites = (struct kunit_suite **)filtered_set.start;
> +       mod->num_kunit_suites = filtered_set.end - filtered_set.start;
> +
> +       if (mod->num_kunit_init_suites > 0)
> +               kfree(suite_set.start);
>
>         if (!action)
> -               kunit_exec_run_tests(&suite_set, false);
> +               kunit_exec_run_tests(&filtered_set, false);
>         else if (!strcmp(action, "list"))
> -               kunit_exec_list_tests(&suite_set, false);
> +               kunit_exec_list_tests(&filtered_set, false);
>         else if (!strcmp(action, "list_attr"))
> -               kunit_exec_list_tests(&suite_set, true);
> +               kunit_exec_list_tests(&filtered_set, true);
>         else
>                 pr_err("kunit: unknown action '%s'\n", action);
>  }
> --
> 2.43.0.rc2.451.g8631bc7472-goog
>