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

[Anjali Kulkarni <anjali.k.kulkarni@oracle.com>]

> On Oct 22, 2024, at 4:50 PM, Stanislav Fomichev <stfomichev@gmail.com> wrote:
> 
> On 10/22, Anjali Kulkarni wrote:
>> 
>> 
>>> On Oct 17, 2024, at 5:55 PM, Stanislav Fomichev <stfomichev@gmail.com> wrote:
>>> 
>>> On 10/18, Anjali Kulkarni wrote:
>>>> 
>>>> 
>>>>> On Oct 17, 2024, at 5:13 PM, Stanislav Fomichev <stfomichev@gmail.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@oracle.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?
>>> 
>>> Yes, make sure all required options are picked up by
>>> "./tools/testing/kunit/kunit.py run" instead of manually adding options
>>> and doing modprobe.
>> 
>> The environment issues are resolved and I am able to run kunit.py, but my tests
>> are not invoked without giving options via —kconfig-add. Other tests are also not
>> invoked. Running with the manual options runs 413 tests, and with just kunit.py
>> runs 389 tests. (I have added 6). Any idea how I can make it run my tests?
> 
> The runner does: ./tools/testing/kunit/kunit.py run --alltests
> Is it not enough in your case? What options do you pass via
> --kconfig-add? Is it because CONNECTOR stuff is disabled by default?

No, it still does not run.
However, I added to tools/testing/kunit/configs/all_tests.config:

CONFIG_CONNECTOR=y
CONFIG_PROC_EVENTS=y
CONFIG_NET=y
CONFIG_CN_HASH_KUNIT_TEST=y

And now it does run.
Should I make the change above? I will also check with the kunit guys.
But I do not understand how it ran for you(and run into the error), or did
it just try to compile?

Anjali