Re: [PATCH v2] rust: add bindings and API for bitmap.h and bitops.h.

[Alice Ryhl <aliceryhl@google.com>]

On Mon, Mar 3, 2025 at 12:55 PM Burak Emir <bqe@google.com> wrote:
>
> Adds a Rust bitmap API and necessary bitmap and bitops bindings.
> These are for porting the approach from commit 15d9da3f818c ("binder:
> use bitmap for faster descriptor lookup") to Rust. The functionality
> in dbitmap.h makes use of bitmap and bitops.
>
> The Rust bitmap API provides an abstraction to underlying bitmap
> and bitops operations. For now, we only include methods that are
> necessary for reimplementing dbitmap.h. It is straightforward to add
> more methods later, as needed. We offer a safe API through
> bounds checks which panic if violated.
>
> We introduce bindings for the non-atomic variants __set_bit and
> __clear_bit, and use the _find_* variants instead of the find_*
> wrappers. The C optimizations enabled by the wrappers does not carry
> over to Rust.
>
> This series uses the usize type for sizes and indices into the bitmap,
> because Rust generally always uses that type for indices and lengths
> and it will be more convenient if the API accepts that type. This means
> that we need to perform some casts to/from u32 and usize, since the C
> headers use unsigned int instead of size_t/unsigned long for these
> numbers in some places.
>
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Burak Emir <bqe@google.com>
> ---
> This is v2 of a patch introducing Rust bitmap API [v1]. Thanks
> for all the helpful comments!
>
> Changes v1 --> v2:
> - Rebased on Yury's v2 patch [1] and Viresh's v2 patch [2]
> - Removed import of `bindings::*`, keeping only prefix (Miguel)
> - Renamed panic methods to make more explicit (Miguel)
> - use markdown in doc comments and added example/kunit test (Miguel)
> - Added maintainer section for BITOPS API BINDINGS [RUST] (Yury)
> - Added M: entry for bitmap.rs which goes to Alice (Viresh, Alice)
> - Changed calls from find_* to _find_*, removed helpers (Yury)
> - Use non-atomic __set_bit and __clear_bit from Bitmap Rust API (Yury)
>
> Question to Yury: we could remove `bitmap_copy` helper and instead do
> the memcpy in Rust. Should we? If so, should we expose a helper for
> `bitmap_size` or duplicate the nbits-to-aligned-nlongs logic?
>
> [1] https://lore.kernel.org/all/20250224233938.3158-1-yury.norov@gmail.com/
> [2] https://lore.kernel.org/all/cover.1740726226.git.viresh.kumar@linaro.org/
> [v1]: https://lore.kernel.org/all/20250227101720.1811578-1-bqe@google.com/
>
> Thanks,
> Burak
>
> ---
>  MAINTAINERS                     |   8 ++
>  rust/bindings/bindings_helper.h |   1 +
>  rust/helpers/bitmap.c           |   8 ++
>  rust/helpers/bitops.c           |  13 +++
>  rust/helpers/helpers.c          |   2 +
>  rust/kernel/bitmap.rs           | 191 ++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs              |   1 +
>  7 files changed, 224 insertions(+)
>  create mode 100644 rust/helpers/bitmap.c
>  create mode 100644 rust/helpers/bitops.c
>  create mode 100644 rust/kernel/bitmap.rs
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6d6e55d8593b..8f42fb1f24c6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4032,12 +4032,15 @@ F:      tools/lib/find_bit.c
>  BITMAP API BINDINGS [RUST]
>  M:     Yury Norov <yury.norov@gmail.com>
>  S:     Maintained
> +F:     rust/helpers/bitmap.c
>  F:     rust/helpers/cpumask.c
>
>  BITMAP API [RUST]
>  M:     Viresh Kumar <viresh.kumar@linaro.org> (cpumask)
> +M:     Alice Ryhl <aliceryhl@google.com> (bitmap)
>  R:     Yury Norov <yury.norov@gmail.com>
>  S:     Maintained
> +F:     rust/kernel/bitmap.rs
>  F:     rust/kernel/cpumask.rs
>
>  BITOPS API
> @@ -4054,6 +4057,11 @@ F:       include/linux/bitops.h
>  F:     lib/test_bitops.c
>  F:     tools/*/bitops*
>
> +BITOPS API BINDINGS [RUST]
> +M:     Yury Norov <yury.norov@gmail.com>
> +S:     Maintained
> +F:     rust/helpers/bitops.c
> +
>  BLINKM RGB LED DRIVER
>  M:     Jan-Simon Moeller <jansimon.moeller@gmx.de>
>  S:     Maintained
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 673b1daa9a58..50416c1a3de9 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -7,6 +7,7 @@
>   */
>
>  #include <kunit/test.h>
> +#include <linux/bitmap.h>
>  #include <linux/blk-mq.h>
>  #include <linux/blk_types.h>
>  #include <linux/blkdev.h>
> diff --git a/rust/helpers/bitmap.c b/rust/helpers/bitmap.c
> new file mode 100644
> index 000000000000..4fa4e4f76110
> --- /dev/null
> +++ b/rust/helpers/bitmap.c
> @@ -0,0 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bitmap.h>
> +
> +void rust_helper_bitmap_copy(unsigned long *dst, const unsigned long *src, unsigned int nbits)
> +{
> +       bitmap_copy(dst, src, nbits);
> +}

I was about to say that this could just be a memcpy, but ...

> +    /// Copy up to `nbits` bits from this bitmap into another.
> +    ///
> +    /// # Panics
> +    ///
> +    /// Panics if `nbits` is too large for this bitmap or destination.
> +    #[inline]
> +    pub fn bitmap_copy(&self, dst: &mut Bitmap, nbits: usize) {
> +        if self.nbits < nbits {
> +            panic_not_in_bounds_le("nbits", self.nbits, nbits);
> +        }
> +        if dst.nbits < nbits {
> +            panic_not_in_bounds_le("nbits", dst.nbits, nbits);
> +        }
> +        // SAFETY: nbits == 0 is supported and access to `self` and `dst` is within bounds.
> +        unsafe { bindings::bitmap_copy(dst.as_mut_ptr(), self.ptr.as_ptr(), nbits as u32) };
> +    }

... then I realized that we're probably not using it correctly. I
would expect this to modify the first `nbits` bits in `dst`, leaving
any remaining bits unmodified. However, if nbits is not divisible by
BITS_PER_LONG it might modify some bits it shouldn't.

That said, Binder needs this only in the case where the sizes are
equal. Perhaps we could rename this to `copy_from_bitmap` with this
signature:
fn copy_from_bitmap(&mut self, src: Bitmap)

and have the function require that the sizes are equal. This mirrors
the standard library function copy_from_slice that takes two slices
that must have the exact same size and copies the contents.

Alice