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

["Nícolas F. R. A. Prado" <n@nfraprado.net>]

Hello,

Any update on the review of this patch? Jiri Pirko?

Thanks,
Nícolas

On Wed, Jul 28, 2021 at 02:31:07PM +0800, David Gow wrote:
> 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
> >