| Message ID | 20210727225847.22185-1-jjoseaquiless@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | lib: use of kunit in test_parman.c | expand |
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
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); - 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); + 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); } 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); + 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"); 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); - 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); } -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>");