Message-ID: <1edfc11c-ab99-4e9d-bf5d-b10f34b3f1da@lucifer.local>
Date: Thu, 4 Jul 2024 00:24:15 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: SeongJae Park <sj@...nel.org>
Cc: 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>,
        David Gow <davidgow@...gle.com>, Rae Moar <rmoar@...gle.com>
Subject: Re: [PATCH 0/7] Make core VMA operations internal and testable

On Wed, Jul 03, 2024 at 03:56:36PM GMT, SeongJae Park 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:
> > >
> > > > Kernel functionality is stubbed and shimmed as needed in tools/testing/vma/
> > > > which contains a fully functional userland vma_internal.h file and which
> > > > imports mm/vma.c and mm/vma.h to be directly tested from userland.
> > >
> > > Cool stuff.
> >
> > Thanks :)
> >
> > >
> > > Now we need to make sure that anyone who messes with vma code has run
> > > the tests.  And has added more testcases, if appropriate.
> > >
> > > Does it make sense to execute this test under selftests/ in some
> > > fashion?  Quite a few people appear to be running the selftest code
> > > regularly and it would be good to make them run this as well.
> >
> > I think it will be useful to do that, yes, but as the tests are currently a
> > skeleton to both provide the stubbing out and to provide essentially an
> > example of how you might test (though enough that it'd now be easy to add a
> > _ton_ of tests), it's not quite ready to be run just yet.
>
> If we will eventually move the files under selftests/, why dont' we place the
> files there from the beginning?  Is there a strict rule saying files that not
> really involved with running tests or not ready cannot be added there?  If so,
> could adding the files after the tests are ready to be run be an option?
> Cc-ing Shuah since I think she might have a comment.

We already have tests under tools/testing which seems like a good place to
put things. It's arguably not 'self' testing but a specific isolation mechanism.

It'd be a whole lot of churn including totally moving all of the radix tree
tests to self test and then totally changing how mm self tests are built
(existing code just runs userland code that uses system calls) for... what
gain? I don't agree with this at all.

The self tests differ from this and other tests using the userland-stubbed
kernel approach in that they test system call invocation and assert
expectations.

My point to Andrew was that we could potentially automatically run these
tests as part of a self-test run as they are so quick, at least in the
future, if that made sense.

>
> 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,

They are real problems. And I totally disagree that these should be kunit
tests. I'm surprised you didn't find my and Liam's arguments compelling?

I suggest you try actually running tools/testing/vma/vma and putting a
break point in gdb in vma_merge(), able to observe all state in great
detail with no interrupts and see for yourself.

>
> 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.

I'm sorry but running VMA code in the smallest possible form in userland is
very clearly faster and you are missing the key point that we can _isolate_
anything we _don't need_.

There's no setup/teardown whatsoever, no clever tricks needed, we get to
keep entirely internal interfaces internal and clean. It's compelling.

You are running the code as fast as you possibly can and that allows for
lots of interesting things like being able to fuzz at scale, being able to
run thousands of cases with basically zero setup/teardown or limits,
etc. etc.

Also, it's basically impossible to explicitly _unit_ test vma merge and vma
split and friends without invoking kernel stuff like TLB handling, MMU
notifier, huge page handling, process setup/teardown, mm setup/teardown,
rlimits, anon vma name handling, uprobes, memory policy handling, interval
tree handling, lock contention, THP behaviour, etc. etc. etc.

With this test we can purely _unit_ test these fundamental operations, AND
have the ability to for example in future - dump maple tree state from a
buggy kernel situation that would result in a panic for instance - and
recreate it immediately for debug.

We also then have the ability to have strong guarantees about the behaviour
of these operations at a fundamental level.

If we want _system_ tests that bring in other kernel components then it
makes more sense to use kunit/selftests. But this offers something else.

Also keep in mind this is a _skeleton_ test designed to prove the point
that this works. We can rework this as we wish later, it's necessary to
include it to demonstrate the purpose of the refactoring bits of the
series.

I really don't want this series to get dragged into too much back + forth
meanwhile blocking a super conflict-inviting refactoring that is actually
valuable in itself.

I think it's more valuable to get the test skeleton in place and to perform
follow up series to adjust if people have philosophical differences.

>
> 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.

Right there are ways around this, but you lose all of the
isolation/performance advantages, and then you end up dirtying the mm/
directory with test code which ends being more or less doing the same thing
I'm doing here only in the kernel rather than stubbing?
>
> 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.

As I said above, I really do not want this series to get stuck on a
back-and-forth about test philosophy. We already have tests like the
_skeleton_ ones I added, we can change this later, and it's going to make
the refactoring part of this more likely to experience conflicts.

>
> To recap, I have no strong opinions about this patch, but I think knowing how
> Selftests and KUnit developers think could be helpful.

With respect it strikes me that you have rather strong feelings on
this. But again I make the plea that we don't hold this up on the basis of
a debate about this vs. other options re: testing.

Kees was agreeable with this approach so I don't think we should really see
too much objection to this.

>
>
> [1] https://lore.kernel.org/202406270957.C0E5E8057@keescook
> [2] https://lore.kernel.org/5zuowniex4sxy6l7erbsg5fiirf4d4f5fbpz2upay2igiwa2xk@vuezoh2wbqf4
> [3] https://lore.kernel.org/f005a7b0-ca31-4d39-b2d5-00f5546d610a@lucifer.local
>
>
> Thanks,
> SJ
>
> [...]

Powered by blists - more mailing lists