On Wed, Oct 13, 2021 at 1:04 PM Brendan Higgins <brendanhiggins@xxxxxxxxxx> wrote: > > On Wed, Oct 13, 2021 at 12:13 PM Daniel Latypov <dlatypov@xxxxxxxxxx> wrote: > > > > Per [1], we might not need the array-of-array of kunit_suite's. > > > > This RFC patch previews the changes we'd make to the executor to > > accommodate that by making the executor automatically flatten the > > kunit_suite*** into a kunit_suite**. > > > > The test filtering support [2] added the largest dependency on the > > current kunit_suite*** layout, so this patch is based on that. > > > > It actually drastically simplifies the code, so it might be useful to > > keep the auto-flattening step until we actually make the change. > > > > [1] https://lore.kernel.org/linux-kselftest/101d12fc9250b7a445ff50a9e7a25cd74d0e16eb.camel@xxxxxxxxxxxxxxxxxxxx/ > > [2] https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=kunit&id=3b29021ddd10cfb6b2565c623595bd3b02036f33 > > > > Cc: Jeremy Kerr <jk@xxxxxxxxxxxxxxxxxxxx> > > Signed-off-by: Daniel Latypov <dlatypov@xxxxxxxxxx> > > I like it! This seems to make a lot of logic simpler (and from the > sounds makes Jeremy's proposed module patch easier?). This would make the subsequent cleanup easier. Jemery, correct me if I misread, but it sounded like you were thinking of leaving the current kunit_suite*** layout until afterwards. Then we'd flatten it as a cleanup. This patch contains the changes necessary to fix compilation and the executor unit test for that new layout. > > > --- > > lib/kunit/executor.c | 132 +++++++++++++++----------------------- > > lib/kunit/executor_test.c | 131 ++++++++++--------------------------- > > 2 files changed, 85 insertions(+), 178 deletions(-) > > > > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c > > index 22640c9ee819..3a7246336625 100644 > > --- a/lib/kunit/executor.c > > +++ b/lib/kunit/executor.c > > @@ -88,60 +88,18 @@ kunit_filter_tests(struct kunit_suite *const suite, const char *test_glob) > > static char *kunit_shutdown; > > core_param(kunit_shutdown, kunit_shutdown, charp, 0644); > > > > -static struct kunit_suite * const * > > -kunit_filter_subsuite(struct kunit_suite * const * const subsuite, > > - struct kunit_test_filter *filter) > > -{ > > - int i, n = 0; > > - struct kunit_suite **filtered, *filtered_suite; > > - > > - n = 0; > > - for (i = 0; subsuite[i]; ++i) { > > - if (glob_match(filter->suite_glob, subsuite[i]->name)) > > - ++n; > > - } > > - > > - if (n == 0) > > - return NULL; > > - > > - filtered = kmalloc_array(n + 1, sizeof(*filtered), GFP_KERNEL); > > - if (!filtered) > > - return NULL; > > - > > - n = 0; > > - for (i = 0; subsuite[i] != NULL; ++i) { > > - if (!glob_match(filter->suite_glob, subsuite[i]->name)) > > - continue; > > - filtered_suite = kunit_filter_tests(subsuite[i], filter->test_glob); > > - if (filtered_suite) > > - filtered[n++] = filtered_suite; > > - } > > - filtered[n] = NULL; > > - > > - return filtered; > > -} > > - > > +/* Stores a NULL-terminated array of suites. */ > > struct suite_set { > > - struct kunit_suite * const * const *start; > > - struct kunit_suite * const * const *end; > > + struct kunit_suite * const *start; > > + struct kunit_suite * const *end; > > }; > > > > -static void kunit_free_subsuite(struct kunit_suite * const *subsuite) > > -{ > > - unsigned int i; > > - > > - for (i = 0; subsuite[i]; i++) > > - kfree(subsuite[i]); > > - > > - kfree(subsuite); > > -} > > - > > static void kunit_free_suite_set(struct suite_set suite_set) > > { > > - struct kunit_suite * const * const *suites; > > + struct kunit_suite * const *suites; > > > > for (suites = suite_set.start; suites < suite_set.end; suites++) > > - kunit_free_subsuite(*suites); > > + kfree(*suites); > > kfree(suite_set.start); > > } > > > > @@ -149,10 +107,11 @@ static struct suite_set kunit_filter_suites(const struct suite_set *suite_set, > > const char *filter_glob) > > { > > int i; > > - struct kunit_suite * const **copy, * const *filtered_subsuite; > > + struct kunit_suite **copy, *filtered_suite; > > struct suite_set filtered; > > struct kunit_test_filter filter; > > > > + /* Note: this includes space for the terminating NULL. */ > > const size_t max = suite_set->end - suite_set->start; > > > > copy = kmalloc_array(max, sizeof(*filtered.start), GFP_KERNEL); > > @@ -164,11 +123,17 @@ static struct suite_set kunit_filter_suites(const struct suite_set *suite_set, > > > > kunit_parse_filter_glob(&filter, filter_glob); > > > > - for (i = 0; i < max; ++i) { > > - filtered_subsuite = kunit_filter_subsuite(suite_set->start[i], &filter); > > - if (filtered_subsuite) > > - *copy++ = filtered_subsuite; > > + for (i = 0; suite_set->start[i] != NULL; i++) { > > + if (!glob_match(filter.suite_glob, suite_set->start[i]->name)) > > + continue; > > + > > + filtered_suite = kunit_filter_tests(suite_set->start[i], filter.test_glob); > > + if (!filtered_suite) > > + continue; > > + > > + *copy++ = filtered_suite; > > } > > + *copy = NULL; > > filtered.end = copy; > > > > kfree(filter.suite_glob); > > @@ -190,52 +155,56 @@ static void kunit_handle_shutdown(void) > > > > } > > > > -static void kunit_print_tap_header(struct suite_set *suite_set) > > -{ > > - struct kunit_suite * const * const *suites, * const *subsuite; > > - int num_of_suites = 0; > > - > > - for (suites = suite_set->start; suites < suite_set->end; suites++) > > - for (subsuite = *suites; *subsuite != NULL; subsuite++) > > - num_of_suites++; > > - > > - pr_info("TAP version 14\n"); > > - pr_info("1..%d\n", num_of_suites); > > -} > > - > > static void kunit_exec_run_tests(struct suite_set *suite_set) > > { > > - struct kunit_suite * const * const *suites; > > - > > - kunit_print_tap_header(suite_set); > > + pr_info("TAP version 14\n"); > > + pr_info("1..%zu\n", suite_set->end - suite_set->start); > > > > - for (suites = suite_set->start; suites < suite_set->end; suites++) > > - __kunit_test_suites_init(*suites); > > + __kunit_test_suites_init(suite_set->start); > > } > > > > static void kunit_exec_list_tests(struct suite_set *suite_set) > > { > > - unsigned int i; > > - struct kunit_suite * const * const *suites; > > + struct kunit_suite * const *suites; > > struct kunit_case *test_case; > > > > /* Hack: print a tap header so kunit.py can find the start of KUnit output. */ > > pr_info("TAP version 14\n"); > > > > for (suites = suite_set->start; suites < suite_set->end; suites++) > > - for (i = 0; (*suites)[i] != NULL; i++) { > > - kunit_suite_for_each_test_case((*suites)[i], test_case) { > > - pr_info("%s.%s\n", (*suites)[i]->name, test_case->name); > > - } > > + kunit_suite_for_each_test_case((*suites), test_case) { > > + pr_info("%s.%s\n", (*suites)->name, test_case->name); > > } > > } > > > > +// TODO(dlatypov@xxxxxxxxxx): delete this when we store suites in a single array. > > +static struct suite_set make_suite_set(void) > > +{ > > + struct suite_set flattened; > > + size_t num_of_suites = 0; > > + > > + struct kunit_suite * const * const *suites, * const *subsuite; > > + struct kunit_suite **end; > > + > > + for (suites = __kunit_suites_start; suites < __kunit_suites_end; suites++) > > + for (subsuite = *suites; *subsuite != NULL; subsuite++) > > + num_of_suites++; > > + > > + end = kcalloc(num_of_suites + 1, sizeof(*flattened.start), GFP_KERNEL); > > + flattened.start = end; > > + > > + for (suites = __kunit_suites_start; suites < __kunit_suites_end; suites++) > > + for (subsuite = *suites; *subsuite != NULL; subsuite++) > > + *end++ = *subsuite; > > + *end = NULL; > > + flattened.end = end; > > + return flattened; > > +} > > + > > int kunit_run_all_tests(void) > > { > > - struct suite_set suite_set = { > > - .start = __kunit_suites_start, > > - .end = __kunit_suites_end, > > - }; > > + struct suite_set suite_set = make_suite_set(); > > + struct kunit_suite * const *unfiltered = suite_set.start; /* need to free at end */ > > > > if (filter_glob_param) > > suite_set = kunit_filter_suites(&suite_set, filter_glob_param); > > @@ -247,9 +216,10 @@ int kunit_run_all_tests(void) > > else > > pr_err("kunit executor: unknown action '%s'\n", action_param); > > > > - if (filter_glob_param) { /* a copy was made of each array */ > > + if (filter_glob_param) { /* a copy was made of each suite */ > > kunit_free_suite_set(suite_set); > > } > > + kfree(unfiltered); > > > > kunit_handle_shutdown(); > > > > diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c > > index 7d2b8dc668b1..d9fce637eb56 100644 > > --- a/lib/kunit/executor_test.c > > +++ b/lib/kunit/executor_test.c > > @@ -9,8 +9,6 @@ > > #include <kunit/test.h> > > > > static void kfree_at_end(struct kunit *test, const void *to_free); > > -static void free_subsuite_at_end(struct kunit *test, > > - struct kunit_suite *const *to_free); > > static struct kunit_suite *alloc_fake_suite(struct kunit *test, > > const char *suite_name, > > struct kunit_case *test_cases); > > @@ -41,124 +39,77 @@ static void parse_filter_test(struct kunit *test) > > kfree(filter.test_glob); > > } > > > > -static void filter_subsuite_test(struct kunit *test) > > +static void filter_suites_test(struct kunit *test) > > { > > struct kunit_suite *subsuite[3] = {NULL, NULL, NULL}; > > - struct kunit_suite * const *filtered; > > - struct kunit_test_filter filter = { > > - .suite_glob = "suite2", > > - .test_glob = NULL, > > - }; > > + struct suite_set suite_set = {.start = subsuite, .end = &subsuite[2]}; > > + struct suite_set got; > > > > subsuite[0] = alloc_fake_suite(test, "suite1", dummy_test_cases); > > subsuite[1] = alloc_fake_suite(test, "suite2", dummy_test_cases); > > > > /* Want: suite1, suite2, NULL -> suite2, NULL */ > > - filtered = kunit_filter_subsuite(subsuite, &filter); > > - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filtered); > > - free_subsuite_at_end(test, filtered); > > + got = kunit_filter_suites(&suite_set, "suite2"); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start); > > + kfree_at_end(test, got.start); > > > > /* Validate we just have suite2 */ > > - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filtered[0]); > > - KUNIT_EXPECT_STREQ(test, (const char *)filtered[0]->name, "suite2"); > > - KUNIT_EXPECT_FALSE(test, filtered[1]); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]); > > + KUNIT_EXPECT_STREQ(test, (const char *)got.start[0]->name, "suite2"); > > + // DO NOT SUBMIT: null-terminated for now. > > Can you elaborate what you mean here? This change makes the array null-terminated for __kunit_test_suites_init to work. E.g. we allocate one more element in the suite_set's array and have to remember to allocate one extra element and write NULL to it $ git show HEAD | grep -P '^\+\s+\*.*= NULL;' + *copy = NULL; + *end = NULL; But since we'd always be tracking how long our array is, we don't actually need that. We could theoretically pass a start/end (or a suite_set) into __kunit_test_suites_init() instead. It's currently unsigned int i; for (i = 0; suites[i] != NULL; i++) { kunit_init_suite(suites[i]); kunit_run_tests(suites[i]); } It could just become // pass in `end` somehow for (; suites < end; ++suites) { kunit_init_suite(*suites); kunit_run_tests(*suites); } Dropping the null-termination would just make the logic for making a filtered copy a bit less subtle. > > > + KUNIT_ASSERT_EQ(test, got.end - got.start, 1); > > + KUNIT_EXPECT_FALSE(test, *got.end); > > } > > > > -static void filter_subsuite_test_glob_test(struct kunit *test) > > +static void filter_suites_test_glob_test(struct kunit *test) > > { > > struct kunit_suite *subsuite[3] = {NULL, NULL, NULL}; > > - struct kunit_suite * const *filtered; > > - struct kunit_test_filter filter = { > > - .suite_glob = "suite2", > > - .test_glob = "test2", > > - }; > > + struct suite_set suite_set = {.start = subsuite, .end = &subsuite[2]}; > > + struct suite_set got; > > > > subsuite[0] = alloc_fake_suite(test, "suite1", dummy_test_cases); > > subsuite[1] = alloc_fake_suite(test, "suite2", dummy_test_cases); > > > > /* Want: suite1, suite2, NULL -> suite2 (just test1), NULL */ > > - filtered = kunit_filter_subsuite(subsuite, &filter); > > - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filtered); > > - free_subsuite_at_end(test, filtered); > > + got = kunit_filter_suites(&suite_set, "suite2.test2"); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start); > > + kfree_at_end(test, got.start); > > > > /* Validate we just have suite2 */ > > - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filtered[0]); > > - KUNIT_EXPECT_STREQ(test, (const char *)filtered[0]->name, "suite2"); > > - KUNIT_EXPECT_FALSE(test, filtered[1]); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]); > > + KUNIT_EXPECT_STREQ(test, (const char *)got.start[0]->name, "suite2"); > > + // DO NOT SUBMIT: null-terminated for now. > > + KUNIT_ASSERT_EQ(test, got.end - got.start, 1); > > + KUNIT_EXPECT_FALSE(test, *got.end); > > > > /* Now validate we just have test2 */ > > - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filtered[0]->test_cases); > > - KUNIT_EXPECT_STREQ(test, (const char *)filtered[0]->test_cases[0].name, "test2"); > > - KUNIT_EXPECT_FALSE(test, filtered[0]->test_cases[1].name); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]->test_cases); > > + KUNIT_EXPECT_STREQ(test, (const char *)got.start[0]->test_cases[0].name, "test2"); > > + KUNIT_EXPECT_FALSE(test, got.start[0]->test_cases[1].name); > > } > > > > -static void filter_subsuite_to_empty_test(struct kunit *test) > > +static void filter_suites_to_empty_test(struct kunit *test) > > { > > struct kunit_suite *subsuite[3] = {NULL, NULL, NULL}; > > - struct kunit_suite * const *filtered; > > - struct kunit_test_filter filter = { > > - .suite_glob = "not_found", > > - .test_glob = NULL, > > - }; > > + struct suite_set suite_set = {.start = subsuite, .end = &subsuite[2]}; > > + struct suite_set got; > > > > subsuite[0] = alloc_fake_suite(test, "suite1", dummy_test_cases); > > subsuite[1] = alloc_fake_suite(test, "suite2", dummy_test_cases); > > > > - filtered = kunit_filter_subsuite(subsuite, &filter); > > - free_subsuite_at_end(test, filtered); /* just in case */ > > - > > - KUNIT_EXPECT_FALSE_MSG(test, filtered, > > - "should be NULL to indicate no match"); > > -} > > - > > -static void kfree_subsuites_at_end(struct kunit *test, struct suite_set *suite_set) > > -{ > > - struct kunit_suite * const * const *suites; > > + got = kunit_filter_suites(&suite_set, "not_found"); > > + kfree_at_end(test, got.start); /* just in case */ > > > > - kfree_at_end(test, suite_set->start); > > - for (suites = suite_set->start; suites < suite_set->end; suites++) > > - free_subsuite_at_end(test, *suites); > > + KUNIT_EXPECT_PTR_EQ_MSG(test, got.start, got.end, > > + "should be empty to indicate no match"); > > } > > > > -static void filter_suites_test(struct kunit *test) > > -{ > > - /* Suites per-file are stored as a NULL terminated array */ > > - struct kunit_suite *subsuites[2][2] = { > > - {NULL, NULL}, > > - {NULL, NULL}, > > - }; > > - /* Match the memory layout of suite_set */ > > - struct kunit_suite * const * const suites[2] = { > > - subsuites[0], subsuites[1], > > - }; > > - > > - const struct suite_set suite_set = { > > - .start = suites, > > - .end = suites + 2, > > - }; > > - struct suite_set filtered = {.start = NULL, .end = NULL}; > > - > > - /* Emulate two files, each having one suite */ > > - subsuites[0][0] = alloc_fake_suite(test, "suite0", dummy_test_cases); > > - subsuites[1][0] = alloc_fake_suite(test, "suite1", dummy_test_cases); > > - > > - /* Filter out suite1 */ > > - filtered = kunit_filter_suites(&suite_set, "suite0"); > > - kfree_subsuites_at_end(test, &filtered); /* let us use ASSERTs without leaking */ > > - KUNIT_ASSERT_EQ(test, filtered.end - filtered.start, (ptrdiff_t)1); > > - > > - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filtered.start); > > - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filtered.start[0]); > > - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filtered.start[0][0]); > > - KUNIT_EXPECT_STREQ(test, (const char *)filtered.start[0][0]->name, "suite0"); > > -} > > > > static struct kunit_case executor_test_cases[] = { > > KUNIT_CASE(parse_filter_test), > > - KUNIT_CASE(filter_subsuite_test), > > - KUNIT_CASE(filter_subsuite_test_glob_test), > > - KUNIT_CASE(filter_subsuite_to_empty_test), > > KUNIT_CASE(filter_suites_test), > > + KUNIT_CASE(filter_suites_test_glob_test), > > + KUNIT_CASE(filter_suites_to_empty_test), > > {} > > }; > > > > @@ -188,20 +139,6 @@ static void kfree_at_end(struct kunit *test, const void *to_free) > > (void *)to_free); > > } > > > > -static void free_subsuite_res_free(struct kunit_resource *res) > > -{ > > - kunit_free_subsuite(res->data); > > -} > > - > > -static void free_subsuite_at_end(struct kunit *test, > > - struct kunit_suite *const *to_free) > > -{ > > - if (IS_ERR_OR_NULL(to_free)) > > - return; > > - kunit_alloc_resource(test, NULL, free_subsuite_res_free, > > - GFP_KERNEL, (void *)to_free); > > -} > > - > > static struct kunit_suite *alloc_fake_suite(struct kunit *test, > > const char *suite_name, > > struct kunit_case *test_cases) > > > > base-commit: e7198adb84dcad671ad4f0e90aaa7e9fabf258dc > > -- > > 2.33.0.882.g93a45727a2-goog > >
