| Message ID | cover.1720006125.git.lorenzo.stoakes@oracle.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Make core VMA operations internal and testable | expand |
On Wed, 3 Jul 2024 12:57:31 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.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. 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. > 51 files changed, 3914 insertions(+), 2453 deletions(-) eep. The best time for me to merge this is late in the -rc cycle so the large skew between mainline and mm.git doesn't spend months hampering ongoing development. But that merge time is right now.
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@oracle.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. > > > 51 files changed, 3914 insertions(+), 2453 deletions(-) > > eep. The best time for me to merge this is late in the -rc cycle so > the large skew between mainline and mm.git doesn't spend months > hampering ongoing development. But that merge time is right now. Argh. Well, the numbers are scary, but it's _mostly_ moving code around with some pretty straightforward refactorings and adding a bunch of userland code that won't impact kernels at all. So I'd argue this is less crazy in size than it might seem...
On Wed, 3 Jul 2024 21:33:00 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > > > > 51 files changed, 3914 insertions(+), 2453 deletions(-) > > > > eep. The best time for me to merge this is late in the -rc cycle so > > the large skew between mainline and mm.git doesn't spend months > > hampering ongoing development. But that merge time is right now. > > Argh. Well, the numbers are scary, but it's _mostly_ moving code around > with some pretty straightforward refactorings and adding a bunch of > userland code that won't impact kernels at all. > > So I'd argue this is less crazy in size than it might seem... OK, let's leave it a couple of days for some feedback then decide. It's still a couple of weeks until we go upstream.
On Wed, 3 Jul 2024 21:33:00 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.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@oracle.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. 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. 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. To recap, I have no strong opinions about this patch, but I think knowing how Selftests and KUnit developers think could be helpful. [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 [...]
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@oracle.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@oracle.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 > > [...]
On Thu, 4 Jul 2024 00:24:15 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > 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@oracle.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@oracle.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. [...] > 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. Ok, I think I was misunderstanding your point on the reply to Andrew. I was thinking you will eventually move the tests to selftests, but not for now, only because it is not ready to run. I understand your points now. > > > > > 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. I read this from the previous thread, and this is really cool. I was thinking it would be really nice if more kernel subsystems and features be able to do this kind of great testing with minimum duplicated efforts. That was one of the motivations of my previous reply. > > 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. As I also previously mentioned, I was assuming you made the decision to not use KUnit based on real limitations of KUnit you found. Thank you so much for this detailed explanations with nice examples. [...] > > 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. No worry, I'm not willing to delay this work with unnecessary discussions. That's why I'm saying I have no strong opinion. I'm rather regret that I don't have enough time to get a credit on this great work by reading the details and provide my Reviewed-by:. What I want to say is that it would be nice to ensure the developers of Kselftest and Kunit, who obviously have experiences on testing, get a chance to be involved in this discussion. I believe that would be nice since they might find something we're misunderstanding about Kselftest and/or Kunit. Also they might find some unknown limitations of Kselftest and/or Kunit that you found. I personally hope it is the latter case and it helps evolving KUnit, so that not only vma but also other kernel subsystems and features be able to enhance their test setups with minimum efforts. Again, I don't think such discussions and possible future works sould be blockers of this work. > > Kees was agreeable with this approach so I don't think we should really see > too much objection to this. You're right. Nonetheless, I found the mail is not Cc-ing KUnit developers, and then I thought giving KUnit developers more chances to be involved would be nice. Thanks, SJ [...]
On Thu, 4 Jul 2024 00:24:15 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > 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. Yes, I was thinking we'd just add a selftest which does (simplified, of course) cd ../../vma make ./whatever simply to cause this new code to be invoked when someone runs the selftest suite.
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. (Personally, I think a userspace implementation of a subset of KUnit or a KUnit-like API would be useful, see below.) On Thu, 4 Jul 2024 at 06:56, SeongJae Park <sj@kernel.org> wrote: > > On Wed, 3 Jul 2024 21:33:00 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.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@oracle.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. > 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. > > 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. 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,. Cheers, -- David [1]: https://lore.kernel.org/all/20240625211803.2750563-5-willy@infradead.org/
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@kernel.org> wrote: > > > > On Wed, 3 Jul 2024 21:33:00 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.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@oracle.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/
There are a number of "core" VMA manipulation functions implemented in mm/mmap.c, notably those concerning VMA merging, splitting, modifying, expanding and shrinking, which logically don't belong there. More importantly this functionality represents an internal implementation detail of memory management and should not be exposed outside of mm/ itself. This patch series isolates core VMA manipulation functionality into its own file, mm/vma.c, and provides an API to the rest of the mm code in mm/vma.h. Importantly, it also carefully implements mm/vma_internal.h, which specifies which headers need to be imported by vma.c, leading to the very useful property that vma.c depends only on mm/vma.h and mm/vma_internal.h. This means we can then re-implement vma_internal.h in userland, adding shims for kernel mechanisms as required, allowing us to unit test internal VMA functionality. This testing is useful as opposed to an e.g. kunit implementation as this way we can avoid all external kernel side-effects while testing, run tests VERY quickly, and iterate on and debug problems quickly. Excitingly this opens the door to, in the future, recreating precise problems observed in production in userland and very quickly debugging problems that might otherwise be very difficult to reproduce. This patch series takes advantage of existing shim logic and full userland maple tree support contained in tools/testing/radix-tree/ and tools/include/linux/, separating out shared components of the radix tree implementation to provide this testing. 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. A simple, skeleton testing implementation is provided in tools/testing/vma/vma.c as a proof-of-concept, asserting that simple VMA merge, modify (testing split), expand and shrink functionality work correctly. v1: * Fix test_simple_modify() to specify correct prev. * Improve vma test Makefile so it picks up dependency changes correctly. * Rename relocate_vma() to relocate_vma_down(). * Remove shift_arg_pages() and invoked relocate_vma_down() directly from setup_arg_pages(). * MAINTAINERS fixups. RFC v2: * Reword commit messages. * Replace vma_expand() / vma_shrink() wrappers with relocate_vma(). * Make move_page_tables() internal too. * Have internal.h import vma.h. * Use header guards to more cleanly implement userland testing code. * Rename main.c to vma.c. * Update mm/vma_internal.h to have fewer superfluous comments. * Rework testing logic so we count test failures, and output test results. * Correct some SPDX license prefixes. * Make VM_xxx_ON() debug asserts forward to xxx_ON() macros. * Update VMA tests to correctly free memory, and re-enable ASAN leak detection. https://lore.kernel.org/all/cover.1719584707.git.lstoakes@gmail.com/ RFC v1: https://lore.kernel.org/all/cover.1719481836.git.lstoakes@gmail.com/ Lorenzo Stoakes (7): userfaultfd: move core VMA manipulation logic to mm/userfaultfd.c mm: move vma_modify() and helpers to internal header mm: move vma_shrink(), vma_expand() to internal header mm: move internal core VMA manipulation functions to own file MAINTAINERS: Add entry for new VMA files tools: separate out shared radix-tree components tools: add skeleton code for userland testing of VMA logic MAINTAINERS | 14 + fs/exec.c | 81 +- fs/userfaultfd.c | 160 +- include/linux/atomic.h | 2 +- include/linux/mm.h | 112 +- include/linux/mmzone.h | 3 +- include/linux/userfaultfd_k.h | 19 + mm/Makefile | 2 +- mm/internal.h | 167 +- mm/mmap.c | 2069 ++--------------- mm/mmu_notifier.c | 2 + mm/userfaultfd.c | 168 ++ mm/vma.c | 1766 ++++++++++++++ mm/vma.h | 362 +++ mm/vma_internal.h | 52 + tools/testing/radix-tree/Makefile | 68 +- tools/testing/radix-tree/maple.c | 14 +- tools/testing/radix-tree/xarray.c | 9 +- tools/testing/shared/autoconf.h | 2 + tools/testing/{radix-tree => shared}/bitmap.c | 0 tools/testing/{radix-tree => shared}/linux.c | 0 .../{radix-tree => shared}/linux/bug.h | 0 .../{radix-tree => shared}/linux/cpu.h | 0 .../{radix-tree => shared}/linux/idr.h | 0 .../{radix-tree => shared}/linux/init.h | 0 .../{radix-tree => shared}/linux/kconfig.h | 0 .../{radix-tree => shared}/linux/kernel.h | 0 .../{radix-tree => shared}/linux/kmemleak.h | 0 .../{radix-tree => shared}/linux/local_lock.h | 0 .../{radix-tree => shared}/linux/lockdep.h | 0 .../{radix-tree => shared}/linux/maple_tree.h | 0 .../{radix-tree => shared}/linux/percpu.h | 0 .../{radix-tree => shared}/linux/preempt.h | 0 .../{radix-tree => shared}/linux/radix-tree.h | 0 .../{radix-tree => shared}/linux/rcupdate.h | 0 .../{radix-tree => shared}/linux/xarray.h | 0 tools/testing/shared/maple-shared.h | 9 + tools/testing/shared/maple-shim.c | 7 + tools/testing/shared/shared.h | 34 + tools/testing/shared/shared.mk | 68 + .../testing/shared/trace/events/maple_tree.h | 5 + tools/testing/shared/xarray-shared.c | 5 + tools/testing/shared/xarray-shared.h | 4 + tools/testing/vma/.gitignore | 6 + tools/testing/vma/Makefile | 16 + tools/testing/vma/errors.txt | 0 tools/testing/vma/generated/autoconf.h | 2 + tools/testing/vma/linux/atomic.h | 12 + tools/testing/vma/linux/mmzone.h | 38 + tools/testing/vma/vma.c | 207 ++ tools/testing/vma/vma_internal.h | 882 +++++++ 51 files changed, 3914 insertions(+), 2453 deletions(-) create mode 100644 mm/vma.c create mode 100644 mm/vma.h create mode 100644 mm/vma_internal.h create mode 100644 tools/testing/shared/autoconf.h rename tools/testing/{radix-tree => shared}/bitmap.c (100%) rename tools/testing/{radix-tree => shared}/linux.c (100%) rename tools/testing/{radix-tree => shared}/linux/bug.h (100%) rename tools/testing/{radix-tree => shared}/linux/cpu.h (100%) rename tools/testing/{radix-tree => shared}/linux/idr.h (100%) rename tools/testing/{radix-tree => shared}/linux/init.h (100%) rename tools/testing/{radix-tree => shared}/linux/kconfig.h (100%) rename tools/testing/{radix-tree => shared}/linux/kernel.h (100%) rename tools/testing/{radix-tree => shared}/linux/kmemleak.h (100%) rename tools/testing/{radix-tree => shared}/linux/local_lock.h (100%) rename tools/testing/{radix-tree => shared}/linux/lockdep.h (100%) rename tools/testing/{radix-tree => shared}/linux/maple_tree.h (100%) rename tools/testing/{radix-tree => shared}/linux/percpu.h (100%) rename tools/testing/{radix-tree => shared}/linux/preempt.h (100%) rename tools/testing/{radix-tree => shared}/linux/radix-tree.h (100%) rename tools/testing/{radix-tree => shared}/linux/rcupdate.h (100%) rename tools/testing/{radix-tree => shared}/linux/xarray.h (100%) create mode 100644 tools/testing/shared/maple-shared.h create mode 100644 tools/testing/shared/maple-shim.c create mode 100644 tools/testing/shared/shared.h create mode 100644 tools/testing/shared/shared.mk create mode 100644 tools/testing/shared/trace/events/maple_tree.h create mode 100644 tools/testing/shared/xarray-shared.c create mode 100644 tools/testing/shared/xarray-shared.h create mode 100644 tools/testing/vma/.gitignore create mode 100644 tools/testing/vma/Makefile create mode 100644 tools/testing/vma/errors.txt create mode 100644 tools/testing/vma/generated/autoconf.h create mode 100644 tools/testing/vma/linux/atomic.h create mode 100644 tools/testing/vma/linux/mmzone.h create mode 100644 tools/testing/vma/vma.c create mode 100644 tools/testing/vma/vma_internal.h -- 2.45.2