Re: [PATCH linux-kselftest/test v4] lib/list-test: add a test for the 'list' doubly linked list
On Sat, Oct 19, 2019 at 1:27 AM Dan Carpenter wrote: > > On Fri, Oct 18, 2019 at 02:55:49PM -0700, David Gow wrote: > > + list4 = kzalloc(sizeof(*list4), GFP_KERNEL); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, list4); > > Why not just use GFP_KERNEL | GFP_NOFAIL and remove the check? I've sent a new version of the patch out (v5) which uses __GFP_NOFAIL instead. The idea had been to exercise KUnit's assertion functionality, in the hope that it'd allow the test to fail (but potentially allow other tests to still run) in the case of allocation failure. Given that we're only allocating enough to store ~4 pointers in total, though, that's probably of little use. > kzalloc() can't return error pointers. If this were an IS_ERR_OR_NULL() > check then it would generate a static checker warning, but static > checkers don't know about KUNIT_ASSERT_NOT_ERR_OR_NULL() yet so you're > safe. Alas, KUnit doesn't have a KUNIT_ASSERT_NOT_NULL() macro, and I'd assumed it was not dangerous (even if not ideal) to check for error pointers, even if kzalloc() can't return them. Perhaps it'd make sense to add a convenient way of checking the NULL-ness of pointers to KUnit (it's possible with the KUNIT_ASSERT_PTR_EQ(), but requires a bit of casting to make the type checker happy) in the future. Once KUnit is properly upstream, it may be worth teaching the static analysis tools about these functions to avoid having warnings in these sorts of tests. For now, though, (and for this test in particular), I agree with the suggestion of just using __GFP_NOFAIL. Thanks a lot for the comments, -- David
Re: [PATCH linux-kselftest/test v4] lib/list-test: add a test for the 'list' doubly linked list
On Fri, Oct 18, 2019 at 02:55:49PM -0700, David Gow wrote:
> Add a KUnit test for the kernel doubly linked list implementation in
> include/linux/list.h
>
> Each test case (list_test_x) is focused on testing the behaviour of the
> list function/macro 'x'. None of the tests pass invalid lists to these
> macros, and so should behave identically with DEBUG_LIST enabled and
> disabled.
>
> Note that, at present, it only tests the list_ types (not the
> singly-linked hlist_), and does not yet test all of the
> list_for_each_entry* macros (and some related things like
> list_prepare_entry).
>
> Signed-off-by: David Gow
> ---
>
> The changes from v3 are mostly to do with naming:
> - The Kconfig entry has been renamed from LIST_TEST to LIST_KUNIT_TEST,
> which matches the SYSCTL_KUNIT_TEST entry,
> - The Kconfig description was updated to better match other KUnit tests,
> specifying that the test is not intended for use in a production
> kernel. A now-redundant mention of the test running a boot was
> removed.
> - The MAINTAINERS entry refers to a "KUNIT TEST" rather than a "UNIT
> TEST"
> - The module name has changed from "list-test" to "list-kunit-test".
>
> Earlier versions of the test can be found:
> v3:
> https://lore.kernel.org/linux-kselftest/20191016215707.95317-1-david...@google.com/
> v2:
> https://lore.kernel.org/linux-kselftest/20191010185631.26541-1-david...@google.com/
> v1:
> https://lore.kernel.org/linux-kselftest/20191007213633.92565-1-david...@google.com/
>
> MAINTAINERS | 5 +
> lib/Kconfig.debug | 18 ++
> lib/Makefile | 3 +
> lib/list-test.c | 740 ++
> 4 files changed, 766 insertions(+)
> create mode 100644 lib/list-test.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7ef985e01457..7ced1b69a3d3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9504,6 +9504,11 @@ F: Documentation/misc-devices/lis3lv02d.rst
> F: drivers/misc/lis3lv02d/
> F: drivers/platform/x86/hp_accel.c
>
> +LIST KUNIT TEST
> +M: David Gow
> +S: Maintained
> +F: lib/list-test.c
> +
> LIVE PATCHING
> M: Josh Poimboeuf
> M: Jiri Kosina
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index a3017a5dadcd..7991b78eb1f3 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1961,6 +1961,24 @@ config SYSCTL_KUNIT_TEST
>
> If unsure, say N.
>
> +config LIST_KUNIT_TEST
> + bool "KUnit Test for Kernel Linked-list structures"
> + depends on KUNIT
> + help
> + This builds the linked list KUnit test suite.
> + It tests that the API and basic functionality of the list_head type
> + and associated macros.
> +
> + KUnit tests run during boot and output the results to the debug log
> + in TAP format (http://testanything.org/). Only useful for kernel devs
> + running the KUnit test harness, and not intended for inclusion into a
> + production build.
> +
> + 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 TEST_UDELAY
> tristate "udelay test driver"
> help
> diff --git a/lib/Makefile b/lib/Makefile
> index bba1fd5485f7..890e581d00c4 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -292,3 +292,6 @@ obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o
> obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o
> obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o
> obj-$(CONFIG_OBJAGG) += objagg.o
> +
> +# KUnit tests
> +obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
> diff --git a/lib/list-test.c b/lib/list-test.c
> new file mode 100644
> index ..75ba3ddac959
> --- /dev/null
> +++ b/lib/list-test.c
> @@ -0,0 +1,740 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit test for the Kernel Linked-list structures.
> + *
> + * Copyright (C) 2019, Google LLC.
> + * Author: David Gow
> + */
> +#include
> +
> +#include
> +
> +struct list_test_struct {
> + int data;
> + struct list_head list;
> +};
> +
> +static void list_test_list_init(struct kunit *test)
> +{
> + /* Test the different ways of initialising a list. */
> + struct list_head list1 = LIST_HEAD_INIT(list1);
> + struct list_head list2;
> + LIST_HEAD(list3);
> + struct list_head *list4;
> + struct list_head *list5;
> +
> + INIT_LIST_HEAD();
> +
> + list4 = kzalloc(sizeof(*list4), GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, list4);
Why not just use GFP_KERNEL | GFP_NOFAIL and remove the check?
kzalloc() can't return error pointers. If this were an IS_ERR_OR_NULL()
check then it would generate a static checker warning, but static
checkers don't know about KUNIT_ASSERT_NOT_ERR_OR_NULL() yet so you're
safe.
But generally NULL is a special case of success. A common situation is
where the user deliberately disables a feature, that means it's not an
error but we also don't have a valid
[PATCH linux-kselftest/test v4] lib/list-test: add a test for the 'list' doubly linked list
Add a KUnit test for the kernel doubly linked list implementation in
include/linux/list.h
Each test case (list_test_x) is focused on testing the behaviour of the
list function/macro 'x'. None of the tests pass invalid lists to these
macros, and so should behave identically with DEBUG_LIST enabled and
disabled.
Note that, at present, it only tests the list_ types (not the
singly-linked hlist_), and does not yet test all of the
list_for_each_entry* macros (and some related things like
list_prepare_entry).
Signed-off-by: David Gow
---
The changes from v3 are mostly to do with naming:
- The Kconfig entry has been renamed from LIST_TEST to LIST_KUNIT_TEST,
which matches the SYSCTL_KUNIT_TEST entry,
- The Kconfig description was updated to better match other KUnit tests,
specifying that the test is not intended for use in a production
kernel. A now-redundant mention of the test running a boot was
removed.
- The MAINTAINERS entry refers to a "KUNIT TEST" rather than a "UNIT
TEST"
- The module name has changed from "list-test" to "list-kunit-test".
Earlier versions of the test can be found:
v3:
https://lore.kernel.org/linux-kselftest/20191016215707.95317-1-david...@google.com/
v2:
https://lore.kernel.org/linux-kselftest/20191010185631.26541-1-david...@google.com/
v1:
https://lore.kernel.org/linux-kselftest/20191007213633.92565-1-david...@google.com/
MAINTAINERS | 5 +
lib/Kconfig.debug | 18 ++
lib/Makefile | 3 +
lib/list-test.c | 740 ++
4 files changed, 766 insertions(+)
create mode 100644 lib/list-test.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 7ef985e01457..7ced1b69a3d3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9504,6 +9504,11 @@ F: Documentation/misc-devices/lis3lv02d.rst
F: drivers/misc/lis3lv02d/
F: drivers/platform/x86/hp_accel.c
+LIST KUNIT TEST
+M: David Gow
+S: Maintained
+F: lib/list-test.c
+
LIVE PATCHING
M: Josh Poimboeuf
M: Jiri Kosina
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index a3017a5dadcd..7991b78eb1f3 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1961,6 +1961,24 @@ config SYSCTL_KUNIT_TEST
If unsure, say N.
+config LIST_KUNIT_TEST
+ bool "KUnit Test for Kernel Linked-list structures"
+ depends on KUNIT
+ help
+ This builds the linked list KUnit test suite.
+ It tests that the API and basic functionality of the list_head type
+ and associated macros.
+
+ KUnit tests run during boot and output the results to the debug log
+ in TAP format (http://testanything.org/). Only useful for kernel devs
+ running the KUnit test harness, and not intended for inclusion into a
+ production build.
+
+ 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 TEST_UDELAY
tristate "udelay test driver"
help
diff --git a/lib/Makefile b/lib/Makefile
index bba1fd5485f7..890e581d00c4 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -292,3 +292,6 @@ obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o
obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o
obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o
obj-$(CONFIG_OBJAGG) += objagg.o
+
+# KUnit tests
+obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
diff --git a/lib/list-test.c b/lib/list-test.c
new file mode 100644
index ..75ba3ddac959
--- /dev/null
+++ b/lib/list-test.c
@@ -0,0 +1,740 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit test for the Kernel Linked-list structures.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: David Gow
+ */
+#include
+
+#include
+
+struct list_test_struct {
+ int data;
+ struct list_head list;
+};
+
+static void list_test_list_init(struct kunit *test)
+{
+ /* Test the different ways of initialising a list. */
+ struct list_head list1 = LIST_HEAD_INIT(list1);
+ struct list_head list2;
+ LIST_HEAD(list3);
+ struct list_head *list4;
+ struct list_head *list5;
+
+ INIT_LIST_HEAD();
+
+ list4 = kzalloc(sizeof(*list4), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, list4);
+ INIT_LIST_HEAD(list4);
+
+ list5 = kmalloc(sizeof(*list5), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, list5);
+ memset(list5, 0xFF, sizeof(*list5));
+ INIT_LIST_HEAD(list5);
+
+ /* list_empty_careful() checks both next and prev. */
+ KUNIT_EXPECT_TRUE(test, list_empty_careful());
+ KUNIT_EXPECT_TRUE(test, list_empty_careful());
+ KUNIT_EXPECT_TRUE(test, list_empty_careful());
+ KUNIT_EXPECT_TRUE(test, list_empty_careful(list4));
+ KUNIT_EXPECT_TRUE(test, list_empty_careful(list5));
+
+ kfree(list4);
+ kfree(list5);
+}
+
+static void list_test_list_add(struct kunit *test)
+{
+ struct list_head a, b;
+
