Re: [PATCH v8 07/10] lib: add basic KUnit test for lib/math
On Mon, May 20, 2024 at 07:51:24PM +0530, Devarsh Thakkar wrote: > On 20/05/24 17:52, Andy Shevchenko wrote: > > On Mon, May 20, 2024 at 05:11:18PM +0530, Devarsh Thakkar wrote: > >> On 18/05/24 01:44, Andy Shevchenko wrote: > >>> On Fri, May 17, 2024 at 11:06:07PM +0530, Devarsh Thakkar wrote: [..] > > Yes, and one should follow IWYU principle and not cargo cult or whatever > > arbitrary lists. > > Agreed. > +#include > >>> > >>> + math.h // obviously > >>> + module.h > >>> > +#include > >>> > >>> + types.h > >> > >> All the above headers are already included as part of kernel.h > > > > Yes, that's why you should not use "proxy" headers. > > Have you read the top comment in the kernel.h? > > Yes, it says it is not recommended to include this inside another header file. > Although here we are adding it inside c file, but I can still try avoid it and > include only the required headers instead of kernel.h as you recommended. Right, but the first sentence there is "This header has combined a lot of unrelated to each other stuff." Can you explain how you use in your code all that unrelated stuff? For example, how do you use *trace_*() calls? Or maybe might_*() calls? or anything else that is directly provided by kernel.h? Besides IWYU principle above, it's good to have a justification for each inclusion the C file has. I believe there is no a such in _this_ case. -- With Best Regards, Andy Shevchenko
Re: [PATCH v8 07/10] lib: add basic KUnit test for lib/math
On 20/05/24 17:52, Andy Shevchenko wrote: > On Mon, May 20, 2024 at 05:11:18PM +0530, Devarsh Thakkar wrote: >> On 18/05/24 01:44, Andy Shevchenko wrote: >>> On Fri, May 17, 2024 at 11:06:07PM +0530, Devarsh Thakkar wrote: > > [..] > [..] > Yes, and one should follow IWYU principle and not cargo cult or whatever > arbitrary lists. > Agreed. +#include >>> >>> + math.h // obviously >>> + module.h >>> +#include >>> >>> + types.h >> >> All the above headers are already included as part of kernel.h > > Yes, that's why you should not use "proxy" headers. > Have you read the top comment in the kernel.h? > Yes, it says it is not recommended to include this inside another header file. Although here we are adding it inside c file, but I can still try avoid it and include only the required headers instead of kernel.h as you recommended. Regards Devarsh
Re: [PATCH v8 07/10] lib: add basic KUnit test for lib/math
On Mon, May 20, 2024 at 05:11:18PM +0530, Devarsh Thakkar wrote: > On 18/05/24 01:44, Andy Shevchenko wrote: > > On Fri, May 17, 2024 at 11:06:07PM +0530, Devarsh Thakkar wrote: [..] > >> +#include > >> +#include > > > >> +#include > > > > Do you know why this header is included? > > It includes all the other required headers (including those you mentioned > below), and header list is probably copied from other files in same directory. > But it does suffice the requirements as I have verified the compilation. Yes, and one should follow IWYU principle and not cargo cult or whatever arbitrary lists. > >> +#include > > > > + math.h // obviously > > + module.h > > > >> +#include > > > > + types.h > > All the above headers are already included as part of kernel.h Yes, that's why you should not use "proxy" headers. Have you read the top comment in the kernel.h? -- With Best Regards, Andy Shevchenko
Re: [PATCH v8 07/10] lib: add basic KUnit test for lib/math
Hi Andy, Daniel, On 18/05/24 01:44, Andy Shevchenko wrote: > On Fri, May 17, 2024 at 11:06:07PM +0530, Devarsh Thakkar wrote: [..] > >> [devarsht: Rebase to 6.9 and change license to GPL] > > I'm not sure that you may change license. It needs the author's confirmation. > As per latest licensing doc [1], It quotes that GPL is same as GPL v2 and GPL v2 exists only for historical reasons as shared below : “GPL v2 Same as “GPL”. It exists for historic reasons." So I think it should be fine and more apt to use GPL. This is also highlighted in below commit: bf7fbeeae6db644ef5995085de2bc5c6121f8c8d module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity >> --- >> Changes since v6: >> * Rebase to linux-next, change license to GPL as suggested by checkpatch. > > Note, checkpatch.pl is not false positives free. Be careful > with what it suggests. > >> +#include >> +#include > >> +#include > > Do you know why this header is included? > It includes all the other required headers (including those you mentioned below), and header list is probably copied from other files in same directory. But it does suffice the requirements as I have verified the compilation. >> +#include > > + math.h // obviously > + module.h > >> +#include > > + types.h > All the above headers are already included as part of kernel.h Kindly let me know if any queries. [1]: https://docs.kernel.org/process/license-rules.html Regards Devarsh
Re: [PATCH v8 07/10] lib: add basic KUnit test for lib/math
On Fri, May 17, 2024 at 01:53:47PM -0700, Daniel Latypov wrote:
> On Fri, May 17, 2024 at 1:14 PM Andy Shevchenko
> wrote:
...
> > > [devarsht: Rebase to 6.9 and change license to GPL]
> >
> > I'm not sure that you may change license. It needs the author's
> > confirmation.
>
> Checking, this is referring to the MODULE_LICENSE, which used to be
> MODULE_LICENSE("GPL v2");
>
> and is now
> MODULE_LICENSE("GPL");
>
> If checkpatch is suggesting that now, then changing it sounds good to me.
In this case I agree on the change as it's purely syntax and not semantic.
> > > ---
> > > Changes since v6:
> > > * Rebase to linux-next, change license to GPL as suggested by checkpatch.
> >
> > Note, checkpatch.pl is not false positives free. Be careful
> > with what it suggests.
> >
> > > +#include
> > > +#include
> >
> > > +#include
> >
> > Do you know why this header is included?
>
> I think I had put it in the original before a lot of the work you did
> to split things out of kernel.h.
> I haven't had time to look apply this patch series locally yet, but
> I'd be pretty sure we can remove it without anything breaking.
Briefly looking at the code I even not sure it was needed before, but maybe
I missed something, in any case, please remove / replace it.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v8 07/10] lib: add basic KUnit test for lib/math
On Fri, May 17, 2024 at 1:14 PM Andy Shevchenko
wrote:
> > [devarsht: Rebase to 6.9 and change license to GPL]
>
> I'm not sure that you may change license. It needs the author's confirmation.
Checking, this is referring to the MODULE_LICENSE, which used to be
MODULE_LICENSE("GPL v2");
and is now
MODULE_LICENSE("GPL");
If checkpatch is suggesting that now, then changing it sounds good to me.
>
> > ---
> > Changes since v6:
> > * Rebase to linux-next, change license to GPL as suggested by checkpatch.
>
> Note, checkpatch.pl is not false positives free. Be careful
> with what it suggests.
>
> > +#include
> > +#include
>
> > +#include
>
> Do you know why this header is included?
I think I had put it in the original before a lot of the work you did
to split things out of kernel.h.
I haven't had time to look apply this patch series locally yet, but
I'd be pretty sure we can remove it without anything breaking.
Thanks,
Daniel
Re: [PATCH v8 07/10] lib: add basic KUnit test for lib/math
On Fri, May 17, 2024 at 11:06:07PM +0530, Devarsh Thakkar wrote: > From: Daniel Latypov > > Add basic test coverage for files that don't require any config options: > * part of math.h (what seem to be the most commonly used macros) > * gcd.c > * lcm.c > * int_sqrt.c > * reciprocal_div.c > (Ignored int_pow.c since it's a simple textbook algorithm.) > > These tests aren't particularly interesting, but they > * provide short and simple examples of parameterized tests > * provide a place to add tests for any new files in this dir > * are written so adding new test cases to cover edge cases should be > easy > * looking at code coverage, we hit all the branches in the .c files ... > [devarsht: Rebase to 6.9 and change license to GPL] I'm not sure that you may change license. It needs the author's confirmation. > --- > Changes since v6: > * Rebase to linux-next, change license to GPL as suggested by checkpatch. Note, checkpatch.pl is not false positives free. Be careful with what it suggests. > +#include > +#include > +#include Do you know why this header is included? > +#include + math.h // obviously + module.h > +#include + types.h ... Other than above, LGTM. -- With Best Regards, Andy Shevchenko
[PATCH v8 07/10] lib: add basic KUnit test for lib/math
From: Daniel Latypov
Add basic test coverage for files that don't require any config options:
* part of math.h (what seem to be the most commonly used macros)
* gcd.c
* lcm.c
* int_sqrt.c
* reciprocal_div.c
(Ignored int_pow.c since it's a simple textbook algorithm.)
These tests aren't particularly interesting, but they
* provide short and simple examples of parameterized tests
* provide a place to add tests for any new files in this dir
* are written so adding new test cases to cover edge cases should be
easy
* looking at code coverage, we hit all the branches in the .c files
Signed-off-by: Daniel Latypov
Reviewed-by: David Gow
[devarsht: Rebase to 6.9 and change license to GPL]
Signed-off-by: Devarsh Thakkar
---
Changes since v6:
* Rebase to linux-next, change license to GPL as suggested by checkpatch.
Changes since v5:
* add in test cases for roundup/rounddown
* address misc comments from David
Changes since v4:
* add in test cases for some math.h macros (abs, round_up/round_down,
div_round_down/closest)
* use parameterized testing less to keep things terser
Changes since v3:
* fix `checkpatch.pl --strict` warnings
* add test cases for gcd(0,0) and lcm(0,0)
* minor: don't test both gcd(a,b) and gcd(b,a) when a == b
Changes since v2: mv math_test.c => math_kunit.c
Changes since v1:
* Rebase and rewrite to use the new parameterized testing support.
* misc: fix overflow in literal and inline int_sqrt format string.
* related: commit 1f0e943df68a ("Documentation: kunit: provide guidance
for testing many inputs") was merged explaining the patterns shown here.
* there's an in-flight patch to update it for parameterized testing.
---
lib/math/Kconfig | 11 ++
lib/math/Makefile | 1 +
lib/math/math_kunit.c | 291 ++
3 files changed, 303 insertions(+)
create mode 100644 lib/math/math_kunit.c
diff --git a/lib/math/Kconfig b/lib/math/Kconfig
index 0634b428d0cb..832c6989ca13 100644
--- a/lib/math/Kconfig
+++ b/lib/math/Kconfig
@@ -15,3 +15,14 @@ config PRIME_NUMBERS
config RATIONAL
tristate
+
+config MATH_KUNIT_TEST
+ tristate "KUnit test for math helper functions"
+ help
+ This builds unit test for math helper functions as defined in lib/math
+ and math.h.
+
+ 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.
diff --git a/lib/math/Makefile b/lib/math/Makefile
index 91fcdb0c9efe..dcf65d10dab2 100644
--- a/lib/math/Makefile
+++ b/lib/math/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_RATIONAL) += rational.o
obj-$(CONFIG_TEST_DIV64) += test_div64.o
obj-$(CONFIG_RATIONAL_KUNIT_TEST) += rational-test.o
+obj-$(CONFIG_MATH_KUNIT_TEST) += math_kunit.o
diff --git a/lib/math/math_kunit.c b/lib/math/math_kunit.c
new file mode 100644
index ..1b00e4195a1a
--- /dev/null
+++ b/lib/math/math_kunit.c
@@ -0,0 +1,291 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Simple KUnit suite for math helper funcs that are always enabled.
+ *
+ * Copyright (C) 2020, Google LLC.
+ * Author: Daniel Latypov
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+static void abs_test(struct kunit *test)
+{
+ KUNIT_EXPECT_EQ(test, abs((char)0), (char)0);
+ KUNIT_EXPECT_EQ(test, abs((char)42), (char)42);
+ KUNIT_EXPECT_EQ(test, abs((char)-42), (char)42);
+
+ /* The expression in the macro is actually promoted to an int. */
+ KUNIT_EXPECT_EQ(test, abs((short)0), 0);
+ KUNIT_EXPECT_EQ(test, abs((short)42), 42);
+ KUNIT_EXPECT_EQ(test, abs((short)-42), 42);
+
+ KUNIT_EXPECT_EQ(test, abs(0), 0);
+ KUNIT_EXPECT_EQ(test, abs(42), 42);
+ KUNIT_EXPECT_EQ(test, abs(-42), 42);
+
+ KUNIT_EXPECT_EQ(test, abs(0L), 0L);
+ KUNIT_EXPECT_EQ(test, abs(42L), 42L);
+ KUNIT_EXPECT_EQ(test, abs(-42L), 42L);
+
+ KUNIT_EXPECT_EQ(test, abs(0LL), 0LL);
+ KUNIT_EXPECT_EQ(test, abs(42LL), 42LL);
+ KUNIT_EXPECT_EQ(test, abs(-42LL), 42LL);
+
+ /* Unsigned types get casted to signed. */
+ KUNIT_EXPECT_EQ(test, abs(0ULL), 0LL);
+ KUNIT_EXPECT_EQ(test, abs(42ULL), 42LL);
+}
+
+static void int_sqrt_test(struct kunit *test)
+{
+ KUNIT_EXPECT_EQ(test, int_sqrt(0UL), 0UL);
+ KUNIT_EXPECT_EQ(test, int_sqrt(1UL), 1UL);
+ KUNIT_EXPECT_EQ(test, int_sqrt(4UL), 2UL);
+ KUNIT_EXPECT_EQ(test, int_sqrt(5UL), 2UL);
+ KUNIT_EXPECT_EQ(test, int_sqrt(8UL), 2UL);
+ KUNIT_EXPECT_EQ(test, int_sqrt(1UL << 30), 1UL << 15);
+}
+
+static void round_up_test(struct kunit *test)
+{
+ KUNIT_EXPECT_EQ(test, round_up(0, 1), 0);
+ KUNIT_EXPECT_EQ(test, round_up(1, 2), 2);
+ KUNIT_EXPECT_EQ(test, round_up(3, 2), 4);
+ KUNIT_EXPECT_EQ(test, round_up((1 << 30) - 1, 2), 1 << 30);
+ KUNIT_EXPECT_EQ(test, round_up((1 << 30) - 1, 1 << 29), 1 << 30);
+}
+
+static voi
