Message-ID: <76f6ffc6-2750-4578-b845-67b4e7920830@lucifer.local>
Date: Thu, 4 Jul 2024 11:18:50 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Gow <davidgow@...gle.com>
Cc: SeongJae Park <sj@...nel.org>, Andrew Morton <akpm@...ux-foundation.org>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        Vlastimil Babka <vbabka@...e.cz>, Matthew Wilcox <willy@...radead.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
        Eric Biederman <ebiederm@...ssion.com>, Kees Cook <kees@...nel.org>,
        Suren Baghdasaryan <surenb@...gle.com>, Shuah Khan <shuah@...nel.org>,
        Brendan Higgins <brendanhiggins@...gle.com>,
        Rae Moar <rmoar@...gle.com>
Subject: Re: [PATCH 0/7] Make core VMA operations internal and testable

On Thu, Jul 04, 2024 at 03:10:16PM GMT, David Gow wrote:
> Thanks, SJ.
>
> While I'd love to have the VMA tests be KUnit tests (and there are
> several advantages, particularly for tooling and automation), I do
> think the more self-contained userspace tests are great in
> circumstances like this where the code is self-contained enough to
> make it possible. Ideally, we'd have some standards and helpers to
> make these consistent — kselftest and KUnit are both not quite perfect
> for this case — but I don't think we should hold up a useful set of
> changes so we can write a whole new framework.

Thanks David!

>
> (Personally, I think a userspace implementation of a subset of KUnit
> or a KUnit-like API would be useful, see below.)

Indeed, yes.

>
> On Thu, 4 Jul 2024 at 06:56, SeongJae Park <sj@...nel.org> wrote:
> >
> > On Wed, 3 Jul 2024 21:33:00 +0100 Lorenzo Stoakes <lorenzo.stoakes@...cle.com> wrote:
> >
> > > On Wed, Jul 03, 2024 at 01:26:53PM GMT, Andrew Morton wrote:
> > > > On Wed,  3 Jul 2024 12:57:31 +0100 Lorenzo Stoakes <lorenzo.stoakes@...cle.com> wrote:
> > > >
>
> [... snip ...]
>
> > Also, I haven't had enough time to read the patches in detail but just the
> > cover letter a little bit.  My humble impression from that is that this might
> > better to eventually be kunit tests.  I know there was a discussion with Kees
> > on RFC v1 [1] which you kindly explained why you decide to implement this in
> > user space.  To my understanding, at least some of the problems are not real
> > problems.  For two things as examples,
> >
> > 1. I understand that you concern the test speed [2].  I think Kunit could be
> > slower than the dedicated user space tests, but to my experience, it's not that
> > bad when using the default UML-based execution.
>
> KUnit/UML can be quite fast, but I do agree that a totally isolated
> test will be faster.

Sure absolutely, the key point here is the essentially zero setup/tear down
and zero code is always faster than _some_ code so as we stub/mock
components naturally we get speed as well as not having to be concerned
about how we might set up fundamental objects like task/mm/vma.

>
>
> > 2. My next humble undrestanding is that you want to test functions that you
> > don't want to export [2,3] to kernel modules.  To my understanding it's not
> > limited on Kunit.  I'm testing such DAMON functions using KUnit by including
> > test code in the c file but protecting it via a config.  For an example, please
> > refer to DAMON_KUNIT_TEST.
> >
> > I understand above are only small parts of the reason for your decision, and
> > some of those would really unsupported by Kunit.  In the case, I think adding
> > this user space tests as is is good.  Nonetheless, I think it would be good to
> > hear some comments from Kunit developers.  IMHO, letting them know the
> > limitations will hopefully help setting their future TODO items.  Cc-ing
> > Brendan, David and Rae for that.
>
> There are a few different ways of working around this, including the
> '#include the source' method, and conditionally exporting symbols to a
> separate namespace (e.g., using VISIBLE_IF_KUNIT and
> EXPORT_SYMBOL_IF_KUNIT()).
>
> Obviously, it's always going to be slightly nasty, but I don't think
> KUnit will fundamentally be uglier than any other similar hack.

Indeed, I mean this patch set makes use of the 'include the source' method
in userland.

To me, the more you think about it and how you might implement testing of
fundamnetals like this the more you end up with a mocked out design as in
this series, unavoidably.

And sadly I think no matter how you do it you have to put the ugly
somewhere, in this instance it's in the stubbed-out vma_internal.h.

>
> >
> > To recap, I have no strong opinions about this patch, but I think knowing how
> > Selftests and KUnit developers think could be helpful.
> >
> >
>
> More generally, we've seen quite a few cases where we want to compile
> a small chunk of kernel code and some tests as a userspace binary, for
> a few different reasons, including:
> - Improved speed/debuggability from being a "normal" userspace binary
> - The desire to test userspace code which lives in the kernel tree
> (e.g., the perf tool)
> - Smaller reproducable test cases to give to other parties (e.g.,
> compiler developers)
>
> So I think there's definitely a case for having these sorts of tests,
> it'd just be nice to be as consistent as we can. There are a few
> existing patches out there (most recently [1]) which implement a
> subset of the KUnit API in userspace, which has the twin advantages of
> making test code more consistent overall, and allowing some tests to
> be available both as KUnit tests and separate userspace tests (so we
> get the best of both worlds). Having a standard 'userspace kunit'
> implementation is definitely something I've thought about before, so
> I'll probably play around with that when I get some time.
>

Well indeed, [1] is what this patch series uses, heavily, to be viable :)

I do absolutely agree going forward that some means of standardisation
would be very useful.

> Otherwise, if Shuah's okay with it, having these userspace tests be
> selftests seems at the very least an appropriate stopgap measure,
> which gets us some tooling and CI. I've always thought of selftests as
> "testing the running kernel", rather than the tree under test, but as
> long as it's clear that this is happening, there's no technical reason
> to avoid it,.

Yeah, this implementation is explicitly intended to be a skeleton to be
built on, providing a minimum implementation with the most important
component provided, i.e. the stubbed out code - in order to demonstrate why
the refactoring bits of the patch sets were done (i.e. to answer 'why so
much churn?') AND to provide the basis to easily move ahead and write
serious tests.

I think it is still viable to add further tests to this as-is (I'd rather
not add too much friction to this hugely valuable exercise - we are
seriously lacking for fundamental VMA unit/regression tests), but moving
forward I think it should also be very easy to adapt this code to use a
consistent userland kunit implementation.

>
> Cheers,
> -- David
>
> [1]: https://lore.kernel.org/all/20240625211803.2750563-5-willy@infradead.org/

Powered by blists - more mailing lists