[RFC PATCH] kunit: flatten kunit_suite*** to kunit_suite** in executor

2 views
Skip to first unread message

Daniel Latypov

unread,
Oct 13, 2021, 3:13:27 PM10/13/21
to brendan...@google.com, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, Daniel Latypov, Jeremy Kerr
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/101d12fc9250b7a445ff50a...@codeconstruct.com.au/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=kunit&id=3b29021ddd10cfb6b2565c623595bd3b02036f33

Cc: Jeremy Kerr <j...@codeconstruct.com.au>
Signed-off-by: Daniel Latypov <dlat...@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(dlat...@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
--
2.33.0.882.g93a45727a2-goog

Brendan Higgins

unread,
Oct 13, 2021, 4:04:00 PM10/13/21
to Daniel Latypov, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, Jeremy Kerr
On Wed, Oct 13, 2021 at 12:13 PM Daniel Latypov <dlat...@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/101d12fc9250b7a445ff50a...@codeconstruct.com.au/
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=kunit&id=3b29021ddd10cfb6b2565c623595bd3b02036f33
>
> Cc: Jeremy Kerr <j...@codeconstruct.com.au>
> Signed-off-by: Daniel Latypov <dlat...@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?).
Can you elaborate what you mean here?

Daniel Latypov

unread,
Oct 13, 2021, 4:15:19 PM10/13/21
to Brendan Higgins, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org, Jeremy Kerr
On Wed, Oct 13, 2021 at 1:04 PM Brendan Higgins
<brendan...@google.com> wrote:
>
> On Wed, Oct 13, 2021 at 12:13 PM Daniel Latypov <dlat...@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/101d12fc9250b7a445ff50a...@codeconstruct.com.au/
> > [2] https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=kunit&id=3b29021ddd10cfb6b2565c623595bd3b02036f33
> >
> > Cc: Jeremy Kerr <j...@codeconstruct.com.au>
> > Signed-off-by: Daniel Latypov <dlat...@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.
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.

Jeremy Kerr

unread,
Oct 13, 2021, 9:55:13 PM10/13/21
to Daniel Latypov, Brendan Higgins, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org
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

David Gow

unread,
Oct 13, 2021, 10:16:48 PM10/13/21
to Daniel Latypov, Brendan Higgins, Linux Kernel Mailing List, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan, Jeremy Kerr
On Thu, Oct 14, 2021 at 3:13 AM Daniel Latypov <dlat...@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/101d12fc9250b7a445ff50a...@codeconstruct.com.au/
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=kunit&id=3b29021ddd10cfb6b2565c623595bd3b02036f33
>
> Cc: Jeremy Kerr <j...@codeconstruct.com.au>
> Signed-off-by: Daniel Latypov <dlat...@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_qxoziNGNVpsUfvUfOReA...@mail.gmail.com/

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 <davi...@google.com>

Cheers,
-- David

Daniel Latypov

unread,
Jan 28, 2022, 4:19:54 PM1/28/22
to Jeremy Kerr, Brendan Higgins, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org
On Wed, Oct 13, 2021 at 6:55 PM Jeremy Kerr <j...@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=kunit
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

Brendan Higgins

unread,
Jan 28, 2022, 4:27:25 PM1/28/22
to Daniel Latypov, Jeremy Kerr, davi...@google.com, linux-...@vger.kernel.org, kuni...@googlegroups.com, linux-k...@vger.kernel.org, sk...@linuxfoundation.org
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
Reply all
Reply to author
Forward
0 new messages