Re: [PATCH v2] lib: use of kunit in test_parman.c

On Wed, Jul 28, 2021 at 12:55 PM José Aquiles Guedes de Rezende jjoseaquiless@gmail.com wrote:

Convert the parman test module to use the KUnit test framework. This makes the test clearer by leveraging KUnit's assertion macros 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

---

Changes in v2:

• Rename TEST_PARMAN config item to PARMAN_KUNIT_TEST and make it work with the kunit framework.
• Change KUNIT_ASSERT_EQ to KUNIT_ASSERT_EQ_MSG.
• Call test_parman_resize(test_parman, 0) when parman_create fail
• Remove kunit_kfree.
• Remove "\n" in error messages
• Remove casts to unsigned long

Awesome! This worked out-of-the-box for me, thanks!

I'll leave a more detailed review to someone who knows what parman is better than I, but it looks good to me from a KUnit point of view.

Nevertheless, this is Tested-by: David Gow davidgow@google.com

-- David

lib/Kconfig.debug | 13 +++-- lib/Makefile | 2 +- lib/test_parman.c | 145 +++++++++++++++++++--------------------------- 3 files changed, 70 insertions(+), 90 deletions(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 831212722924..e68a27e5e5b0 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -2231,12 +2231,15 @@ config TEST_HASH config TEST_IDA tristate "Perform selftest on IDA functions"

-config TEST_PARMAN

•   tristate "Perform selftest on priority array manager"
  

•   depends on PARMAN
  

+config PARMAN_KUNIT_TEST

•   tristate "Kunit test for priority array manager" if !KUNIT_ALL_TESTS
  

•   select PARMAN
  

•   depends on KUNIT
  

•   default KUNIT_ALL_TESTS
    help
  

•     Enable this option to test priority array manager on boot
  

•     (or module load).
  

•     Enable this option to test priority array manager on boot.
  

•     For more information on KUnit and unit tests in general please refer
  

•     to the KUnit documentation in Documentation/dev-tools/kunit/.
  
      If unsure, say N.
  
  

diff --git a/lib/Makefile b/lib/Makefile index 5efd1b435a37..deb8946735e8 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -88,7 +88,7 @@ obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o obj-$(CONFIG_TEST_STRSCPY) += test_strscpy.o obj-$(CONFIG_TEST_UUID) += test_uuid.o obj-$(CONFIG_TEST_XARRAY) += test_xarray.o -obj-$(CONFIG_TEST_PARMAN) += test_parman.o +obj-$(CONFIG_PARMAN_KUNIT_TEST) += test_parman.o obj-$(CONFIG_TEST_KMOD) += test_kmod.o obj-$(CONFIG_TEST_DEBUG_VIRTUAL) += test_debug_virtual.o obj-$(CONFIG_TEST_MEMCAT_P) += test_memcat_p.o diff --git a/lib/test_parman.c b/lib/test_parman.c index 35e32243693c..512f874bc71c 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,41 @@ 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);
  

•   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_MSG(test, err, 0, "test_parman_resize failed");
  

•   test_parman->parman = parman_create(&test_parman_lsort_ops, test_parman);
  

•   if (IS_ERR_OR_NULL(test_parman->parman)) {
  

•           test_parman_resize(test_parman, 0);
  

•           KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_parman->parman);
    }
  

•   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);
  

}

static bool test_parman_run_check_budgets(struct test_parman *test_parman) @@ -265,8 +268,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 +285,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_MSG(test, err, 0, "parman_item_add failed");
  

•                   test_parman->prio_array[item->parman_item.index] = item;
                    test_parman->used_items++;
            } else {
  

@@ -294,22 +298,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)",
  

•           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 +319,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");
  
            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)",
  

•                   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, i,
  

•                   "Item has different index in compare to where it actually is (%lu != %d)",
  

•                   item->parman_item.index, i);
  
  

•   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)",
  

•           used_items, test_parman->used_items);
  
  

•   return 0;
  

•   KUNIT_ASSERT_LT_MSG(test, last_unused_items, TEST_PARMAN_RESIZE_STEP_COUNT,
  

•           "Number of unused item at the end of array is bigger than resize step (%u >= %lu)",
  

•           last_unused_items, TEST_PARMAN_RESIZE_STEP_COUNT);
  

}

-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