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/101d12fc9250b7a445ff50a9e7a25cd74d0e... [2] https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/co...
Cc: Jeremy Kerr jk@codeconstruct.com.au Signed-off-by: Daniel Latypov dlatypov@google.com --- 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@google.com): 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. + 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
On Wed, Oct 13, 2021 at 12:13 PM Daniel Latypov dlatypov@google.com 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/101d12fc9250b7a445ff50a9e7a25cd74d0e... [2] https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/co...
Cc: Jeremy Kerr jk@codeconstruct.com.au Signed-off-by: Daniel Latypov dlatypov@google.com
I like it! This seems to make a lot of logic simpler (and from the sounds makes Jeremy's proposed module patch easier?).
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@google.com): 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?
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
On Wed, Oct 13, 2021 at 1:04 PM Brendan Higgins brendanhiggins@google.com wrote:
On Wed, Oct 13, 2021 at 12:13 PM Daniel Latypov dlatypov@google.com 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/101d12fc9250b7a445ff50a9e7a25cd74d0e... [2] https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/co...
Cc: Jeremy Kerr jk@codeconstruct.com.au Signed-off-by: Daniel Latypov dlatypov@google.com
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@google.com): 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
Hi Daniel,
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.
Yep, that was my rough plan. From the current situation:
An object (module or built-in) defines a:
kunit_test_suite(my_suite)
[or, equivalently, a kunit_test_suites(&my_suite). The only place that I can see where we add more than one suite through kunit_test_suites is the kunit code, so let's ignore that for now..]
That expands to:
static struct kunit_suite *unique_array[] = { &my_suite, NULL }; static struct kunit_suite **unique_suites __used __section(".kunit_test_suites") = unique_array;
[assume actual unique values for unique_array & unique_suites]
So the .kunit_test_suites section content ends up being: an array of pointers to (null-terminated) arrays of pointers to struct kunit_suite. But those latter arrays only have one entry anyway.
Once we're reading the suites out of the section, we have the section size too, so can determine the number of "things" (suites/arrays-of-suites/whatever) from that.
So, given we only have one entry in the array, we can probably drop the null-termination, and expand the kunit_test_suite() macro to:
kunit_test_suite(my_suite) ->
static struct kunit_suite *unique_array[] = { &my_suite }; kunit_test_suites_for_module(unique_array); static struct kunit_suite **unique_suites __used __section(".kunit_test_suites") = unique_array;
Then, the array becomes pretty pointless, so we can reduce that to:
kunit_test_suite(my_suite) ->
static struct kunit_suite *unique_array = &my_suite; kunit_test_suites_for_module(unique_array); static struct kunit_suite *unique_suites __used __section(".kunit_test_suites") = unique_array;
But then there's no need for the double pointers, so we could just do:
kunit_test_suite(my_suite) ->
static struct kunit_suite *unique_suite __used __section(".kunit_test_suites") = &my_suite;
Resulting in the .kunit_test_suites section just being a set of contiguous pointers to struct kunit_suite. We get the number of suites from the section size.
[We could go one step further and put the suites themselves in .kunit_test_suites, rather than the pointers. However, the downside there is that we'd need macros for the suite declarations, which isn't as clean]
Modules with multiple suites (currently: kunit itself) can just have multiple occurrences of kunit_test_suite(), as they no longer conflict, and we'd no longer need kunit_test_suites() at all.
That was my thinking, anyway. I think it probably makes sense to do that cleanup after the section patch, as that means we don't need any post-processing on the suites arrays.
Cheers,
Jeremy
On Wed, Oct 13, 2021 at 6:55 PM Jeremy Kerr jk@codeconstruct.com.au wrote:
Resulting in the .kunit_test_suites section just being a set of contiguous pointers to struct kunit_suite. We get the number of suites from the section size.
<snip>
That was my thinking, anyway. I think it probably makes sense to do that cleanup after the section patch, as that means we don't need any post-processing on the suites arrays.
To be honest, I'm actually tempted to pay the cost of postprocessing and proposing a change like this for real. Going from kunit_suite*** to ** shaves off a lot of code from the unit test and the filtering code path.
Specifically I'm thinking this can go into the kunit branch, https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/?h... Then when we have the series reworking modules, one of two things can happen. 1. if we get to kunit_suite** with null-terminated arrays, fixing the executor just means dropping the post-processing step. 2. If we get to kunit_suite* as mentioned above, then there'll be a bit more work, but not as much.
Alternatively, I can wait and send you an updated version of this patch to include at the start of your series like PATCH 1/x: this patch with post-processing, using either * or ** ... PATCH x/x: final rework, and drop the postprocessing
It's just that the prospect of submitting a patch that reduces so much code makes me eager to try and get it submitted :) Brendan and David seem ok with paying the bit of runtime overhead for post-processing, esp. if we time it so this patch lands in the same Linux release as the module rework. But I can hold off if it'll make your life more difficult.
Cheers,
Jeremy
On Fri, Jan 28, 2022 at 4:19 PM Daniel Latypov dlatypov@google.com wrote:
On Wed, Oct 13, 2021 at 6:55 PM Jeremy Kerr jk@codeconstruct.com.au wrote:
Resulting in the .kunit_test_suites section just being a set of contiguous pointers to struct kunit_suite. We get the number of suites from the section size.
<snip>
That was my thinking, anyway. I think it probably makes sense to do that cleanup after the section patch, as that means we don't need any post-processing on the suites arrays.
To be honest, I'm actually tempted to pay the cost of postprocessing and proposing a change like this for real. Going from kunit_suite*** to ** shaves off a lot of code from the unit test and the filtering code path.
Specifically I'm thinking this can go into the kunit branch, https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/?h... Then when we have the series reworking modules, one of two things can happen.
- if we get to kunit_suite** with null-terminated arrays, fixing the
executor just means dropping the post-processing step. 2. If we get to kunit_suite* as mentioned above, then there'll be a bit more work, but not as much.
Alternatively, I can wait and send you an updated version of this patch to include at the start of your series like PATCH 1/x: this patch with post-processing, using either * or ** ... PATCH x/x: final rework, and drop the postprocessing
It's just that the prospect of submitting a patch that reduces so much code makes me eager to try and get it submitted :)
I agree. Honestly, just changing the type signature from `struct kunit_suite * const * const *` to `struct kunit_suite * const *` in the suite_set struct sells me on this. I missed this before, but now that I am aware of this, I would like to see it go in soon.
Brendan and David seem ok with paying the bit of runtime overhead for post-processing, esp. if we time it so this patch lands in the same
I'm absolutely fine with it. We are nowhere near the point where that matters at all.
Linux release as the module rework. But I can hold off if it'll make your life more difficult.
Also agree. I am excited to see Jeremy's new module code. I don't want to make his life any harder than it already is ;-)
Cheers,
Jeremy
On Thu, Oct 14, 2021 at 3:13 AM Daniel Latypov dlatypov@google.com 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/101d12fc9250b7a445ff50a9e7a25cd74d0e... [2] https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/co...
Cc: Jeremy Kerr jk@codeconstruct.com.au Signed-off-by: Daniel Latypov dlatypov@google.com
I really like this. My only real concern is that it's a little unclear exactly what the resulting layout is, particularly as to what the "make_suite_set" function does. It'd be nice to have some more documentation, either as a comment on the function or a more detailed commit message, which explicitly describes the old format (an array (with start and end pointers) of NULL-terminated arrays of suites), and the new format (a single, NULL-terminated array with both start and end pointers).
Re: NULL termination. If we're already using both start and end pointers, the NULL terminator seems useless. (And if we've got a NULL terminator, why are we passing the end pointer around.) It's not super-clear why we'd want both, though the comments in this reply do clarify things a bit: https://lore.kernel.org/linux-kselftest/CAGS_qxoziNGNVpsUfvUfOReADY0PdriV2gJ...
Finally, if we do want a runtime way of adding suites to the executor's list at runtime (which was suggested as a way of working around some suites which might need extra, global, initialisation), this might change how that'd have to be implemented a bit. I'm not too worried about that, though: it's something that's probably better served with something like a linked list of suite_sets or the like, anyway.
In any case, I've tested this in the non-module case, and it seems to work fine. Tested-by: David Gow davidgow@google.com
Cheers, -- David
linux-kselftest-mirror@lists.linaro.org
-
Brendan Higgins -
Daniel Latypov -
David Gow -
Jeremy Kerr