On Tue, Jul 27, 2021 at 4:04 PM José Aquiles Guedes de Rezende jjoseaquiless@gmail.com wrote:
Convert the parman test module to use the KUnit test framework. This makes thetest clearer by leveraging KUnit's assertion macros
nit: s/thetest/the test
and test case definitions,as well as helps standardize on a testing framework.
Co-developed-by: Matheus Henrique de Souza Silva matheushenriquedesouzasilva@protonmail.com Signed-off-by: Matheus Henrique de Souza Silva matheushenriquedesouzasilva@protonmail.com Signed-off-by: José Aquiles Guedes de Rezende jjoseaquiless@gmail.com
Acked-by: Daniel Latypov dlatypov@google.com
I just briefly looked over the usage of KUnit a bit and left some suggestions.
It's nice to see the use of current->kunit_test in an ops struct. I wrote that up as a potential use case in commit 359a376081d4 ("kunit: support failure from dynamic analysis tools"), and I think this is the first example of it being used as such :)
lib/test_parman.c | 145 +++++++++++++++++++--------------------------- 1 file changed, 60 insertions(+), 85 deletions(-)
diff --git a/lib/test_parman.c b/lib/test_parman.c index 35e32243693c..bd5010f0a412 100644 --- a/lib/test_parman.c +++ b/lib/test_parman.c @@ -41,6 +41,8 @@ #include <linux/err.h> #include <linux/random.h> #include <linux/parman.h> +#include <linux/sched.h> +#include <kunit/test.h>
#define TEST_PARMAN_PRIO_SHIFT 7 /* defines number of prios for testing */ #define TEST_PARMAN_PRIO_COUNT BIT(TEST_PARMAN_PRIO_SHIFT) @@ -91,12 +93,14 @@ struct test_parman {
static int test_parman_resize(void *priv, unsigned long new_count) {
struct kunit *test = current->kunit_test; struct test_parman *test_parman = priv; struct test_parman_item **prio_array; unsigned long old_count; prio_array = krealloc(test_parman->prio_array, ITEM_PTRS_SIZE(new_count), GFP_KERNEL);KUNIT_EXPECT_NOT_ERR_OR_NULL(test, prio_array); if (new_count == 0) return 0; if (!prio_array)@@ -214,42 +218,39 @@ static void test_parman_items_fini(struct test_parman *test_parman) } }
-static struct test_parman *test_parman_create(const struct parman_ops *ops) +static int test_parman_create(struct kunit *test) { struct test_parman *test_parman; int err;
test_parman = kzalloc(sizeof(*test_parman), GFP_KERNEL);if (!test_parman)return ERR_PTR(-ENOMEM);
test_parman = kunit_kzalloc(test, sizeof(*test_parman), GFP_KERNEL);KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_parman);err = test_parman_resize(test_parman, TEST_PARMAN_BASE_COUNT);
Would it be clearer to do
KUNIT_ASSERT_EQ(test, 0, test_parman_resize(test_parman, TEST_PARMAN_BASE_COUNT));
or change the line below to:
KUNIT_ASSERT_EQ_MSG(test, err, 0, "test_parman_resize failed");
Otherwise, if the test fails there, the error message isn't too clear.
if (err)goto err_resize;test_parman->parman = parman_create(ops, test_parman);if (!test_parman->parman) {err = -ENOMEM;goto err_parman_create;}
KUNIT_ASSERT_EQ(test, err, 0);test_parman->parman = parman_create(&test_parman_lsort_ops, test_parman);KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_parman->parman);
Hmm, this won't call `test_parman_resize(test_parman, 0)` on error like it did before.
Unfortunately, KUnit is a bit clunky for cases like this right now, where there's cleanups that need to happen but aren't handle already by the resource API (the underlying thing behind kunit_kzalloc that frees the mem).
We could do something like
if (IS_ERR_OR_NULL(test_parman->parman)) { // we can use KUNIT_FAIL to just directly fail the test, or use the assert macro for the autogenerated failure message KUNIT_FAIL(test, "....") / KUNIT_EXPECT_NOT_ERR_OR_NULL(test, ...); goto err_parman_create; }
test_parman_rnd_init(test_parman); test_parman_prios_init(test_parman); test_parman_items_init(test_parman); test_parman->run_budget = TEST_PARMAN_RUN_BUDGET;
return test_parman;-err_parman_create:
test_parman_resize(test_parman, 0);-err_resize:
kfree(test_parman);return ERR_PTR(err);
test->priv = test_parman;return 0;}
-static void test_parman_destroy(struct test_parman *test_parman) +static void test_parman_destroy(struct kunit *test) {
struct test_parman *test_parman = test->priv;if (!test_parman)return; test_parman_items_fini(test_parman); test_parman_prios_fini(test_parman); parman_destroy(test_parman->parman); test_parman_resize(test_parman, 0);
kfree(test_parman);
kunit_kfree(test, test_parman);
This works as-is, but FYI, it isn't necessary as we've used kunit_kzalloc() to allocate it above. When the test exists, it'll automatically call this function, basically.
}
static bool test_parman_run_check_budgets(struct test_parman *test_parman) @@ -265,8 +266,9 @@ static bool test_parman_run_check_budgets(struct test_parman *test_parman) return true; }
-static int test_parman_run(struct test_parman *test_parman) +static void test_parman_run(struct kunit *test) {
struct test_parman *test_parman = test->priv; unsigned int i = test_parman_rnd_get(test_parman); int err;@@ -281,8 +283,8 @@ static int test_parman_run(struct test_parman *test_parman) err = parman_item_add(test_parman->parman, &item->prio->parman_prio, &item->parman_item);
if (err)return err;
KUNIT_ASSERT_EQ(test, err, 0);
Echoing my suggestion above, we can either do KUNIT_ASSERT_EQ(test, 0, parman_item_add(...)); or something like KUNIT_ASSERT_EQ_MSG(test, err, 0, "parman_item_add failed")
test_parman->prio_array[item->parman_item.index] = item; test_parman->used_items++; } else {@@ -294,22 +296,19 @@ static int test_parman_run(struct test_parman *test_parman) } item->used = !item->used; }
return 0;}
-static int test_parman_check_array(struct test_parman *test_parman,
bool gaps_allowed)+static void test_parman_check_array(struct kunit *test, bool gaps_allowed) { unsigned int last_unused_items = 0; unsigned long last_priority = 0; unsigned int used_items = 0; int i;
struct test_parman *test_parman = test->priv;
if (test_parman->prio_array_limit < TEST_PARMAN_BASE_COUNT) {pr_err("Array limit is lower than the base count (%lu < %lu)\n",test_parman->prio_array_limit, TEST_PARMAN_BASE_COUNT);return -EINVAL;}
KUNIT_ASSERT_GE_MSG(test, test_parman->prio_array_limit, TEST_PARMAN_BASE_COUNT,"Array limit is lower than the base count (%lu < %lu)\n",test_parman->prio_array_limit, TEST_PARMAN_BASE_COUNT); for (i = 0; i < test_parman->prio_array_limit; i++) { struct test_parman_item *item = test_parman->prio_array[i];@@ -318,77 +317,53 @@ static int test_parman_check_array(struct test_parman *test_parman, last_unused_items++; continue; }
if (last_unused_items && !gaps_allowed) {pr_err("Gap found in array even though they are forbidden\n");return -EINVAL;}
KUNIT_ASSERT_FALSE_MSG(test, last_unused_items && !gaps_allowed,"Gap found in array even though they are forbidden\n");
FYI, you don't need the \n for these. KUnit will put one automatically.
Ditto for the other instances.
last_unused_items = 0; used_items++;
if (item->prio->priority < last_priority) {pr_err("Item belongs under higher priority then the last one (current: %lu, previous: %lu)\n",item->prio->priority, last_priority);return -EINVAL;}last_priority = item->prio->priority;
KUNIT_ASSERT_GE_MSG(test, item->prio->priority, last_priority,"Item belongs under higher priority then the last one (current: %lu, previous: %lu)\n",item->prio->priority, last_priority);
if (item->parman_item.index != i) {pr_err("Item has different index in compare to where it actually is (%lu != %d)\n",item->parman_item.index, i);return -EINVAL;}}
last_priority = item->prio->priority;
if (used_items != test_parman->used_items) {pr_err("Number of used items in array does not match (%u != %u)\n",used_items, test_parman->used_items);return -EINVAL;}
KUNIT_ASSERT_EQ_MSG(test, item->parman_item.index, (unsigned long)i,"Item has different index in compare to where it actually is (%lu != %d)\n",item->parman_item.index, i);
Note: the cast to `unsigned long` here shouldn't be necessary anymore, I don't think. See 6e62dfa6d14f ("kunit: Do not typecheck binary assertions").
if (last_unused_items >= TEST_PARMAN_RESIZE_STEP_COUNT) {pr_err("Number of unused item at the end of array is bigger than resize step (%u >= %lu)\n",last_unused_items, TEST_PARMAN_RESIZE_STEP_COUNT);return -EINVAL; }pr_info("Priority array check successful\n");
KUNIT_ASSERT_EQ_MSG(test, used_items, test_parman->used_items,"Number of used items in array does not match (%u != %u)\n",used_items, test_parman->used_items);
return 0;
KUNIT_ASSERT_LT_MSG(test, (unsigned long)last_unused_items, TEST_PARMAN_RESIZE_STEP_COUNT,"Number of unused item at the end of array is bigger than resize step (%u >= %lu)\n",last_unused_items, TEST_PARMAN_RESIZE_STEP_COUNT);
ditto here, I think the casting can be dropped now.
}
-static int test_parman_lsort(void) +static void test_parman_lsort(struct kunit *test) {
struct test_parman *test_parman;int err;test_parman = test_parman_create(&test_parman_lsort_ops);if (IS_ERR(test_parman))return PTR_ERR(test_parman);err = test_parman_run(test_parman);if (err)goto out;err = test_parman_check_array(test_parman, false);if (err)goto out;-out:
test_parman_destroy(test_parman);return err;
test_parman_run(test);test_parman_check_array(test, false);}
-static int __init test_parman_init(void) -{
return test_parman_lsort();-} +static struct kunit_case parman_test_case[] = {
KUNIT_CASE(test_parman_lsort),{}+};
-static void __exit test_parman_exit(void) -{ -} +static struct kunit_suite parman_test_suite = {
.name = "parman",.init = test_parman_create,.exit = test_parman_destroy,.test_cases = parman_test_case,+};
-module_init(test_parman_init); -module_exit(test_parman_exit); +kunit_test_suite(parman_test_suite);
MODULE_LICENSE("Dual BSD/GPL"); MODULE_AUTHOR("Jiri Pirko jiri@mellanox.com"); -- 2.32.0