rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/6] Use Rust Bitmap from Rust Binder driver
@ 2025-11-10 13:05 Alice Ryhl
  2025-11-10 13:05 ` [PATCH v4 1/6] rust: bitmap: add MAX_LEN and NO_ALLOC_MAX_LEN constants Alice Ryhl
                   ` (5 more replies)
  0 siblings, 6 replies; 15+ messages in thread
From: Alice Ryhl @ 2025-11-10 13:05 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Yury Norov
  Cc: Arve Hjønnevåg, Todd Kjos, Martijn Coenen,
	Joel Fernandes, Christian Brauner, Carlos Llamas,
	Suren Baghdasaryan, Burak Emir, Miguel Ojeda, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Trevor Gross, Danilo Krummrich, rust-for-linux, linux-kernel,
	Alice Ryhl

See commit message for rust binder commit for details.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Changes in v4:
- Keep existing IdPool constructor (renamed to with_capacity).
- Undo docs changes that were due to the removal of the existing IdPool
  constructor.
- List id < pool.map.len() as invariant for UnusedId and mention it in
  as_u32() comment.
- Fix [`acquire`] docs links.
- Link to v3: https://lore.kernel.org/r/20251028-binder-bitmap-v3-0-32822d4b3207@google.com

Changes in v3:
- Also update documentation to fix compile errors.
- Use new_inline() instead of new_small().
- Reword commit message of "rust: id_pool: do not supply starting capacity"
- Change UnusedId::acquire() to return usize.
- Link to v2: https://lore.kernel.org/r/20251021-binder-bitmap-v2-0-e652d172c62b@google.com

Changes in v2:
- Use id_pool.
- Link to v1: https://lore.kernel.org/r/20251020-binder-bitmap-v1-0-879bec9cddc1@google.com

---
Alice Ryhl (6):
      rust: bitmap: add MAX_LEN and NO_ALLOC_MAX_LEN constants
      rust: bitmap: add BitmapVec::new_inline()
      rust: bitmap: rename IdPool::new to with_capacity
      rust: id_pool: do not supply starting capacity
      rust: id_pool: do not immediately acquire new ids
      rust_binder: use bitmap for allocation of handles

 drivers/android/binder/process.rs |  63 ++++++++++++++++------
 rust/kernel/bitmap.rs             |  26 ++++++++--
 rust/kernel/id_pool.rs            | 106 ++++++++++++++++++++++++++++++--------
 3 files changed, 152 insertions(+), 43 deletions(-)
---
base-commit: 211ddde0823f1442e4ad052a2f30f050145ccada
change-id: 20251007-binder-bitmap-5aa0d966fb1f

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v4 1/6] rust: bitmap: add MAX_LEN and NO_ALLOC_MAX_LEN constants
  2025-11-10 13:05 [PATCH v4 0/6] Use Rust Bitmap from Rust Binder driver Alice Ryhl
@ 2025-11-10 13:05 ` Alice Ryhl
  2025-11-10 13:59   ` Tamir Duberstein
  2025-11-10 13:05 ` [PATCH v4 2/6] rust: bitmap: add BitmapVec::new_inline() Alice Ryhl
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Alice Ryhl @ 2025-11-10 13:05 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Yury Norov
  Cc: Arve Hjønnevåg, Todd Kjos, Martijn Coenen,
	Joel Fernandes, Christian Brauner, Carlos Llamas,
	Suren Baghdasaryan, Burak Emir, Miguel Ojeda, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Trevor Gross, Danilo Krummrich, rust-for-linux, linux-kernel,
	Alice Ryhl

To avoid hard-coding these values in drivers, define constants for them
that drivers can reference.

Acked-by: Danilo Krummrich <dakr@kernel.org>
Reviewed-by: Burak Emir <bqe@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/bitmap.rs | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
index aa8fc7bf06fc99865ae755d8694e4bec3dc8e7f0..15fa23b45054b9272415fcc000e3e3b52c74d7c1 100644
--- a/rust/kernel/bitmap.rs
+++ b/rust/kernel/bitmap.rs
@@ -149,14 +149,14 @@ macro_rules! bitmap_assert_return {
 ///
 /// # Invariants
 ///
-/// * `nbits` is `<= i32::MAX` and never changes.
+/// * `nbits` is `<= MAX_LEN`.
 /// * if `nbits <= bindings::BITS_PER_LONG`, then `repr` is a `usize`.
 /// * otherwise, `repr` holds a non-null pointer to an initialized
 ///   array of `unsigned long` that is large enough to hold `nbits` bits.
 pub struct BitmapVec {
     /// Representation of bitmap.
     repr: BitmapRepr,
-    /// Length of this bitmap. Must be `<= i32::MAX`.
+    /// Length of this bitmap. Must be `<= MAX_LEN`.
     nbits: usize,
 }
 
@@ -226,10 +226,16 @@ fn drop(&mut self) {
 }
 
 impl BitmapVec {
+    /// The maximum possible length of a `BitmapVec`.
+    pub const MAX_LEN: usize = i32::MAX as usize;
+
+    /// The maximum length that avoids allocating.
+    pub const NO_ALLOC_MAX_LEN: usize = BITS_PER_LONG;
+
     /// Constructs a new [`BitmapVec`].
     ///
     /// Fails with [`AllocError`] when the [`BitmapVec`] could not be allocated. This
-    /// includes the case when `nbits` is greater than `i32::MAX`.
+    /// includes the case when `nbits` is greater than `MAX_LEN`.
     #[inline]
     pub fn new(nbits: usize, flags: Flags) -> Result<Self, AllocError> {
         if nbits <= BITS_PER_LONG {
@@ -238,11 +244,11 @@ pub fn new(nbits: usize, flags: Flags) -> Result<Self, AllocError> {
                 nbits,
             });
         }
-        if nbits > i32::MAX.try_into().unwrap() {
+        if nbits > Self::MAX_LEN {
             return Err(AllocError);
         }
         let nbits_u32 = u32::try_from(nbits).unwrap();
-        // SAFETY: `BITS_PER_LONG < nbits` and `nbits <= i32::MAX`.
+        // SAFETY: `BITS_PER_LONG < nbits` and `nbits <= MAX_LEN`.
         let ptr = unsafe { bindings::bitmap_zalloc(nbits_u32, flags.as_raw()) };
         let ptr = NonNull::new(ptr).ok_or(AllocError)?;
         // INVARIANT: `ptr` returned by C `bitmap_zalloc` and `nbits` checked.

-- 
2.51.2.1041.gc1ab5b90ca-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 2/6] rust: bitmap: add BitmapVec::new_inline()
  2025-11-10 13:05 [PATCH v4 0/6] Use Rust Bitmap from Rust Binder driver Alice Ryhl
  2025-11-10 13:05 ` [PATCH v4 1/6] rust: bitmap: add MAX_LEN and NO_ALLOC_MAX_LEN constants Alice Ryhl
@ 2025-11-10 13:05 ` Alice Ryhl
  2025-11-10 13:05 ` [PATCH v4 3/6] rust: bitmap: rename IdPool::new to with_capacity Alice Ryhl
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 15+ messages in thread
From: Alice Ryhl @ 2025-11-10 13:05 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Yury Norov
  Cc: Arve Hjønnevåg, Todd Kjos, Martijn Coenen,
	Joel Fernandes, Christian Brauner, Carlos Llamas,
	Suren Baghdasaryan, Burak Emir, Miguel Ojeda, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Trevor Gross, Danilo Krummrich, rust-for-linux, linux-kernel,
	Alice Ryhl

This constructor is useful when you just want to create a BitmapVec
without allocating but don't care how large it is.

Acked-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
Reviewed-by: Burak Emir <bqe@google.com>
Reviewed-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/bitmap.rs | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
index 15fa23b45054b9272415fcc000e3e3b52c74d7c1..f385a539a90ba87a7b9f6d686e150e95e358bbef 100644
--- a/rust/kernel/bitmap.rs
+++ b/rust/kernel/bitmap.rs
@@ -232,6 +232,16 @@ impl BitmapVec {
     /// The maximum length that avoids allocating.
     pub const NO_ALLOC_MAX_LEN: usize = BITS_PER_LONG;
 
+    /// Construct a longest possible inline [`BitmapVec`].
+    #[inline]
+    pub fn new_inline() -> Self {
+        // INVARIANT: `nbits <= NO_ALLOC_MAX_LEN`, so an inline bitmap is the right repr.
+        BitmapVec {
+            repr: BitmapRepr { bitmap: 0 },
+            nbits: BitmapVec::NO_ALLOC_MAX_LEN,
+        }
+    }
+
     /// Constructs a new [`BitmapVec`].
     ///
     /// Fails with [`AllocError`] when the [`BitmapVec`] could not be allocated. This

-- 
2.51.2.1041.gc1ab5b90ca-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 3/6] rust: bitmap: rename IdPool::new to with_capacity
  2025-11-10 13:05 [PATCH v4 0/6] Use Rust Bitmap from Rust Binder driver Alice Ryhl
  2025-11-10 13:05 ` [PATCH v4 1/6] rust: bitmap: add MAX_LEN and NO_ALLOC_MAX_LEN constants Alice Ryhl
  2025-11-10 13:05 ` [PATCH v4 2/6] rust: bitmap: add BitmapVec::new_inline() Alice Ryhl
@ 2025-11-10 13:05 ` Alice Ryhl
  2025-11-10 14:12   ` Burak Emir
  2025-11-10 13:05 ` [PATCH v4 4/6] rust: id_pool: do not supply starting capacity Alice Ryhl
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Alice Ryhl @ 2025-11-10 13:05 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Yury Norov
  Cc: Arve Hjønnevåg, Todd Kjos, Martijn Coenen,
	Joel Fernandes, Christian Brauner, Carlos Llamas,
	Suren Baghdasaryan, Burak Emir, Miguel Ojeda, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Trevor Gross, Danilo Krummrich, rust-for-linux, linux-kernel,
	Alice Ryhl

We want to change ::new() to take no parameters and produce a pool that
is as large as possible while also being inline because that is the
constructor that Rust Binder actually needs.

However, to avoid complications in examples, we still need the current
constructor. So rename it to with_capacity(), which is the idiomatic
Rust name for this kind constructor.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/id_pool.rs | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/rust/kernel/id_pool.rs b/rust/kernel/id_pool.rs
index a41a3404213ca92d53b14c80101afff6ac8c416e..5942f678db015e902fa482eb64c38512a468e449 100644
--- a/rust/kernel/id_pool.rs
+++ b/rust/kernel/id_pool.rs
@@ -28,7 +28,7 @@
 /// use kernel::alloc::{AllocError, flags::GFP_KERNEL};
 /// use kernel::id_pool::IdPool;
 ///
-/// let mut pool = IdPool::new(64, GFP_KERNEL)?;
+/// let mut pool = IdPool::with_capacity(64, GFP_KERNEL)?;
 /// for i in 0..64 {
 ///     assert_eq!(i, pool.acquire_next_id(i).ok_or(ENOSPC)?);
 /// }
@@ -95,14 +95,14 @@ pub fn realloc(&self, flags: Flags) -> Result<PoolResizer, AllocError> {
 }
 
 impl IdPool {
-    /// Constructs a new [`IdPool`].
+    /// Constructs a new [`IdPool`] with space for a specific number of bits.
     ///
-    /// A capacity below [`BITS_PER_LONG`] is adjusted to
-    /// [`BITS_PER_LONG`].
+    /// A capacity below [`NO_ALLOC_MAX_LEN`] is adjusted to
+    /// [`NO_ALLOC_MAX_LEN`].
     ///
-    /// [`BITS_PER_LONG`]: srctree/include/asm-generic/bitsperlong.h
+    /// [`NO_ALLOC_MAX_LEN`]: BitmapVec::NO_ALLOC_MAX_LEN
     #[inline]
-    pub fn new(num_ids: usize, flags: Flags) -> Result<Self, AllocError> {
+    pub fn with_capacity(num_ids: usize, flags: Flags) -> Result<Self, AllocError> {
         let num_ids = core::cmp::max(num_ids, BITS_PER_LONG);
         let map = BitmapVec::new(num_ids, flags)?;
         Ok(Self { map })
@@ -126,7 +126,7 @@ pub fn capacity(&self) -> usize {
     /// use kernel::alloc::{AllocError, flags::GFP_KERNEL};
     /// use kernel::id_pool::{ReallocRequest, IdPool};
     ///
-    /// let mut pool = IdPool::new(1024, GFP_KERNEL)?;
+    /// let mut pool = IdPool::with_capacity(1024, GFP_KERNEL)?;
     /// let alloc_request = pool.shrink_request().ok_or(AllocError)?;
     /// let resizer = alloc_request.realloc(GFP_KERNEL)?;
     /// pool.shrink(resizer);

-- 
2.51.2.1041.gc1ab5b90ca-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 4/6] rust: id_pool: do not supply starting capacity
  2025-11-10 13:05 [PATCH v4 0/6] Use Rust Bitmap from Rust Binder driver Alice Ryhl
                   ` (2 preceding siblings ...)
  2025-11-10 13:05 ` [PATCH v4 3/6] rust: bitmap: rename IdPool::new to with_capacity Alice Ryhl
@ 2025-11-10 13:05 ` Alice Ryhl
  2025-11-10 13:05 ` [PATCH v4 5/6] rust: id_pool: do not immediately acquire new ids Alice Ryhl
  2025-11-10 13:05 ` [PATCH v4 6/6] rust_binder: use bitmap for allocation of handles Alice Ryhl
  5 siblings, 0 replies; 15+ messages in thread
From: Alice Ryhl @ 2025-11-10 13:05 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Yury Norov
  Cc: Arve Hjønnevåg, Todd Kjos, Martijn Coenen,
	Joel Fernandes, Christian Brauner, Carlos Llamas,
	Suren Baghdasaryan, Burak Emir, Miguel Ojeda, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Trevor Gross, Danilo Krummrich, rust-for-linux, linux-kernel,
	Alice Ryhl

Rust Binder wants to use inline bitmaps whenever possible to avoid
allocations, so introduce a constructor for an IdPool with arbitrary
capacity that stores the bitmap inline.

The existing constructor could be renamed to with_capacity() to match
constructors for other similar types, but it is removed as there is
currently no user for it.

Acked-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
Reviewed-by: Burak Emir <bqe@google.com>
Reviewed-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/id_pool.rs | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/rust/kernel/id_pool.rs b/rust/kernel/id_pool.rs
index 5942f678db015e902fa482eb64c38512a468e449..6f607f04331c9bd3fc79c6d91d04005bd0685839 100644
--- a/rust/kernel/id_pool.rs
+++ b/rust/kernel/id_pool.rs
@@ -95,6 +95,18 @@ pub fn realloc(&self, flags: Flags) -> Result<PoolResizer, AllocError> {
 }
 
 impl IdPool {
+    /// Constructs a new [`IdPool`].
+    ///
+    /// The pool will have a capacity of [`NO_ALLOC_MAX_LEN`].
+    ///
+    /// [`NO_ALLOC_MAX_LEN`]: BitmapVec::NO_ALLOC_MAX_LEN
+    #[inline]
+    pub fn new() -> Self {
+        Self {
+            map: BitmapVec::new_inline(),
+        }
+    }
+
     /// Constructs a new [`IdPool`] with space for a specific number of bits.
     ///
     /// A capacity below [`NO_ALLOC_MAX_LEN`] is adjusted to
@@ -224,3 +236,10 @@ pub fn release_id(&mut self, id: usize) {
         self.map.clear_bit(id);
     }
 }
+
+impl Default for IdPool {
+    #[inline]
+    fn default() -> Self {
+        Self::new()
+    }
+}

-- 
2.51.2.1041.gc1ab5b90ca-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 5/6] rust: id_pool: do not immediately acquire new ids
  2025-11-10 13:05 [PATCH v4 0/6] Use Rust Bitmap from Rust Binder driver Alice Ryhl
                   ` (3 preceding siblings ...)
  2025-11-10 13:05 ` [PATCH v4 4/6] rust: id_pool: do not supply starting capacity Alice Ryhl
@ 2025-11-10 13:05 ` Alice Ryhl
  2025-11-10 13:05 ` [PATCH v4 6/6] rust_binder: use bitmap for allocation of handles Alice Ryhl
  5 siblings, 0 replies; 15+ messages in thread
From: Alice Ryhl @ 2025-11-10 13:05 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Yury Norov
  Cc: Arve Hjønnevåg, Todd Kjos, Martijn Coenen,
	Joel Fernandes, Christian Brauner, Carlos Llamas,
	Suren Baghdasaryan, Burak Emir, Miguel Ojeda, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Trevor Gross, Danilo Krummrich, rust-for-linux, linux-kernel,
	Alice Ryhl

When Rust Binder assigns a new ID, it performs various fallible
operations before it "commits" to actually using the new ID. To support
this pattern, change acquire_next_id() so that it does not immediately
call set_bit(), but instead returns an object that may be used to call
set_bit() later.

The UnusedId type holds a exclusive reference to the IdPool, so it's
guaranteed that nobody else can call find_unused_id() while the UnusedId
object is live.

Reviewed-by: Burak Emir <bqe@google.com>
Reviewed-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/id_pool.rs | 75 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 60 insertions(+), 15 deletions(-)

diff --git a/rust/kernel/id_pool.rs b/rust/kernel/id_pool.rs
index 6f607f04331c9bd3fc79c6d91d04005bd0685839..bf1aad1d7fcc54794b79484d1cf8861c328f02fb 100644
--- a/rust/kernel/id_pool.rs
+++ b/rust/kernel/id_pool.rs
@@ -25,8 +25,8 @@
 /// Basic usage
 ///
 /// ```
-/// use kernel::alloc::{AllocError, flags::GFP_KERNEL};
-/// use kernel::id_pool::IdPool;
+/// use kernel::alloc::AllocError;
+/// use kernel::id_pool::{IdPool, UnusedId};
 ///
 /// let mut pool = IdPool::with_capacity(64, GFP_KERNEL)?;
 /// for i in 0..64 {
@@ -34,13 +34,13 @@
 /// }
 ///
 /// pool.release_id(23);
-/// assert_eq!(23, pool.acquire_next_id(0).ok_or(ENOSPC)?);
+/// assert_eq!(23, pool.find_unused_id(0).ok_or(ENOSPC)?.acquire());
 ///
-/// assert_eq!(None, pool.acquire_next_id(0));  // time to realloc.
+/// assert!(pool.find_unused_id(0).is_none());  // time to realloc.
 /// let resizer = pool.grow_request().ok_or(ENOSPC)?.realloc(GFP_KERNEL)?;
 /// pool.grow(resizer);
 ///
-/// assert_eq!(pool.acquire_next_id(0), Some(64));
+/// assert_eq!(pool.find_unused_id(0).ok_or(ENOSPC)?.acquire(), 64);
 /// # Ok::<(), Error>(())
 /// ```
 ///
@@ -54,8 +54,8 @@
 /// fn get_id_maybe_realloc(guarded_pool: &SpinLock<IdPool>) -> Result<usize, AllocError> {
 ///     let mut pool = guarded_pool.lock();
 ///     loop {
-///         match pool.acquire_next_id(0) {
-///             Some(index) => return Ok(index),
+///         match pool.find_unused_id(0) {
+///             Some(index) => return Ok(index.acquire()),
 ///             None => {
 ///                 let alloc_request = pool.grow_request();
 ///                 drop(pool);
@@ -216,18 +216,18 @@ pub fn grow(&mut self, mut resizer: PoolResizer) {
         self.map = resizer.new;
     }
 
-    /// Acquires a new ID by finding and setting the next zero bit in the
-    /// bitmap.
+    /// Finds an unused ID in the bitmap.
     ///
     /// Upon success, returns its index. Otherwise, returns [`None`]
     /// to indicate that a [`Self::grow_request`] is needed.
     #[inline]
-    pub fn acquire_next_id(&mut self, offset: usize) -> Option<usize> {
-        let next_zero_bit = self.map.next_zero_bit(offset);
-        if let Some(nr) = next_zero_bit {
-            self.map.set_bit(nr);
-        }
-        next_zero_bit
+    #[must_use]
+    pub fn find_unused_id(&mut self, offset: usize) -> Option<UnusedId<'_>> {
+        // INVARIANT: `next_zero_bit()` returns None or an integer less than `map.len()`
+        Some(UnusedId {
+            id: self.map.next_zero_bit(offset)?,
+            pool: self,
+        })
     }
 
     /// Releases an ID.
@@ -237,6 +237,51 @@ pub fn release_id(&mut self, id: usize) {
     }
 }
 
+/// Represents an unused id in an [`IdPool`].
+///
+/// # Invariants
+///
+/// The value of `id` is less than `pool.map.len()`.
+pub struct UnusedId<'pool> {
+    id: usize,
+    pool: &'pool mut IdPool,
+}
+
+impl<'pool> UnusedId<'pool> {
+    /// Get the unused id as an usize.
+    ///
+    /// Be aware that the id has not yet been acquired in the pool. The
+    /// [`acquire`] method must be called to prevent others from taking the id.
+    ///
+    /// [`acquire`]: UnusedId::acquire()
+    #[inline]
+    #[must_use]
+    pub fn as_usize(&self) -> usize {
+        self.id
+    }
+
+    /// Get the unused id as an u32.
+    ///
+    /// Be aware that the id has not yet been acquired in the pool. The
+    /// [`acquire`] method must be called to prevent others from taking the id.
+    ///
+    /// [`acquire`]: UnusedId::acquire()
+    #[inline]
+    #[must_use]
+    pub fn as_u32(&self) -> u32 {
+        // CAST: By the type invariants:
+        // `self.id < pool.map.len() <= BitmapVec::MAX_LEN = i32::MAX`.
+        self.id as u32
+    }
+
+    /// Acquire the unused id.
+    #[inline]
+    pub fn acquire(self) -> usize {
+        self.pool.map.set_bit(self.id);
+        self.id
+    }
+}
+
 impl Default for IdPool {
     #[inline]
     fn default() -> Self {

-- 
2.51.2.1041.gc1ab5b90ca-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 6/6] rust_binder: use bitmap for allocation of handles
  2025-11-10 13:05 [PATCH v4 0/6] Use Rust Bitmap from Rust Binder driver Alice Ryhl
                   ` (4 preceding siblings ...)
  2025-11-10 13:05 ` [PATCH v4 5/6] rust: id_pool: do not immediately acquire new ids Alice Ryhl
@ 2025-11-10 13:05 ` Alice Ryhl
  5 siblings, 0 replies; 15+ messages in thread
From: Alice Ryhl @ 2025-11-10 13:05 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Yury Norov
  Cc: Arve Hjønnevåg, Todd Kjos, Martijn Coenen,
	Joel Fernandes, Christian Brauner, Carlos Llamas,
	Suren Baghdasaryan, Burak Emir, Miguel Ojeda, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Trevor Gross, Danilo Krummrich, rust-for-linux, linux-kernel,
	Alice Ryhl

To find an unused Binder handle, Rust Binder currently iterates the
red/black tree from the beginning until it finds a gap in the keys. This
is extremely slow.

To improve the performance, add a bitmap that keeps track of which
indices are actually in use. This allows us to quickly find an unused
key in the red/black tree.

This logic matches the approach used by C Binder. It was chosen
partially because it's the most memory efficient solution.

Reviewed-by: Burak Emir <bqe@google.com>
Acked-by: Carlos Llamas <cmllamas@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 drivers/android/binder/process.rs | 63 ++++++++++++++++++++++++++++-----------
 1 file changed, 46 insertions(+), 17 deletions(-)

diff --git a/drivers/android/binder/process.rs b/drivers/android/binder/process.rs
index f13a747e784c84a0fb09cbf47442712106eba07c..933b0f737b38ffac536b19c9330dfc63ffc72f2b 100644
--- a/drivers/android/binder/process.rs
+++ b/drivers/android/binder/process.rs
@@ -19,6 +19,7 @@
     cred::Credential,
     error::Error,
     fs::file::{self, File},
+    id_pool::IdPool,
     list::{List, ListArc, ListArcField, ListLinks},
     mm,
     prelude::*,
@@ -367,6 +368,8 @@ impl ListItem<{Self::LIST_NODE}> for NodeRefInfo {
 struct ProcessNodeRefs {
     /// Used to look up nodes using the 32-bit id that this process knows it by.
     by_handle: RBTree<u32, ListArc<NodeRefInfo, { NodeRefInfo::LIST_PROC }>>,
+    /// Used to quickly find unused ids in `by_handle`.
+    handle_is_present: IdPool,
     /// Used to look up nodes without knowing their local 32-bit id. The usize is the address of
     /// the underlying `Node` struct as returned by `Node::global_id`.
     by_node: RBTree<usize, u32>,
@@ -381,6 +384,7 @@ impl ProcessNodeRefs {
     fn new() -> Self {
         Self {
             by_handle: RBTree::new(),
+            handle_is_present: IdPool::new(),
             by_node: RBTree::new(),
             freeze_listeners: RBTree::new(),
         }
@@ -775,7 +779,7 @@ pub(crate) fn get_node(
     pub(crate) fn insert_or_update_handle(
         self: ArcBorrow<'_, Process>,
         node_ref: NodeRef,
-        is_mananger: bool,
+        is_manager: bool,
     ) -> Result<u32> {
         {
             let mut refs = self.node_refs.lock();
@@ -794,7 +798,32 @@ pub(crate) fn insert_or_update_handle(
         let reserve2 = RBTreeNodeReservation::new(GFP_KERNEL)?;
         let info = UniqueArc::new_uninit(GFP_KERNEL)?;
 
-        let mut refs = self.node_refs.lock();
+        let mut refs_lock = self.node_refs.lock();
+        let mut refs = &mut *refs_lock;
+
+        let (unused_id, by_handle_slot) = loop {
+            // ID 0 may only be used by the manager.
+            let start = if is_manager { 0 } else { 1 };
+
+            if let Some(res) = refs.handle_is_present.find_unused_id(start) {
+                match refs.by_handle.entry(res.as_u32()) {
+                    rbtree::Entry::Vacant(entry) => break (res, entry),
+                    rbtree::Entry::Occupied(_) => {
+                        pr_err!("Detected mismatch between handle_is_present and by_handle");
+                        res.acquire();
+                        continue;
+                    }
+                }
+            }
+
+            let grow_request = refs.handle_is_present.grow_request().ok_or(ENOMEM)?;
+            drop(refs_lock);
+            let resizer = grow_request.realloc(GFP_KERNEL)?;
+            refs_lock = self.node_refs.lock();
+            refs = &mut *refs_lock;
+            refs.handle_is_present.grow(resizer);
+        };
+        let handle = unused_id.as_u32();
 
         // Do a lookup again as node may have been inserted before the lock was reacquired.
         if let Some(handle_ref) = refs.by_node.get(&node_ref.node.global_id()) {
@@ -804,20 +833,9 @@ pub(crate) fn insert_or_update_handle(
             return Ok(handle);
         }
 
-        // Find id.
-        let mut target: u32 = if is_mananger { 0 } else { 1 };
-        for handle in refs.by_handle.keys() {
-            if *handle > target {
-                break;
-            }
-            if *handle == target {
-                target = target.checked_add(1).ok_or(ENOMEM)?;
-            }
-        }
-
         let gid = node_ref.node.global_id();
         let (info_proc, info_node) = {
-            let info_init = NodeRefInfo::new(node_ref, target, self.into());
+            let info_init = NodeRefInfo::new(node_ref, handle, self.into());
             match info.pin_init_with(info_init) {
                 Ok(info) => ListArc::pair_from_pin_unique(info),
                 // error is infallible
@@ -838,9 +856,10 @@ pub(crate) fn insert_or_update_handle(
         // `info_node` into the right node's `refs` list.
         unsafe { info_proc.node_ref2().node.insert_node_info(info_node) };
 
-        refs.by_node.insert(reserve1.into_node(gid, target));
-        refs.by_handle.insert(reserve2.into_node(target, info_proc));
-        Ok(target)
+        refs.by_node.insert(reserve1.into_node(gid, handle));
+        by_handle_slot.insert(info_proc, reserve2);
+        unused_id.acquire();
+        Ok(handle)
     }
 
     pub(crate) fn get_transaction_node(&self, handle: u32) -> BinderResult<NodeRef> {
@@ -905,6 +924,16 @@ pub(crate) fn update_ref(
                 let id = info.node_ref().node.global_id();
                 refs.by_handle.remove(&handle);
                 refs.by_node.remove(&id);
+                refs.handle_is_present.release_id(handle as usize);
+
+                if let Some(shrink) = refs.handle_is_present.shrink_request() {
+                    drop(refs);
+                    // This intentionally ignores allocation failures.
+                    if let Ok(new_bitmap) = shrink.realloc(GFP_KERNEL) {
+                        refs = self.node_refs.lock();
+                        refs.handle_is_present.shrink(new_bitmap);
+                    }
+                }
             }
         } else {
             // All refs are cleared in process exit, so this warning is expected in that case.

-- 
2.51.2.1041.gc1ab5b90ca-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/6] rust: bitmap: add MAX_LEN and NO_ALLOC_MAX_LEN constants
  2025-11-10 13:05 ` [PATCH v4 1/6] rust: bitmap: add MAX_LEN and NO_ALLOC_MAX_LEN constants Alice Ryhl
@ 2025-11-10 13:59   ` Tamir Duberstein
  2025-11-10 14:20     ` Alice Ryhl
  0 siblings, 1 reply; 15+ messages in thread
From: Tamir Duberstein @ 2025-11-10 13:59 UTC (permalink / raw)
  To: Alice Ryhl
  Cc: Greg Kroah-Hartman, Yury Norov, Arve Hjønnevåg,
	Todd Kjos, Martijn Coenen, Joel Fernandes, Christian Brauner,
	Carlos Llamas, Suren Baghdasaryan, Burak Emir, Miguel Ojeda,
	Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich, rust-for-linux,
	linux-kernel

On Mon, Nov 10, 2025 at 8:06 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> To avoid hard-coding these values in drivers, define constants for them
> that drivers can reference.
>
> Acked-by: Danilo Krummrich <dakr@kernel.org>
> Reviewed-by: Burak Emir <bqe@google.com>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/bitmap.rs | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
> index aa8fc7bf06fc99865ae755d8694e4bec3dc8e7f0..15fa23b45054b9272415fcc000e3e3b52c74d7c1 100644
> --- a/rust/kernel/bitmap.rs
> +++ b/rust/kernel/bitmap.rs
> @@ -149,14 +149,14 @@ macro_rules! bitmap_assert_return {
>  ///
>  /// # Invariants
>  ///
> -/// * `nbits` is `<= i32::MAX` and never changes.
> +/// * `nbits` is `<= MAX_LEN`.
>  /// * if `nbits <= bindings::BITS_PER_LONG`, then `repr` is a `usize`.

Should this and other references to bindings::BITS_PER_LONG be
`NO_ALLOC_MAX_LEN` instead?

>  /// * otherwise, `repr` holds a non-null pointer to an initialized
>  ///   array of `unsigned long` that is large enough to hold `nbits` bits.
>  pub struct BitmapVec {
>      /// Representation of bitmap.
>      repr: BitmapRepr,
> -    /// Length of this bitmap. Must be `<= i32::MAX`.
> +    /// Length of this bitmap. Must be `<= MAX_LEN`.
>      nbits: usize,
>  }
>
> @@ -226,10 +226,16 @@ fn drop(&mut self) {
>  }
>
>  impl BitmapVec {
> +    /// The maximum possible length of a `BitmapVec`.
> +    pub const MAX_LEN: usize = i32::MAX as usize;
> +
> +    /// The maximum length that avoids allocating.
> +    pub const NO_ALLOC_MAX_LEN: usize = BITS_PER_LONG;
> +
>      /// Constructs a new [`BitmapVec`].
>      ///
>      /// Fails with [`AllocError`] when the [`BitmapVec`] could not be allocated. This
> -    /// includes the case when `nbits` is greater than `i32::MAX`.
> +    /// includes the case when `nbits` is greater than `MAX_LEN`.
>      #[inline]
>      pub fn new(nbits: usize, flags: Flags) -> Result<Self, AllocError> {
>          if nbits <= BITS_PER_LONG {
> @@ -238,11 +244,11 @@ pub fn new(nbits: usize, flags: Flags) -> Result<Self, AllocError> {
>                  nbits,
>              });
>          }
> -        if nbits > i32::MAX.try_into().unwrap() {
> +        if nbits > Self::MAX_LEN {
>              return Err(AllocError);
>          }
>          let nbits_u32 = u32::try_from(nbits).unwrap();
> -        // SAFETY: `BITS_PER_LONG < nbits` and `nbits <= i32::MAX`.
> +        // SAFETY: `BITS_PER_LONG < nbits` and `nbits <= MAX_LEN`.
>          let ptr = unsafe { bindings::bitmap_zalloc(nbits_u32, flags.as_raw()) };
>          let ptr = NonNull::new(ptr).ok_or(AllocError)?;
>          // INVARIANT: `ptr` returned by C `bitmap_zalloc` and `nbits` checked.
>
> --
> 2.51.2.1041.gc1ab5b90ca-goog
>
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 3/6] rust: bitmap: rename IdPool::new to with_capacity
  2025-11-10 13:05 ` [PATCH v4 3/6] rust: bitmap: rename IdPool::new to with_capacity Alice Ryhl
@ 2025-11-10 14:12   ` Burak Emir
  0 siblings, 0 replies; 15+ messages in thread
From: Burak Emir @ 2025-11-10 14:12 UTC (permalink / raw)
  To: Alice Ryhl
  Cc: Greg Kroah-Hartman, Yury Norov, Arve Hjønnevåg,
	Todd Kjos, Martijn Coenen, Joel Fernandes, Christian Brauner,
	Carlos Llamas, Suren Baghdasaryan, Miguel Ojeda, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Trevor Gross, Danilo Krummrich, rust-for-linux, linux-kernel

On Mon, Nov 10, 2025 at 2:06 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> We want to change ::new() to take no parameters and produce a pool that
> is as large as possible while also being inline because that is the
> constructor that Rust Binder actually needs.
>
> However, to avoid complications in examples, we still need the current
> constructor. So rename it to with_capacity(), which is the idiomatic
> Rust name for this kind constructor.
>
Reviewed-by: Burak Emir <bqe@google.com>

> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/id_pool.rs | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/rust/kernel/id_pool.rs b/rust/kernel/id_pool.rs
> index a41a3404213ca92d53b14c80101afff6ac8c416e..5942f678db015e902fa482eb64c38512a468e449 100644
> --- a/rust/kernel/id_pool.rs
> +++ b/rust/kernel/id_pool.rs
> @@ -28,7 +28,7 @@
>  /// use kernel::alloc::{AllocError, flags::GFP_KERNEL};
>  /// use kernel::id_pool::IdPool;
>  ///
> -/// let mut pool = IdPool::new(64, GFP_KERNEL)?;
> +/// let mut pool = IdPool::with_capacity(64, GFP_KERNEL)?;
>  /// for i in 0..64 {
>  ///     assert_eq!(i, pool.acquire_next_id(i).ok_or(ENOSPC)?);
>  /// }
> @@ -95,14 +95,14 @@ pub fn realloc(&self, flags: Flags) -> Result<PoolResizer, AllocError> {
>  }
>
>  impl IdPool {
> -    /// Constructs a new [`IdPool`].
> +    /// Constructs a new [`IdPool`] with space for a specific number of bits.
>      ///
> -    /// A capacity below [`BITS_PER_LONG`] is adjusted to
> -    /// [`BITS_PER_LONG`].
> +    /// A capacity below [`NO_ALLOC_MAX_LEN`] is adjusted to
> +    /// [`NO_ALLOC_MAX_LEN`].
>      ///
> -    /// [`BITS_PER_LONG`]: srctree/include/asm-generic/bitsperlong.h
> +    /// [`NO_ALLOC_MAX_LEN`]: BitmapVec::NO_ALLOC_MAX_LEN
>      #[inline]
> -    pub fn new(num_ids: usize, flags: Flags) -> Result<Self, AllocError> {
> +    pub fn with_capacity(num_ids: usize, flags: Flags) -> Result<Self, AllocError> {
>          let num_ids = core::cmp::max(num_ids, BITS_PER_LONG);
>          let map = BitmapVec::new(num_ids, flags)?;
>          Ok(Self { map })
> @@ -126,7 +126,7 @@ pub fn capacity(&self) -> usize {
>      /// use kernel::alloc::{AllocError, flags::GFP_KERNEL};
>      /// use kernel::id_pool::{ReallocRequest, IdPool};
>      ///
> -    /// let mut pool = IdPool::new(1024, GFP_KERNEL)?;
> +    /// let mut pool = IdPool::with_capacity(1024, GFP_KERNEL)?;
>      /// let alloc_request = pool.shrink_request().ok_or(AllocError)?;
>      /// let resizer = alloc_request.realloc(GFP_KERNEL)?;
>      /// pool.shrink(resizer);
>
> --
> 2.51.2.1041.gc1ab5b90ca-goog
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/6] rust: bitmap: add MAX_LEN and NO_ALLOC_MAX_LEN constants
  2025-11-10 13:59   ` Tamir Duberstein
@ 2025-11-10 14:20     ` Alice Ryhl
  2025-11-10 15:01       ` Yury Norov
  0 siblings, 1 reply; 15+ messages in thread
From: Alice Ryhl @ 2025-11-10 14:20 UTC (permalink / raw)
  To: Tamir Duberstein
  Cc: Greg Kroah-Hartman, Yury Norov, Arve Hjønnevåg,
	Todd Kjos, Martijn Coenen, Joel Fernandes, Christian Brauner,
	Carlos Llamas, Suren Baghdasaryan, Burak Emir, Miguel Ojeda,
	Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich, rust-for-linux,
	linux-kernel

On Mon, Nov 10, 2025 at 08:59:36AM -0500, Tamir Duberstein wrote:
> On Mon, Nov 10, 2025 at 8:06 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > To avoid hard-coding these values in drivers, define constants for them
> > that drivers can reference.
> >
> > Acked-by: Danilo Krummrich <dakr@kernel.org>
> > Reviewed-by: Burak Emir <bqe@google.com>
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > ---
> >  rust/kernel/bitmap.rs | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
> > index aa8fc7bf06fc99865ae755d8694e4bec3dc8e7f0..15fa23b45054b9272415fcc000e3e3b52c74d7c1 100644
> > --- a/rust/kernel/bitmap.rs
> > +++ b/rust/kernel/bitmap.rs
> > @@ -149,14 +149,14 @@ macro_rules! bitmap_assert_return {
> >  ///
> >  /// # Invariants
> >  ///
> > -/// * `nbits` is `<= i32::MAX` and never changes.
> > +/// * `nbits` is `<= MAX_LEN`.
> >  /// * if `nbits <= bindings::BITS_PER_LONG`, then `repr` is a `usize`.
> 
> Should this and other references to bindings::BITS_PER_LONG be
> `NO_ALLOC_MAX_LEN` instead?

Ah yeah it probably makes sense to update this in a bunch of places.

Alice

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/6] rust: bitmap: add MAX_LEN and NO_ALLOC_MAX_LEN constants
  2025-11-10 14:20     ` Alice Ryhl
@ 2025-11-10 15:01       ` Yury Norov
  2025-11-10 15:09         ` Yury Norov
  2025-11-10 15:11         ` Alice Ryhl
  0 siblings, 2 replies; 15+ messages in thread
From: Yury Norov @ 2025-11-10 15:01 UTC (permalink / raw)
  To: Alice Ryhl
  Cc: Tamir Duberstein, Greg Kroah-Hartman, Arve Hjønnevåg,
	Todd Kjos, Martijn Coenen, Joel Fernandes, Christian Brauner,
	Carlos Llamas, Suren Baghdasaryan, Burak Emir, Miguel Ojeda,
	Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich, rust-for-linux,
	linux-kernel

On Mon, Nov 10, 2025 at 02:20:17PM +0000, Alice Ryhl wrote:
> On Mon, Nov 10, 2025 at 08:59:36AM -0500, Tamir Duberstein wrote:
> > On Mon, Nov 10, 2025 at 8:06 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > >
> > > To avoid hard-coding these values in drivers, define constants for them
> > > that drivers can reference.
> > >
> > > Acked-by: Danilo Krummrich <dakr@kernel.org>
> > > Reviewed-by: Burak Emir <bqe@google.com>
> > > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > > ---
> > >  rust/kernel/bitmap.rs | 16 +++++++++++-----
> > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
> > > index aa8fc7bf06fc99865ae755d8694e4bec3dc8e7f0..15fa23b45054b9272415fcc000e3e3b52c74d7c1 100644
> > > --- a/rust/kernel/bitmap.rs
> > > +++ b/rust/kernel/bitmap.rs
> > > @@ -149,14 +149,14 @@ macro_rules! bitmap_assert_return {
> > >  ///
> > >  /// # Invariants
> > >  ///
> > > -/// * `nbits` is `<= i32::MAX` and never changes.
> > > +/// * `nbits` is `<= MAX_LEN`.
> > >  /// * if `nbits <= bindings::BITS_PER_LONG`, then `repr` is a `usize`.
> > 
> > Should this and other references to bindings::BITS_PER_LONG be
> > `NO_ALLOC_MAX_LEN` instead?
> 
> Ah yeah it probably makes sense to update this in a bunch of places.

Yes, please. 

NO_ALLOC sounds a bit weird in exported API. Maybe NBITS_INPLACE
or similar?

Also, at this point we're really close to:

   pub const NBITS_INPLACE: usize = CONFIG_NBITS_INPLACE;

   union BitmapRepr {
       bitmap: [usize, BITS_TO_LONGS(NBITS_INPLACE)]
       ptr: NonNull<usize>,
   }

That would be a very useful addition for some particular scenarios,
I believe. Even if you don't want to make it configurable, let's
keep this option in mind?

Thanks,
Yury

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/6] rust: bitmap: add MAX_LEN and NO_ALLOC_MAX_LEN constants
  2025-11-10 15:01       ` Yury Norov
@ 2025-11-10 15:09         ` Yury Norov
  2025-11-10 15:11         ` Alice Ryhl
  1 sibling, 0 replies; 15+ messages in thread
From: Yury Norov @ 2025-11-10 15:09 UTC (permalink / raw)
  To: Alice Ryhl
  Cc: Tamir Duberstein, Greg Kroah-Hartman, Arve Hjønnevåg,
	Todd Kjos, Martijn Coenen, Joel Fernandes, Christian Brauner,
	Carlos Llamas, Suren Baghdasaryan, Burak Emir, Miguel Ojeda,
	Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich, rust-for-linux,
	linux-kernel

On Mon, Nov 10, 2025 at 10:01:32AM -0500, Yury Norov wrote:
> On Mon, Nov 10, 2025 at 02:20:17PM +0000, Alice Ryhl wrote:
> > On Mon, Nov 10, 2025 at 08:59:36AM -0500, Tamir Duberstein wrote:
> > > On Mon, Nov 10, 2025 at 8:06 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > > >
> > > > To avoid hard-coding these values in drivers, define constants for them
> > > > that drivers can reference.
> > > >
> > > > Acked-by: Danilo Krummrich <dakr@kernel.org>
> > > > Reviewed-by: Burak Emir <bqe@google.com>
> > > > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > > > ---
> > > >  rust/kernel/bitmap.rs | 16 +++++++++++-----
> > > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
> > > > index aa8fc7bf06fc99865ae755d8694e4bec3dc8e7f0..15fa23b45054b9272415fcc000e3e3b52c74d7c1 100644
> > > > --- a/rust/kernel/bitmap.rs
> > > > +++ b/rust/kernel/bitmap.rs
> > > > @@ -149,14 +149,14 @@ macro_rules! bitmap_assert_return {
> > > >  ///
> > > >  /// # Invariants
> > > >  ///
> > > > -/// * `nbits` is `<= i32::MAX` and never changes.
> > > > +/// * `nbits` is `<= MAX_LEN`.
> > > >  /// * if `nbits <= bindings::BITS_PER_LONG`, then `repr` is a `usize`.
> > > 
> > > Should this and other references to bindings::BITS_PER_LONG be
> > > `NO_ALLOC_MAX_LEN` instead?
> > 
> > Ah yeah it probably makes sense to update this in a bunch of places.
> 
> Yes, please. 
> 
> NO_ALLOC sounds a bit weird in exported API. Maybe NBITS_INPLACE
> or similar?

You add a 'new_inline()' constructor in this series, so this parameter
should be LEN_INLINE or NBITS_INLINE, like that.
 
> Also, at this point we're really close to:
> 
>    pub const NBITS_INPLACE: usize = CONFIG_NBITS_INPLACE;
> 
>    union BitmapRepr {
>        bitmap: [usize, BITS_TO_LONGS(NBITS_INPLACE)]
>        ptr: NonNull<usize>,
>    }
> 
> That would be a very useful addition for some particular scenarios,
> I believe. Even if you don't want to make it configurable, let's
> keep this option in mind?
> 
> Thanks,
> Yury

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/6] rust: bitmap: add MAX_LEN and NO_ALLOC_MAX_LEN constants
  2025-11-10 15:01       ` Yury Norov
  2025-11-10 15:09         ` Yury Norov
@ 2025-11-10 15:11         ` Alice Ryhl
  2025-11-10 15:39           ` Yury Norov
  1 sibling, 1 reply; 15+ messages in thread
From: Alice Ryhl @ 2025-11-10 15:11 UTC (permalink / raw)
  To: Yury Norov
  Cc: Tamir Duberstein, Greg Kroah-Hartman, Arve Hjønnevåg,
	Todd Kjos, Martijn Coenen, Joel Fernandes, Christian Brauner,
	Carlos Llamas, Suren Baghdasaryan, Burak Emir, Miguel Ojeda,
	Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich, rust-for-linux,
	linux-kernel

On Mon, Nov 10, 2025 at 10:01:29AM -0500, Yury Norov wrote:
> On Mon, Nov 10, 2025 at 02:20:17PM +0000, Alice Ryhl wrote:
> > On Mon, Nov 10, 2025 at 08:59:36AM -0500, Tamir Duberstein wrote:
> > > On Mon, Nov 10, 2025 at 8:06 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > > >
> > > > To avoid hard-coding these values in drivers, define constants for them
> > > > that drivers can reference.
> > > >
> > > > Acked-by: Danilo Krummrich <dakr@kernel.org>
> > > > Reviewed-by: Burak Emir <bqe@google.com>
> > > > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > > > ---
> > > >  rust/kernel/bitmap.rs | 16 +++++++++++-----
> > > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
> > > > index aa8fc7bf06fc99865ae755d8694e4bec3dc8e7f0..15fa23b45054b9272415fcc000e3e3b52c74d7c1 100644
> > > > --- a/rust/kernel/bitmap.rs
> > > > +++ b/rust/kernel/bitmap.rs
> > > > @@ -149,14 +149,14 @@ macro_rules! bitmap_assert_return {
> > > >  ///
> > > >  /// # Invariants
> > > >  ///
> > > > -/// * `nbits` is `<= i32::MAX` and never changes.
> > > > +/// * `nbits` is `<= MAX_LEN`.
> > > >  /// * if `nbits <= bindings::BITS_PER_LONG`, then `repr` is a `usize`.
> > > 
> > > Should this and other references to bindings::BITS_PER_LONG be
> > > `NO_ALLOC_MAX_LEN` instead?
> > 
> > Ah yeah it probably makes sense to update this in a bunch of places.
> 
> Yes, please. 
> 
> NO_ALLOC sounds a bit weird in exported API. Maybe NBITS_INPLACE
> or similar?

Ah, good point. We started using the "inplace" wording in other places,
so lets also do so here.

> Also, at this point we're really close to:
> 
>    pub const NBITS_INPLACE: usize = CONFIG_NBITS_INPLACE;
> 
>    union BitmapRepr {
>        bitmap: [usize, BITS_TO_LONGS(NBITS_INPLACE)]
>        ptr: NonNull<usize>,
>    }
> 
> That would be a very useful addition for some particular scenarios,
> I believe. Even if you don't want to make it configurable, let's
> keep this option in mind?

Actually, one option here is to define BitmapVec like this:

pub struct BitmapVec<const INPLACE_LEN: usize = 1> {
    repr: BitmapRepr<INPLACE_LEN>,
    nbits: usize,
}

union BitmapRepr<const INPLACE_LEN: usize> {
    bitmap: [usize; INPLACE_LEN],
    ptr: NonNull<usize>,
}

This way, the driver can specify this by saying: BitmapVec<4> for a
BitmapVec where the inline capacity is 4 longs.

And if Binder wanted to make that configurable, Binder could define a
constant based on a Binder specific CONFIG_* that controls what value
Binder passes.

Since I wrote `= 1` in the struct, you may also write BitmapVec without
specifying any number and get the default.

It may be possible to specify the number in bits rather than longs too,
but then we have to decide what to do if it's not divisible by
BITS_PER_LONG.

(But in the case of Rust Binder, the value we want is one long worth of
bits.)

Alice

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/6] rust: bitmap: add MAX_LEN and NO_ALLOC_MAX_LEN constants
  2025-11-10 15:11         ` Alice Ryhl
@ 2025-11-10 15:39           ` Yury Norov
  2025-11-12 12:49             ` Alice Ryhl
  0 siblings, 1 reply; 15+ messages in thread
From: Yury Norov @ 2025-11-10 15:39 UTC (permalink / raw)
  To: Alice Ryhl
  Cc: Tamir Duberstein, Greg Kroah-Hartman, Arve Hjønnevåg,
	Todd Kjos, Martijn Coenen, Joel Fernandes, Christian Brauner,
	Carlos Llamas, Suren Baghdasaryan, Burak Emir, Miguel Ojeda,
	Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich, rust-for-linux,
	linux-kernel

On Mon, Nov 10, 2025 at 03:11:35PM +0000, Alice Ryhl wrote:
> On Mon, Nov 10, 2025 at 10:01:29AM -0500, Yury Norov wrote:
> > On Mon, Nov 10, 2025 at 02:20:17PM +0000, Alice Ryhl wrote:
> > > On Mon, Nov 10, 2025 at 08:59:36AM -0500, Tamir Duberstein wrote:
> > > > On Mon, Nov 10, 2025 at 8:06 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > > > >
> > > > > To avoid hard-coding these values in drivers, define constants for them
> > > > > that drivers can reference.
> > > > >
> > > > > Acked-by: Danilo Krummrich <dakr@kernel.org>
> > > > > Reviewed-by: Burak Emir <bqe@google.com>
> > > > > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > > > > ---
> > > > >  rust/kernel/bitmap.rs | 16 +++++++++++-----
> > > > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
> > > > > index aa8fc7bf06fc99865ae755d8694e4bec3dc8e7f0..15fa23b45054b9272415fcc000e3e3b52c74d7c1 100644
> > > > > --- a/rust/kernel/bitmap.rs
> > > > > +++ b/rust/kernel/bitmap.rs
> > > > > @@ -149,14 +149,14 @@ macro_rules! bitmap_assert_return {
> > > > >  ///
> > > > >  /// # Invariants
> > > > >  ///
> > > > > -/// * `nbits` is `<= i32::MAX` and never changes.
> > > > > +/// * `nbits` is `<= MAX_LEN`.
> > > > >  /// * if `nbits <= bindings::BITS_PER_LONG`, then `repr` is a `usize`.
> > > > 
> > > > Should this and other references to bindings::BITS_PER_LONG be
> > > > `NO_ALLOC_MAX_LEN` instead?
> > > 
> > > Ah yeah it probably makes sense to update this in a bunch of places.
> > 
> > Yes, please. 
> > 
> > NO_ALLOC sounds a bit weird in exported API. Maybe NBITS_INPLACE
> > or similar?
> 
> Ah, good point. We started using the "inplace" wording in other places,
> so lets also do so here.
> 
> > Also, at this point we're really close to:
> > 
> >    pub const NBITS_INPLACE: usize = CONFIG_NBITS_INPLACE;
> > 
> >    union BitmapRepr {
> >        bitmap: [usize, BITS_TO_LONGS(NBITS_INPLACE)]
> >        ptr: NonNull<usize>,
> >    }
> > 
> > That would be a very useful addition for some particular scenarios,
> > I believe. Even if you don't want to make it configurable, let's
> > keep this option in mind?
> 
> Actually, one option here is to define BitmapVec like this:
> 
> pub struct BitmapVec<const INPLACE_LEN: usize = 1> {
>     repr: BitmapRepr<INPLACE_LEN>,
>     nbits: usize,
> }
> 
> union BitmapRepr<const INPLACE_LEN: usize> {
>     bitmap: [usize; INPLACE_LEN],
>     ptr: NonNull<usize>,
> }
> 
> This way, the driver can specify this by saying: BitmapVec<4> for a
> BitmapVec where the inline capacity is 4 longs.
> 
> And if Binder wanted to make that configurable, Binder could define a
> constant based on a Binder specific CONFIG_* that controls what value
> Binder passes.
> 
> Since I wrote `= 1` in the struct, you may also write BitmapVec without
> specifying any number and get the default.
> 
> It may be possible to specify the number in bits rather than longs too,
> but then we have to decide what to do if it's not divisible by
> BITS_PER_LONG.
> 
> (But in the case of Rust Binder, the value we want is one long worth of
> bits.)

It's better to define the actual number of bits. One reason is 32 vs
64 bit portability. Another one is readability - when dealing with
bit structures, it's better to think of it as a set of bits.

Those providing unaligned defaults... - you can drop a comment for
them. :)

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/6] rust: bitmap: add MAX_LEN and NO_ALLOC_MAX_LEN constants
  2025-11-10 15:39           ` Yury Norov
@ 2025-11-12 12:49             ` Alice Ryhl
  0 siblings, 0 replies; 15+ messages in thread
From: Alice Ryhl @ 2025-11-12 12:49 UTC (permalink / raw)
  To: Yury Norov
  Cc: Tamir Duberstein, Greg Kroah-Hartman, Arve Hjønnevåg,
	Todd Kjos, Martijn Coenen, Joel Fernandes, Christian Brauner,
	Carlos Llamas, Suren Baghdasaryan, Burak Emir, Miguel Ojeda,
	Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich, rust-for-linux,
	linux-kernel

On Mon, Nov 10, 2025 at 4:39 PM Yury Norov <yury.norov@gmail.com> wrote:
>
> On Mon, Nov 10, 2025 at 03:11:35PM +0000, Alice Ryhl wrote:
> > On Mon, Nov 10, 2025 at 10:01:29AM -0500, Yury Norov wrote:
> > > On Mon, Nov 10, 2025 at 02:20:17PM +0000, Alice Ryhl wrote:
> > > > On Mon, Nov 10, 2025 at 08:59:36AM -0500, Tamir Duberstein wrote:
> > > > > On Mon, Nov 10, 2025 at 8:06 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > > > > >
> > > > > > To avoid hard-coding these values in drivers, define constants for them
> > > > > > that drivers can reference.
> > > > > >
> > > > > > Acked-by: Danilo Krummrich <dakr@kernel.org>
> > > > > > Reviewed-by: Burak Emir <bqe@google.com>
> > > > > > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > > > > > ---
> > > > > >  rust/kernel/bitmap.rs | 16 +++++++++++-----
> > > > > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
> > > > > > index aa8fc7bf06fc99865ae755d8694e4bec3dc8e7f0..15fa23b45054b9272415fcc000e3e3b52c74d7c1 100644
> > > > > > --- a/rust/kernel/bitmap.rs
> > > > > > +++ b/rust/kernel/bitmap.rs
> > > > > > @@ -149,14 +149,14 @@ macro_rules! bitmap_assert_return {
> > > > > >  ///
> > > > > >  /// # Invariants
> > > > > >  ///
> > > > > > -/// * `nbits` is `<= i32::MAX` and never changes.
> > > > > > +/// * `nbits` is `<= MAX_LEN`.
> > > > > >  /// * if `nbits <= bindings::BITS_PER_LONG`, then `repr` is a `usize`.
> > > > >
> > > > > Should this and other references to bindings::BITS_PER_LONG be
> > > > > `NO_ALLOC_MAX_LEN` instead?
> > > >
> > > > Ah yeah it probably makes sense to update this in a bunch of places.
> > >
> > > Yes, please.
> > >
> > > NO_ALLOC sounds a bit weird in exported API. Maybe NBITS_INPLACE
> > > or similar?
> >
> > Ah, good point. We started using the "inplace" wording in other places,
> > so lets also do so here.
> >
> > > Also, at this point we're really close to:
> > >
> > >    pub const NBITS_INPLACE: usize = CONFIG_NBITS_INPLACE;
> > >
> > >    union BitmapRepr {
> > >        bitmap: [usize, BITS_TO_LONGS(NBITS_INPLACE)]
> > >        ptr: NonNull<usize>,
> > >    }
> > >
> > > That would be a very useful addition for some particular scenarios,
> > > I believe. Even if you don't want to make it configurable, let's
> > > keep this option in mind?
> >
> > Actually, one option here is to define BitmapVec like this:
> >
> > pub struct BitmapVec<const INPLACE_LEN: usize = 1> {
> >     repr: BitmapRepr<INPLACE_LEN>,
> >     nbits: usize,
> > }
> >
> > union BitmapRepr<const INPLACE_LEN: usize> {
> >     bitmap: [usize; INPLACE_LEN],
> >     ptr: NonNull<usize>,
> > }
> >
> > This way, the driver can specify this by saying: BitmapVec<4> for a
> > BitmapVec where the inline capacity is 4 longs.
> >
> > And if Binder wanted to make that configurable, Binder could define a
> > constant based on a Binder specific CONFIG_* that controls what value
> > Binder passes.
> >
> > Since I wrote `= 1` in the struct, you may also write BitmapVec without
> > specifying any number and get the default.
> >
> > It may be possible to specify the number in bits rather than longs too,
> > but then we have to decide what to do if it's not divisible by
> > BITS_PER_LONG.
> >
> > (But in the case of Rust Binder, the value we want is one long worth of
> > bits.)
>
> It's better to define the actual number of bits. One reason is 32 vs
> 64 bit portability. Another one is readability - when dealing with
> bit structures, it's better to think of it as a set of bits.
>
> Those providing unaligned defaults... - you can drop a comment for
> them. :)

Makes sense. I'm not going to do it in this series, but it seems like
a reasonable idea.

Alice

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2025-11-12 12:49 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-10 13:05 [PATCH v4 0/6] Use Rust Bitmap from Rust Binder driver Alice Ryhl
2025-11-10 13:05 ` [PATCH v4 1/6] rust: bitmap: add MAX_LEN and NO_ALLOC_MAX_LEN constants Alice Ryhl
2025-11-10 13:59   ` Tamir Duberstein
2025-11-10 14:20     ` Alice Ryhl
2025-11-10 15:01       ` Yury Norov
2025-11-10 15:09         ` Yury Norov
2025-11-10 15:11         ` Alice Ryhl
2025-11-10 15:39           ` Yury Norov
2025-11-12 12:49             ` Alice Ryhl
2025-11-10 13:05 ` [PATCH v4 2/6] rust: bitmap: add BitmapVec::new_inline() Alice Ryhl
2025-11-10 13:05 ` [PATCH v4 3/6] rust: bitmap: rename IdPool::new to with_capacity Alice Ryhl
2025-11-10 14:12   ` Burak Emir
2025-11-10 13:05 ` [PATCH v4 4/6] rust: id_pool: do not supply starting capacity Alice Ryhl
2025-11-10 13:05 ` [PATCH v4 5/6] rust: id_pool: do not immediately acquire new ids Alice Ryhl
2025-11-10 13:05 ` [PATCH v4 6/6] rust_binder: use bitmap for allocation of handles 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).