Re: [PATCH v5 3/4] rust: add bitmap API.

[David Gow <davidgow@google.com>]

[-- Attachment #1: Type: text/plain, Size: 3928 bytes --]

On Sat, 22 Mar 2025 at 02:50, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> Hi Yury,
>
> A couple comments in case it helps regarding the docs/comments discussion...
>
> On Fri, Mar 21, 2025 at 5:04 PM Yury Norov <yury.norov@gmail.com> wrote:
> >
> > In C we separate declarations from function body with an empty line.
> > Can you do that in rust? Can you point to a rust coding style? Do you
> > guys really use 2-whitespace tabs?
>
> Please see https://docs.kernel.org/rust/coding-guidelines.html.
>
> You are right that the example block you quoted should have the
> formatting fixed.
>
> I am not sure what you mean by separating declarations from body here.
> I guess you mean variable declarations in C vs. the rest of the body
> -- in Rust it is not done, i.e. declaring new bindings in the middle
> as they are needed is expected (and sometimes needed).
>
> > I think I already asked to make the test a separate unit. It's amazing
> > that rust understands scattered commented blocks of code and can turn
> > them into unit tests. Unfortunately, I'm not.
> >
> > Please create a separate unit and test everything there, just like we
> > do with normal C code.
>
> APIs should have examples, ideally good ones etc. The above looks like
> an standard example, but maybe I am missing something.
>
> The examples are meant to be documentation first and foremost, that
> happens to double as a test (so that it does not get out of sync
> etc.). It is how we document everything else in Rust, and in fact we
> are becoming stricter in requesting more examples when people add more
> APIs (otherwise they never get added :)
>
> If actual tests are needed (i.e. on top of what examples provide),
> then we just finally landed in -next `#[test]`-like support, i.e. unit
> tests that can be written separately. We try to have tests as close as
> possible to the code they exercise, too, but in some cases it may be
> best to separate them (e.g. if there are way too many).
>
> > For find_bit functions we have a lib/find_bit_benchmark.c Can you add
> > a similar rust test, so we'll make sure you're not introducing
> > performance regressions with your wrappers?
> >
> > Please don't use KUNITs. It's not ready for benchmarks, and tests built
> > against it don't run on major distros.
>
> Cc'ing David here in case he has more context around this...
>
> I agree having a good "integrated benchmark" system would be nice, and
> being able to mark particular "tests" as benchmarks etc.
>
> Regarding distributions, it sounds an orthogonal issue to me, but I
> may be lacking context...
>

Yeah: nothing has particularly changed here. KUnit is not aimed at
benchmarks specifically, and while it isn't impossible to get
something "benchmark-like" to work with it, it's definitely
suboptimal. And there aren't any immediate plans to add any more
benchmarking functionality to KUnit, though I'd be happy to hear any
proposals. :-)

If requiring CONFIG_KUNIT to be set (which, as mentioned in previous
discussions, is not the default on many distros) is a dealbreaker for
running tests, then of course that limits any tests to not using
KUnit. That being said, I suspect that supporting the "just build this
one test module against your existing kernel" case is going to be a
bit more of a pain here anyway, as it might end up depending on having
exactly the same toolchain/config/etc due to Rust's ABI not being
stable. (Am I missing anything here, Miguel?) And, of course, Rust's
built-in tests here would get automatically compiled down to KUnit
tests if enabled.

So, what I suspect you're looking for here is a separate module /
crate which benchmarks the bitmap type. With the way Rust crates are
laid out, I suspect this would need to live in a separate directory
and look something like samples/rust/rust_minimal.rs?

-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5281 bytes --]