| Message ID | 20210421183222.2557747-1-dlatypov@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] kunit: introduce kunit_kmalloc_array/kunit_kcalloc() helpers | expand |
On Thu, Apr 22, 2021 at 2:32 AM Daniel Latypov <dlatypov@google.com> wrote: > > Functionally, this just means that the test output will be slightly > changed and it'll now depend on CONFIG_KUNIT=y/m. > > It'll still run at boot time and can still be built as a loadable > module. > > There was a pre-existing patch to convert this test that I found later, > here [1]. Compared to [1], this patch doesn't rename files and uses > KUnit features more heavily (i.e. does more than converting pr_err() > calls to KUNIT_FAIL()). > > What this conversion gives us: > * a shorter test thanks to KUnit's macros > * a way to run this a bit more easily via kunit.py (and > CONFIG_KUNIT_ALL_TESTS=y) [2] > * a structured way of reporting pass/fail > * uses kunit-managed allocations to avoid the risk of memory leaks > * more descriptive error messages: > * i.e. it prints out which fields are invalid, what the expected > values are, etc. > > What this conversion does not do: > * change the name of the file (and thus the name of the module) > * change the name of the config option > > Leaving these as-is for now to minimize the impact to people wanting to > run this test. IMO, that concern trumps following KUnit's style guide > for both names, at least for now. > > [1] https://lore.kernel.org/linux-kselftest/20201015014616.309000-1-vitor@massaru.org/ > [2] Can be run via > $ ./tools/testing/kunit/kunit.py run --kunitconfig /dev/stdin <<EOF > CONFIG_KUNIT=y > CONFIG_TEST_LIST_SORT=y > EOF > > [16:55:56] Configuring KUnit Kernel ... > [16:55:56] Building KUnit Kernel ... > [16:56:29] Starting KUnit Kernel ... > [16:56:32] ============================================================ > [16:56:32] ======== [PASSED] list_sort ======== > [16:56:32] [PASSED] list_sort_test > [16:56:32] ============================================================ > [16:56:32] Testing complete. 1 tests run. 0 failed. 0 crashed. > [16:56:32] Elapsed time: 35.668s total, 0.001s configuring, 32.725s building, 0.000s running > > Note: the build time is as after a `make mrproper`. > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > --- This looks good to me: I'm not an expert in the test, though, so I may have missed something. I did run it, though, and it seemed to work fine. It's a shame the functions can no-longer be marked __init, but I think the advantages of KUnit outweigh it, particularly since this is unlikely to be being used in production. (BTW: This doesn't appear to be posted as a reply to Patch 1/2, which made it a bit trickier to find.) This is Tested-by: David Gow <davidgow@google.com> -- David
On Wed, Apr 21, 2021 at 11:32 AM Daniel Latypov <dlatypov@google.com> wrote: > > Functionally, this just means that the test output will be slightly > changed and it'll now depend on CONFIG_KUNIT=y/m. > > It'll still run at boot time and can still be built as a loadable > module. > > There was a pre-existing patch to convert this test that I found later, > here [1]. Compared to [1], this patch doesn't rename files and uses > KUnit features more heavily (i.e. does more than converting pr_err() > calls to KUNIT_FAIL()). > > What this conversion gives us: > * a shorter test thanks to KUnit's macros > * a way to run this a bit more easily via kunit.py (and > CONFIG_KUNIT_ALL_TESTS=y) [2] > * a structured way of reporting pass/fail > * uses kunit-managed allocations to avoid the risk of memory leaks > * more descriptive error messages: > * i.e. it prints out which fields are invalid, what the expected > values are, etc. > > What this conversion does not do: > * change the name of the file (and thus the name of the module) > * change the name of the config option > > Leaving these as-is for now to minimize the impact to people wanting to > run this test. IMO, that concern trumps following KUnit's style guide > for both names, at least for now. > > [1] https://lore.kernel.org/linux-kselftest/20201015014616.309000-1-vitor@massaru.org/ > [2] Can be run via > $ ./tools/testing/kunit/kunit.py run --kunitconfig /dev/stdin <<EOF > CONFIG_KUNIT=y > CONFIG_TEST_LIST_SORT=y > EOF > > [16:55:56] Configuring KUnit Kernel ... > [16:55:56] Building KUnit Kernel ... > [16:56:29] Starting KUnit Kernel ... > [16:56:32] ============================================================ > [16:56:32] ======== [PASSED] list_sort ======== > [16:56:32] [PASSED] list_sort_test > [16:56:32] ============================================================ > [16:56:32] Testing complete. 1 tests run. 0 failed. 0 crashed. > [16:56:32] Elapsed time: 35.668s total, 0.001s configuring, 32.725s building, 0.000s running > > Note: the build time is as after a `make mrproper`. > > Signed-off-by: Daniel Latypov <dlatypov@google.com> Acked-by: Brendan Higgins <brendanhiggins@google.com>
Hi, Can we change this to CONFIG_LIST_SORT_KUNIT_TEST to follow the convention used by other KUNIT tests? Maintainers? thoughts? I recently posted patches to convert some of the other tests that break this format [1]. Cheers, -- Nico [1] - https://lkml.org/lkml/2021/4/14/310 On 4/21/21 2:32 PM, Daniel Latypov wrote: > [SNIP...] > config TEST_LIST_SORT > - tristate "Linked list sorting test" > - depends on DEBUG_KERNEL || m > + tristate "Linked list sorting test" if !KUNIT_ALL_TESTS > + depends on KUNIT > + default KUNIT_ALL_TESTS > [SNAP...]
On Thu, Apr 22, 2021 at 1:16 PM Nico Pache <npache@redhat.com> wrote: > > Hi, > > Can we change this to CONFIG_LIST_SORT_KUNIT_TEST to follow the convention used by other KUNIT tests? I mentioned this in the commit description briefly, but I don't know who is using this test. Nor do I really know who to ask. So I didn't want to risk breaking anyone's workflow for now (more than now requiring them to set CONFIG_KUNIT=y/m). Side note: maybe CONFIG_KUNIT can default to =y when DEBUG_KERNEL is set? Then there'd be even less change than how this worked before... If it's not a concern, I'll happily update the file name and config option to follow the conventions. > > Maintainers? thoughts? I recently posted patches to convert some of the other tests that break this format [1]. > > Cheers, > > -- Nico > > [1] - https://lkml.org/lkml/2021/4/14/310 > > On 4/21/21 2:32 PM, Daniel Latypov wrote: > > [SNIP...] > > config TEST_LIST_SORT > > - tristate "Linked list sorting test" > > - depends on DEBUG_KERNEL || m > > + tristate "Linked list sorting test" if !KUNIT_ALL_TESTS > > + depends on KUNIT > > + default KUNIT_ALL_TESTS > > [SNAP...] > > -- > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/d7b2b598-7087-0445-4647-8521f3238dc2%40redhat.com.
I've left the names as-is for now in v2, https://lore.kernel.org/linux-kselftest/20210503205835.1370850-2-dlatypov@google.com I could send a v3 w/ the rename and see if anyone complains, but I'm tempted to push that off. On Thu, Apr 22, 2021 at 1:43 PM Daniel Latypov <dlatypov@google.com> wrote: > > On Thu, Apr 22, 2021 at 1:16 PM Nico Pache <npache@redhat.com> wrote: > > > > Hi, > > > > Can we change this to CONFIG_LIST_SORT_KUNIT_TEST to follow the convention used by other KUNIT tests? > > I mentioned this in the commit description briefly, but I don't know > who is using this test. Nor do I really know who to ask. > So I didn't want to risk breaking anyone's workflow for now (more than > now requiring them to set CONFIG_KUNIT=y/m). > > Side note: maybe CONFIG_KUNIT can default to =y when DEBUG_KERNEL is set? > Then there'd be even less change than how this worked before... > > If it's not a concern, I'll happily update the file name and config > option to follow the conventions. > > > > > Maintainers? thoughts? I recently posted patches to convert some of the other tests that break this format [1]. > > > > Cheers, > > > > -- Nico > > > > [1] - https://lkml.org/lkml/2021/4/14/310 > > > > On 4/21/21 2:32 PM, Daniel Latypov wrote: > > > [SNIP...] > > > config TEST_LIST_SORT > > > - tristate "Linked list sorting test" > > > - depends on DEBUG_KERNEL || m > > > + tristate "Linked list sorting test" if !KUNIT_ALL_TESTS > > > + depends on KUNIT > > > + default KUNIT_ALL_TESTS > > > [SNAP...] > > > > -- > > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/d7b2b598-7087-0445-4647-8521f3238dc2%40redhat.com.
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 417c3d3e521b..09a0cc8a55cc 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1999,8 +1999,9 @@ config LKDTM Documentation/fault-injection/provoke-crashes.rst config TEST_LIST_SORT - tristate "Linked list sorting test" - depends on DEBUG_KERNEL || m + tristate "Linked list sorting test" if !KUNIT_ALL_TESTS + depends on KUNIT + default KUNIT_ALL_TESTS help Enable this to turn on 'list_sort()' function test. This test is executed only once during system boot (so affects only boot time), diff --git a/lib/test_list_sort.c b/lib/test_list_sort.c index 1f017d3b610e..ccfd98dbf57c 100644 --- a/lib/test_list_sort.c +++ b/lib/test_list_sort.c @@ -1,5 +1,5 @@ // SPDX-License-Identifier: GPL-2.0-only -#define pr_fmt(fmt) "list_sort_test: " fmt +#include <kunit/test.h> #include <linux/kernel.h> #include <linux/list_sort.h> @@ -23,67 +23,52 @@ struct debug_el { struct list_head list; unsigned int poison2; int value; - unsigned serial; + unsigned int serial; }; -/* Array, containing pointers to all elements in the test list */ -static struct debug_el **elts __initdata; - -static int __init check(struct debug_el *ela, struct debug_el *elb) +static void check(struct kunit *test, struct debug_el *ela, struct debug_el *elb) { - if (ela->serial >= TEST_LIST_LEN) { - pr_err("error: incorrect serial %d\n", ela->serial); - return -EINVAL; - } - if (elb->serial >= TEST_LIST_LEN) { - pr_err("error: incorrect serial %d\n", elb->serial); - return -EINVAL; - } - if (elts[ela->serial] != ela || elts[elb->serial] != elb) { - pr_err("error: phantom element\n"); - return -EINVAL; - } - if (ela->poison1 != TEST_POISON1 || ela->poison2 != TEST_POISON2) { - pr_err("error: bad poison: %#x/%#x\n", - ela->poison1, ela->poison2); - return -EINVAL; - } - if (elb->poison1 != TEST_POISON1 || elb->poison2 != TEST_POISON2) { - pr_err("error: bad poison: %#x/%#x\n", - elb->poison1, elb->poison2); - return -EINVAL; - } - return 0; + struct debug_el **elts = test->priv; + + KUNIT_EXPECT_LT_MSG(test, ela->serial, (unsigned int)TEST_LIST_LEN, "incorrect serial"); + KUNIT_EXPECT_LT_MSG(test, elb->serial, (unsigned int)TEST_LIST_LEN, "incorrect serial"); + + KUNIT_EXPECT_PTR_EQ_MSG(test, elts[ela->serial], ela, "phantom element"); + KUNIT_EXPECT_PTR_EQ_MSG(test, elts[elb->serial], elb, "phantom element"); + + KUNIT_EXPECT_EQ_MSG(test, ela->poison1, TEST_POISON1, "bad poison"); + KUNIT_EXPECT_EQ_MSG(test, ela->poison2, TEST_POISON2, "bad poison"); + + KUNIT_EXPECT_EQ_MSG(test, elb->poison1, TEST_POISON1, "bad poison"); + KUNIT_EXPECT_EQ_MSG(test, elb->poison2, TEST_POISON2, "bad poison"); } -static int __init cmp(void *priv, struct list_head *a, struct list_head *b) +/* `priv` is the test pointer so check() can fail the test if the list is invalid. */ +static int cmp(void *priv, struct list_head *a, struct list_head *b) { struct debug_el *ela, *elb; ela = container_of(a, struct debug_el, list); elb = container_of(b, struct debug_el, list); - check(ela, elb); + check(priv, ela, elb); return ela->value - elb->value; } -static int __init list_sort_test(void) +static void list_sort_test(struct kunit *test) { - int i, count = 1, err = -ENOMEM; - struct debug_el *el; + int i, count = 1; + struct debug_el *el, **elts; struct list_head *cur; LIST_HEAD(head); - pr_debug("start testing list_sort()\n"); - - elts = kcalloc(TEST_LIST_LEN, sizeof(*elts), GFP_KERNEL); - if (!elts) - return err; + elts = kunit_kcalloc(test, TEST_LIST_LEN, sizeof(*elts), GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, elts); + test->priv = elts; for (i = 0; i < TEST_LIST_LEN; i++) { - el = kmalloc(sizeof(*el), GFP_KERNEL); - if (!el) - goto exit; + el = kunit_kmalloc(test, sizeof(*el), GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, el); /* force some equivalencies */ el->value = prandom_u32() % (TEST_LIST_LEN / 3); @@ -94,55 +79,44 @@ static int __init list_sort_test(void) list_add_tail(&el->list, &head); } - list_sort(NULL, &head, cmp); + list_sort(test, &head, cmp); - err = -EINVAL; for (cur = head.next; cur->next != &head; cur = cur->next) { struct debug_el *el1; int cmp_result; - if (cur->next->prev != cur) { - pr_err("error: list is corrupted\n"); - goto exit; - } + KUNIT_ASSERT_PTR_EQ_MSG(test, cur->next->prev, cur, + "list is corrupted"); - cmp_result = cmp(NULL, cur, cur->next); - if (cmp_result > 0) { - pr_err("error: list is not sorted\n"); - goto exit; - } + cmp_result = cmp(test, cur, cur->next); + KUNIT_ASSERT_LE_MSG(test, cmp_result, 0, "list is not sorted"); el = container_of(cur, struct debug_el, list); el1 = container_of(cur->next, struct debug_el, list); - if (cmp_result == 0 && el->serial >= el1->serial) { - pr_err("error: order of equivalent elements not " - "preserved\n"); - goto exit; + if (cmp_result == 0) { + KUNIT_ASSERT_LE_MSG(test, el->serial, el1->serial, + "order of equivalent elements not preserved"); } - if (check(el, el1)) { - pr_err("error: element check failed\n"); - goto exit; - } + check(test, el, el1); count++; } - if (head.prev != cur) { - pr_err("error: list is corrupted\n"); - goto exit; - } + KUNIT_EXPECT_PTR_EQ_MSG(test, head.prev, cur, "list is corrupted"); + KUNIT_EXPECT_EQ_MSG(test, count, TEST_LIST_LEN, + "list length changed after sorting!"); +} - if (count != TEST_LIST_LEN) { - pr_err("error: bad list length %d", count); - goto exit; - } +static struct kunit_case list_sort_cases[] = { + KUNIT_CASE(list_sort_test), + {} +}; + +static struct kunit_suite list_sort_suite = { + .name = "list_sort", + .test_cases = list_sort_cases, +}; + +kunit_test_suites(&list_sort_suite); - err = 0; -exit: - for (i = 0; i < TEST_LIST_LEN; i++) - kfree(elts[i]); - kfree(elts); - return err; -} -module_init(list_sort_test); MODULE_LICENSE("GPL");
Functionally, this just means that the test output will be slightly changed and it'll now depend on CONFIG_KUNIT=y/m. It'll still run at boot time and can still be built as a loadable module. There was a pre-existing patch to convert this test that I found later, here [1]. Compared to [1], this patch doesn't rename files and uses KUnit features more heavily (i.e. does more than converting pr_err() calls to KUNIT_FAIL()). What this conversion gives us: * a shorter test thanks to KUnit's macros * a way to run this a bit more easily via kunit.py (and CONFIG_KUNIT_ALL_TESTS=y) [2] * a structured way of reporting pass/fail * uses kunit-managed allocations to avoid the risk of memory leaks * more descriptive error messages: * i.e. it prints out which fields are invalid, what the expected values are, etc. What this conversion does not do: * change the name of the file (and thus the name of the module) * change the name of the config option Leaving these as-is for now to minimize the impact to people wanting to run this test. IMO, that concern trumps following KUnit's style guide for both names, at least for now. [1] https://lore.kernel.org/linux-kselftest/20201015014616.309000-1-vitor@massaru.org/ [2] Can be run via $ ./tools/testing/kunit/kunit.py run --kunitconfig /dev/stdin <<EOF CONFIG_KUNIT=y CONFIG_TEST_LIST_SORT=y EOF [16:55:56] Configuring KUnit Kernel ... [16:55:56] Building KUnit Kernel ... [16:56:29] Starting KUnit Kernel ... [16:56:32] ============================================================ [16:56:32] ======== [PASSED] list_sort ======== [16:56:32] [PASSED] list_sort_test [16:56:32] ============================================================ [16:56:32] Testing complete. 1 tests run. 0 failed. 0 crashed. [16:56:32] Elapsed time: 35.668s total, 0.001s configuring, 32.725s building, 0.000s running Note: the build time is as after a `make mrproper`. Signed-off-by: Daniel Latypov <dlatypov@google.com> --- lib/Kconfig.debug | 5 +- lib/test_list_sort.c | 128 +++++++++++++++++-------------------------- 2 files changed, 54 insertions(+), 79 deletions(-)