Re: [PATCH net-next v2 2/3] connector/cn_proc: Kunit tests for threads hash table

From: Liam R. Howlett
Date: Tue Oct 15 2024 - 15:21:59 EST

Next message: Jacob Keller: "Re: [PATCH net-next 3/8] lib: packing: add pack_fields() and unpack_fields()"
Previous message: Frank Li: "[PATCH v5 3/3] PCI: imx6: Remove cpu_addr_fixup()"
In reply to: Anjali Kulkarni: "[PATCH net-next v2 2/3] connector/cn_proc: Kunit tests for threads hash table"
Next in thread: Anjali Kulkarni: "Re: [PATCH net-next v2 2/3] connector/cn_proc: Kunit tests for threads hash table"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

* Anjali Kulkarni <anjali.k.kulkarni@xxxxxxxxxx> [241015 13:30]:
> Kunit tests to test hash table add, delete, duplicate add and delete.
> Add following configs and compile kernel code:
>
> CONFIG_CONNECTOR=y
> CONFIG_PROC_EVENTS=y
> CONFIG_NET=y
> CONFIG_KUNIT=m
> CONFIG_CN_HASH_KUNIT_TEST=m
>
> To run kunit tests:
> sudo modprobe cn_hash_test
>
> Output of kunit tests and hash table contents are displayed in
> /var/log/messages (at KERN_DEBUG level).
>
> Signed-off-by: Anjali Kulkarni <anjali.k.kulkarni@xxxxxxxxxx>
> ---
> drivers/connector/cn_hash.c | 49 +++++++++-
> drivers/connector/connector.c | 15 ++-
> include/linux/connector.h | 8 +-
> lib/Kconfig.debug | 17 ++++
> lib/Makefile | 1 +
> lib/cn_hash_test.c | 167 ++++++++++++++++++++++++++++++++++
> lib/cn_hash_test.h | 12 +++
> 7 files changed, 264 insertions(+), 5 deletions(-)
> create mode 100644 lib/cn_hash_test.c
> create mode 100644 lib/cn_hash_test.h
>
> diff --git a/drivers/connector/cn_hash.c b/drivers/connector/cn_hash.c
> index a0211cd99132..8f0eb6acb158 100644
> --- a/drivers/connector/cn_hash.c
> +++ b/drivers/connector/cn_hash.c
> @@ -166,7 +166,7 @@ __u32 cn_hash_del_get_exval(struct cn_hash_dev *hdev, pid_t pid)
> return 0;
> }
>
> -__u32 cn_hash_get_exval(struct cn_hash_dev *hdev, pid_t pid)
> +int cn_hash_get_exval(struct cn_hash_dev *hdev, pid_t pid)

Is this return type change supposed to be here?

> {
> struct uexit_pid_hnode *hnode;
> __u32 excde;
> @@ -189,7 +189,52 @@ __u32 cn_hash_get_exval(struct cn_hash_dev *hdev, pid_t pid)
> return -EINVAL;
> }
>
> +int cn_hash_display_hlist(struct cn_hash_dev *hdev, pid_t pid, int max_len,
> + int *hkey, int *key_display)
> +{
> + struct uexit_pid_hnode *hnode;
> + int key, count = 0;
> +
> + mutex_lock(&hdev->uexit_hash_lock);
> + key = hash_min(pid, HASH_BITS(hdev->uexit_pid_htable));
> + pr_debug("Bucket: %d\n", key);
> +
> + hlist_for_each_entry(hnode,
> + &hdev->uexit_pid_htable[key],
> + uexit_pid_hlist) {
> + if (key_display[key] != 1) {
> + if (hnode->uexit_pid_hlist.next == NULL)
> + pr_debug("pid %d ", hnode->pid);
> + else
> + pr_debug("pid %d --> ", hnode->pid);
> + }
> + count++;
> + }
> +
> + mutex_unlock(&hdev->uexit_hash_lock);
> +
> + if ((key_display[key] != 1) && !count)
> + pr_debug("(empty)\n");
> +
> + pr_debug("\n");

It seems like you maybe added an extra \n if you print the (empty) line
above?

> +
> + *hkey = key;
> +
> + if (count > max_len) {
> + pr_err("%d entries in hlist for key %d, expected %d\n",
> + count, key, max_len);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> bool cn_hash_table_empty(struct cn_hash_dev *hdev)
> {
> - return hash_empty(hdev->uexit_pid_htable);
> + bool is_empty;
> +
> + is_empty = hash_empty(hdev->uexit_pid_htable);
> + pr_debug("Hash table is %s\n", (is_empty ? "empty" : "not empty"));
> +
> + return is_empty;

It seems odd to have this change in the kunit change.

> }
> diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
> index 506e3cbedf85..28e60c8b0fdf 100644
> --- a/drivers/connector/connector.c
> +++ b/drivers/connector/connector.c
> @@ -308,7 +308,7 @@ int cn_add_elem(__u32 uexit_code, pid_t pid)
> }
> EXPORT_SYMBOL_GPL(cn_add_elem);
>
> -__u32 cn_get_exval(pid_t pid)
> +int cn_get_exval(pid_t pid)
> {
> struct cn_dev *dev = &cdev;
> __u32 exval;
> @@ -321,6 +321,19 @@ __u32 cn_get_exval(pid_t pid)
> }
> EXPORT_SYMBOL_GPL(cn_get_exval);
>
> +int cn_display_hlist(pid_t pid, int max_len, int *hkey,
> + int *key_display)

This can probably be one line here.

> +{
> + struct cn_dev *dev = &cdev;
> +
> + if (!cn_already_initialized)
> + return 0;
> +
> + return cn_hash_display_hlist(dev->hdev, pid, max_len,
> + hkey, key_display);
> +}
> +EXPORT_SYMBOL_GPL(cn_display_hlist);
> +
> bool cn_table_empty(void)
> {
> struct cn_dev *dev = &cdev;
> diff --git a/include/linux/connector.h b/include/linux/connector.h
> index 094e1730a4f6..af801c5005e8 100644
> --- a/include/linux/connector.h
> +++ b/include/linux/connector.h
> @@ -165,7 +165,7 @@ __u32 cn_hash_del_get_exval(struct cn_hash_dev *hdev, pid_t pid);
> int cn_add_elem(__u32 uexit_code, pid_t pid);
> int cn_del_elem(pid_t pid);
> __u32 cn_del_get_exval(pid_t pid);
> -__u32 cn_get_exval(pid_t pid);
> +int cn_get_exval(pid_t pid);
>
> struct cn_hash_dev *cn_hash_alloc_dev(const char *name);
> void cn_hash_free_dev(struct cn_hash_dev *hdev);
> @@ -175,9 +175,13 @@ void cn_hash_free_elem(struct uexit_pid_hnode *elem);
> int cn_hash_add_elem(struct cn_hash_dev *hdev, __u32 uexit_code, pid_t pid);
> int cn_hash_del_elem(struct cn_hash_dev *hdev, pid_t pid);
> __u32 cn_hash_del_get_exval(struct cn_hash_dev *hdev, pid_t pid);
> -__u32 cn_hash_get_exval(struct cn_hash_dev *hdev, pid_t pid);
> +int cn_hash_get_exval(struct cn_hash_dev *hdev, pid_t pid);

It seems like the change in type should be with another change or on its
own?

>
> bool cn_table_empty(void);
> bool cn_hash_table_empty(struct cn_hash_dev *hdev);
>
> +int cn_display_hlist(pid_t pid, int max_len, int *hkey, int *key_display);
> +int cn_hash_display_hlist(struct cn_hash_dev *hdev, pid_t pid, int max_len,
> + int *hkey, int *key_display);
> +
> #endif /* __CONNECTOR_H */
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 7315f643817a..23599beb9bec 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2705,6 +2705,23 @@ config HASHTABLE_KUNIT_TEST
>
> If unsure, say N.
>
> +config CONFIG_CN_HASH_KUNIT_TEST
> + tristate "KUnit Test for connector hashtable code" if !KUNIT_ALL_TESTS
> + depends on KUNIT
> + default KUNIT_ALL_TESTS
> + help
> + This builds the hashtable KUnit test suite.
> + It tests the basic functionality of the API defined in
> + drivers/connector/cn_hash.c.
> + CONFIG_CONNECTOR=y, CONFIG_PROC_EVENTS=y as well as CONFIG_NET=y
> + needs to be enabled along with CONFIG_CN_HASH_KUNIT_TEST=m and
> + CONFIG_KUNIT=m in .config file to compile and then test as a kernel
> + module with "modprobe cn_hash_test".
> + 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.
> +
> config LINEAR_RANGES_TEST
> tristate "KUnit test for linear_ranges"
> depends on KUNIT
> diff --git a/lib/Makefile b/lib/Makefile
> index 811ba12c8cd0..2c59c82b0b18 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -379,6 +379,7 @@ obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o
> obj-$(CONFIG_SLUB_KUNIT_TEST) += slub_kunit.o
> obj-$(CONFIG_MEMCPY_KUNIT_TEST) += memcpy_kunit.o
> obj-$(CONFIG_IS_SIGNED_TYPE_KUNIT_TEST) += is_signed_type_kunit.o
> +obj-$(CONFIG_CN_HASH_KUNIT_TEST) += cn_hash_test.o
> CFLAGS_overflow_kunit.o = $(call cc-disable-warning, tautological-constant-out-of-range-compare)
> obj-$(CONFIG_OVERFLOW_KUNIT_TEST) += overflow_kunit.o
> CFLAGS_stackinit_kunit.o += $(call cc-disable-warning, switch-unreachable)
> diff --git a/lib/cn_hash_test.c b/lib/cn_hash_test.c
> new file mode 100644
> index 000000000000..78c39c0a0997
> --- /dev/null
> +++ b/lib/cn_hash_test.c
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit test for the connector threads hashtable code.
> + *
> + * Copyright (c) 2024 Oracle and/or its affiliates.
> + * Author: Anjali Kulkarni <anjali.k.kulkarni@xxxxxxxxxx>
> + */
> +#include <kunit/test.h>
> +
> +#include "cn_hash_test.h"
> +
> +#define ARR_SIZE 4
> +#define HASH_TABLE_LEN 1024
> +
> +struct add_data {
> + pid_t pid;
> + int exit_val;
> + int key;
> +};
> +
> +static struct add_data adata[ARR_SIZE];
> +static int key_display[HASH_TABLE_LEN];
> +
> +static int cn_hash_init(struct kunit *test)
> +{
> + for (int i = 0; i < HASH_TABLE_LEN; i++)
> + key_display[i] = 0;
> +
> + return 0;
> +}
> +
> +static void cn_display_htable(struct kunit *test, int len)
> +{
> + int i, err;
> +
> + cn_hash_init(test);
> +
> + pr_debug("\n");
> + pr_debug("Displaying hash table:\n");
> +
> + for (i = 0; i < len; i++) {
> + err = cn_display_hlist(adata[i].pid, len, &adata[i].key,
> + key_display);
> + key_display[adata[i].key] = 1;
> + KUNIT_EXPECT_EQ(test, err, 0);
> + }
> +}
> +
> +static void cn_hash_test_add(struct kunit *test)
> +{
> + int err, i;
> + int exit_val;
> +
> + adata[0].pid = 1;
> + adata[0].exit_val = 45;
> +
> + adata[1].pid = 2;
> + adata[1].exit_val = 13;
> +
> + adata[2].pid = 1024;
> + adata[2].exit_val = 16;
> +
> + adata[3].pid = 1023;
> + adata[3].exit_val = 71;
> +
> + for (i = 0; i < ARRAY_SIZE(adata); i++) {
> + err = cn_add_elem(adata[i].exit_val, adata[i].pid);
> + KUNIT_EXPECT_EQ_MSG(test, 0, err,
> + "Adding pid %d returned err %d",
> + adata[i].pid, err);
> +
> + exit_val = cn_get_exval(adata[i].pid);
> + KUNIT_EXPECT_EQ(test, adata[i].exit_val, exit_val);
> + }
> +
> + cn_display_htable(test, ARRAY_SIZE(adata));
> +}
> +
> +static void cn_hash_test_del(struct kunit *test)
> +{
> + int i, err;
> + int exit_val;
> +
> + for (i = 0; i < ARRAY_SIZE(adata); i++) {
> + err = cn_del_elem(adata[i].pid);
> + KUNIT_EXPECT_EQ_MSG(test, 0, err,
> + "Deleting pid %d returned err %d",
> + adata[i].pid, err);
> +
> + exit_val = cn_get_exval(adata[i].pid);
> + KUNIT_EXPECT_EQ(test, -EINVAL, exit_val);
> + }
> +
> + cn_display_htable(test, ARRAY_SIZE(adata));
> + KUNIT_EXPECT_TRUE(test, cn_table_empty());
> +}
> +
> +static void cn_hash_test_del_get_exval(struct kunit *test)
> +{
> + int i, exval;
> +
> + for (i = 0; i < ARRAY_SIZE(adata); i++) {
> + exval = cn_del_get_exval(adata[i].pid);
> + KUNIT_EXPECT_EQ(test, adata[i].exit_val, exval);
> +
> + cn_display_htable(test, ARRAY_SIZE(adata));
> + }
> +
> + KUNIT_EXPECT_TRUE(test, cn_table_empty());
> +}
> +static void cn_hash_test_dup_add(struct kunit *test)
> +{
> + int err, exit_val;
> +
> + adata[0].pid = 10;
> + adata[0].exit_val = 21;
> +
> + err = cn_add_elem(adata[0].exit_val, adata[0].pid);
> + KUNIT_EXPECT_EQ(test, 0, err);
> +
> + exit_val = cn_get_exval(adata[0].pid);
> + KUNIT_EXPECT_EQ(test, 21, exit_val);
> +
> + adata[1].pid = 10;
> + adata[1].exit_val = 12;
> +
> + err = cn_add_elem(adata[1].exit_val, adata[1].pid);
> + KUNIT_EXPECT_EQ(test, -EEXIST, err);
> +
> + exit_val = cn_get_exval(adata[1].pid);
> + KUNIT_EXPECT_EQ(test, 21, exit_val);
> +
> + cn_display_htable(test, 1);
> +}
> +
> +static void cn_hash_test_dup_del(struct kunit *test)
> +{
> + int err;
> +
> + err = cn_del_elem(adata[0].pid);
> + KUNIT_EXPECT_EQ(test, 0, err);
> +
> + err = cn_del_elem(adata[0].pid);
> + KUNIT_EXPECT_EQ(test, -EINVAL, err);
> +
> + KUNIT_EXPECT_TRUE(test, cn_table_empty());
> +}
> +
> +static struct kunit_case cn_hashtable_test_cases[] = {
> + KUNIT_CASE(cn_hash_test_add),
> + KUNIT_CASE(cn_hash_test_del),
> + KUNIT_CASE(cn_hash_test_dup_add),
> + KUNIT_CASE(cn_hash_test_dup_del),
> + KUNIT_CASE(cn_hash_test_add),
> + KUNIT_CASE(cn_hash_test_del_get_exval),
> + {},
> +};
> +
> +static struct kunit_suite cn_hashtable_test_module = {
> + .name = "cn_hashtable",
> + .init = cn_hash_init,
> + .test_cases = cn_hashtable_test_cases,
> +};
> +kunit_test_suite(cn_hashtable_test_module);
> +
> +MODULE_DESCRIPTION("KUnit test for the connector threads hashtable code");
> +MODULE_LICENSE("GPL");
> diff --git a/lib/cn_hash_test.h b/lib/cn_hash_test.h
> new file mode 100644
> index 000000000000..46fcda31b25c
> --- /dev/null
> +++ b/lib/cn_hash_test.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (c) 2024 Oracle and/or its affiliates.
> + * Author: Anjali Kulkarni <anjali.k.kulkarni@xxxxxxxxxx>
> + */
> +extern int cn_display_hlist(pid_t pid, int max_len, int *hkey,
> + int *key_display);
> +extern int cn_del_elem(pid_t pid);
> +extern int cn_add_elem(__u32 uexit_code, pid_t pid);
> +extern __u32 cn_del_get_exval(pid_t pid);
> +extern int cn_get_exval(pid_t pid);
> +extern bool cn_table_empty(void);

I think we are not adding externs to headers anymore.

> --
> 2.46.0
>

Next message: Jacob Keller: "Re: [PATCH net-next 3/8] lib: packing: add pack_fields() and unpack_fields()"
Previous message: Frank Li: "[PATCH v5 3/3] PCI: imx6: Remove cpu_addr_fixup()"
In reply to: Anjali Kulkarni: "[PATCH net-next v2 2/3] connector/cn_proc: Kunit tests for threads hash table"
Next in thread: Anjali Kulkarni: "Re: [PATCH net-next v2 2/3] connector/cn_proc: Kunit tests for threads hash table"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]