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

* Anjali Kulkarni anjali.k.kulkarni@oracle.com [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@oracle.com

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@oracle.com
• */

+#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@oracle.com
• */

+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