Message-ID: <aCtsyA6-kzNLlf4L@yury>
Date: Mon, 19 May 2025 13:39:20 -0400
From: Yury Norov <yury.norov@...il.com>
To: Burak Emir <bqe@...gle.com>
Cc: Kees Cook <kees@...nel.org>,
	Rasmus Villemoes <linux@...musvillemoes.dk>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	"Gustavo A . R . Silva" <gustavoars@...nel.org>,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH v8 4/5] rust: add find_bit_benchmark_rust module.

On Mon, May 19, 2025 at 04:17:04PM +0000, Burak Emir wrote:
> Microbenchmark protected by a config FIND_BIT_BENCHMARK_RUST,
> following `find_bit_benchmark.c` but testing the Rust Bitmap API.

I already asked this: please print the output of this test together
wit C version, and please make sure it's collected on bare metal
machine.
 
> We add a fill_random() method protected by the config in order to
> maintain the abstraction.
> 
> Suggested-by: Alice Ryhl <aliceryhl@...gle.com>
> Suggested-by: Yury Norov <yury.norov@...il.com>
> Signed-off-by: Burak Emir <bqe@...gle.com>
> ---
>  MAINTAINERS                     |  1 +
>  lib/Kconfig.debug               | 13 +++++
>  lib/Makefile                    |  1 +
>  lib/find_bit_benchmark_rust.rs  | 94 +++++++++++++++++++++++++++++++++
>  rust/bindings/bindings_helper.h |  1 +
>  rust/kernel/bitmap.rs           | 14 +++++
>  6 files changed, 124 insertions(+)
>  create mode 100644 lib/find_bit_benchmark_rust.rs
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 565eaa015d9e..943d85ed1876 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4132,6 +4132,7 @@ M:	Alice Ryhl <aliceryhl@...gle.com>
>  M:	Burak Emir <bqe@...gle.com>
>  R:	Yury Norov <yury.norov@...il.com>
>  S:	Maintained
> +F:	lib/find_bit_benchmark_rust.rs
>  F:	rust/kernel/bitmap.rs
>  
>  BITOPS API
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index f9051ab610d5..37a07559243e 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2605,6 +2605,19 @@ config FIND_BIT_BENCHMARK
>  
>  	  If unsure, say N.
>  
> +config FIND_BIT_BENCHMARK_RUST

Shouldn't this depend on config RUST, and maybe something else?

> +	tristate "Test find_bit functions in Rust"
> +	help
> +	  This builds the "find_bit_benchmark_rust" module. It is a micro
> +          benchmark that measures the performance of Rust functions that
> +          correspond to the find_*_bit() operations in C. It follows the
> +          FIND_BIT_BENCHMARK closely but will in general not yield same
> +          numbers due to extra bounds checks and overhead of foreign
> +          function calls.
> +
> +	  If unsure, say N.
> +
> +
>  config TEST_FIRMWARE
>  	tristate "Test firmware loading via userspace interface"
>  	depends on FW_LOADER
> diff --git a/lib/Makefile b/lib/Makefile
> index f07b24ce1b3f..99e49a8f5bf8 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -62,6 +62,7 @@ obj-y += hexdump.o
>  obj-$(CONFIG_TEST_HEXDUMP) += test_hexdump.o
>  obj-y += kstrtox.o
>  obj-$(CONFIG_FIND_BIT_BENCHMARK) += find_bit_benchmark.o
> +obj-$(CONFIG_FIND_BIT_BENCHMARK_RUST) += find_bit_benchmark_rust.o
>  obj-$(CONFIG_TEST_BPF) += test_bpf.o
>  test_dhry-objs := dhry_1.o dhry_2.o dhry_run.o
>  obj-$(CONFIG_TEST_DHRY) += test_dhry.o
> diff --git a/lib/find_bit_benchmark_rust.rs b/lib/find_bit_benchmark_rust.rs
> new file mode 100644
> index 000000000000..13830477a8d2
> --- /dev/null
> +++ b/lib/find_bit_benchmark_rust.rs
> @@ -0,0 +1,94 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//! Benchmark for find_bit-like methods in Bitmap Rust API.
> +
> +use kernel::alloc::flags::GFP_KERNEL;
> +use kernel::bindings;
> +use kernel::bitmap::Bitmap;
> +use kernel::error::{code, Result};
> +use kernel::pr_err;
> +use kernel::prelude::module;
> +use kernel::time::Ktime;
> +use kernel::ThisModule;
> +
> +const BITMAP_LEN: usize = 4096 * 8 * 10;
> +// Reciprocal of the fraction of bits that are set in sparse bitmap.
> +const SPARSENESS: usize = 500;
> +
> +/// Test module that benchmarks performance of traversing bitmaps.
> +struct FindBitBenchmarkModule();
> +
> +fn test_next_bit(bitmap: &Bitmap) {
> +    let mut time = Ktime::ktime_get();
> +    let mut cnt = 0;
> +    let mut i = 0;
> +
> +    while let Some(index) = bitmap.next_bit(i) {
> +        cnt += 1;
> +        i = index + 1;
> +    }
> +
> +    time = Ktime::ktime_get() - time;
> +    pr_err!(
> +        "next_bit:           {:18} ns, {:6} iterations\n",
> +        time.to_ns(),
> +        cnt
> +    );
> +}
> +
> +fn test_next_zero_bit(bitmap: &Bitmap) {
> +    let mut time = Ktime::ktime_get();
> +    let mut cnt = 0;
> +    let mut i = 0;
> +
> +    while let Some(index) = bitmap.next_zero_bit(i) {
> +        cnt += 1;
> +        i = index + 1;
> +    }
> +
> +    time = Ktime::ktime_get() - time;
> +    pr_err!(
> +        "next_zero_bit:      {:18} ns, {:6} iterations\n",
> +        time.to_ns(),
> +        cnt
> +    );
> +}
> +
> +fn find_bit_test() {
> +    pr_err!("Start testing find_bit() Rust with random-filled bitmap\n");
> +
> +    let mut bitmap = Bitmap::new(BITMAP_LEN, GFP_KERNEL).expect("alloc bitmap failed");
> +    bitmap.fill_random();
> +
> +    test_next_bit(&bitmap);
> +    test_next_zero_bit(&bitmap);
> +
> +    pr_err!("Start testing find_bit() Rust with sparse bitmap\n");
> +
> +    let mut bitmap = Bitmap::new(BITMAP_LEN, GFP_KERNEL).expect("alloc sparse bitmap failed");
> +    let nbits = BITMAP_LEN / SPARSENESS;
> +    for _i in 0..nbits {
> +        // SAFETY: BITMAP_LEN fits in 32 bits.
> +        let bit: usize =
> +            unsafe { bindings::__get_random_u32_below(BITMAP_LEN.try_into().unwrap()) as _ };
> +        bitmap.set_bit(bit);
> +    }
> +
> +    test_next_bit(&bitmap);
> +    test_next_zero_bit(&bitmap);
> +}
> +
> +impl kernel::Module for FindBitBenchmarkModule {
> +    fn init(_module: &'static ThisModule) -> Result<Self> {
> +        find_bit_test();
> +        // Return error so test module can be inserted again without rmmod.
> +        Err(code::EINVAL)
> +    }
> +}
> +
> +module! {
> +    type: FindBitBenchmarkModule,

Can you explain the meaning of 'type' section? To me your type is too
unique. This way, we'll end up having the number of types equal to
the number of modules.

Maybe just 'Benchmark'?

> +    name: "find_bit_benchmark_rust_module",
> +    authors: ["Rust for Linux Contributors"],

To me it's pretty useless. I think this 'authors' section exists to
help those having troubles with the module to reach the right people.
Can you please add your name and email here?

> +    description: "Module with benchmark for bitmap code!",
> +    license: "GPL v2",
> +}
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index b6bf3b039c1b..f6ca7f1dd08b 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -31,6 +31,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/poll.h>
>  #include <linux/property.h>
> +#include <linux/random.h>
>  #include <linux/refcount.h>
>  #include <linux/sched.h>
>  #include <linux/security.h>
> diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
> index 943dbef7948b..fb0c687420cd 100644
> --- a/rust/kernel/bitmap.rs
> +++ b/rust/kernel/bitmap.rs
> @@ -124,6 +124,20 @@ pub fn len(&self) -> usize {
>          self.nbits
>      }
>  
> +    /// Fills this `Bitmap` with random bits.
> +    #[cfg(CONFIG_FIND_BIT_BENCHMARK_RUST)]
> +    pub fn fill_random(&mut self) {
> +        // SAFETY: `self.as_mut_ptr` points to either an array of the
> +        // appropriate length or one usize.
> +        unsafe {
> +            bindings::get_random_bytes(
> +                self.as_mut_ptr() as *mut ffi::c_void,
> +                usize::div_ceil(self.nbits, bindings::BITS_PER_LONG as usize)
> +                    * bindings::BITS_PER_LONG as usize,
> +            );
> +        }
> +    }
> +
>      /// Returns a mutable raw pointer to the backing [`Bitmap`].
>      #[inline]
>      fn as_mut_ptr(&mut self) -> *mut usize {
> -- 
> 2.49.0.1101.gccaa498523-goog

Powered by blists - more mailing lists