Re: [PATCH v1] list: test: Test the klist structure

From: David Gow
Date: Thu Mar 30 2023 - 02:15:43 EST

Next message: Leon Romanovsky: "Re: [net PATCH 3/7] octeontx2-af: Add validation for lmac type"
Previous message: Greg Kroah-Hartman: "Re: [PATCH 0/5] Remove acpi.h implicit include of of.h"
In reply to: Brendan Higgins: "Re: [PATCH v1] list: test: Test the klist structure"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, 29 Mar 2023 at 19:07, Sadiya Kazi <sadiyakazi@xxxxxxxxxx> wrote:
>
> Add KUnit tests to the klist linked-list structure.
> These perform testing for different variations of node add
> and node delete in the klist data structure (<linux/klist.h>).
>
> Limitation: Since we use a static global variable, and if
> multiple instances of this test are run concurrently, the test may fail.
>
> Signed-off-by: Sadiya Kazi <sadiyakazi@xxxxxxxxxx>
> ---

Thanks very much! This will be a great addition to the list tests (and
the device model tests).

A couple of super-minor suggestions re: comment formatting, otherwise
good-to-go.

Reviewed-by: David Gow <davidgow@xxxxxxxxxx>

Cheers,
-- David

> lib/list-test.c | 298 +++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 297 insertions(+), 1 deletion(-)
>
> diff --git a/lib/list-test.c b/lib/list-test.c
> index d374cf5d1a57..af651cacb8f4 100644
> --- a/lib/list-test.c
> +++ b/lib/list-test.c
> @@ -8,6 +8,7 @@
> #include <kunit/test.h>
>
> #include <linux/list.h>
> +#include <linux/klist.h>
>
> struct list_test_struct {
> int data;
> @@ -1199,6 +1200,301 @@ static struct kunit_suite hlist_test_module = {
> .test_cases = hlist_test_cases,
> };
>
> -kunit_test_suites(&list_test_module, &hlist_test_module);
> +
> +struct klist_test_struct {
> + int data;
> + struct klist klist;
> + struct klist_node klist_node;
> +};
> +
> +/* counts the number of nodes*/

This comment doesn't really add anything,

> +static int node_count;
> +static struct klist_node *last_node_count;

This isn't a count... can we rename it?

> +
> +static void check_node(struct klist_node *node_ptr)
> +{
> + node_count++;
> + last_node_count = node_ptr;
> +}
> +
> +static void check_delete_node(struct klist_node *node_ptr)
> +{
> + node_count--;
> + last_node_count = node_ptr;
> +}
> +
> +static void klist_test_add_tail(struct kunit *test)
> +{
> + struct klist_node a, b;
> + struct klist mylist;
> + struct klist_iter i;
> +
> + node_count = 0;
> + klist_init(&mylist, &check_node, NULL);
> +
> + klist_add_tail(&a, &mylist);
> + KUNIT_EXPECT_EQ(test, node_count, 1);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &a);
> +
> + klist_add_tail(&b, &mylist);
> + KUNIT_EXPECT_EQ(test, node_count, 2);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &b);
> +
> + /* should be [list] -> a -> b */
> + klist_iter_init(&mylist, &i);
> +
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &a);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &b);
> + KUNIT_EXPECT_NULL(test, klist_next(&i));
> +
> + klist_iter_exit(&i);
> +
> +}
> +
> +static void klist_test_add_head(struct kunit *test)
> +{
> + struct klist_node a, b;
> + struct klist mylist;
> + struct klist_iter i;
> +
> + node_count = 0;
> + klist_init(&mylist, &check_node, NULL);
> +
> + klist_add_head(&a, &mylist);
> + KUNIT_EXPECT_EQ(test, node_count, 1);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &a);
> +
> + klist_add_head(&b, &mylist);
> + KUNIT_EXPECT_EQ(test, node_count, 2);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &b);
> +
> + /* should be [list] -> b -> a */
> + klist_iter_init(&mylist, &i);
> +
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &b);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &a);
> + KUNIT_EXPECT_NULL(test, klist_next(&i));
> +
> + klist_iter_exit(&i);
> +
> +}
> +
> +static void klist_test_add_behind(struct kunit *test)
> +{
> + struct klist_node a, b, c, d;
> + struct klist mylist;
> + struct klist_iter i;
> +
> + node_count = 0;
> + klist_init(&mylist, &check_node, NULL);
> +
> + klist_add_head(&a, &mylist);
> + klist_add_head(&b, &mylist);
> +
> + klist_add_behind(&c, &a);
> + KUNIT_EXPECT_EQ(test, node_count, 3);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &c);
> +
> + klist_add_behind(&d, &b);
> + KUNIT_EXPECT_EQ(test, node_count, 4);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &d);
> +
> + klist_iter_init(&mylist, &i);
> +
> + /* should be [list] -> b -> d -> a -> c*/
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &b);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &d);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &a);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &c);
> + KUNIT_EXPECT_NULL(test, klist_next(&i));
> +
> + klist_iter_exit(&i);
> +
> +}
> +
> +static void klist_test_add_before(struct kunit *test)
> +{
> + struct klist_node a, b, c, d;
> + struct klist mylist;
> + struct klist_iter i;
> +
> + node_count = 0;
> + klist_init(&mylist, &check_node, NULL);
> +
> + klist_add_head(&a, &mylist);
> + klist_add_head(&b, &mylist);
> + klist_add_before(&c, &a);
> + KUNIT_EXPECT_EQ(test, node_count, 3);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &c);
> +
> + klist_add_before(&d, &b);
> + KUNIT_EXPECT_EQ(test, node_count, 4);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &d);
> +
> + klist_iter_init(&mylist, &i);
> +
> + /* should be [list] -> b -> d -> a -> c*/
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &d);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &b);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &c);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &a);
> + KUNIT_EXPECT_NULL(test, klist_next(&i));
> +
> + klist_iter_exit(&i);
> +
> +}
> +
> +/* Verify that klist_del() delays the deletion of a node until there
> + * are no other references to it
> + */

These comments are in the wrong style:
https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting

They should be of the form:
/*
* Text
* More text
*/
(with the empty line at the top)

> +static void klist_test_del_refcount_greater_than_zero(struct kunit *test)
> +{
> + struct klist_node a, b, c, d;
> + struct klist mylist;
> + struct klist_iter i;
> +
> + node_count = 0;
> + klist_init(&mylist, &check_node, &check_delete_node);
> +
> + /* Add nodes a,b,c,d to the list*/
> + klist_add_tail(&a, &mylist);
> + klist_add_tail(&b, &mylist);
> + klist_add_tail(&c, &mylist);
> + klist_add_tail(&d, &mylist);
> +
> + klist_iter_init(&mylist, &i);
> +
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &a);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &b);
> + /* Advance the iterator to point to node c*/
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &c);
> +
> + /* Try to delete node c while there is a reference to it*/
> + klist_del(&c);
> +
> + /*
> + * Verify that node c is still attached to the list even after being
> + * deleted. Since the iterator still points to c, the reference count is not
> + * decreased to 0
> + */
> + KUNIT_EXPECT_TRUE(test, klist_node_attached(&c));
> +
> + /* Check that node c has not been removed yet*/
> + KUNIT_EXPECT_EQ(test, node_count, 4);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &d);
> +
> + klist_iter_exit(&i);
> +
> + /* Since the iterator is no longer pointing to node c, node c is removed
> + * from the list
> + */

Nit: comment formatting. The comment above ("Verify that node c...") looks good.

> + KUNIT_EXPECT_EQ(test, node_count, 3);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &c);
> +
> +}
> +
> +/* Verify that klist_del() deletes a node immediately when there are no
> + * other references to it.
> + */

As above, comment formatting.

> +static void klist_test_del_refcount_zero(struct kunit *test)
> +{
> + struct klist_node a, b, c, d;
> + struct klist mylist;
> + struct klist_iter i;
> +
> + node_count = 0;
> + klist_init(&mylist, &check_node, &check_delete_node);
> +
> + /* Add nodes a,b,c,d to the list*/
> + klist_add_tail(&a, &mylist);
> + klist_add_tail(&b, &mylist);
> + klist_add_tail(&c, &mylist);
> + klist_add_tail(&d, &mylist);
> + /* Delete node c*/
> + klist_del(&c);
> +
> + /* Check that node c is deleted from the list*/
> + KUNIT_EXPECT_EQ(test, node_count, 3);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &c);
> +
> + /* Should be [list] -> a -> b -> d*/
> + klist_iter_init(&mylist, &i);
> +
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &a);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &b);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &d);
> + KUNIT_EXPECT_NULL(test, klist_next(&i));
> +
> + klist_iter_exit(&i);
> +
> +}
> +
> +static void klist_test_remove(struct kunit *test)
> +{
> + /* This test doesn't check correctness under concurrent access */
> + struct klist_node a, b, c, d;
> + struct klist mylist;
> + struct klist_iter i;
> +
> + node_count = 0;
> + klist_init(&mylist, &check_node, &check_delete_node);
> +
> + /* Add nodes a,b,c,d to the list*/
> + klist_add_tail(&a, &mylist);
> + klist_add_tail(&b, &mylist);
> + klist_add_tail(&c, &mylist);
> + klist_add_tail(&d, &mylist);
> + /* Delete node c*/
> + klist_remove(&c);
> +
> + /* Check the nodes in the list*/
> + KUNIT_EXPECT_EQ(test, node_count, 3);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &c);
> +
> + /* should be [list] -> a -> b -> d*/
> + klist_iter_init(&mylist, &i);
> +
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &a);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &b);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &d);
> + KUNIT_EXPECT_NULL(test, klist_next(&i));
> +
> + klist_iter_exit(&i);
> +
> +}
> +
> +static void klist_test_node_attached(struct kunit *test)
> +{
> + struct klist_node a = {};
> + struct klist mylist;
> +
> + klist_init(&mylist, NULL, NULL);
> +
> + KUNIT_EXPECT_FALSE(test, klist_node_attached(&a));
> + klist_add_head(&a, &mylist);
> + KUNIT_EXPECT_TRUE(test, klist_node_attached(&a));
> + klist_del(&a);
> + KUNIT_EXPECT_FALSE(test, klist_node_attached(&a));
> +
> +}
> +
> +static struct kunit_case klist_test_cases[] = {
> + KUNIT_CASE(klist_test_add_tail),
> + KUNIT_CASE(klist_test_add_head),
> + KUNIT_CASE(klist_test_add_behind),
> + KUNIT_CASE(klist_test_add_before),
> + KUNIT_CASE(klist_test_del_refcount_greater_than_zero),
> + KUNIT_CASE(klist_test_del_refcount_zero),
> + KUNIT_CASE(klist_test_remove),
> + KUNIT_CASE(klist_test_node_attached),
> + {},
> +};
> +
> +static struct kunit_suite klist_test_module = {
> + .name = "klist",
> + .test_cases = klist_test_cases,
> +};
> +
> +kunit_test_suites(&list_test_module, &hlist_test_module, &klist_test_module);
>
> MODULE_LICENSE("GPL v2");
> --
> 2.40.0.348.gf938b09366-goog
>

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature

Next message: Leon Romanovsky: "Re: [net PATCH 3/7] octeontx2-af: Add validation for lmac type"
Previous message: Greg Kroah-Hartman: "Re: [PATCH 0/5] Remove acpi.h implicit include of of.h"
In reply to: Brendan Higgins: "Re: [PATCH v1] list: test: Test the klist structure"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]