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

On Tue, 5 Dec 2023 at 06:19, Rae Moar rmoar@google.com wrote:

Add example_init_test_suite to allow for testing the feature of running test suites marked as init to indicate they use init data and/or functions.

This suite should always pass and uses a simple init function.

This suite can also be used to test the is_init attribute introduced in the next patch.

Signed-off-by: Rae Moar rmoar@google.com

Can we make the actual test function __init as well? I don't think it should be _necessary_ for this to work, but I'd like us to have the option of entire tests being marked __init, and so discarded after boot.

This seems to work here if the test is marked __init, and the example_init_test_cases and example_init_test_suite are marked __initdata.

But, as mentioned in the previous patch, that might not work if we need to ensure example_init_test_suite and other metadata still exists. So maybe we can document that here?

Otherwise, looks good.

Reviewed-by: David Gow davidgow@google.com

Cheers, -- David

lib/kunit/kunit-example-test.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)

diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c index 6bb5c2ef6696..262a68a59800 100644 --- a/lib/kunit/kunit-example-test.c +++ b/lib/kunit/kunit-example-test.c @@ -287,4 +287,33 @@ static struct kunit_suite example_test_suite = { */ kunit_test_suites(&example_test_suite);

+static int __init init_add(int x, int y) +{

•   return (x + y);
  

+}

+/*

• • This test should always pass. Can be used to test init suites.
• */

+static void example_init_test(struct kunit *test) +{

•   KUNIT_EXPECT_EQ(test, init_add(1, 1), 2);
  

+}

+static struct kunit_case example_init_test_cases[] = {

•   KUNIT_CASE(example_init_test),
  

•   {}
  

+};

+static struct kunit_suite example_init_test_suite = {

•   .name = "example_init",
  

•   .test_cases = example_init_test_cases,
  

+};

+/*

• • This registers the test suite and marks the suite as using init data
• • and/or functions.
• */

+kunit_test_init_section_suites(&example_init_test_suite);

MODULE_LICENSE("GPL v2");

2.43.0.rc2.451.g8631bc7472-goog

On Sat, Dec 9, 2023 at 2:58 AM David Gow davidgow@google.com wrote:

On Tue, 5 Dec 2023 at 06:19, Rae Moar rmoar@google.com wrote:

...

Expand the documentation on the KUnit debugfs filesystem on the run_manual.rst page.

Add section describing how to access results using debugfs.

Add section describing how to run tests after boot using debugfs.

Signed-off-by: Rae Moar rmoar@google.com

Looks good to me, some nitpicks below.

The other thing I'd really want to add is some documentation on writing init-section suites, which covers the pitfalls better (as mentioned in the previous emails). Though that could be a separate patch if you want to keep this one debugfs-specific.

Otherwise, Reviewed-by: David Gow davidgow@google.com

Cheers, -- David

Hello!

I have responded to your comments below. I would also be happy to add documentation to the init-section suites either in this patch series or in a future one.

Thanks! -Rae

...

Changes since v2:

• Add info to documentation about cleaning up data, init tests, and running tests concurrently

Documentation/dev-tools/kunit/run_manual.rst | 49 ++++++++++++++++++-- 1 file changed, 45 insertions(+), 4 deletions(-)

diff --git a/Documentation/dev-tools/kunit/run_manual.rst b/Documentation/dev-tools/kunit/run_manual.rst index e7b46421f247..aebb52ba9605 100644 --- a/Documentation/dev-tools/kunit/run_manual.rst +++ b/Documentation/dev-tools/kunit/run_manual.rst @@ -49,9 +49,50 @@ loaded.

The results will appear in TAP format in ``dmesg``.

+debugfs +=======

+``debugfs`` is a file system that enables user interaction with the files to +make kernel information available to user space (See more information at +Documentation/filesystems/debugfs.html)

Nit: reference debugfs.rst here, not debugfs.html -- sphinx ought to update the link automatically.

Thanks for catching this! I didn't realize Sphinx would update it.

Also, maybe we can make this introduction a _little_ bit more KUnit-specific. I'd personally start by saying that KUnit can be accessed from userspace via the debugfs filesystem (link), usually mounted in /sys/kernel/debug/kunit, etc, if CONFIG_KUNIT_DEBUGFS is enabled.

Ok I will add this for the next version.

...

+By default, only the root user has access to the debugfs directory.

+If ``CONFIG_KUNIT_DEBUGFS`` is enabled, you can use KUnit debugfs +filesystem to perform the following actions.

+Retrieve Test Results +=====================

+You can use debugfs to retrieve KUnit test results. The test results are +accessible from the debugfs filesystem in the following read-only file:

+.. code-block :: bash

•   /sys/kernel/debug/kunit/<test_suite>/results
  

+The test results are available in KTAP format.

Do we want to note that this is a separate KTAP document, and so may have different suite numbering from the dmesg log?

Sure! I will add this for the next version.

...

+Run Tests After Kernel Has Booted +=================================

+You can use the debugfs filesystem to trigger built-in tests to run after +boot. To run the test suite, you can use the following command to write to +the ``/sys/kernel/debug/kunit/<test_suite>/run`` file:

+.. code-block :: bash

•   echo "any string" > /sys/kernel/debugfs/kunit/<test_suite>/run
  

+As a result, the test suite runs and the results are printed to the kernel +log.

+However, this feature is not available with KUnit tests that use init data.

Let's expand this slightly, and mention that this is because the data may have already been discarded, and that you can find such tests by either looking for the kunit_test_init_section_suites() macro or the is_init attribute.

Got it. I will definitely expand this more.

...

+Also, you cannot use this feature to run tests concurrently as there is a +mutex lock around running KUnit tests at the same time.

Instead of mentioning the mutex, which is an implementation detail, just mention that tests will either wait for other tests to complete, or fail having timed out.

I will definitely switch this out in the next version.

...

.. note ::

•   If ``CONFIG_KUNIT_DEBUGFS`` is enabled, KUnit test results will
  

•   be accessible from the ``debugfs`` filesystem (if mounted).
  

•   They will be in ``/sys/kernel/debug/kunit/<test_suite>/results``, in
  

•   TAP format.
  

•   For test authors, to use this feature, tests will need to correctly initialise
  

•   and/or clean up any data, so the test runs correctly a second time.
  

-- 2.43.0.rc2.451.g8631bc7472-goog

On Sat, Dec 9, 2023 at 2:48 AM David Gow davidgow@google.com wrote:

On Tue, 5 Dec 2023 at 06:19, Rae Moar rmoar@google.com wrote:

...

Alter the linker section of KUNIT_TABLE to move it out of INIT_DATA and into DATA_DATA.

Data for KUnit tests does not need to be in the init section.

In order to run tests again after boot the KUnit data cannot be labeled as init data as the kernel could write over it.

Add a KUNIT_INIT_TABLE in the next patch for KUnit tests that test init data/functions.

Signed-off-by: Rae Moar rmoar@google.com

I think this actually fixes a potential bug, as we loop through the list of suites after init has ended in the debugfs logic.

So maybe this is: Fixes: 90a025a859a3 ("vmlinux.lds.h: add linker section for KUnit test suites")

Regardless, I'd love to get this in, even if we don't manage to get the rest of the series in soon.

Reviewed-by: David Gow davidgow@google.com

Cheers, -- David

Hello!

Thanks for reviewing! I will be adding this fixes tag. Should I make this a separate patch for the next version?

Thanks! -Rae

...

include/asm-generic/vmlinux.lds.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index bae0fe4d499b..1107905d37fc 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -370,7 +370,8 @@ BRANCH_PROFILE() \ TRACE_PRINTKS() \ BPF_RAW_TP() \

•   TRACEPOINT_STR()
  

•   TRACEPOINT_STR()                                                \
  

•   KUNIT_TABLE()
  
  

/*

• Data section helpers

@@ -699,8 +700,7 @@ THERMAL_TABLE(governor) \ EARLYCON_TABLE() \ LSM_TABLE() \

•   EARLY_LSM_TABLE()                                               \
  

•   KUNIT_TABLE()
  

•   EARLY_LSM_TABLE()
  
  

#define INIT_TEXT \ *(.init.text .init.text.*) \

base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86

2.43.0.rc2.451.g8631bc7472-goog