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@xxxxxxxxxx> wrote: > > On Wed, 3 Jul 2024 21:33:00 +0100 Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxx/
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
