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

[Yury Norov <yury.norov@gmail.com>]

On Thu, Feb 27, 2025 at 10:08:41AM +0000, Burak Emir wrote:
> Adds a bitmap and bitops bindings and bitmap Rust API.
> 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 bitmaps
> 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.
> 
> 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.
> 
> Adds F: entries to MAINTAINERS.
> 
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Burak Emir <bqe@google.com>
> ---
> This series adds a Rust abstraction for bitmap, and binding helpers
> for inline methods of bitmap.h bitops.h.
> 
> It depends on [1] and [2] which add bitmap helpers, MAINTAINERS entries
> and an abstraction that is part of the bitmaps Rust API.
> 
> Question for Yury: What would you like us to do for the MAINTAINERS
> file? For now I just added the new files as F: entries.
> 
> [1] https://lore.kernel.org/all/20250224233938.3158-1-yury.norov@gmail.com/
> [2] https://lore.kernel.org/all/cover.1740475625.git.viresh.kumar@linaro.org/
> 
>  MAINTAINERS                     |   4 +
>  rust/bindings/bindings_helper.h |   1 +
>  rust/helpers/bitmap.c           |   8 ++
>  rust/helpers/bitops.c           |  13 +++
>  rust/helpers/find.c             |  15 +++
>  rust/helpers/helpers.c          |   3 +
>  rust/kernel/bitmap.rs           | 182 ++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs              |   1 +
>  8 files changed, 227 insertions(+)
>  create mode 100644 rust/helpers/bitmap.c
>  create mode 100644 rust/helpers/bitops.c
>  create mode 100644 rust/helpers/find.c
>  create mode 100644 rust/kernel/bitmap.rs
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6d6e55d8593b..359f09e8e2c0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4033,12 +4033,16 @@ BITMAP API BINDINGS [RUST]
>  M:	Yury Norov <yury.norov@gmail.com>
>  S:	Maintained
>  F:	rust/helpers/cpumask.c
> +F:	rust/helpers/find.c
> +F:	rust/helpers/bitmap.c
> +F:	rust/helpers/bitops.c

bitops.c is part of BITOPS API, not the BITMAP one. I think we need a
new record for it?

>  
>  BITMAP API [RUST]
>  M:	Viresh Kumar <viresh.kumar@linaro.org> (cpumask)
>  R:	Yury Norov <yury.norov@gmail.com>
>  S:	Maintained
>  F:	rust/kernel/cpumask.rs
> +F:	rust/kernel/bitmap.rs
>  
>  BITOPS API
>  M:	Yury Norov <yury.norov@gmail.com>
> 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);
> +}
> diff --git a/rust/helpers/bitops.c b/rust/helpers/bitops.c
> new file mode 100644
> index 000000000000..191ef0341fd5
> --- /dev/null
> +++ b/rust/helpers/bitops.c
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bitops.h>
> +
> +void rust_helper_set_bit(unsigned int nr, volatile unsigned long *addr)
> +{
> +	set_bit(nr, addr);
> +}
> +
> +void rust_helper_clear_bit(unsigned int nr, volatile unsigned long *addr)
> +{
> +	clear_bit(nr, addr);
> +}

So you mention that you're rewriting dbitmap, and I took a brief look
at drivers/android/dbitmap.h.

What I can say is that at least dbitmap_acquire_next_zero_bit() abuses 
the set_bit() API. It should use non-atomic __set_bit(). If you're
going to re-write it, can you review the existing code and make sure
you're using the right API in a right way?

> diff --git a/rust/helpers/find.c b/rust/helpers/find.c
> new file mode 100644
> index 000000000000..3841d3f0330f
> --- /dev/null
> +++ b/rust/helpers/find.c
> @@ -0,0 +1,15 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/find.h>
> +
> +unsigned long rust_helper_find_last_bit(const unsigned long *addr, unsigned long size)
> +{
> +	return find_last_bit(addr, size);
> +}
> +
> +
> +unsigned long rust_helper_find_next_zero_bit(const unsigned long *addr, unsigned long size,
> +				 unsigned long offset)
> +{
> +	return find_next_zero_bit(addr, size, offset);
> +}

For those two, the find_*_bit() are the wrappers around _find_*_bit()
in lib/find_bit.c, with a few exceptions. The reason for having the
wrappers is an optimization that improves code generation for small
bitmaps.

In your case, when you wrap a macro, the optimization becomes impossible,
and the macro is unneeded.

From technical perspective, the underscored _find_*_bit() functions
should be accessible by rust directly. Can you confirm that? If so,
maybe just use them and avoid bloating?

Thanks,
Yury