* [PATCH] rust: add bindings and API for bitmap.h and bitops.h.
@ 2025-02-27 10:08 Burak Emir
2025-02-27 10:51 ` Viresh Kumar
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Burak Emir @ 2025-02-27 10:08 UTC (permalink / raw)
To: Yury Norov
Cc: Burak Emir, Rasmus Villemoes, Viresh Kumar, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
rust-for-linux, linux-kernel
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
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);
+}
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);
+}
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index de2341cfd917..86ebbc97a57d 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -7,6 +7,8 @@
* Sorted alphabetically.
*/
+#include "bitmap.c"
+#include "bitops.c"
#include "blk.c"
#include "bug.c"
#include "build_assert.c"
@@ -15,6 +17,7 @@
#include "cred.c"
#include "device.c"
#include "err.c"
+#include "find.c"
#include "fs.c"
#include "io.c"
#include "jump_label.c"
diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
new file mode 100644
index 000000000000..25fe07a7073a
--- /dev/null
+++ b/rust/kernel/bitmap.rs
@@ -0,0 +1,182 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2025 Google LLC.
+
+//! Rust API for bitmap.
+//!
+//! C headers: [`include/linux/bitmap.h`](srctree/include/linux/bitmap.h).
+
+use crate::alloc::{AllocError, Flags};
+use crate::bindings::{
+ bitmap_copy, bitmap_free, bitmap_zalloc, clear_bit, find_last_bit, find_next_zero_bit, set_bit,
+};
+use core::ptr::NonNull;
+
+/// Wraps underlying C bitmap structure.
+///
+/// # Invariants
+///
+/// * `ptr` is obtained from a successful call to bitmap_zalloc and
+/// holds the address of a zero-initialized array of unsigned long
+/// that is large enough to hold `nbits` bits.
+pub struct Bitmap {
+ /// Pointer to an array of unsigned long.
+ ptr: NonNull<usize>,
+ /// How many bits this bitmap stores. Must be < 2^32.
+ nbits: usize,
+}
+
+impl Drop for Bitmap {
+ fn drop(&mut self) {
+ // SAFETY: `self.ptr` was returned by bitmap_zalloc.
+ unsafe { bitmap_free(self.as_mut_ptr()) };
+ }
+}
+
+#[cold]
+fn not_in_bounds_lt(arg: &'static str, len: usize, val: usize) -> ! {
+ panic!("{arg} must be less than length {len}, was {val}");
+}
+
+#[cold]
+fn not_in_bounds_le(arg: &'static str, len: usize, val: usize) -> ! {
+ panic!("{arg} must be less than or equal to length {len}, was {val}");
+}
+
+impl Bitmap {
+ /// Constructs a new Bitmap.
+ ///
+ /// Fails with AllocError if `nbits` is greater than or equal to 2^32,
+ /// or when the bitmap could not be allocated.
+ #[inline]
+ pub fn new(nbits: usize, flags: Flags) -> Result<Self, AllocError> {
+ if let Ok(nbits_u32) = u32::try_from(nbits) {
+ // SAFETY: nbits == 0 is permitted and nbits fits in u32.
+ let ptr = unsafe { bitmap_zalloc(nbits_u32, flags.as_raw()) };
+ // Zero-size allocation is ok and yields a dangling pointer.
+ let ptr = NonNull::new(ptr).ok_or(AllocError)?;
+ Ok(Bitmap { ptr, nbits })
+ } else {
+ Err(AllocError)
+ }
+ }
+
+ /// Returns how many bits this bitmap holds.
+ #[inline]
+ pub fn len(&self) -> usize {
+ self.nbits
+ }
+
+ /// Returns true if this bitmap has length 0.
+ #[inline]
+ pub fn is_empty(&self) -> bool {
+ self.nbits == 0
+ }
+
+ /// Returns a mutable raw pointer to the backing bitmap.
+ #[inline]
+ pub fn as_mut_ptr(&mut self) -> *mut usize {
+ self.ptr.as_ptr()
+ }
+
+ /// Returns a raw pointer to the backing bitmap.
+ #[inline]
+ pub fn as_ptr(&self) -> *const usize {
+ self.ptr.as_ptr()
+ }
+
+ /// Sets bit with number `nr`.
+ ///
+ /// # Panics
+ ///
+ /// Panics if `nr` is greater than or equal to `self.nbits`.
+ #[inline]
+ pub fn set_bit(&mut self, nr: usize) {
+ if self.nbits <= nr {
+ not_in_bounds_lt("nr", self.nbits, nr)
+ }
+ // SAFETY: Bit nr is within bounds.
+ unsafe { set_bit(nr as u32, self.as_mut_ptr()) };
+ }
+
+ /// Clears bit with number `nr`.
+ ///
+ /// # Panics
+ ///
+ /// Panics if `nr` is greater than or equal to `self.nbits`.
+ #[inline]
+ pub fn clear_bit(&mut self, nr: usize) {
+ if self.nbits <= nr {
+ not_in_bounds_lt("nr", self.nbits, nr);
+ }
+ // SAFETY: Bit nr is within bounds.
+ unsafe { clear_bit(nr as u32, self.as_mut_ptr()) };
+ }
+
+ /// 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 {
+ not_in_bounds_le("nbits", self.nbits, nbits);
+ }
+ if dst.nbits < nbits {
+ not_in_bounds_le("nbits", dst.nbits, nbits);
+ }
+ // SAFETY: nbits == 0 is supported and access to `self` and `dst` is within bounds.
+ unsafe { bitmap_copy(dst.as_mut_ptr(), self.ptr.as_ptr(), nbits as u32) };
+ }
+
+ /// Finds the last bit.
+ #[inline]
+ pub fn find_last_bit(&self) -> usize {
+ // SAFETY: nbits == 0 is supported and access is within bounds.
+ unsafe { find_last_bit(self.as_ptr(), self.nbits) }
+ }
+
+ /// Finds the last bit, searching up to `nbits` bits.
+ ///
+ /// # Panics
+ ///
+ /// Panics if `nbits` is too large for this bitmap.
+ #[inline]
+ pub fn find_last_bit_upto(&self, nbits: usize) -> usize {
+ if self.nbits < nbits {
+ not_in_bounds_le("nbits", self.nbits, nbits);
+ }
+ // SAFETY: nbits == 0 is supported and access is within bounds.
+ unsafe { find_last_bit(self.as_ptr(), nbits) }
+ }
+
+ /// Finds the next zero bit, searching up to `nbits`.
+ ///
+ /// # Panics
+ ///
+ /// Panics if `nbits` is too large for this bitmap.
+ #[inline]
+ pub fn find_next_zero_bit_upto(&self, nbits: usize) -> usize {
+ if self.nbits < nbits {
+ not_in_bounds_le("nbits", self.nbits, nbits);
+ }
+ // SAFETY: nbits == 0 is supported and access is within bounds.
+ unsafe {
+ find_next_zero_bit(self.as_ptr(), nbits, 0 /* offset */)
+ }
+ }
+
+ /// Finds the next zero bit, searching up to `nbits` bits, with offset `offset`.
+ ///
+ /// # Panics
+ ///
+ /// Panics if `nbits` is too large for this bitmap.
+ pub fn find_next_zero_bit_upto_offset(&self, nbits: usize, offset: usize) -> usize {
+ if self.nbits < nbits {
+ not_in_bounds_le("nbits", self.nbits, nbits);
+ }
+ // SAFETY: nbits == 0 and out-of-bounds offset is supported, and access is within bounds.
+ unsafe { find_next_zero_bit(self.as_ptr(), nbits, offset) }
+ }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index efbd7be98dab..be06ffc47473 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -36,6 +36,7 @@
pub use ffi;
pub mod alloc;
+pub mod bitmap;
#[cfg(CONFIG_BLOCK)]
pub mod block;
#[doc(hidden)]
--
2.48.1.658.g4767266eb4-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] rust: add bindings and API for bitmap.h and bitops.h.
2025-02-27 10:08 [PATCH] rust: add bindings and API for bitmap.h and bitops.h Burak Emir
@ 2025-02-27 10:51 ` Viresh Kumar
2025-02-28 9:12 ` Alice Ryhl
2025-02-27 17:35 ` Miguel Ojeda
2025-02-27 18:30 ` Yury Norov
2 siblings, 1 reply; 6+ messages in thread
From: Viresh Kumar @ 2025-02-27 10:51 UTC (permalink / raw)
To: Burak Emir
Cc: Yury Norov, Rasmus Villemoes, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, rust-for-linux,
linux-kernel
On 27-02-25, 10:08, Burak Emir wrote:
> 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.
> diff --git a/MAINTAINERS b/MAINTAINERS
> BITMAP API [RUST]
I would suggest adding this too.
+ M: Burak Emir <bqe@google.com> (bitmap)
> 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
--
viresh
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rust: add bindings and API for bitmap.h and bitops.h.
2025-02-27 10:08 [PATCH] rust: add bindings and API for bitmap.h and bitops.h Burak Emir
2025-02-27 10:51 ` Viresh Kumar
@ 2025-02-27 17:35 ` Miguel Ojeda
2025-02-27 18:30 ` Yury Norov
2 siblings, 0 replies; 6+ messages in thread
From: Miguel Ojeda @ 2025-02-27 17:35 UTC (permalink / raw)
To: Burak Emir
Cc: Yury Norov, Rasmus Villemoes, Viresh Kumar, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
rust-for-linux, linux-kernel
On Thu, Feb 27, 2025 at 11:17 AM Burak Emir <bqe@google.com> wrote:
>
> +use crate::bindings::{
> + bitmap_copy, bitmap_free, bitmap_zalloc, clear_bit, find_last_bit, find_next_zero_bit, set_bit,
> +};
I think it may be best to avoid importing `bindings::*` things, so
that it is clear when they happen and so that they are easier to grep
for.
I guess some of these may benefit from being inline when small
constants are involved, similar to what the C docs mention -- do we
want to have equivalents to the C "generic" ones that could be inlined
well?
> +/// * `ptr` is obtained from a successful call to bitmap_zalloc and
Please use Markdown wherever possible, e.g. `bitmap_zalloc`.
> +#[cold]
> +fn not_in_bounds_lt(arg: &'static str, len: usize, val: usize) -> ! {
> + panic!("{arg} must be less than length {len}, was {val}");
> +}
Should these be more explicit? e.g. `panic_if_not_in_bounds_lt`?
Do we want to only have infallible versions?
> +impl Bitmap {
> + /// Constructs a new Bitmap.
Please use intra-doc links where possible (and Markdown), e.g. [`Bitmap`].
> + /// Fails with AllocError if `nbits` is greater than or equal to 2^32,
> + /// or when the bitmap could not be allocated.
Some examples would be nice to add, which double as KUnit tests.
Thanks!
Cheers,
Miguel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rust: add bindings and API for bitmap.h and bitops.h.
2025-02-27 10:08 [PATCH] rust: add bindings and API for bitmap.h and bitops.h Burak Emir
2025-02-27 10:51 ` Viresh Kumar
2025-02-27 17:35 ` Miguel Ojeda
@ 2025-02-27 18:30 ` Yury Norov
2025-02-28 9:16 ` Alice Ryhl
2 siblings, 1 reply; 6+ messages in thread
From: Yury Norov @ 2025-02-27 18:30 UTC (permalink / raw)
To: Burak Emir
Cc: Rasmus Villemoes, Viresh Kumar, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, rust-for-linux,
linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rust: add bindings and API for bitmap.h and bitops.h.
2025-02-27 10:51 ` Viresh Kumar
@ 2025-02-28 9:12 ` Alice Ryhl
0 siblings, 0 replies; 6+ messages in thread
From: Alice Ryhl @ 2025-02-28 9:12 UTC (permalink / raw)
To: Viresh Kumar
Cc: Burak Emir, Yury Norov, Rasmus Villemoes, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross, rust-for-linux,
linux-kernel
On Thu, Feb 27, 2025 at 11:51 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 27-02-25, 10:08, Burak Emir wrote:
> > 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.
>
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > BITMAP API [RUST]
>
> I would suggest adding this too.
>
> + M: Burak Emir <bqe@google.com> (bitmap)
If a new maintainer is needed, it should probably be me. Burak is
helping me out with some Binder changes, but I will be the owner of
the Binder code.
Alice
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rust: add bindings and API for bitmap.h and bitops.h.
2025-02-27 18:30 ` Yury Norov
@ 2025-02-28 9:16 ` Alice Ryhl
0 siblings, 0 replies; 6+ messages in thread
From: Alice Ryhl @ 2025-02-28 9:16 UTC (permalink / raw)
To: Yury Norov
Cc: Burak Emir, Rasmus Villemoes, Viresh Kumar, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross, rust-for-linux,
linux-kernel
On Thu, Feb 27, 2025 at 7:30 PM Yury Norov <yury.norov@gmail.com> wrote:
>
> 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?
Huh, I did not realize the method was atomic. I had assumed it wasn't
from how Binder is using it.
> > 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?
That sounds likely.
Alice
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-02-28 9:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-27 10:08 [PATCH] rust: add bindings and API for bitmap.h and bitops.h Burak Emir
2025-02-27 10:51 ` Viresh Kumar
2025-02-28 9:12 ` Alice Ryhl
2025-02-27 17:35 ` Miguel Ojeda
2025-02-27 18:30 ` Yury Norov
2025-02-28 9:16 ` Alice Ryhl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).