[PATCH] selftests/nolibc: add 7 tests for memcmp()

[PATCH] selftests/nolibc: add 7 tests for memcmp()

[Willy Tarreau]

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(+)

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 */
-- 
2.17.5

Re: [PATCH] selftests/nolibc: add 7 tests for memcmp()

[Paul E. McKenney]

On Fri, Oct 21, 2022 at 08:03:40AM +0200, Willy Tarreau wrote:
> 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>

I have pulled both of these in, thank you!

One thing, though...  I had to do "make clean" in both tools/include/nolibc
and tools/testing/selftests/nolibc to make those two "[FAIL]" indications
go away.  Does this mean that I am doing something wrong?

It would be good to know before I send the pull request containing these,
so that we can let Linus know of anything special he needs to do to
ensure a valid test result.

							Thanx, Paul

> ---
>  tools/testing/selftests/nolibc/nolibc-test.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> 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 */
> -- 
> 2.17.5
> 

Re: [PATCH] selftests/nolibc: add 7 tests for memcmp()

[Willy Tarreau]

On Fri, Oct 21, 2022 at 08:56:45AM -0700, Paul E. McKenney wrote:
> On Fri, Oct 21, 2022 at 08:03:40AM +0200, Willy Tarreau wrote:
> > 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>
> 
> I have pulled both of these in, thank you!
 
Thanks!

> One thing, though...  I had to do "make clean" in both tools/include/nolibc
> and tools/testing/selftests/nolibc to make those two "[FAIL]" indications
> go away.  Does this mean that I am doing something wrong?

No you didn't do anything wrong, it was the same for me and initially it
was intentional, but probably it wasn't that good an idea. What happens
is that we first prepare a pseudo-sysroot with kernel headers and nolibc
headers, then we build the test based on this sysroot. Thus if any uapi
header or nolibc header changes, nothing is detected. And I'm not much
willing to always reinstall everything for every single test, nor to
detect long dependency chains. Maybe I should think about adding another
target to clean+test at the same time, or maybe make the current
"nolibc-test" target do that and have a "retest" to only rebuild. But
that needs to be thought about with the QEMU test as well (because most
of the time for a quick test I don't build the kernel nor start QEMU, I
just call the executable directly).

Any ideas or suggestions are welcome, of course. We could consider that
if we build a kernel and start QEMU, it's long enough to justify a
systematic clean maybe ?

> It would be good to know before I send the pull request containing these,
> so that we can let Linus know of anything special he needs to do to
> ensure a valid test result.

I see. In the worst case, a preliminary "make clean" will do it. We just
need to decide what's the best solution for everyone (i.e. not waste too
much time between tests while not getting misleading results by accident).

Thanks!
Willy

Re: [PATCH] selftests/nolibc: add 7 tests for memcmp()

[Paul E. McKenney]

On Fri, Oct 21, 2022 at 07:01:34PM +0200, Willy Tarreau wrote:
> On Fri, Oct 21, 2022 at 08:56:45AM -0700, Paul E. McKenney wrote:
> > On Fri, Oct 21, 2022 at 08:03:40AM +0200, Willy Tarreau wrote:
> > > 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>
> > 
> > I have pulled both of these in, thank you!
>  
> Thanks!
> 
> > One thing, though...  I had to do "make clean" in both tools/include/nolibc
> > and tools/testing/selftests/nolibc to make those two "[FAIL]" indications
> > go away.  Does this mean that I am doing something wrong?
> 
> No you didn't do anything wrong, it was the same for me and initially it
> was intentional, but probably it wasn't that good an idea. What happens
> is that we first prepare a pseudo-sysroot with kernel headers and nolibc
> headers, then we build the test based on this sysroot. Thus if any uapi
> header or nolibc header changes, nothing is detected. And I'm not much
> willing to always reinstall everything for every single test, nor to
> detect long dependency chains. Maybe I should think about adding another
> target to clean+test at the same time, or maybe make the current
> "nolibc-test" target do that and have a "retest" to only rebuild. But
> that needs to be thought about with the QEMU test as well (because most
> of the time for a quick test I don't build the kernel nor start QEMU, I
> just call the executable directly).
> 
> Any ideas or suggestions are welcome, of course. We could consider that
> if we build a kernel and start QEMU, it's long enough to justify a
> systematic clean maybe ?
> 
> > It would be good to know before I send the pull request containing these,
> > so that we can let Linus know of anything special he needs to do to
> > ensure a valid test result.
> 
> I see. In the worst case, a preliminary "make clean" will do it. We just
> need to decide what's the best solution for everyone (i.e. not waste too
> much time between tests while not getting misleading results by accident).

Maybe just document the careful/slow way, then people doing it more
frequently can do it the clever/fast way.

My guess is that the careful/slow is this:

	pushd tools/include/nolibc
	make clean
	make
	popd
	pushd tools/testing/selftests/nolibc
	make clean
	make -j32 run

Or did I miss a turn in there somewhere?

							Thanx, Paul

Re: [PATCH] selftests/nolibc: add 7 tests for memcmp()

[Willy Tarreau]

On Fri, Oct 21, 2022 at 10:07:38AM -0700, Paul E. McKenney wrote:
> > I see. In the worst case, a preliminary "make clean" will do it. We just
> > need to decide what's the best solution for everyone (i.e. not waste too
> > much time between tests while not getting misleading results by accident).
> 
> Maybe just document the careful/slow way, then people doing it more
> frequently can do it the clever/fast way.
> 
> My guess is that the careful/slow is this:
> 
> 	pushd tools/include/nolibc
> 	make clean
> 	make
> 	popd
> 	pushd tools/testing/selftests/nolibc
> 	make clean
> 	make -j32 run
> 
> Or did I miss a turn in there somewhere?

It's even easier, you don't even need the clean phase in include/nolibc.
I'm doing this and it's sufficient:

  make -C tools/testing/selftests/nolibc clean
  make -C tools/testing/selftests/nolibc nolibc-test
  tools/testing/selftests/nolibc/nolibc-test

Or for the test under QEMU, which involves a kernel build:

  make -C tools/testing/selftests/nolibc clean
  make -C tools/testing/selftests/nolibc -j $(nproc) run

Where would you first look for such a hint ? Maybe the help output of
the default "make" command could send as a hint that a clean is needed
after patching nolibc and that could be sufficient ? I just want to make
sure users don't waste their time trying to find what they could be doing
wrong.

Willy

Re: [PATCH] selftests/nolibc: add 7 tests for memcmp()

[Paul E. McKenney]

On Fri, Oct 21, 2022 at 07:20:26PM +0200, Willy Tarreau wrote:
> On Fri, Oct 21, 2022 at 10:07:38AM -0700, Paul E. McKenney wrote:
> > > I see. In the worst case, a preliminary "make clean" will do it. We just
> > > need to decide what's the best solution for everyone (i.e. not waste too
> > > much time between tests while not getting misleading results by accident).
> > 
> > Maybe just document the careful/slow way, then people doing it more
> > frequently can do it the clever/fast way.
> > 
> > My guess is that the careful/slow is this:
> > 
> > 	pushd tools/include/nolibc
> > 	make clean
> > 	make
> > 	popd
> > 	pushd tools/testing/selftests/nolibc
> > 	make clean
> > 	make -j32 run
> > 
> > Or did I miss a turn in there somewhere?
> 
> It's even easier, you don't even need the clean phase in include/nolibc.
> I'm doing this and it's sufficient:
> 
>   make -C tools/testing/selftests/nolibc clean
>   make -C tools/testing/selftests/nolibc nolibc-test
>   tools/testing/selftests/nolibc/nolibc-test
> 
> Or for the test under QEMU, which involves a kernel build:
> 
>   make -C tools/testing/selftests/nolibc clean
>   make -C tools/testing/selftests/nolibc -j $(nproc) run
> 
> Where would you first look for such a hint ? Maybe the help output of
> the default "make" command could send as a hint that a clean is needed
> after patching nolibc and that could be sufficient ? I just want to make
> sure users don't waste their time trying to find what they could be doing
> wrong.

Maybe it suffices for the near term for me to put this information in
the signed tag for the pull request?

Another approach would be to remind about "make clean" in the case of
a test failure.  Or make test failure combined with a detected change
trigger an automatic "make clean" and a retry.  Or other schemes of
increasing complexity and fragility.  ;-)

Other approaches?

							Thanx, Paul

Re: [PATCH] selftests/nolibc: add 7 tests for memcmp()

[Willy Tarreau]

On Fri, Oct 21, 2022 at 11:00:40AM -0700, Paul E. McKenney wrote:
> > It's even easier, you don't even need the clean phase in include/nolibc.
> > I'm doing this and it's sufficient:
> > 
> >   make -C tools/testing/selftests/nolibc clean
> >   make -C tools/testing/selftests/nolibc nolibc-test
> >   tools/testing/selftests/nolibc/nolibc-test
> > 
> > Or for the test under QEMU, which involves a kernel build:
> > 
> >   make -C tools/testing/selftests/nolibc clean
> >   make -C tools/testing/selftests/nolibc -j $(nproc) run
> > 
> > Where would you first look for such a hint ? Maybe the help output of
> > the default "make" command could send as a hint that a clean is needed
> > after patching nolibc and that could be sufficient ? I just want to make
> > sure users don't waste their time trying to find what they could be doing
> > wrong.
> 
> Maybe it suffices for the near term for me to put this information in
> the signed tag for the pull request?

It can be sufficient for short term indeed, but it can be easy as well
for me to mention it in the make output.

> Another approach would be to remind about "make clean" in the case of
> a test failure.  Or make test failure combined with a detected change
> trigger an automatic "make clean" and a retry.

In fact failures are not the only case. For me it was the opposite. I
applied Rasmus' fix, then I developed the test, verified that it worked,
then reverted Rasmus' fix... to find that the test didn't catch the
failure. I had a second look at the original patch and figured that
the -192..+192 values were really not possible with a char so I
concluded that a clean was needed. But leaving something in a claimed
working state while it's not can be sufficiently misleading and make
one waste significant time, because in such cases we rarely search why
it works.

> Or other schemes of increasing complexity and fragility.  ;-)

That's exactly what I'd like to avoid with such a lightweight component.
If it takes more time to figure why something is going wrong than to
write a test, we'll all give up. I think that a clean for QEMU is worth
it because the kernel is rebuilt and its dependencies are quite robust,
so that one would be a surprise. For other tests, probably leaving it
explicit with a hint that it's needed should suffice. I'll recheck what
conditions the installation of uapi headers because that's really what
I don't want to see happening all the time. The rest is discrete, it's
just a few files being copied, maybe it can be done every time.

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.

Thanks for keeping the conversation flowing, that helps me!
Willy

Re: [PATCH] selftests/nolibc: add 7 tests for memcmp()

[Paul E. McKenney]

On Sat, Oct 22, 2022 at 01:22:28PM +0200, Willy Tarreau wrote:
> On Fri, Oct 21, 2022 at 11:00:40AM -0700, Paul E. McKenney wrote:
> > > It's even easier, you don't even need the clean phase in include/nolibc.
> > > I'm doing this and it's sufficient:
> > > 
> > >   make -C tools/testing/selftests/nolibc clean
> > >   make -C tools/testing/selftests/nolibc nolibc-test
> > >   tools/testing/selftests/nolibc/nolibc-test
> > > 
> > > Or for the test under QEMU, which involves a kernel build:
> > > 
> > >   make -C tools/testing/selftests/nolibc clean
> > >   make -C tools/testing/selftests/nolibc -j $(nproc) run
> > > 
> > > Where would you first look for such a hint ? Maybe the help output of
> > > the default "make" command could send as a hint that a clean is needed
> > > after patching nolibc and that could be sufficient ? I just want to make
> > > sure users don't waste their time trying to find what they could be doing
> > > wrong.
> > 
> > Maybe it suffices for the near term for me to put this information in
> > the signed tag for the pull request?
> 
> It can be sufficient for short term indeed, but it can be easy as well
> for me to mention it in the make output.

Why not both?  ;-)

> > Another approach would be to remind about "make clean" in the case of
> > a test failure.  Or make test failure combined with a detected change
> > trigger an automatic "make clean" and a retry.
> 
> In fact failures are not the only case. For me it was the opposite. I
> applied Rasmus' fix, then I developed the test, verified that it worked,
> then reverted Rasmus' fix... to find that the test didn't catch the
> failure. I had a second look at the original patch and figured that
> the -192..+192 values were really not possible with a char so I
> concluded that a clean was needed. But leaving something in a claimed
> working state while it's not can be sufficiently misleading and make
> one waste significant time, because in such cases we rarely search why
> it works.

Fair point!  False negatives can be quite annoying as well.

> > Or other schemes of increasing complexity and fragility.  ;-)
> 
> That's exactly what I'd like to avoid with such a lightweight component.
> If it takes more time to figure why something is going wrong than to
> write a test, we'll all give up. I think that a clean for QEMU is worth
> it because the kernel is rebuilt and its dependencies are quite robust,
> so that one would be a surprise. For other tests, probably leaving it
> explicit with a hint that it's needed should suffice. I'll recheck what
> conditions the installation of uapi headers because that's really what
> I don't want to see happening all the time. The rest is discrete, it's
> just a few files being copied, maybe it can be done every time.
> 
> 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!

							Thanx, Paul

Re: [PATCH] selftests/nolibc: add 7 tests for memcmp()

[Willy Tarreau]

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

Re: [PATCH] selftests/nolibc: add 7 tests for memcmp()

[Rasmus Villemoes]

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.

Re: [PATCH] selftests/nolibc: add 7 tests for memcmp()

[Paul E. McKenney]

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

Re: [PATCH] selftests/nolibc: add 7 tests for memcmp()

[Willy Tarreau]

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

Re: [PATCH] selftests/nolibc: add 7 tests for memcmp()

[Willy Tarreau]

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

Re: [PATCH] selftests/nolibc: add 7 tests for memcmp()

[Rasmus Villemoes]

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

Re: [PATCH] selftests/nolibc: add 7 tests for memcmp()

[Willy Tarreau]

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