linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Paul E. McKenney" <paulmck@kernel.org>
To: Willy Tarreau <w@1wt.eu>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>,
	linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] selftests/nolibc: add 7 tests for memcmp()
Date: Mon, 24 Oct 2022 08:53:57 -0700	[thread overview]
Message-ID: <20221024155357.GZ5600@paulmck-ThinkPad-P17-Gen-1> (raw)
In-Reply-To: <20221022112228.GB30596@1wt.eu>

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

  reply	other threads:[~2022-10-24 17:21 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-21  6:03 [PATCH] selftests/nolibc: add 7 tests for memcmp() Willy Tarreau
2022-10-21 15:56 ` Paul E. McKenney
2022-10-21 17:01   ` Willy Tarreau
2022-10-21 17:07     ` Paul E. McKenney
2022-10-21 17:20       ` Willy Tarreau
2022-10-21 18:00         ` Paul E. McKenney
2022-10-22 11:22           ` Willy Tarreau
2022-10-24 15:53             ` Paul E. McKenney [this message]
2022-10-26  5:39               ` Willy Tarreau
2022-10-26  9:08                 ` Rasmus Villemoes
2022-10-26 19:52                   ` Willy Tarreau
2022-10-27  9:09                     ` Rasmus Villemoes
2022-10-27 17:20                       ` Willy Tarreau
2022-10-26 13:57                 ` Paul E. McKenney
2022-10-26 14:17                   ` Willy Tarreau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20221024155357.GZ5600@paulmck-ThinkPad-P17-Gen-1 \
    --to=paulmck@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=linux@rasmusvillemoes.dk \
    --cc=w@1wt.eu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).