Re: [PATCH] kfence: test: use kunit_skip() to skip tests

From: David Gow <davidgow@google.com>
To: Marco Elver <elver@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Alexander Potapenko <glider@google.com>,
	 Dmitry Vyukov <dvyukov@google.com>,
	Aleksandr Nogikh <nogikh@google.com>,
	 Taras Madan <tarasmadan@google.com>,
	 Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	 Linux Memory Management List <linux-mm@kvack.org>,
	kasan-dev <kasan-dev@googlegroups.com>
Subject: Re: [PATCH] kfence: test: use kunit_skip() to skip tests
Date: Fri, 24 Sep 2021 02:10:48 +0800	[thread overview]
Message-ID: <CABVgOSk3iY7-8h=uJRNwN-UoWYxVZ1dNALzuE1MMLswKUkXfqA@mail.gmail.com> (raw)
In-Reply-To: <CANpmjNN1VVe682haDKFLMOoHOqSizh9y1sGAc4dZXc4WnBsCbQ@mail.gmail.com>

On Fri, Sep 24, 2021 at 1:58 AM Marco Elver <elver@google.com> wrote:
>
> On Thu, 23 Sept 2021 at 19:39, David Gow <davidgow@google.com> wrote:
> > On Thu, Sep 23, 2021 at 2:26 AM Marco Elver <elver@google.com> wrote:
> > >
> > > Use the new kunit_skip() to skip tests if requirements were not met. It
> > > makes it easier to see in KUnit's summary if there were skipped tests.
> > >
> > > Signed-off-by: Marco Elver <elver@google.com>
> > > ---
> >
> > Thanks: I'm glad these features are proving useful. I've tested these
> > under qemu, and it works pretty well.
> >
> > Certainly from the KUnit point of view, this is:
> > Reviewed-by: David Gow <davidgow@google.com>
>
> Thanks!
>
> > (A couple of unrelated complaints about the kfence tests are that
> > TRACEPOINTS isn't selected by default, and that the manual
> > registering/unregistering of the tracepoints does break some of the
> > kunit tooling when several tests are built-in. That's something that
> > exists independently of this patch, though, and possibly requires some
> > KUnit changes to be fixed cleanly (kfence isn't the only thing to do
> > this). So not something to hold up this patch.)
>
> I think there was a reason we wanted it to "depends on TRACEPOINTS".
> If it were to select it, then if you do a CONFIG_KUNIT_ALL_TESTS=y,
> and also have KFENCE on, you'll always select tracepoints. In certain
> situations this may not be wanted. If we didn't have
> CONFIG_KUNIT_ALL_TESTS, then certainly, auto-selecting TRACEPOINTS
> would be ok.
>
> If you can live with that, we can of course switch it to do "select
> TRACEPOINTS".

That's probably more convenient for me, but I confess that my use case
is almost always wanting to run the KUnit tests, so I'm not unbiased.
:-)

>
> On a whole I err on the side of fewer auto-selected Kconfig options.

Yeah, it's perfectly sensible to do it either way. Maybe the right
option is to have a .kunitconfig file which has TRACEPOINTS enabled.

It's probably not worth doing if there's still issues with kunit_tool
parsing the results when the test is built-in, so this should probably
wait until KUnit has a way of running code on init/exit of suites as
well as individual tests within those suites. KFENCE is not the only
test suite which needs something like that (nor the only one which
does some module_init or late_initcall stuff which causes some
formatting issues with builtin tests).

Cheers,
-- David