Message-ID: <4DDCECB9-86A9-4796-8A27-34113A79C3EC@oracle.com>
Date: Fri, 18 Oct 2024 00:34:01 +0000
From: Anjali Kulkarni <anjali.k.kulkarni@...cle.com>
To: Stanislav Fomichev <stfomichev@...il.com>
CC: "davem@...emloft.net" <davem@...emloft.net>,
        Liam Howlett
	<liam.howlett@...cle.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "peterz@...radead.org"
	<peterz@...radead.org>,
        "juri.lelli@...hat.com" <juri.lelli@...hat.com>,
        "vincent.guittot@...aro.org" <vincent.guittot@...aro.org>,
        "dietmar.eggemann@....com" <dietmar.eggemann@....com>,
        "rostedt@...dmis.org"
	<rostedt@...dmis.org>,
        "bsegall@...gle.com" <bsegall@...gle.com>,
        "mgorman@...e.de" <mgorman@...e.de>,
        "vschneid@...hat.com"
	<vschneid@...hat.com>,
        "jiri@...nulli.us" <jiri@...nulli.us>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "shuah@...nel.org"
	<shuah@...nel.org>,
        "linux-kselftest@...r.kernel.org"
	<linux-kselftest@...r.kernel.org>,
        Pei Li <peili.io@...cle.com>
Subject: Re: [PATCH net-next v5 2/3] connector/cn_proc: Kunit tests for
 threads hash table



> On Oct 17, 2024, at 5:13 PM, Stanislav Fomichev <stfomichev@...il.com> wrote:
> 
> On 10/17, Anjali Kulkarni wrote:
>> 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@...cle.com>
>> ---
>> drivers/connector/cn_hash.c   |  40 ++++++++
>> drivers/connector/connector.c |  12 +++
>> include/linux/connector.h     |   4 +
>> lib/Kconfig.debug             |  17 ++++
>> lib/Makefile                  |   1 +
>> lib/cn_hash_test.c            | 167 ++++++++++++++++++++++++++++++++++
>> lib/cn_hash_test.h            |  10 ++
>> 7 files changed, 251 insertions(+)
>> 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 a079e9bcea6d..40099b5908ac 100644
>> --- a/drivers/connector/cn_hash.c
>> +++ b/drivers/connector/cn_hash.c
>> @@ -170,6 +170,46 @@ int 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");
>> +
>> + *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)
>> {
>> bool is_empty;
>> diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
>> index c1c0dcec53c0..2be2fe1adc12 100644
>> --- a/drivers/connector/connector.c
>> +++ b/drivers/connector/connector.c
>> @@ -304,6 +304,18 @@ int 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)
>> +{
>> + 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 5384e4bb98e8..a75c3fcf182a 100644
>> --- a/include/linux/connector.h
>> +++ b/include/linux/connector.h
>> @@ -168,4 +168,8 @@ int cn_get_exval(pid_t pid);
>> 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..290cf0a6befa 100644
>> --- a/lib/Kconfig.debug
>> +++ b/lib/Kconfig.debug
>> @@ -2705,6 +2705,23 @@ config HASHTABLE_KUNIT_TEST
>> 
>>  If unsure, say N.
>> 
>> +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 and 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.
>> +
> 
> Looks like this needs to depend on CONFIG_CONNECTOR? Otherwise, the
> existing kunit tester complains about the missing symbols (see below).
> Please also hold off reposting for a couple of days to give people some
> time to review.
> 
> ERROR:root:ld: vmlinux.o: in function `cn_hash_test_dup_del':
> cn_hash_test.c:(.text+0x3e9dc3): undefined reference to `cn_del_get_exval'
> ld: cn_hash_test.c:(.text+0x3e9dee): undefined reference to `cn_del_get_exval'
> ld: cn_hash_test.c:(.text+0x3e9e22): undefined reference to `cn_table_empty'
> ld: vmlinux.o: in function `cn_display_htable':
> cn_hash_test.c:(.text+0x3e9f67): undefined reference to `cn_display_hlist'
> ld: vmlinux.o: in function `cn_hash_test_del_get_exval':
> cn_hash_test.c:(.text+0x3ea037): undefined reference to `cn_del_get_exval'
> ld: cn_hash_test.c:(.text+0x3ea088): undefined reference to `cn_table_empty'
> ld: vmlinux.o: in function `cn_hash_test_dup_add':
> cn_hash_test.c:(.text+0x3ea176): undefined reference to `cn_add_elem'
> ld: cn_hash_test.c:(.text+0x3ea19e): undefined reference to `cn_get_exval'
> ld: cn_hash_test.c:(.text+0x3ea1dc): undefined reference to `cn_add_elem'
> ld: cn_hash_test.c:(.text+0x3ea205): undefined reference to `cn_get_exval'
> ld: vmlinux.o: in function `cn_hash_test_del':
> cn_hash_test.c:(.text+0x3ea387): undefined reference to `cn_del_get_exval'
> ld: cn_hash_test.c:(.text+0x3ea3ab): undefined reference to `cn_get_exval'
> ld: cn_hash_test.c:(.text+0x3ea3fd): undefined reference to `cn_table_empty'
> ld: vmlinux.o: in function `cn_hash_test_add':
> cn_hash_test.c:(.text+0x3ea571): undefined reference to `cn_add_elem'
> ld: cn_hash_test.c:(.text+0x3ea591): undefined reference to `cn_get_exval'
> make[3]: *** [../scripts/Makefile.vmlinux:34: vmlinux] Error 1
> make[2]: *** [/home/kunit/testing/Makefile:1166: vmlinux] Error 2
> make[1]: *** [/home/kunit/testing/Makefile:224: __sub-make] Error 2
> make: *** [Makefile:224: __sub-make] Error 2

Yes, I have added in the comments for CN_HASH_KUNIT_TEST, it depends on:
CONFIG_CONNECTOR, CONFIG_PROC_EVENTS, CONFIG_NET. I didn’t realize
I could add these to the “depends” field.
So something like this: (let me know if you see any issues)

        tristate "KUnit Test for connector hashtable code" if !KUNIT_ALL_TESTS
        depends on KUNIT
+       depends on CONNECTOR && PROC_EVENTS
+       depends on NET
        default KUNIT_ALL_TESTS

These are the configs I add to my .config file and compile it as a module and then
do modprobe to test.
Are you running the kunit tester with kunit.py?

Thanks
Anjali

> 
> ---
> pw-bot: cr

Powered by blists - more mailing lists