| Message ID | 20221021060340.7515-1-w@1wt.eu |
|---|---|
| State | Accepted |
| Commit | c80b5a0a22b673d5a02e64626a8dfc2f738be7d9 |
| Headers | show |
| Series | selftests/nolibc: add 7 tests for memcmp() | expand |
Hi Paul, On Mon, Oct 24, 2022 at 08:53:57AM -0700, Paul E. McKenney wrote: > > Will keep thinking about it and hopefully propose a patch to make the > > tests easier to use before we're too far in the 6.1 release. > > Another possibility is to have a separate developers' and maintainers' > option. Linus and I do "make whatever" for some value of "whatever" > that builds from scratch, doing whatever cleaning that might be required. > Developers use targets that are faster but have the possibility of false > positives and false negatives. > > But maybe you have something better in mind. > > > Thanks for keeping the conversation flowing, that helps me! > > Looking forward to seeing what you come up with! I could finally figure what was taking time in the installation process. Interestingly, it's "make headers", which is not redone without a "make clean" at the kernel level. The "make headers_install" only takes a few hundred milliseconds, so issuing a systematic "make clean" in the nolibc test dir only takes ~800ms here to perform a full rebuild, which is totally acceptable to me. Thus what I've done is to mark the sysroot target as .phony and start it with a removal of the current include dir so that we systematically rebuild it. Now there's no such risk of running a test against an earlier version anymore, and there are no "make clean" to worry about anymore either. That looks much better to me! And I could confirm that just issuing: $ time make -j8 -C tools/testing/selftests/nolibc run after reverting Rasmus' fix led me to this pretty quickly: ... Kernel: arch/x86/boot/bzImage is ready (#3) make[1]: Leaving directory '/k' 15 memcmp_20_e0 = 64 [FAIL] 16 memcmp_e0_20 = -64 [FAIL] See all results in /k/tools/testing/selftests/nolibc/run.out make: Leaving directory '/k/tools/testing/selftests/nolibc' real 0m14.538s user 0m27.828s sys 0m4.576s No more false positives nor false negatives anymore. I'm sending you the patch separately. Thanks for the discussion, the solution is way better now! Willy
On 26/10/2022 07.39, Willy Tarreau wrote: > > No more false positives nor false negatives anymore. I'm sending you > the patch separately. While you're at it, may I suggest also adding a few test cases where the buffers differ by 128, e.g. 0x0 v 0x80 and 0x40 v 0xc0.
On Wed, Oct 26, 2022 at 07:39:22AM +0200, Willy Tarreau wrote: > Hi Paul, > > On Mon, Oct 24, 2022 at 08:53:57AM -0700, Paul E. McKenney wrote: > > > Will keep thinking about it and hopefully propose a patch to make the > > > tests easier to use before we're too far in the 6.1 release. > > > > Another possibility is to have a separate developers' and maintainers' > > option. Linus and I do "make whatever" for some value of "whatever" > > that builds from scratch, doing whatever cleaning that might be required. > > Developers use targets that are faster but have the possibility of false > > positives and false negatives. > > > > But maybe you have something better in mind. > > > > > Thanks for keeping the conversation flowing, that helps me! > > > > Looking forward to seeing what you come up with! > > I could finally figure what was taking time in the installation process. > Interestingly, it's "make headers", which is not redone without a "make > clean" at the kernel level. The "make headers_install" only takes a few > hundred milliseconds, so issuing a systematic "make clean" in the nolibc > test dir only takes ~800ms here to perform a full rebuild, which is totally > acceptable to me. > > Thus what I've done is to mark the sysroot target as .phony and start it > with a removal of the current include dir so that we systematically rebuild > it. Now there's no such risk of running a test against an earlier version > anymore, and there are no "make clean" to worry about anymore either. > That looks much better to me! > > And I could confirm that just issuing: > > $ time make -j8 -C tools/testing/selftests/nolibc run > > after reverting Rasmus' fix led me to this pretty quickly: > > ... > Kernel: arch/x86/boot/bzImage is ready (#3) > make[1]: Leaving directory '/k' > 15 memcmp_20_e0 = 64 [FAIL] > 16 memcmp_e0_20 = -64 [FAIL] > See all results in /k/tools/testing/selftests/nolibc/run.out > make: Leaving directory '/k/tools/testing/selftests/nolibc' > > real 0m14.538s > user 0m27.828s > sys 0m4.576s > > No more false positives nor false negatives anymore. I'm sending you > the patch separately. > > Thanks for the discussion, the solution is way better now! Nice, looking forward to the patch! Thanx, Paul
On Wed, Oct 26, 2022 at 06:57:33AM -0700, Paul E. McKenney wrote: > On Wed, Oct 26, 2022 at 07:39:22AM +0200, Willy Tarreau wrote: > > Hi Paul, > > > > On Mon, Oct 24, 2022 at 08:53:57AM -0700, Paul E. McKenney wrote: > > > > Will keep thinking about it and hopefully propose a patch to make the > > > > tests easier to use before we're too far in the 6.1 release. > > > > > > Another possibility is to have a separate developers' and maintainers' > > > option. Linus and I do "make whatever" for some value of "whatever" > > > that builds from scratch, doing whatever cleaning that might be required. > > > Developers use targets that are faster but have the possibility of false > > > positives and false negatives. > > > > > > But maybe you have something better in mind. > > > > > > > Thanks for keeping the conversation flowing, that helps me! > > > > > > Looking forward to seeing what you come up with! > > > > I could finally figure what was taking time in the installation process. > > Interestingly, it's "make headers", which is not redone without a "make > > clean" at the kernel level. The "make headers_install" only takes a few > > hundred milliseconds, so issuing a systematic "make clean" in the nolibc > > test dir only takes ~800ms here to perform a full rebuild, which is totally > > acceptable to me. > > > > Thus what I've done is to mark the sysroot target as .phony and start it > > with a removal of the current include dir so that we systematically rebuild > > it. Now there's no such risk of running a test against an earlier version > > anymore, and there are no "make clean" to worry about anymore either. > > That looks much better to me! > > > > And I could confirm that just issuing: > > > > $ time make -j8 -C tools/testing/selftests/nolibc run > > > > after reverting Rasmus' fix led me to this pretty quickly: > > > > ... > > Kernel: arch/x86/boot/bzImage is ready (#3) > > make[1]: Leaving directory '/k' > > 15 memcmp_20_e0 = 64 [FAIL] > > 16 memcmp_e0_20 = -64 [FAIL] > > See all results in /k/tools/testing/selftests/nolibc/run.out > > make: Leaving directory '/k/tools/testing/selftests/nolibc' > > > > real 0m14.538s > > user 0m27.828s > > sys 0m4.576s > > > > No more false positives nor false negatives anymore. I'm sending you > > the patch separately. > > > > Thanks for the discussion, the solution is way better now! > > Nice, looking forward to the patch! In case you don't have it, it's this one: https://lore.kernel.org/all/20221026054508.19634-1-w@1wt.eu/ Do not hesitate to let me know if I should resend it. Thanks! Willy
On Wed, Oct 26, 2022 at 11:08:41AM +0200, Rasmus Villemoes wrote: > On 26/10/2022 07.39, Willy Tarreau wrote: > > > > No more false positives nor false negatives anymore. I'm sending you > > the patch separately. > > While you're at it, may I suggest also adding a few test cases where the > buffers differ by 128, e.g. 0x0 v 0x80 and 0x40 v 0xc0. I initially thought about it but changed my mind for +/- 0xc0 that covered the same cases in my opinion. Do you have a particular error case in mind that would be caught by this one that the other one does not catch ? I'm fine for proposing a respin of the patch to improve it if it brings some value, but I'm still failing to figure when that would be the case. Thanks, Willy
On 26/10/2022 21.52, Willy Tarreau wrote: > On Wed, Oct 26, 2022 at 11:08:41AM +0200, Rasmus Villemoes wrote: >> On 26/10/2022 07.39, Willy Tarreau wrote: >>> >>> No more false positives nor false negatives anymore. I'm sending you >>> the patch separately. >> >> While you're at it, may I suggest also adding a few test cases where the >> buffers differ by 128, e.g. 0x0 v 0x80 and 0x40 v 0xc0. > > I initially thought about it but changed my mind for +/- 0xc0 that > covered the same cases in my opinion. Do you have a particular error > case in mind that would be caught by this one that the other one does > not catch ? Not really, but in a sense the opposite: for the +/- 0xc0 case, both ways of comparison will give the wrong sign because -192 becomes +64 and vice versa. For +/- 0x80, one way of doing the comparison will "accidentally" produce the right answer, and I thought that might also be a little interesting. I'm fine for proposing a respin of the patch to improve > it if it brings some value, It's your call, you can respin, do an incremental patch, or just ignore me :) Rasmus
Hi Rasmus, On Thu, Oct 27, 2022 at 11:09:55AM +0200, Rasmus Villemoes wrote: > >> While you're at it, may I suggest also adding a few test cases where the > >> buffers differ by 128, e.g. 0x0 v 0x80 and 0x40 v 0xc0. > > > > I initially thought about it but changed my mind for +/- 0xc0 that > > covered the same cases in my opinion. Do you have a particular error > > case in mind that would be caught by this one that the other one does > > not catch ? > > Not really, but in a sense the opposite: for the +/- 0xc0 case, both > ways of comparison will give the wrong sign because -192 becomes +64 and > vice versa. For +/- 0x80, one way of doing the comparison will > "accidentally" produce the right answer, and I thought that might also > be a little interesting. OK, initially I thought you were trying to make the comparison return a match when there is none. I now see better what you mean there. > > I'm fine for proposing a respin of the patch to improve > > it if it brings some value, > > It's your call, you can respin, do an incremental patch, or just ignore > me :) I would like to propose you something. Till now I'm the only one having added tests to this file, and I'm still lacking feedback on the usability. I would very much appreciate it if you could try to add this test case yourself on top of existing ones (those present in Paul's rcu/next branch here: https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/ ). Then your criticism of what you would find unclear, unconvenient, poorly thought, unintuitive etc, and of course suggestions, would be welcome. That doesn't mean I'd have a quick solution of course but the more eyes there at the early stages, the better so that it becomes friendly to use for other contributors. If you don't want to, that's no big deal, but if you do I'll really appreciate it. Thank you, Willy
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 78bced95ac63..f14f5076fb6d 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -565,6 +565,13 @@ int run_stdlib(int min, int max) CASE_TEST(strchr_foobar_z); EXPECT_STRZR(1, strchr("foobar", 'z')); break; CASE_TEST(strrchr_foobar_o); EXPECT_STREQ(1, strrchr("foobar", 'o'), "obar"); break; CASE_TEST(strrchr_foobar_z); EXPECT_STRZR(1, strrchr("foobar", 'z')); break; + CASE_TEST(memcmp_20_20); EXPECT_EQ(1, memcmp("aaa\x20", "aaa\x20", 4), 0); break; + CASE_TEST(memcmp_20_60); EXPECT_LT(1, memcmp("aaa\x20", "aaa\x60", 4), 0); break; + CASE_TEST(memcmp_60_20); EXPECT_GT(1, memcmp("aaa\x60", "aaa\x20", 4), 0); break; + CASE_TEST(memcmp_20_e0); EXPECT_LT(1, memcmp("aaa\x20", "aaa\xe0", 4), 0); break; + CASE_TEST(memcmp_e0_20); EXPECT_GT(1, memcmp("aaa\xe0", "aaa\x20", 4), 0); break; + CASE_TEST(memcmp_80_e0); EXPECT_LT(1, memcmp("aaa\x80", "aaa\xe0", 4), 0); break; + CASE_TEST(memcmp_e0_80); EXPECT_GT(1, memcmp("aaa\xe0", "aaa\x80", 4), 0); break; case __LINE__: return ret; /* must be last */ /* note: do not set any defaults so as to permit holes above */
This adds 7 combinations of input values for memcmp() using signed and unsigned bytes, which will trigger on the original code before Rasmus' fix. This is mostly aimed at helping backporters verify their work, and showing how tests for corner cases can be added to the selftests suite. Before the fix it reports: 12 memcmp_20_20 = 0 [OK] 13 memcmp_20_60 = -64 [OK] 14 memcmp_60_20 = 64 [OK] 15 memcmp_20_e0 = 64 [FAIL] 16 memcmp_e0_20 = -64 [FAIL] 17 memcmp_80_e0 = -96 [OK] 18 memcmp_e0_80 = 96 [OK] And after: 12 memcmp_20_20 = 0 [OK] 13 memcmp_20_60 = -64 [OK] 14 memcmp_60_20 = 64 [OK] 15 memcmp_20_e0 = -192 [OK] 16 memcmp_e0_20 = 192 [OK] 17 memcmp_80_e0 = -96 [OK] 18 memcmp_e0_80 = 96 [OK] Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> Signed-off-by: Willy Tarreau <w@1wt.eu> --- tools/testing/selftests/nolibc/nolibc-test.c | 7 +++++++ 1 file changed, 7 insertions(+)