Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 5 May 2020 07:16:17 -0700
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     Marco Elver <elver@google.com>
Cc:     David Gow <davidgow@google.com>,
        KUnit Development <kunit-dev@googlegroups.com>,
        Brendan Higgins <brendanhiggins@google.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        Alexander Potapenko <glider@google.com>,
        Andrey Konovalov <andreyknvl@google.com>,
        kasan-dev <kasan-dev@googlegroups.com>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] kcsan: Add test suite
Message-ID: <20200505141617.GY2869@paulmck-ThinkPad-P72>
Reply-To: paulmck@kernel.org
References: <20200427143507.49654-1-elver@google.com>
 <CANpmjNOv7VXv9LtWHWBx1-an+1+WxjtzDNBF+rKsOm+ybmvwog@mail.gmail.com>
 <CABVgOSnr8CX5tN9u_wafxSiyyVcM9nL_nX2ufrSdRi=jdWjerg@mail.gmail.com>
 <CANpmjNMhVcR6TiLv29HqSvVVurUMwtHiokodPyzvwFSeE6UpZw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CANpmjNMhVcR6TiLv29HqSvVVurUMwtHiokodPyzvwFSeE6UpZw@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, May 05, 2020 at 03:01:45PM +0200, Marco Elver wrote:
> On Tue, 5 May 2020 at 07:00, David Gow <davidgow@google.com> wrote:
> >
> > On Mon, Apr 27, 2020 at 11:23 PM 'Marco Elver' via kasan-dev
> > <kasan-dev@googlegroups.com> wrote:
> > >
> > > On Mon, 27 Apr 2020 at 16:35, Marco Elver <elver@google.com> wrote:
> > > >
> > > > This adds KCSAN test focusing on behaviour of the integrated runtime.
> > > > Tests various race scenarios, and verifies the reports generated to
> > > > console. Makes use of KUnit for test organization, and the Torture
> > > > framework for test thread control.
> > > >
> > > > Signed-off-by: Marco Elver <elver@google.com>
> > > > ---
> > >
> > > +KUnit devs
> > > We had some discussions on how to best test sanitizer runtimes, and we
> > > believe that this test is what testing sanitizer runtimes should
> > > roughly look like. Note that, for KCSAN there are various additional
> > > complexities like multiple threads, and report generation isn't
> > > entirely deterministic (need to run some number of iterations to get
> > > reports, may get multiple reports, etc.).
> >
> > Thanks very much for writing the test. I do think that it goes a
> > little outside what we'd normally expect of a unit test (notably with
> > the issues around determinism and threading), but it's good to see
> > KUnit being pushed in new directions a bit.
> >
> > The biggest issue in my mind is the possibility that the
> > non-determinism of the tests could cause false positives. If we're
> > trying to run as many KUnit tests as possible as part of continuous
> > integration systems or as a condition for accepting patches, having
> > flaky tests could be annoying. The KCSAN tests seem to break/fail
> > as-is when run on single-core machines (at least, under qemu), so some
> > way of documenting this as a requirement would probably be necessary,
> > too.
> 
> True. Although note that we require CONFIG_KCSAN=y for this test to be
> enabled, so I don't think it's a big problem for a regular CI setups.
> For a KCSAN setup, I'd expect that we know that running on a
> single-core system doesn't yield much interesting results regardless
> of tests being run.
> 
> The non-deterministic nature of concurrent tests will never entirely
> go away, but I think with the right preconditions met (at least N
> CPUs, where N depends on PREEMPT_NONE, PREEMPT_VOLUNTARY or PREEMPT)
> the tests here should not normally fail.
> 
> > One possibility would be to add support for "skipped" tests to KUnit
> > (the TAP specification allows for it), so that the KCSAN test could
> > detect cases where it's not reliable, and skip itself (leaving a note
> > as to why). In the short term, though, we'd absolutely need some
> > documentation around the dependencies for the test.
> 
> That would be nice. For the time being, I will add a precondition
> check to test_init(), and print a warning if the test needs to be
> skipped.
> 
> > (For the record, the failures I saw were all due to running under qemu
> > emulating as a uniprocessor/single-core machine: with
> > CONFIG_PREEMPT_VOLUNTARY, it would just hang after creating the first
> > couple of threads. With CONFIG_PREEMPT, the tests completed, but the
> > majority of them failed.)
> 
> Right, let me try to fix those at least. I'll send v2.
> 
> (Paul: If you prefer a separate patch rather than v2, let me know.)

A v2 would work well, thank you!

							Thanx, Paul

> > > The main thing, however, is that we want to verify the actual output
> > > (or absence of it) to console. This is what the KCSAN test does using
> > > the 'console' tracepoint. Could KUnit provide some generic
> > > infrastructure to check console output, like is done in the test here?
> > > Right now I couldn't say what the most useful generalization of this
> > > would be (without it just being a wrapper around the console
> > > tracepoint), because the way I've decided to capture and then match
> > > console output is quite test-specific. For now we can replicate this
> > > logic on a per-test basis, but it would be extremely useful if there
> > > was a generic interface that KUnit could provide in future.
> >
> > This is something we've discussed here a couple of times as well.
> > While I'll confess to being a little bit wary of having tests rely too
> > heavily on console output: it risks being a bit fragile if the exact
> > contents or formatting of messages change, or ends up having a lot of
> > string formatting and/or parsing code in the tests. I do agree,
> > though, that it probably needs to be at least a part of testing things
> > like sanitizers where the ultimate goal is to produce console output.
> > I'm not exactly sure how we'd implement it yet, so it's probably not
> > going to happen extremely soon, but what you have here looks to me
> > like a good example we can generalise as needed.
> 
> The fragility due to formatting etc. for the sanitizers is exactly
> what we want, since any change in console output could be a bug. But
> as you say, for other tests, it might not make much sense.
> 
> Thanks,
> -- Marco