Re: [PATCH 1/2] selftests: add a kselftest for SLUB debugging functionality

From: Marco Elver <elver@google.com>
To: Vlastimil Babka <vbabka@suse.cz>
Cc: glittao@gmail.com, Christoph Lameter <cl@linux.com>,
	Pekka Enberg <penberg@kernel.org>,
	David Rientjes <rientjes@google.com>,
	Joonsoo Kim <iamjoonsoo.kim@lge.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Shuah Khan <shuah@kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Linux Memory Management List <linux-mm@kvack.org>,
	"open list:KERNEL SELFTEST FRAMEWORK" 
	<linux-kselftest@vger.kernel.org>
Subject: Re: [PATCH 1/2] selftests: add a kselftest for SLUB debugging functionality
Date: Fri, 19 Mar 2021 12:19:58 +0100	[thread overview]
Message-ID: <CANpmjNPzO2ORHMb=WdSmZs-1k2NRzVda5rtLjvRnaGkb7xcCNw@mail.gmail.com> (raw)
In-Reply-To: <3ba2228a-1442-40b4-578f-f693d9a054e7@suse.cz>

On Fri, 19 Mar 2021 at 11:46, Vlastimil Babka <vbabka@suse.cz> wrote:
> On 3/18/21 12:47 PM, Marco Elver wrote:
> > On Tue, Mar 16, 2021 at 01:41PM +0100, glittao@gmail.com wrote:
> >> From: Oliver Glitta <glittao@gmail.com>
> >>
> >> SLUB has resiliency_test() function which is hidden behind #ifdef
> >> SLUB_RESILIENCY_TEST that is not part of Kconfig, so nobody
> >> runs it. Kselftest should proper replacement for it.
> >>
> >> Try changing byte in redzone after allocation and changing
> >> pointer to next free node, first byte, 50th byte and redzone
> >> byte. Check if validation finds errors.
> >>
> >> There are several differences from the original resiliency test:
> >> Tests create own caches with known state instead of corrupting
> >> shared kmalloc caches.
> >>
> >> The corruption of freepointer uses correct offset, the original
> >> resiliency test got broken with freepointer changes.
> >>
> >> Scratch changing random byte test, because it does not have
> >> meaning in this form where we need deterministic results.
> >>
> >> Add new option CONFIG_TEST_SLUB in Kconfig.
> >>
> >> Add parameter to function validate_slab_cache() to return
> >> number of errors in cache.
> >>
> >> Signed-off-by: Oliver Glitta <glittao@gmail.com>
> >
> > No objection per-se, but have you considered a KUnit-based test instead?
>
> To be honest, we didn't realize about that option.
>
> > There is no user space portion required to run this test, and a pure
> > in-kernel KUnit test would be cleaner. Various boiler-plate below,
> > including pr_err()s, the kselftest script etc. would simply not be
> > necessary.
> >
> > This is only a suggestion, but just want to make sure you've considered
> > the option and weighed its pros/cons.
>
> Thanks for the suggestion. But I hope we would expand the tests later to e.g.
> check the contents of various SLUB related sysfs files or even write to them,
> and for that goal kselftest seems to be a better starting place?

Not sure, but I would probably go about it this way:

A. Anything that is purely in-kernel and doesn't require a user space
component should be a KUnit test.

B. For any test that requires a user space component, it'd be a kselftest.

And I think the best design here would also clearly separate those 2
types of tests, and I wouldn't lump tests of type A into modules that
are also used for B. That way, running tests of type A also is a bit
easier, and if somebody wants to just quickly run those it's e.g. very
quick to do so with kunit-tool.

Thanks,
-- Marco