lib: use of kunit in test_parman.c

Convert the parman test module to use the KUnit test framework.
This makes thetest 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>
---
 lib/test_parman.c | 145 +++++++++++++++++++---------------------------
 1 file changed, 60 insertions(+), 85 deletions(-)

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
>

On Wed, Jul 28, 2021 at 7:04 AM 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
> 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>
> ---

Thanks for porting this! A few more notes from the KUnit side.

>  lib/test_parman.c | 145 +++++++++++++++++++---------------------------
>  1 file changed, 60 insertions(+), 85 deletions(-)

This seems to be missing changes to lib/Kconfig.debug: you'll want to
modify the TEST_PARMAN config item to
- depend on KUNIT
- only appear if !KUNIT_ALL_TESTS
- default KUNIT_ALL_TESTS

It might also be nice to:
- select PARMAN (it's otherwise extremely unlikely a config will
actually pick up the test)
- maybe rename TEST_PARMAN to PARMAN_KUNIT_TEST (this is optional,
since this is a port of an existing test, but does make it clearer, so
it really depends on what existing usage looks like)

>
> 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

The rest of this looks okay to me, but you should check out Daniel's
comments in his email, too.

Cheers,
-- David

Hi "José,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on ipvs/master]
[also build test ERROR on linux/master linus/master v5.14-rc3 next-20210727]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jos-Aquiles-Guedes-de-Rezende/lib-use-of-kunit-in-test_parman-c/20210728-070506
base:   https://git.kernel.org/pub/scm/linux/kernel/git/horms/ipvs.git master
config: powerpc64-buildonly-randconfig-r006-20210727 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project c49df15c278857adecd12db6bb1cdc96885f7079)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc64 cross compiling tool for clang build
        # apt-get install binutils-powerpc64-linux-gnu
        # https://github.com/0day-ci/linux/commit/28790f5c89e89dc96adf7bd4c748ddd505a52391
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jos-Aquiles-Guedes-de-Rezende/lib-use-of-kunit-in-test_parman-c/20210728-070506
        git checkout 28790f5c89e89dc96adf7bd4c748ddd505a52391
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross O=build_dir ARCH=powerpc SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> lib/test_parman.c:96:32: error: no member named 'kunit_test' in 'struct task_struct'
           struct kunit *test = current->kunit_test;
                                ~~~~~~~  ^
   1 error generated.


vim +96 lib/test_parman.c

    93	
    94	static int test_parman_resize(void *priv, unsigned long new_count)
    95	{
  > 96		struct kunit *test = current->kunit_test;
    97		struct test_parman *test_parman = priv;
    98		struct test_parman_item **prio_array;
    99		unsigned long old_count;
   100	
   101		prio_array = krealloc(test_parman->prio_array,
   102				      ITEM_PTRS_SIZE(new_count), GFP_KERNEL);
   103		KUNIT_EXPECT_NOT_ERR_OR_NULL(test, prio_array);
   104		if (new_count == 0)
   105			return 0;
   106		if (!prio_array)
   107			return -ENOMEM;
   108		old_count = test_parman->prio_array_limit;
   109		if (new_count > old_count)
   110			memset(&prio_array[old_count], 0,
   111			       ITEM_PTRS_SIZE(new_count - old_count));
   112		test_parman->prio_array = prio_array;
   113		test_parman->prio_array_limit = new_count;
   114		return 0;
   115	}
   116	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org