Message ID | 20220507052451.12890-1-ojeda@kernel.org (mailing list archive) |
---|---|
Headers | show |
Series | Rust support | expand |
On Sat, May 07, 2022 at 07:23:58AM +0200, Miguel Ojeda wrote: > ## Patch series status > > The Rust support is still to be considered experimental. However, > support is good enough that kernel developers can start working on the > Rust abstractions for subsystems and write drivers and other modules. I'd really like to see this landed for a few reasons: - It's under active development, and I'd rather review the changes "normally", incrementally, etc. Right now it can be hard to re-review some of the "mostly the same each version" patches in the series. - I'd like to break the catch-22 of "ask for a new driver to be written in rust but the rust support isn't landed" vs "the rust support isn't landed because there aren't enough drivers". It really feels like "release early, release often" is needed here; it's hard to develop against -next. :) Should we give it a try for this coming merge window?
On Sat, May 7, 2022 at 1:25 PM Miguel Ojeda <ojeda@kernel.org> wrote: > > Rust support > <...> > - Support running documentation tests in-kernel, based on KUnit. > > Rust documentation tests are typically examples of usage of any > item (e.g. function, struct, module...). They are very convenient > because they are just written alongside the documentation, e.g.: > > /// Sums two numbers. > /// > /// # Examples > /// > /// ``` > /// assert_eq!(mymod::f(10, 20), 30); > /// ``` > pub fn f(a: i32, b: i32) -> i32 { > a + b > } > > So far, we were compiling and running them in the host as any > other Rust documentation test. However, that meant we could not > run tests that used kernel APIs (though we were compile-testing > them, which was already useful to keep the documentation in sync > with the code). > > Now, the documentation tests for the `kernel` crate are > transformed into a KUnit test suite during compilation and run > within the kernel at boot time, if enabled. This means now we can > run the tests that use kernel APIs. > > They look like this (their name is generated by `rustdoc`, based > on the file and line): > > [ 0.581961] TAP version 14 > [ 0.582092] 1..1 > [ 0.582267] # Subtest: rust_kernel_doctests > [ 0.582358] 1..70 > [ 0.583626] ok 1 - rust_kernel_doctest_build_assert_rs_12_0 > [ 0.584579] ok 2 - rust_kernel_doctest_build_assert_rs_55_0 > [ 0.587357] ok 3 - rust_kernel_doctest_device_rs_361_0 > [ 0.588037] ok 4 - rust_kernel_doctest_device_rs_386_0 > > ... > > [ 0.659249] ok 69 - rust_kernel_doctest_types_rs_445_0 > [ 0.660451] ok 70 - rust_kernel_doctest_types_rs_509_0 > [ 0.660680] # rust_kernel_doctests: pass:70 fail:0 skip:0 total:70 > [ 0.660894] # Totals: pass:70 fail:0 skip:0 total:70 > [ 0.661135] ok 1 - rust_kernel_doctests > > There are other benefits from this, such as being able to remove > unneeded wrapper functions (that were used to avoid running > some tests) as well as ensuring test code would actually compile > within the kernel (e.g. `alloc` used different `cfg`s). It's great to see some KUnit support here! It's also possible to run these tests using the KUnit wrapper tool with: $ ./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_RUST=y --make_options LLVM=1 --arch x86_64 'rust_kernel_doctests' That also nicely formats the results. (It obviously doesn't run under UML yet, though I did get it to work after indiscriminately hacking out everything that wasn't supported. Assuming we can hide the irq and iomem stuff behind the appropriate config options, and rework some of the architecture detection to either support SUBARCH or check for X86_64 instead of X86, it should be pretty easy to get going.) That all being said, I can't say I'm thrilled with the test names here: none of them are particularly descriptive, and they'll probably not be static (which would make it difficult to track results / regressions / etc between kernel versions). Neither of those are necessarily deal breakers, though it might make sense to hide them behind a kernel option (like all other KUnit tests) so that they can easily be excluded where they would otherwise clutter up results. (And if there's a way to properly name them, or maybe even split them into per-file or per-module suites, that would make them a bit easier to deal.) Additionally, there are some plans to taint the kernel[1] when KUnit tests run, so having a way to turn them off would be very useful. Regardless, this is very neat, and I'm looking forward to taking a closer look at it. Cheers, -- David [1]: https://lore.kernel.org/linux-kselftest/20220429043913.626647-1-davidgow@google.com/
Hi David, On Sat, May 7, 2022 at 11:29 AM David Gow <davidgow@google.com> wrote: > > It's great to see some KUnit support here! Thanks! > It's also possible to run these tests using the KUnit wrapper tool with: > $ ./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_RUST=y > --make_options LLVM=1 --arch x86_64 'rust_kernel_doctests' > > That also nicely formats the results. Indeed! [16:55:52] ============ rust_kernel_doctests (70 subtests) ============ [16:55:52] [PASSED] rust_kernel_doctest_build_assert_rs_12_0 [16:55:52] [PASSED] rust_kernel_doctest_build_assert_rs_55_0 ... [16:55:52] [PASSED] rust_kernel_doctest_types_rs_445_0 [16:55:52] [PASSED] rust_kernel_doctest_types_rs_509_0 [16:55:52] ============== [PASSED] rust_kernel_doctests =============== [16:55:52] ============================================================ [16:55:52] Testing complete. Passed: 70, Failed: 0, Crashed: 0, Skipped: 0, Errors: 0 > That all being said, I can't say I'm thrilled with the test names > here: none of them are particularly descriptive, and they'll probably > not be static (which would make it difficult to track results / > regressions / etc between kernel versions). Neither of those are Yeah, the names are not great and would change from time to time across kernel versions. We could ask example writers to give each example a name, but that would make them fairly less convenient. For instance, sometimes they may be very small snippets interleaved with docs' prose (where giving a name may feel a bit of a burden, and people may end writing `foo_example1`, `foo_example2` etc. for each small "step" of an explanation). In other cases they may be very long, testing a wide API surface (e.g. when describing a module or type), where it is also hard to give non-generic names like `rbtree_doctest`. In those kind of cases, I think we would end up with not much better names than automatically generated ones. The other aspect is that, given they are part of the documentation, the prose or how things are explained/split may change, thus the doctests as well. For instance, one may need to split a very long `rbtree_doctest` in pieces, and then the name would need to change anyway. So I think we should avoid asking documentation writers to add a manual name, even if that means a bit ugly test names. Also this way they are consistently named. What do you think? One idea could be giving them a name based on the hash of the content and avoiding the line number, so that there is a higher chance for the name to stay the same even when the file gets modified for other reasons. > necessarily deal breakers, though it might make sense to hide them > behind a kernel option (like all other KUnit tests) so that they can > easily be excluded where they would otherwise clutter up results. (And Currently they are under `CONFIG_RUST_KERNEL_KUNIT_TEST` -- or do you mean something else? > if there's a way to properly name them, or maybe even split them into > per-file or per-module suites, that would make them a bit easier to > deal.) Additionally, there are some plans to taint the kernel[1] when Yeah, splitting them further is definitely possible. We are also likely splitting the `kernel` crate into several, which would also make the suites smaller etc. so perhaps further splits may not be needed. > Regardless, this is very neat, and I'm looking forward to taking a > closer look at it. Thanks again for taking a look and playing with it, I am glad you liked it! (even if it is just a first approximation, and only supports the `kernel` crate, etc.). Cheers, Miguel
On Sat, May 07, 2022 at 01:06:18AM -0700, Kees Cook wrote: > On Sat, May 07, 2022 at 07:23:58AM +0200, Miguel Ojeda wrote: > > ## Patch series status > > > > The Rust support is still to be considered experimental. However, > > support is good enough that kernel developers can start working on the > > Rust abstractions for subsystems and write drivers and other modules. > > I'd really like to see this landed for a few reasons: > > - It's under active development, and I'd rather review the changes > "normally", incrementally, etc. Right now it can be hard to re-review > some of the "mostly the same each version" patches in the series. > > - I'd like to break the catch-22 of "ask for a new driver to be > written in rust but the rust support isn't landed" vs "the rust > support isn't landed because there aren't enough drivers". It > really feels like "release early, release often" is needed here; > it's hard to develop against -next. :) > > Should we give it a try for this coming merge window? I'm broadly in favour of that. It's just code, we can always drop it again or fix it. There's sufficient development community around it that it's hardly going to become abandonware.
On Sat, May 07, 2022 at 01:06:18AM -0700, Kees Cook wrote: > On Sat, May 07, 2022 at 07:23:58AM +0200, Miguel Ojeda wrote: > > ## Patch series status > > > > The Rust support is still to be considered experimental. However, > > support is good enough that kernel developers can start working on the > > Rust abstractions for subsystems and write drivers and other modules. > > I'd really like to see this landed for a few reasons: > > - It's under active development, and I'd rather review the changes > "normally", incrementally, etc. Right now it can be hard to re-review > some of the "mostly the same each version" patches in the series. > > - I'd like to break the catch-22 of "ask for a new driver to be > written in rust but the rust support isn't landed" vs "the rust > support isn't landed because there aren't enough drivers". It > really feels like "release early, release often" is needed here; > it's hard to develop against -next. :) +1 to both points. :-)
On Sat, May 7, 2022 at 11:03 PM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > Hi David, > > On Sat, May 7, 2022 at 11:29 AM David Gow <davidgow@google.com> wrote: > > > > It's great to see some KUnit support here! > > Thanks! > > > It's also possible to run these tests using the KUnit wrapper tool with: > > $ ./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_RUST=y > > --make_options LLVM=1 --arch x86_64 'rust_kernel_doctests' > > > > That also nicely formats the results. > > Indeed! > > [16:55:52] ============ rust_kernel_doctests (70 subtests) ============ > [16:55:52] [PASSED] rust_kernel_doctest_build_assert_rs_12_0 > [16:55:52] [PASSED] rust_kernel_doctest_build_assert_rs_55_0 > ... > [16:55:52] [PASSED] rust_kernel_doctest_types_rs_445_0 > [16:55:52] [PASSED] rust_kernel_doctest_types_rs_509_0 > [16:55:52] ============== [PASSED] rust_kernel_doctests =============== > [16:55:52] ============================================================ > [16:55:52] Testing complete. Passed: 70, Failed: 0, Crashed: 0, > Skipped: 0, Errors: 0 > I've just sent out a pull request to get this working under UML as well, which would simplify running these further: https://github.com/Rust-for-Linux/linux/pull/766 > > That all being said, I can't say I'm thrilled with the test names > > here: none of them are particularly descriptive, and they'll probably > > not be static (which would make it difficult to track results / > > regressions / etc between kernel versions). Neither of those are > > Yeah, the names are not great and would change from time to time > across kernel versions. > > We could ask example writers to give each example a name, but that > would make them fairly less convenient. For instance, sometimes they > may be very small snippets interleaved with docs' prose (where giving > a name may feel a bit of a burden, and people may end writing > `foo_example1`, `foo_example2` etc. for each small "step" of an > explanation). In other cases they may be very long, testing a wide API > surface (e.g. when describing a module or type), where it is also hard > to give non-generic names like `rbtree_doctest`. In those kind of > cases, I think we would end up with not much better names than > automatically generated ones. > > The other aspect is that, given they are part of the documentation, > the prose or how things are explained/split may change, thus the > doctests as well. For instance, one may need to split a very long > `rbtree_doctest` in pieces, and then the name would need to change > anyway. > > So I think we should avoid asking documentation writers to add a > manual name, even if that means a bit ugly test names. Also this way > they are consistently named. What do you think? Yeah, these are all fair points: particularly for small doctests. Maybe having an optional name, which more significant tests could use to override the file:line names? That could be useful for a few of the larger, more often referenced tests. > One idea could be giving them a name based on the hash of the content > and avoiding the line number, so that there is a higher chance for the > name to stay the same even when the file gets modified for other > reasons. Ugh: it's a bit ugly either way. I suspect that file:line is still probably better, if only because we need some way of looking up the test in the code if it fails. I'd hate for people to be randomly hashing bits of just to find out what test is failing. > > necessarily deal breakers, though it might make sense to hide them > > behind a kernel option (like all other KUnit tests) so that they can > > easily be excluded where they would otherwise clutter up results. (And > > Currently they are under `CONFIG_RUST_KERNEL_KUNIT_TEST` -- or do you > mean something else? > Oops: I missed that (one of the issues with testing this on a different machine which had a rust toolchain). Looks good to me. > > if there's a way to properly name them, or maybe even split them into > > per-file or per-module suites, that would make them a bit easier to > > deal.) Additionally, there are some plans to taint the kernel[1] when > > Yeah, splitting them further is definitely possible. We are also > likely splitting the `kernel` crate into several, which would also > make the suites smaller etc. so perhaps further splits may not be > needed. Ah: I didn't realise the plan was always to have crate-specific suites, and possibly to split things up. The KTAP output specification does actually support arbitrary nesting (though KUnit itself doesn't at the moment), which would potentially be an option if (e.g.) providing the complete module nesting made sense. I'm not convinced that'd make things easier to read, though. > > Regardless, this is very neat, and I'm looking forward to taking a > > closer look at it. > > Thanks again for taking a look and playing with it, I am glad you > liked it! (even if it is just a first approximation, and only supports > the `kernel` crate, etc.). > > Cheers, > Miguel Thanks, -- David
Hi David, On Tue, May 10, 2022 at 6:45 AM David Gow <davidgow@google.com> wrote: > > I've just sent out a pull request to get this working under UML as > well, which would simplify running these further: > https://github.com/Rust-for-Linux/linux/pull/766 Thanks a lot! > Yeah, these are all fair points: particularly for small doctests. > > Maybe having an optional name, which more significant tests could use > to override the file:line names? That could be useful for a few of the > larger, more often referenced tests. Sounds reasonable. I can add support for that. > Ugh: it's a bit ugly either way. I suspect that file:line is still > probably better, if only because we need some way of looking up the > test in the code if it fails. I'd hate for people to be randomly > hashing bits of just to find out what test is failing. One redeeming quality is that the assertion prints the line/file number in the generated file, so it would still be possible to check where it came from: [13:13:43] # rust_kernel_doctest_str_rs_somehash: ASSERTION FAILED at rust/doctests_kernel_generated.rs:2209 [13:13:43] Expected 2 > 3 to be true, but is false [13:13:43] not ok 43 - rust_kernel_doctest_str_rs_somehash [13:13:43] [FAILED] rust_kernel_doctest_str_rs_somehash Another alternative is to keep the file:line information around without embedding it into the test name, e.g. in a TAP comment or a mapping file (which `kunit.py` could read). But, yeah, before doing hashes or things like that, I would just go for simplicity and keep things as they are unless some use case really needs doctests to be stable. > Oops: I missed that (one of the issues with testing this on a > different machine which had a rust toolchain). Looks good to me. > > Ah: I didn't realise the plan was always to have crate-specific > suites, and possibly to split things up. > > The KTAP output specification does actually support arbitrary nesting > (though KUnit itself doesn't at the moment), which would potentially > be an option if (e.g.) providing the complete module nesting made > sense. I'm not convinced that'd make things easier to read, though. That is useful to know in case we need it, thanks! Cheers, Miguel