* [PATCH v5 1/5] rust: bitmap: use function-level cfg on kunit test
2026-08-12 8:51 [PATCH v5 0/5] rust: Add support for reserving of ranges of IDs Eliot Courtney
@ 2026-08-12 8:51 ` Eliot Courtney
2026-08-12 22:23 ` Yury Norov
2026-08-12 8:51 ` [PATCH v5 2/5] rust: bitmap: restrict bitmap length to at most i32::MAX Eliot Courtney
` (3 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Eliot Courtney @ 2026-08-12 8:51 UTC (permalink / raw)
To: Alice Ryhl, Burak Emir, Yury Norov, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, David Airlie, Simona Vetter
Cc: Greg Kroah-Hartman, John Hubbard, Alistair Popple, Timur Tabi,
Zhi Wang, rust-for-linux, linux-kernel, nova-gpu, dri-devel,
Eliot Courtney
Since commit c652dc44192d ("rust: kunit: allow `cfg` on `test`s"),
we no longer need this workaround.
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
rust/kernel/bitmap.rs | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
index b27e0ec80d64..a43bfe0ec3dc 100644
--- a/rust/kernel/bitmap.rs
+++ b/rust/kernel/bitmap.rs
@@ -572,24 +572,21 @@ fn bitmap_set_clear_find() -> Result<(), AllocError> {
}
#[test]
+ #[cfg(not(CONFIG_RUST_BITMAP_HARDENED))]
fn owned_bitmap_out_of_bounds() -> Result<(), AllocError> {
- // TODO: Kunit #[test]s do not support `cfg` yet,
- // so we add it here in the body.
- #[cfg(not(CONFIG_RUST_BITMAP_HARDENED))]
- {
- let mut b = BitmapVec::new(128, GFP_KERNEL)?;
- b.set_bit(2048);
- b.set_bit_atomic(2048);
- b.clear_bit(2048);
- b.clear_bit_atomic(2048);
- assert_eq!(None, b.next_bit(2048));
- assert_eq!(None, b.next_zero_bit(2048));
- assert_eq!(None, b.last_bit());
- }
+ let mut b = BitmapVec::new(128, GFP_KERNEL)?;
+
+ b.set_bit(2048);
+ b.set_bit_atomic(2048);
+ b.clear_bit(2048);
+ b.clear_bit_atomic(2048);
+ assert_eq!(None, b.next_bit(2048));
+ assert_eq!(None, b.next_zero_bit(2048));
+ assert_eq!(None, b.last_bit());
Ok(())
}
- // TODO: uncomment once kunit supports [should_panic] and `cfg`.
+ // TODO: uncomment once kunit supports `#[should_panic]`.
// #[cfg(CONFIG_RUST_BITMAP_HARDENED)]
// #[test]
// #[should_panic]
--
2.55.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v5 1/5] rust: bitmap: use function-level cfg on kunit test
2026-08-12 8:51 ` [PATCH v5 1/5] rust: bitmap: use function-level cfg on kunit test Eliot Courtney
@ 2026-08-12 22:23 ` Yury Norov
0 siblings, 0 replies; 20+ messages in thread
From: Yury Norov @ 2026-08-12 22:23 UTC (permalink / raw)
To: Eliot Courtney
Cc: Alice Ryhl, Burak Emir, Yury Norov, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, David Airlie, Simona Vetter,
Greg Kroah-Hartman, John Hubbard, Alistair Popple, Timur Tabi,
Zhi Wang, rust-for-linux, linux-kernel, nova-gpu, dri-devel
On Wed, Aug 12, 2026 at 05:51:21PM +0900, Eliot Courtney wrote:
> Since commit c652dc44192d ("rust: kunit: allow `cfg` on `test`s"),
> we no longer need this workaround.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
Reviewed-by: Yury Norov <ynorov@nvidia.com>
> ---
> rust/kernel/bitmap.rs | 25 +++++++++++--------------
> 1 file changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
> index b27e0ec80d64..a43bfe0ec3dc 100644
> --- a/rust/kernel/bitmap.rs
> +++ b/rust/kernel/bitmap.rs
> @@ -572,24 +572,21 @@ fn bitmap_set_clear_find() -> Result<(), AllocError> {
> }
>
> #[test]
> + #[cfg(not(CONFIG_RUST_BITMAP_HARDENED))]
> fn owned_bitmap_out_of_bounds() -> Result<(), AllocError> {
> - // TODO: Kunit #[test]s do not support `cfg` yet,
> - // so we add it here in the body.
> - #[cfg(not(CONFIG_RUST_BITMAP_HARDENED))]
> - {
> - let mut b = BitmapVec::new(128, GFP_KERNEL)?;
> - b.set_bit(2048);
> - b.set_bit_atomic(2048);
> - b.clear_bit(2048);
> - b.clear_bit_atomic(2048);
> - assert_eq!(None, b.next_bit(2048));
> - assert_eq!(None, b.next_zero_bit(2048));
> - assert_eq!(None, b.last_bit());
> - }
> + let mut b = BitmapVec::new(128, GFP_KERNEL)?;
> +
> + b.set_bit(2048);
> + b.set_bit_atomic(2048);
> + b.clear_bit(2048);
> + b.clear_bit_atomic(2048);
> + assert_eq!(None, b.next_bit(2048));
> + assert_eq!(None, b.next_zero_bit(2048));
> + assert_eq!(None, b.last_bit());
> Ok(())
> }
>
> - // TODO: uncomment once kunit supports [should_panic] and `cfg`.
> + // TODO: uncomment once kunit supports `#[should_panic]`.
> // #[cfg(CONFIG_RUST_BITMAP_HARDENED)]
> // #[test]
> // #[should_panic]
>
> --
> 2.55.0
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v5 2/5] rust: bitmap: restrict bitmap length to at most i32::MAX
2026-08-12 8:51 [PATCH v5 0/5] rust: Add support for reserving of ranges of IDs Eliot Courtney
2026-08-12 8:51 ` [PATCH v5 1/5] rust: bitmap: use function-level cfg on kunit test Eliot Courtney
@ 2026-08-12 8:51 ` Eliot Courtney
2026-08-12 19:44 ` Yury Norov
2026-08-12 8:51 ` [PATCH v5 3/5] rust: bitmap: add contiguous area operations Eliot Courtney
` (2 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Eliot Courtney @ 2026-08-12 8:51 UTC (permalink / raw)
To: Alice Ryhl, Burak Emir, Yury Norov, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, David Airlie, Simona Vetter
Cc: Greg Kroah-Hartman, John Hubbard, Alistair Popple, Timur Tabi,
Zhi Wang, rust-for-linux, linux-kernel, nova-gpu, dri-devel,
Eliot Courtney
It is currently possible to construct a non-`BitmapVec` backed
`Bitmap` using `Bitmap::from_raw` that is larger than `i32::MAX`, and
it is not part of the unsafe requirements. Restricting all bitmaps
(even non-`BitmapVec` backed ones) to a maximum size of `i32::MAX`
simplifies a few things and matches `BitmapVec::MAX_LEN`.
Add that requirement to the unsafe requirements on `Bitmap::from_raw`
and `Bitmap::from_raw_mut`, and to the invariants on `Bitmap`.
This also fixes u32 casts truncating in `copy_and_extend`, which could
otherwise lead to OOB writes.
Fixes: 11eca92a2cae ("rust: add bitmap API.")
Link: https://lore.kernel.org/DKG0U8RLO7LZ.2I1AIH0S38PAP@nvidia.com
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
rust/kernel/bitmap.rs | 68 +++++++++++++++++++++++++++++++++++----------------
1 file changed, 47 insertions(+), 21 deletions(-)
diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
index a43bfe0ec3dc..fdcfc0409773 100644
--- a/rust/kernel/bitmap.rs
+++ b/rust/kernel/bitmap.rs
@@ -17,24 +17,57 @@
/// # Invariants
///
/// Must reference a `[c_ulong]` long enough to fit `data.len()` bits.
+/// Must not be longer than `i32::MAX` bits.
#[cfg_attr(CONFIG_64BIT, repr(align(8)))]
#[cfg_attr(not(CONFIG_64BIT), repr(align(4)))]
pub struct Bitmap {
data: [()],
}
+macro_rules! bitmap_assert {
+ ($cond:expr, $($arg:tt)+) => {
+ #[cfg(CONFIG_RUST_BITMAP_HARDENED)]
+ assert!($cond, $($arg)*);
+ }
+}
+
+macro_rules! bitmap_assert_return {
+ ($cond:expr, $($arg:tt)+) => {
+ #[cfg(CONFIG_RUST_BITMAP_HARDENED)]
+ assert!($cond, $($arg)*);
+
+ #[cfg(not(CONFIG_RUST_BITMAP_HARDENED))]
+ if !($cond) {
+ pr_err!($($arg)*);
+ return
+ }
+ }
+}
+
impl Bitmap {
/// Borrows a C bitmap.
///
+ /// # Panics
+ ///
+ /// Panics if CONFIG_RUST_BITMAP_HARDENED is enabled and `nbits` exceeds `i32::MAX`.
+ ///
/// # Safety
///
/// * `ptr` holds a non-null address of an initialized array of `unsigned long`
/// that is large enough to hold `nbits` bits.
+ /// * `nbits` must not exceed `i32::MAX`.
/// * the array must not be freed for the lifetime of this [`Bitmap`]
/// * concurrent access only happens through atomic operations
pub unsafe fn from_raw<'a>(ptr: *const usize, nbits: usize) -> &'a Bitmap {
+ bitmap_assert!(
+ nbits <= i32::MAX as usize,
+ "`nbits` must be <= {}, was {}",
+ i32::MAX,
+ nbits
+ );
let data: *const [()] = core::ptr::slice_from_raw_parts(ptr.cast(), nbits);
// INVARIANT: `data` references an initialized array that can hold `nbits` bits.
+ // INVARIANT: the caller guarantees that `nbits` does not exceed `i32::MAX`.
// SAFETY:
// The caller guarantees that `data` (derived from `ptr` and `nbits`)
// points to a valid, initialized, and appropriately sized memory region
@@ -51,15 +84,27 @@ pub unsafe fn from_raw<'a>(ptr: *const usize, nbits: usize) -> &'a Bitmap {
/// Borrows a C bitmap exclusively.
///
+ /// # Panics
+ ///
+ /// Panics if CONFIG_RUST_BITMAP_HARDENED is enabled and `nbits` exceeds `i32::MAX`.
+ ///
/// # Safety
///
/// * `ptr` holds a non-null address of an initialized array of `unsigned long`
/// that is large enough to hold `nbits` bits.
+ /// * `nbits` must not exceed `i32::MAX`.
/// * the array must not be freed for the lifetime of this [`Bitmap`]
/// * no concurrent access may happen.
pub unsafe fn from_raw_mut<'a>(ptr: *mut usize, nbits: usize) -> &'a mut Bitmap {
+ bitmap_assert!(
+ nbits <= i32::MAX as usize,
+ "`nbits` must be <= {}, was {}",
+ i32::MAX,
+ nbits
+ );
let data: *mut [()] = core::ptr::slice_from_raw_parts_mut(ptr.cast(), nbits);
// INVARIANT: `data` references an initialized array that can hold `nbits` bits.
+ // INVARIANT: the caller guarantees that `nbits` does not exceed `i32::MAX`.
// SAFETY:
// The caller guarantees that `data` (derived from `ptr` and `nbits`)
// points to a valid, initialized, and appropriately sized memory region
@@ -96,26 +141,6 @@ union BitmapRepr {
ptr: NonNull<usize>,
}
-macro_rules! bitmap_assert {
- ($cond:expr, $($arg:tt)+) => {
- #[cfg(CONFIG_RUST_BITMAP_HARDENED)]
- assert!($cond, $($arg)*);
- }
-}
-
-macro_rules! bitmap_assert_return {
- ($cond:expr, $($arg:tt)+) => {
- #[cfg(CONFIG_RUST_BITMAP_HARDENED)]
- assert!($cond, $($arg)*);
-
- #[cfg(not(CONFIG_RUST_BITMAP_HARDENED))]
- if !($cond) {
- pr_err!($($arg)*);
- return
- }
- }
-}
-
/// Represents an owned bitmap.
///
/// Wraps underlying C bitmap API. See [`Bitmap`] for available
@@ -415,7 +440,8 @@ pub fn clear_bit_atomic(&self, index: usize) {
#[inline]
pub fn copy_and_extend(&mut self, src: &Bitmap) {
let len = core::cmp::min(src.len(), self.len());
- // SAFETY: access to `self` and `src` is within bounds.
+ // SAFETY: access to `self` and `src` is within bounds. Both lengths fit in `u32`
+ // because a `Bitmap` is at most `i32::MAX` bits, so the casts are lossless.
unsafe {
bindings::bitmap_copy_and_extend(
self.as_mut_ptr(),
--
2.55.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v5 2/5] rust: bitmap: restrict bitmap length to at most i32::MAX
2026-08-12 8:51 ` [PATCH v5 2/5] rust: bitmap: restrict bitmap length to at most i32::MAX Eliot Courtney
@ 2026-08-12 19:44 ` Yury Norov
0 siblings, 0 replies; 20+ messages in thread
From: Yury Norov @ 2026-08-12 19:44 UTC (permalink / raw)
To: Eliot Courtney
Cc: Alice Ryhl, Burak Emir, Yury Norov, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, David Airlie, Simona Vetter,
Greg Kroah-Hartman, John Hubbard, Alistair Popple, Timur Tabi,
Zhi Wang, rust-for-linux, linux-kernel, nova-gpu, dri-devel
On Wed, Aug 12, 2026 at 05:51:22PM +0900, Eliot Courtney wrote:
> It is currently possible to construct a non-`BitmapVec` backed
> `Bitmap` using `Bitmap::from_raw` that is larger than `i32::MAX`, and
> it is not part of the unsafe requirements. Restricting all bitmaps
> (even non-`BitmapVec` backed ones) to a maximum size of `i32::MAX`
> simplifies a few things and matches `BitmapVec::MAX_LEN`.
>
> Add that requirement to the unsafe requirements on `Bitmap::from_raw`
> and `Bitmap::from_raw_mut`, and to the invariants on `Bitmap`.
>
> This also fixes u32 casts truncating in `copy_and_extend`, which could
> otherwise lead to OOB writes.
>
> Fixes: 11eca92a2cae ("rust: add bitmap API.")
> Link: https://lore.kernel.org/DKG0U8RLO7LZ.2I1AIH0S38PAP@nvidia.com
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
Reviewed-by: Yury Norov <ynorov@nvidia.com>
> ---
> rust/kernel/bitmap.rs | 68 +++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 47 insertions(+), 21 deletions(-)
>
> diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
> index a43bfe0ec3dc..fdcfc0409773 100644
> --- a/rust/kernel/bitmap.rs
> +++ b/rust/kernel/bitmap.rs
> @@ -17,24 +17,57 @@
> /// # Invariants
> ///
> /// Must reference a `[c_ulong]` long enough to fit `data.len()` bits.
> +/// Must not be longer than `i32::MAX` bits.
> #[cfg_attr(CONFIG_64BIT, repr(align(8)))]
> #[cfg_attr(not(CONFIG_64BIT), repr(align(4)))]
> pub struct Bitmap {
> data: [()],
> }
>
> +macro_rules! bitmap_assert {
> + ($cond:expr, $($arg:tt)+) => {
> + #[cfg(CONFIG_RUST_BITMAP_HARDENED)]
> + assert!($cond, $($arg)*);
> + }
> +}
> +
> +macro_rules! bitmap_assert_return {
> + ($cond:expr, $($arg:tt)+) => {
> + #[cfg(CONFIG_RUST_BITMAP_HARDENED)]
> + assert!($cond, $($arg)*);
> +
> + #[cfg(not(CONFIG_RUST_BITMAP_HARDENED))]
> + if !($cond) {
> + pr_err!($($arg)*);
> + return
> + }
> + }
> +}
> +
> impl Bitmap {
> /// Borrows a C bitmap.
> ///
> + /// # Panics
> + ///
> + /// Panics if CONFIG_RUST_BITMAP_HARDENED is enabled and `nbits` exceeds `i32::MAX`.
> + ///
> /// # Safety
> ///
> /// * `ptr` holds a non-null address of an initialized array of `unsigned long`
> /// that is large enough to hold `nbits` bits.
> + /// * `nbits` must not exceed `i32::MAX`.
> /// * the array must not be freed for the lifetime of this [`Bitmap`]
> /// * concurrent access only happens through atomic operations
> pub unsafe fn from_raw<'a>(ptr: *const usize, nbits: usize) -> &'a Bitmap {
> + bitmap_assert!(
> + nbits <= i32::MAX as usize,
> + "`nbits` must be <= {}, was {}",
> + i32::MAX,
> + nbits
> + );
> let data: *const [()] = core::ptr::slice_from_raw_parts(ptr.cast(), nbits);
> // INVARIANT: `data` references an initialized array that can hold `nbits` bits.
> + // INVARIANT: the caller guarantees that `nbits` does not exceed `i32::MAX`.
> // SAFETY:
> // The caller guarantees that `data` (derived from `ptr` and `nbits`)
> // points to a valid, initialized, and appropriately sized memory region
> @@ -51,15 +84,27 @@ pub unsafe fn from_raw<'a>(ptr: *const usize, nbits: usize) -> &'a Bitmap {
>
> /// Borrows a C bitmap exclusively.
> ///
> + /// # Panics
> + ///
> + /// Panics if CONFIG_RUST_BITMAP_HARDENED is enabled and `nbits` exceeds `i32::MAX`.
> + ///
> /// # Safety
> ///
> /// * `ptr` holds a non-null address of an initialized array of `unsigned long`
> /// that is large enough to hold `nbits` bits.
> + /// * `nbits` must not exceed `i32::MAX`.
> /// * the array must not be freed for the lifetime of this [`Bitmap`]
> /// * no concurrent access may happen.
> pub unsafe fn from_raw_mut<'a>(ptr: *mut usize, nbits: usize) -> &'a mut Bitmap {
> + bitmap_assert!(
> + nbits <= i32::MAX as usize,
> + "`nbits` must be <= {}, was {}",
> + i32::MAX,
> + nbits
> + );
> let data: *mut [()] = core::ptr::slice_from_raw_parts_mut(ptr.cast(), nbits);
> // INVARIANT: `data` references an initialized array that can hold `nbits` bits.
> + // INVARIANT: the caller guarantees that `nbits` does not exceed `i32::MAX`.
> // SAFETY:
> // The caller guarantees that `data` (derived from `ptr` and `nbits`)
> // points to a valid, initialized, and appropriately sized memory region
> @@ -96,26 +141,6 @@ union BitmapRepr {
> ptr: NonNull<usize>,
> }
>
> -macro_rules! bitmap_assert {
> - ($cond:expr, $($arg:tt)+) => {
> - #[cfg(CONFIG_RUST_BITMAP_HARDENED)]
> - assert!($cond, $($arg)*);
> - }
> -}
> -
> -macro_rules! bitmap_assert_return {
> - ($cond:expr, $($arg:tt)+) => {
> - #[cfg(CONFIG_RUST_BITMAP_HARDENED)]
> - assert!($cond, $($arg)*);
> -
> - #[cfg(not(CONFIG_RUST_BITMAP_HARDENED))]
> - if !($cond) {
> - pr_err!($($arg)*);
> - return
> - }
> - }
> -}
> -
> /// Represents an owned bitmap.
> ///
> /// Wraps underlying C bitmap API. See [`Bitmap`] for available
> @@ -415,7 +440,8 @@ pub fn clear_bit_atomic(&self, index: usize) {
> #[inline]
> pub fn copy_and_extend(&mut self, src: &Bitmap) {
> let len = core::cmp::min(src.len(), self.len());
> - // SAFETY: access to `self` and `src` is within bounds.
> + // SAFETY: access to `self` and `src` is within bounds. Both lengths fit in `u32`
> + // because a `Bitmap` is at most `i32::MAX` bits, so the casts are lossless.
> unsafe {
> bindings::bitmap_copy_and_extend(
> self.as_mut_ptr(),
>
> --
> 2.55.0
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v5 3/5] rust: bitmap: add contiguous area operations
2026-08-12 8:51 [PATCH v5 0/5] rust: Add support for reserving of ranges of IDs Eliot Courtney
2026-08-12 8:51 ` [PATCH v5 1/5] rust: bitmap: use function-level cfg on kunit test Eliot Courtney
2026-08-12 8:51 ` [PATCH v5 2/5] rust: bitmap: restrict bitmap length to at most i32::MAX Eliot Courtney
@ 2026-08-12 8:51 ` Eliot Courtney
2026-08-12 20:31 ` Yury Norov
2026-08-12 8:51 ` [PATCH v5 4/5] rust: id_pool: add contiguous area allocation Eliot Courtney
2026-08-12 8:51 ` [PATCH v5 5/5] gpu: nova-core: add ChannelIdPool Eliot Courtney
4 siblings, 1 reply; 20+ messages in thread
From: Eliot Courtney @ 2026-08-12 8:51 UTC (permalink / raw)
To: Alice Ryhl, Burak Emir, Yury Norov, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, David Airlie, Simona Vetter
Cc: Greg Kroah-Hartman, John Hubbard, Alistair Popple, Timur Tabi,
Zhi Wang, rust-for-linux, linux-kernel, nova-gpu, dri-devel,
Eliot Courtney
Add bindings for area operations on bitmaps. Each one is
made safe by adding some extra checks compared to the underlying C code
(for example, checking bounds) and with additional checks to catch
likely erroneous usage if `CONFIG_RUST_BITMAP_HARDENED` is on.
Add tests demonstrating the edge cases.
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
rust/kernel/bitmap.rs | 236 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 236 insertions(+)
diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
index fdcfc0409773..74c92cc452c9 100644
--- a/rust/kernel/bitmap.rs
+++ b/rust/kernel/bitmap.rs
@@ -10,6 +10,7 @@
use crate::bindings;
#[cfg(not(CONFIG_RUST_BITMAP_HARDENED))]
use crate::pr_err;
+use crate::ptr::Alignment;
use core::ptr::NonNull;
/// Represents a C bitmap. Wraps underlying C bitmap API.
@@ -523,6 +524,139 @@ pub fn next_zero_bit(&self, start: usize) -> Option<usize> {
Some(index)
}
}
+
+ /// Finds a contiguous area of `nbits` zero bits at or after `start`, where the area plus
+ /// `align_offset` is aligned to `align`.
+ ///
+ /// Returns the bit index of the start of the area, or [`None`] if no such area fitting in
+ /// the bitmap exists.
+ ///
+ /// The returned index plus `align_offset` is a multiple of `align`.
+ ///
+ /// # Panics
+ ///
+ /// Panics if CONFIG_RUST_BITMAP_HARDENED is enabled and `start` is out of bounds.
+ #[inline]
+ pub fn next_zero_area_off(
+ &self,
+ start: usize,
+ nbits: usize,
+ align: Alignment,
+ align_offset: usize,
+ ) -> Option<usize> {
+ bitmap_assert!(
+ start < self.len(),
+ "`start` must be < {}, was {}",
+ self.len(),
+ start
+ );
+
+ let nr = u32::try_from(nbits).ok()?;
+ let align_mask = align.as_usize() - 1;
+
+ // The C alignment and end arithmetic must not overflow, or it can read out of bounds.
+ // Overflow is only possible on 32-bit.
+ #[cfg(not(CONFIG_64BIT))]
+ align_mask.checked_add(self.len())?.checked_add(nbits)?;
+
+ // SAFETY: `bitmap_find_next_zero_area_off` is safe to use with an out of bounds `start`
+ // value and, given the overflow check above, never reads beyond `self.len()` bits.
+ let index = unsafe {
+ bindings::bitmap_find_next_zero_area_off(
+ self.as_ptr().cast_mut(),
+ self.len(),
+ start,
+ nr,
+ align_mask,
+ align_offset,
+ )
+ };
+
+ (index < self.len()).then_some(index)
+ }
+
+ /// Finds a contiguous area of `nbits` zero bits at or after `start`, aligned to `align`.
+ ///
+ /// Returns the bit index of the start of the area, or [`None`] if no such area fitting in
+ /// the bitmap exists.
+ ///
+ /// The returned index is a multiple of `align`.
+ ///
+ /// # Panics
+ ///
+ /// Panics if CONFIG_RUST_BITMAP_HARDENED is enabled and `start` is out of bounds.
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// use kernel::alloc::{AllocError, flags::GFP_KERNEL};
+ /// use kernel::bitmap::BitmapVec;
+ /// use kernel::ptr::Alignment;
+ ///
+ /// let mut b = BitmapVec::new(64, GFP_KERNEL)?;
+ /// let unaligned = Alignment::new::<1>();
+ ///
+ /// assert_eq!(Some(0), b.next_zero_area(0, 8, unaligned));
+ /// b.set(0, 5);
+ /// assert_eq!(Some(5), b.next_zero_area(0, 8, unaligned));
+ /// assert_eq!(Some(8), b.next_zero_area(0, 8, Alignment::new::<8>()));
+ /// assert_eq!(None, b.next_zero_area(0, 65, unaligned));
+ /// # Ok::<(), AllocError>(())
+ /// ```
+ #[inline]
+ pub fn next_zero_area(&self, start: usize, nbits: usize, align: Alignment) -> Option<usize> {
+ self.next_zero_area_off(start, nbits, align, 0)
+ }
+
+ /// Sets a contiguous area of `nbits` bits starting at `start`.
+ ///
+ /// If CONFIG_RUST_BITMAP_HARDENED is not enabled and the area `start..start + nbits` is out of
+ /// bounds, does nothing.
+ ///
+ /// # Panics
+ ///
+ /// Panics if CONFIG_RUST_BITMAP_HARDENED is enabled and the area `start..start + nbits` is out
+ /// of bounds.
+ #[inline]
+ pub fn set(&mut self, start: usize, nbits: usize) {
+ bitmap_assert_return!(
+ start
+ .checked_add(nbits)
+ .is_some_and(|end| end <= self.len()),
+ "Area `start..start + nbits` ({}..{}) must be within bounds {}",
+ start,
+ start.saturating_add(nbits),
+ self.len()
+ );
+ // SAFETY: The area `start..start + nbits` is within bounds and a `Bitmap` is at most
+ // `i32::MAX` bits, so the casts are lossless.
+ unsafe { bindings::__bitmap_set(self.as_mut_ptr(), start as u32, nbits as i32) };
+ }
+
+ /// Clears a contiguous area of `nbits` bits starting at `start`.
+ ///
+ /// If CONFIG_RUST_BITMAP_HARDENED is not enabled and the area `start..start + nbits` is out of
+ /// bounds, does nothing.
+ ///
+ /// # Panics
+ ///
+ /// Panics if CONFIG_RUST_BITMAP_HARDENED is enabled and the area `start..start + nbits` is out
+ /// of bounds.
+ #[inline]
+ pub fn clear(&mut self, start: usize, nbits: usize) {
+ bitmap_assert_return!(
+ start
+ .checked_add(nbits)
+ .is_some_and(|end| end <= self.len()),
+ "Area `start..start + nbits` ({}..{}) must be within bounds {}",
+ start,
+ start.saturating_add(nbits),
+ self.len()
+ );
+ // SAFETY: The area `start..start + nbits` is within bounds and a `Bitmap` is at most
+ // `i32::MAX` bits, so the casts are lossless.
+ unsafe { bindings::__bitmap_clear(self.as_mut_ptr(), start as u32, nbits as i32) };
+ }
}
#[cfg(CONFIG_RUST_BITMAP_KUNIT_TEST)]
@@ -640,4 +774,106 @@ fn bitmap_copy_and_extend() -> Result<(), AllocError> {
assert_eq!(Some(17), long_bitmap.last_bit());
Ok(())
}
+
+ #[test]
+ fn bitmap_area_set_clear_find() -> Result<(), AllocError> {
+ let mut b = BitmapVec::new(128, GFP_KERNEL)?;
+ let unaligned = Alignment::new::<1>();
+
+ assert_eq!(Some(0), b.next_zero_area(0, 5, unaligned));
+ b.set(0, 5); // Now contains {[0, 5)}.
+
+ assert_eq!(Some(0), b.next_bit(0));
+ assert_eq!(Some(4), b.next_bit(4));
+ assert_eq!(Some(5), b.next_zero_bit(0));
+ assert_eq!(Some(5), b.next_zero_area(0, 5, unaligned));
+ assert_eq!(Some(8), b.next_zero_area(0, 5, Alignment::new::<8>()));
+
+ b.set(8, 8); // Now contains {[0, 5), [8, 16)}.
+ assert_eq!(Some(16), b.next_zero_area(0, 4, Alignment::new::<16>()));
+ assert_eq!(Some(16), b.next_zero_area(0, 4, unaligned));
+
+ b.clear(0, 5); // Now contains {[8, 16)}.
+ assert_eq!(Some(0), b.next_zero_area(0, 5, unaligned));
+ assert_eq!(Some(8), b.next_bit(0));
+ assert_eq!(Some(15), b.last_bit());
+
+ b.clear(16, 0); // Zero-length in-bounds clears are no-ops.
+ assert_eq!(Some(8), b.next_bit(0));
+ assert_eq!(Some(15), b.last_bit());
+
+ // A zero-length request returns the first aligned position at or
+ // after the next zero bit, even if that position's own bit is set.
+ assert_eq!(Some(1), b.next_zero_area(1, 0, unaligned));
+ assert_eq!(Some(8), b.next_zero_area(1, 0, Alignment::new::<8>()));
+
+ b.set(60, 10); // Now contains {[8, 16), [60, 70)}.
+ assert_eq!(Some(60), b.next_bit(16));
+ assert_eq!(Some(69), b.last_bit());
+ assert_eq!(Some(16), b.next_zero_area(9, 40, unaligned));
+ assert_eq!(Some(70), b.next_zero_area(0, 45, unaligned));
+
+ b.clear(62, 6); // Now contains {[8, 16), [60, 62), [68, 70)}.
+ assert_eq!(Some(62), b.next_zero_area(60, 6, unaligned));
+ assert_eq!(Some(61), b.next_bit(61));
+ assert_eq!(Some(69), b.last_bit());
+
+ b.set(64, 0); // Zero-length in-bounds sets are no-ops.
+ assert_eq!(Some(62), b.next_zero_bit(62));
+ Ok(())
+ }
+
+ #[test]
+ fn bitmap_area_exhaustion() -> Result<(), AllocError> {
+ let mut b = BitmapVec::new(64, GFP_KERNEL)?;
+ let unaligned = Alignment::new::<1>();
+
+ assert_eq!(None, b.next_zero_area(0, 65, unaligned));
+ assert_eq!(None, b.next_zero_area(0, usize::MAX, unaligned));
+ assert_eq!(None, b.next_zero_area(1, usize::MAX, unaligned));
+
+ b.set_bit(0); // Now contains {[0, 1)}.
+ assert_eq!(None, b.next_zero_area(0, usize::MAX, unaligned));
+
+ b.set(0, 61); // Now contains {[0, 61)}.
+ assert_eq!(None, b.next_zero_area(0, 4, unaligned));
+ assert_eq!(Some(61), b.next_zero_area(0, 3, unaligned));
+ assert_eq!(None, b.next_zero_area(0, 1, Alignment::new::<64>()));
+ Ok(())
+ }
+
+ #[test]
+ fn bitmap_area_off() -> Result<(), AllocError> {
+ let mut b = BitmapVec::new(64, GFP_KERNEL)?;
+ let align8 = Alignment::new::<8>();
+
+ b.set(0, 5); // Now contains {[0, 5)}.
+
+ // The area plus align_offset starts at a multiple of the alignment.
+ assert_eq!(Some(7), b.next_zero_area_off(0, 8, align8, 1));
+ assert_eq!(Some(5), b.next_zero_area_off(0, 8, align8, 3));
+
+ // A zero offset behaves like next_zero_area().
+ assert_eq!(
+ b.next_zero_area(0, 8, align8),
+ b.next_zero_area_off(0, 8, align8, 0)
+ );
+ Ok(())
+ }
+
+ #[test]
+ #[cfg(not(CONFIG_RUST_BITMAP_HARDENED))]
+ fn owned_bitmap_area_out_of_bounds() -> Result<(), AllocError> {
+ let mut b = BitmapVec::new(64, GFP_KERNEL)?;
+
+ // Should be ignored since out of bounds.
+ b.set(64, 4);
+ b.set(62, 8);
+ b.set(usize::MAX, 0);
+ b.clear(usize::MAX, 0);
+ b.clear(2048, 8);
+ assert_eq!(None, b.next_bit(0));
+ assert_eq!(None, b.next_zero_area(64, 1, Alignment::new::<1>()));
+ Ok(())
+ }
}
--
2.55.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v5 3/5] rust: bitmap: add contiguous area operations
2026-08-12 8:51 ` [PATCH v5 3/5] rust: bitmap: add contiguous area operations Eliot Courtney
@ 2026-08-12 20:31 ` Yury Norov
2026-08-13 7:27 ` Eliot Courtney
0 siblings, 1 reply; 20+ messages in thread
From: Yury Norov @ 2026-08-12 20:31 UTC (permalink / raw)
To: Eliot Courtney
Cc: Alice Ryhl, Burak Emir, Yury Norov, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, David Airlie, Simona Vetter,
Greg Kroah-Hartman, John Hubbard, Alistair Popple, Timur Tabi,
Zhi Wang, rust-for-linux, linux-kernel, nova-gpu, dri-devel
On Wed, Aug 12, 2026 at 05:51:23PM +0900, Eliot Courtney wrote:
> Add bindings for area operations on bitmaps. Each one is
> made safe by adding some extra checks compared to the underlying C code
> (for example, checking bounds) and with additional checks to catch
> likely erroneous usage if `CONFIG_RUST_BITMAP_HARDENED` is on.
>
> Add tests demonstrating the edge cases.
>
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
> rust/kernel/bitmap.rs | 236 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 236 insertions(+)
>
> diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
> index fdcfc0409773..74c92cc452c9 100644
> --- a/rust/kernel/bitmap.rs
> +++ b/rust/kernel/bitmap.rs
> @@ -10,6 +10,7 @@
> use crate::bindings;
> #[cfg(not(CONFIG_RUST_BITMAP_HARDENED))]
> use crate::pr_err;
> +use crate::ptr::Alignment;
> use core::ptr::NonNull;
>
> /// Represents a C bitmap. Wraps underlying C bitmap API.
> @@ -523,6 +524,139 @@ pub fn next_zero_bit(&self, start: usize) -> Option<usize> {
> Some(index)
> }
> }
> +
> + /// Finds a contiguous area of `nbits` zero bits at or after `start`, where the area plus
> + /// `align_offset` is aligned to `align`.
> + ///
> + /// Returns the bit index of the start of the area, or [`None`] if no such area fitting in
> + /// the bitmap exists.
> + ///
> + /// The returned index plus `align_offset` is a multiple of `align`.
> + ///
> + /// # Panics
> + ///
> + /// Panics if CONFIG_RUST_BITMAP_HARDENED is enabled and `start` is out of bounds.
> + #[inline]
> + pub fn next_zero_area_off(
> + &self,
> + start: usize,
> + nbits: usize,
> + align: Alignment,
> + align_offset: usize,
> + ) -> Option<usize> {
> + bitmap_assert!(
> + start < self.len(),
> + "`start` must be < {}, was {}",
> + self.len(),
> + start
> + );
> +
> + let nr = u32::try_from(nbits).ok()?;
What about nbits == 0? In C, this is a undef, and thus in the current
rust implementation. Maybe make it NonZero?
The same question about align and align_offset.
> + let align_mask = align.as_usize() - 1;
> +
> + // The C alignment and end arithmetic must not overflow, or it can read out of bounds.
> + // Overflow is only possible on 32-bit.
> + #[cfg(not(CONFIG_64BIT))]
> + align_mask.checked_add(self.len())?.checked_add(nbits)?;
> +
> + // SAFETY: `bitmap_find_next_zero_area_off` is safe to use with an out of bounds `start`
> + // value and, given the overflow check above, never reads beyond `self.len()` bits.
> + let index = unsafe {
> + bindings::bitmap_find_next_zero_area_off(
> + self.as_ptr().cast_mut(),
> + self.len(),
> + start,
> + nr,
> + align_mask,
> + align_offset,
> + )
> + };
> +
> + (index < self.len()).then_some(index)
> + }
> +
> + /// Finds a contiguous area of `nbits` zero bits at or after `start`, aligned to `align`.
> + ///
> + /// Returns the bit index of the start of the area, or [`None`] if no such area fitting in
> + /// the bitmap exists.
> + ///
> + /// The returned index is a multiple of `align`.
> + ///
> + /// # Panics
> + ///
> + /// Panics if CONFIG_RUST_BITMAP_HARDENED is enabled and `start` is out of bounds.
> + ///
> + /// # Examples
> + ///
> + /// ```
> + /// use kernel::alloc::{AllocError, flags::GFP_KERNEL};
> + /// use kernel::bitmap::BitmapVec;
> + /// use kernel::ptr::Alignment;
> + ///
> + /// let mut b = BitmapVec::new(64, GFP_KERNEL)?;
> + /// let unaligned = Alignment::new::<1>();
> + ///
> + /// assert_eq!(Some(0), b.next_zero_area(0, 8, unaligned));
> + /// b.set(0, 5);
> + /// assert_eq!(Some(5), b.next_zero_area(0, 8, unaligned));
> + /// assert_eq!(Some(8), b.next_zero_area(0, 8, Alignment::new::<8>()));
> + /// assert_eq!(None, b.next_zero_area(0, 65, unaligned));
> + /// # Ok::<(), AllocError>(())
> + /// ```
> + #[inline]
> + pub fn next_zero_area(&self, start: usize, nbits: usize, align: Alignment) -> Option<usize> {
> + self.next_zero_area_off(start, nbits, align, 0)
> + }
> +
> + /// Sets a contiguous area of `nbits` bits starting at `start`.
> + ///
> + /// If CONFIG_RUST_BITMAP_HARDENED is not enabled and the area `start..start + nbits` is out of
> + /// bounds, does nothing.
> + ///
> + /// # Panics
> + ///
> + /// Panics if CONFIG_RUST_BITMAP_HARDENED is enabled and the area `start..start + nbits` is out
> + /// of bounds.
> + #[inline]
> + pub fn set(&mut self, start: usize, nbits: usize) {
> + bitmap_assert_return!(
> + start
> + .checked_add(nbits)
> + .is_some_and(|end| end <= self.len()),
> + "Area `start..start + nbits` ({}..{}) must be within bounds {}",
> + start,
> + start.saturating_add(nbits),
> + self.len()
> + );
> + // SAFETY: The area `start..start + nbits` is within bounds and a `Bitmap` is at most
> + // `i32::MAX` bits, so the casts are lossless.
> + unsafe { bindings::__bitmap_set(self.as_mut_ptr(), start as u32, nbits as i32) };
> + }
In the case of bitmap_set/clear(), nbits == 0 makes it a no-op, and
guarantees that the pointer is not dereferenced. So, no undefined
behavior. But in rust case, I believe, it should be a stronger policy.
I'd add an assertion, at least, or better make it NonZero.
Thanks,
Yury
> +
> + /// Clears a contiguous area of `nbits` bits starting at `start`.
> + ///
> + /// If CONFIG_RUST_BITMAP_HARDENED is not enabled and the area `start..start + nbits` is out of
> + /// bounds, does nothing.
> + ///
> + /// # Panics
> + ///
> + /// Panics if CONFIG_RUST_BITMAP_HARDENED is enabled and the area `start..start + nbits` is out
> + /// of bounds.
> + #[inline]
> + pub fn clear(&mut self, start: usize, nbits: usize) {
> + bitmap_assert_return!(
> + start
> + .checked_add(nbits)
> + .is_some_and(|end| end <= self.len()),
> + "Area `start..start + nbits` ({}..{}) must be within bounds {}",
> + start,
> + start.saturating_add(nbits),
> + self.len()
> + );
> + // SAFETY: The area `start..start + nbits` is within bounds and a `Bitmap` is at most
> + // `i32::MAX` bits, so the casts are lossless.
> + unsafe { bindings::__bitmap_clear(self.as_mut_ptr(), start as u32, nbits as i32) };
> + }
> }
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 3/5] rust: bitmap: add contiguous area operations
2026-08-12 20:31 ` Yury Norov
@ 2026-08-13 7:27 ` Eliot Courtney
0 siblings, 0 replies; 20+ messages in thread
From: Eliot Courtney @ 2026-08-13 7:27 UTC (permalink / raw)
To: Yury Norov, Eliot Courtney
Cc: Alice Ryhl, Burak Emir, Yury Norov, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, David Airlie, Simona Vetter,
Greg Kroah-Hartman, John Hubbard, Alistair Popple, Timur Tabi,
Zhi Wang, rust-for-linux, linux-kernel, nova-gpu, dri-devel,
dri-devel
On Thu Aug 13, 2026 at 5:31 AM JST, Yury Norov wrote:
> On Wed, Aug 12, 2026 at 05:51:23PM +0900, Eliot Courtney wrote:
>> Add bindings for area operations on bitmaps. Each one is
>> made safe by adding some extra checks compared to the underlying C code
>> (for example, checking bounds) and with additional checks to catch
>> likely erroneous usage if `CONFIG_RUST_BITMAP_HARDENED` is on.
>>
>> Add tests demonstrating the edge cases.
>>
>> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
>> ---
>> rust/kernel/bitmap.rs | 236 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 236 insertions(+)
>>
>> diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
>> index fdcfc0409773..74c92cc452c9 100644
>> --- a/rust/kernel/bitmap.rs
>> +++ b/rust/kernel/bitmap.rs
>> @@ -10,6 +10,7 @@
>> use crate::bindings;
>> #[cfg(not(CONFIG_RUST_BITMAP_HARDENED))]
>> use crate::pr_err;
>> +use crate::ptr::Alignment;
>> use core::ptr::NonNull;
>>
>> /// Represents a C bitmap. Wraps underlying C bitmap API.
>> @@ -523,6 +524,139 @@ pub fn next_zero_bit(&self, start: usize) -> Option<usize> {
>> Some(index)
>> }
>> }
>> +
>> + /// Finds a contiguous area of `nbits` zero bits at or after `start`, where the area plus
>> + /// `align_offset` is aligned to `align`.
>> + ///
>> + /// Returns the bit index of the start of the area, or [`None`] if no such area fitting in
>> + /// the bitmap exists.
>> + ///
>> + /// The returned index plus `align_offset` is a multiple of `align`.
>> + ///
>> + /// # Panics
>> + ///
>> + /// Panics if CONFIG_RUST_BITMAP_HARDENED is enabled and `start` is out of bounds.
>> + #[inline]
>> + pub fn next_zero_area_off(
>> + &self,
>> + start: usize,
>> + nbits: usize,
>> + align: Alignment,
>> + align_offset: usize,
>> + ) -> Option<usize> {
>> + bitmap_assert!(
>> + start < self.len(),
>> + "`start` must be < {}, was {}",
>> + self.len(),
>> + start
>> + );
>> +
>> + let nr = u32::try_from(nbits).ok()?;
>
> What about nbits == 0? In C, this is a undef, and thus in the current
> rust implementation. Maybe make it NonZero?
>
> The same question about align and align_offset.
NonZero sounds good to me for `nbits`. For `align`, it's already
guaranteed to be at least 1. For `align_offset`, passing 0 is normal and
valid (and we need to for implementing `next_zero_area` just below)
>
>> + let align_mask = align.as_usize() - 1;
>> +
>> + // The C alignment and end arithmetic must not overflow, or it can read out of bounds.
>> + // Overflow is only possible on 32-bit.
>> + #[cfg(not(CONFIG_64BIT))]
>> + align_mask.checked_add(self.len())?.checked_add(nbits)?;
>> +
>> + // SAFETY: `bitmap_find_next_zero_area_off` is safe to use with an out of bounds `start`
>> + // value and, given the overflow check above, never reads beyond `self.len()` bits.
>> + let index = unsafe {
>> + bindings::bitmap_find_next_zero_area_off(
>> + self.as_ptr().cast_mut(),
>> + self.len(),
>> + start,
>> + nr,
>> + align_mask,
>> + align_offset,
>> + )
>> + };
>> +
>> + (index < self.len()).then_some(index)
>> + }
>> +
>> + /// Finds a contiguous area of `nbits` zero bits at or after `start`, aligned to `align`.
>> + ///
>> + /// Returns the bit index of the start of the area, or [`None`] if no such area fitting in
>> + /// the bitmap exists.
>> + ///
>> + /// The returned index is a multiple of `align`.
>> + ///
>> + /// # Panics
>> + ///
>> + /// Panics if CONFIG_RUST_BITMAP_HARDENED is enabled and `start` is out of bounds.
>> + ///
>> + /// # Examples
>> + ///
>> + /// ```
>> + /// use kernel::alloc::{AllocError, flags::GFP_KERNEL};
>> + /// use kernel::bitmap::BitmapVec;
>> + /// use kernel::ptr::Alignment;
>> + ///
>> + /// let mut b = BitmapVec::new(64, GFP_KERNEL)?;
>> + /// let unaligned = Alignment::new::<1>();
>> + ///
>> + /// assert_eq!(Some(0), b.next_zero_area(0, 8, unaligned));
>> + /// b.set(0, 5);
>> + /// assert_eq!(Some(5), b.next_zero_area(0, 8, unaligned));
>> + /// assert_eq!(Some(8), b.next_zero_area(0, 8, Alignment::new::<8>()));
>> + /// assert_eq!(None, b.next_zero_area(0, 65, unaligned));
>> + /// # Ok::<(), AllocError>(())
>> + /// ```
>> + #[inline]
>> + pub fn next_zero_area(&self, start: usize, nbits: usize, align: Alignment) -> Option<usize> {
>> + self.next_zero_area_off(start, nbits, align, 0)
>> + }
>> +
>> + /// Sets a contiguous area of `nbits` bits starting at `start`.
>> + ///
>> + /// If CONFIG_RUST_BITMAP_HARDENED is not enabled and the area `start..start + nbits` is out of
>> + /// bounds, does nothing.
>> + ///
>> + /// # Panics
>> + ///
>> + /// Panics if CONFIG_RUST_BITMAP_HARDENED is enabled and the area `start..start + nbits` is out
>> + /// of bounds.
>> + #[inline]
>> + pub fn set(&mut self, start: usize, nbits: usize) {
>> + bitmap_assert_return!(
>> + start
>> + .checked_add(nbits)
>> + .is_some_and(|end| end <= self.len()),
>> + "Area `start..start + nbits` ({}..{}) must be within bounds {}",
>> + start,
>> + start.saturating_add(nbits),
>> + self.len()
>> + );
>> + // SAFETY: The area `start..start + nbits` is within bounds and a `Bitmap` is at most
>> + // `i32::MAX` bits, so the casts are lossless.
>> + unsafe { bindings::__bitmap_set(self.as_mut_ptr(), start as u32, nbits as i32) };
>> + }
>
> In the case of bitmap_set/clear(), nbits == 0 makes it a no-op, and
> guarantees that the pointer is not dereferenced. So, no undefined
> behavior. But in rust case, I believe, it should be a stronger policy.
>
> I'd add an assertion, at least, or better make it NonZero.
>
> Thanks,
> Yury
Yeah, NonZero sounds good to me here too. Thanks!
>
>> +
>> + /// Clears a contiguous area of `nbits` bits starting at `start`.
>> + ///
>> + /// If CONFIG_RUST_BITMAP_HARDENED is not enabled and the area `start..start + nbits` is out of
>> + /// bounds, does nothing.
>> + ///
>> + /// # Panics
>> + ///
>> + /// Panics if CONFIG_RUST_BITMAP_HARDENED is enabled and the area `start..start + nbits` is out
>> + /// of bounds.
>> + #[inline]
>> + pub fn clear(&mut self, start: usize, nbits: usize) {
>> + bitmap_assert_return!(
>> + start
>> + .checked_add(nbits)
>> + .is_some_and(|end| end <= self.len()),
>> + "Area `start..start + nbits` ({}..{}) must be within bounds {}",
>> + start,
>> + start.saturating_add(nbits),
>> + self.len()
>> + );
>> + // SAFETY: The area `start..start + nbits` is within bounds and a `Bitmap` is at most
>> + // `i32::MAX` bits, so the casts are lossless.
>> + unsafe { bindings::__bitmap_clear(self.as_mut_ptr(), start as u32, nbits as i32) };
>> + }
>> }
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v5 4/5] rust: id_pool: add contiguous area allocation
2026-08-12 8:51 [PATCH v5 0/5] rust: Add support for reserving of ranges of IDs Eliot Courtney
` (2 preceding siblings ...)
2026-08-12 8:51 ` [PATCH v5 3/5] rust: bitmap: add contiguous area operations Eliot Courtney
@ 2026-08-12 8:51 ` Eliot Courtney
2026-08-12 21:16 ` Yury Norov
2026-08-12 8:51 ` [PATCH v5 5/5] gpu: nova-core: add ChannelIdPool Eliot Courtney
4 siblings, 1 reply; 20+ messages in thread
From: Eliot Courtney @ 2026-08-12 8:51 UTC (permalink / raw)
To: Alice Ryhl, Burak Emir, Yury Norov, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, David Airlie, Simona Vetter
Cc: Greg Kroah-Hartman, John Hubbard, Alistair Popple, Timur Tabi,
Zhi Wang, rust-for-linux, linux-kernel, nova-gpu, dri-devel,
Eliot Courtney
Add support for contiguous area allocation. Add a new type,
`UnusedArea`, following the same pattern as `UnusedId`.
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
rust/kernel/id_pool.rs | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 69 insertions(+)
diff --git a/rust/kernel/id_pool.rs b/rust/kernel/id_pool.rs
index 384753fe0e44..eb911a0e3217 100644
--- a/rust/kernel/id_pool.rs
+++ b/rust/kernel/id_pool.rs
@@ -4,8 +4,14 @@
//! Rust API for an ID pool backed by a [`BitmapVec`].
+use core::{
+ num::NonZero,
+ ops::Range, //
+};
+
use crate::alloc::{AllocError, Flags};
use crate::bitmap::BitmapVec;
+use crate::ptr::Alignment;
/// Represents a dynamic ID pool backed by a [`BitmapVec`].
///
@@ -240,6 +246,33 @@ pub fn find_unused_id(&mut self, offset: usize) -> Option<UnusedId<'_>> {
pub fn release_id(&mut self, id: usize) {
self.map.clear_bit(id);
}
+
+ /// Finds a contiguous area of `count` unused IDs at or after `offset`.
+ ///
+ /// The start of the returned area is a multiple of `align`.
+ ///
+ /// Returns an [`UnusedArea`] upon success, or [`None`] if no such area could be found.
+ #[inline]
+ #[must_use]
+ pub fn find_unused_area(
+ &mut self,
+ offset: usize,
+ count: NonZero<usize>,
+ align: Alignment,
+ ) -> Option<UnusedArea<'_>> {
+ let start = self.map.next_zero_area(offset, count.get(), align)?;
+ // INVARIANT: `next_zero_area()` returns None or a start with `start + count <= map.len()`.
+ Some(UnusedArea {
+ range: start..start + count.get(),
+ pool: self,
+ })
+ }
+
+ /// Releases a contiguous area of IDs.
+ #[inline]
+ pub fn release_area(&mut self, range: &Range<usize>) {
+ self.map.clear(range.start, range.len());
+ }
}
/// Represents an unused id in an [`IdPool`].
@@ -287,6 +320,42 @@ pub fn acquire(self) -> usize {
}
}
+/// Represents an unused, contiguous area of IDs in an [`IdPool`].
+///
+/// # Invariants
+///
+/// `range.start <= range.end <= pool.map.len()`.
+#[must_use = "the ID range is not reserved unless acquired"]
+pub struct UnusedArea<'pool> {
+ range: Range<usize>,
+ pool: &'pool mut IdPool,
+}
+
+impl<'pool> UnusedArea<'pool> {
+ /// Returns the unused ID range.
+ ///
+ /// Be aware that the area has not yet been acquired in the pool. The
+ /// [`acquire`] method must be called to prevent others from taking it.
+ ///
+ /// [`acquire`]: UnusedArea::acquire()
+ #[inline]
+ #[must_use]
+ pub fn range(&self) -> Range<usize> {
+ self.range.clone()
+ }
+
+ /// Acquires the area.
+ ///
+ /// Returns the now-reserved ID range.
+ #[inline]
+ pub fn acquire(self) -> Range<usize> {
+ let Self { range, pool } = self;
+ // By the type invariants, the range is within bounds.
+ pool.map.set(range.start, range.end - range.start);
+ range
+ }
+}
+
impl Default for IdPool {
#[inline]
fn default() -> Self {
--
2.55.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v5 4/5] rust: id_pool: add contiguous area allocation
2026-08-12 8:51 ` [PATCH v5 4/5] rust: id_pool: add contiguous area allocation Eliot Courtney
@ 2026-08-12 21:16 ` Yury Norov
2026-08-13 7:29 ` Eliot Courtney
0 siblings, 1 reply; 20+ messages in thread
From: Yury Norov @ 2026-08-12 21:16 UTC (permalink / raw)
To: Eliot Courtney
Cc: Alice Ryhl, Burak Emir, Yury Norov, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, David Airlie, Simona Vetter,
Greg Kroah-Hartman, John Hubbard, Alistair Popple, Timur Tabi,
Zhi Wang, rust-for-linux, linux-kernel, nova-gpu, dri-devel
On Wed, Aug 12, 2026 at 05:51:24PM +0900, Eliot Courtney wrote:
> Add support for contiguous area allocation. Add a new type,
> `UnusedArea`, following the same pattern as `UnusedId`.
>
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
> rust/kernel/id_pool.rs | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 69 insertions(+)
>
> diff --git a/rust/kernel/id_pool.rs b/rust/kernel/id_pool.rs
> index 384753fe0e44..eb911a0e3217 100644
> --- a/rust/kernel/id_pool.rs
> +++ b/rust/kernel/id_pool.rs
> @@ -4,8 +4,14 @@
>
> //! Rust API for an ID pool backed by a [`BitmapVec`].
>
> +use core::{
> + num::NonZero,
> + ops::Range, //
> +};
> +
> use crate::alloc::{AllocError, Flags};
> use crate::bitmap::BitmapVec;
> +use crate::ptr::Alignment;
>
> /// Represents a dynamic ID pool backed by a [`BitmapVec`].
> ///
> @@ -240,6 +246,33 @@ pub fn find_unused_id(&mut self, offset: usize) -> Option<UnusedId<'_>> {
> pub fn release_id(&mut self, id: usize) {
> self.map.clear_bit(id);
> }
> +
> + /// Finds a contiguous area of `count` unused IDs at or after `offset`.
> + ///
> + /// The start of the returned area is a multiple of `align`.
> + ///
> + /// Returns an [`UnusedArea`] upon success, or [`None`] if no such area could be found.
> + #[inline]
> + #[must_use]
> + pub fn find_unused_area(
> + &mut self,
> + offset: usize,
> + count: NonZero<usize>,
> + align: Alignment,
> + ) -> Option<UnusedArea<'_>> {
> + let start = self.map.next_zero_area(offset, count.get(), align)?;
> + // INVARIANT: `next_zero_area()` returns None or a start with `start + count <= map.len()`.
> + Some(UnusedArea {
> + range: start..start + count.get(),
> + pool: self,
> + })
> + }
> +
> + /// Releases a contiguous area of IDs.
> + #[inline]
> + pub fn release_area(&mut self, range: &Range<usize>) {
> + self.map.clear(range.start, range.len());
> + }
> }
>
> /// Represents an unused id in an [`IdPool`].
> @@ -287,6 +320,42 @@ pub fn acquire(self) -> usize {
> }
> }
>
> +/// Represents an unused, contiguous area of IDs in an [`IdPool`].
> +///
> +/// # Invariants
> +///
> +/// `range.start <= range.end <= pool.map.len()`.
> +#[must_use = "the ID range is not reserved unless acquired"]
> +pub struct UnusedArea<'pool> {
> + range: Range<usize>,
> + pool: &'pool mut IdPool,
> +}
So, the compilation message refers the "ID range", not the UnusedArea.
To me, this 'unused' language is confusing. What should I do with the
area that I just allocated? Drop the 'unused' one and create the 'used'?
Can you rename it to id_range please? Then the API would look more
consistent, at least to me.
> +
> +impl<'pool> UnusedArea<'pool> {
> + /// Returns the unused ID range.
> + ///
> + /// Be aware that the area has not yet been acquired in the pool. The
> + /// [`acquire`] method must be called to prevent others from taking it.
> + ///
> + /// [`acquire`]: UnusedArea::acquire()
So maybe implement the find_acquire() method? In the caller you
serialize it with:
let mut ids = self.inner.lock();
Is it possible to pass this down to the suggested find_acquire()? In
my experience, having non-atomic sequence of find + acquire that
requires the external locking is the recipe for troubles.
> + #[inline]
> + #[must_use]
> + pub fn range(&self) -> Range<usize> {
> + self.range.clone()
> + }
> +
> + /// Acquires the area.
> + ///
> + /// Returns the now-reserved ID range.
> + #[inline]
> + pub fn acquire(self) -> Range<usize> {
> + let Self { range, pool } = self;
> + // By the type invariants, the range is within bounds.
> + pool.map.set(range.start, range.end - range.start);
> + range
From hierarchy perspective, the UnusedArea wraps the Range, and
passing the Range to the higher layer breaks the hierarchy. If you
follow my suggestion, the hierarchy will be enforced stricter:
ChannelIdRange -> IdRange-> Range
instead of
ChannelIdArea -> UnusedArea-> Range
|
-> Range
Or I misunderstand the concept of the UnusedArea?
> + }
> +}
> +
> impl Default for IdPool {
> #[inline]
> fn default() -> Self {
>
> --
> 2.55.0
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 4/5] rust: id_pool: add contiguous area allocation
2026-08-12 21:16 ` Yury Norov
@ 2026-08-13 7:29 ` Eliot Courtney
0 siblings, 0 replies; 20+ messages in thread
From: Eliot Courtney @ 2026-08-13 7:29 UTC (permalink / raw)
To: Yury Norov, Eliot Courtney
Cc: Alice Ryhl, Burak Emir, Yury Norov, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, David Airlie, Simona Vetter,
Greg Kroah-Hartman, John Hubbard, Alistair Popple, Timur Tabi,
Zhi Wang, rust-for-linux, linux-kernel, nova-gpu, dri-devel,
dri-devel
On Thu Aug 13, 2026 at 6:16 AM JST, Yury Norov wrote:
> On Wed, Aug 12, 2026 at 05:51:24PM +0900, Eliot Courtney wrote:
>> Add support for contiguous area allocation. Add a new type,
>> `UnusedArea`, following the same pattern as `UnusedId`.
>>
>> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
>> ---
>> rust/kernel/id_pool.rs | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 69 insertions(+)
>>
>> diff --git a/rust/kernel/id_pool.rs b/rust/kernel/id_pool.rs
>> index 384753fe0e44..eb911a0e3217 100644
>> --- a/rust/kernel/id_pool.rs
>> +++ b/rust/kernel/id_pool.rs
>> @@ -4,8 +4,14 @@
>>
>> //! Rust API for an ID pool backed by a [`BitmapVec`].
>>
>> +use core::{
>> + num::NonZero,
>> + ops::Range, //
>> +};
>> +
>> use crate::alloc::{AllocError, Flags};
>> use crate::bitmap::BitmapVec;
>> +use crate::ptr::Alignment;
>>
>> /// Represents a dynamic ID pool backed by a [`BitmapVec`].
>> ///
>> @@ -240,6 +246,33 @@ pub fn find_unused_id(&mut self, offset: usize) -> Option<UnusedId<'_>> {
>> pub fn release_id(&mut self, id: usize) {
>> self.map.clear_bit(id);
>> }
>> +
>> + /// Finds a contiguous area of `count` unused IDs at or after `offset`.
>> + ///
>> + /// The start of the returned area is a multiple of `align`.
>> + ///
>> + /// Returns an [`UnusedArea`] upon success, or [`None`] if no such area could be found.
>> + #[inline]
>> + #[must_use]
>> + pub fn find_unused_area(
>> + &mut self,
>> + offset: usize,
>> + count: NonZero<usize>,
>> + align: Alignment,
>> + ) -> Option<UnusedArea<'_>> {
>> + let start = self.map.next_zero_area(offset, count.get(), align)?;
>> + // INVARIANT: `next_zero_area()` returns None or a start with `start + count <= map.len()`.
>> + Some(UnusedArea {
>> + range: start..start + count.get(),
>> + pool: self,
>> + })
>> + }
>> +
>> + /// Releases a contiguous area of IDs.
>> + #[inline]
>> + pub fn release_area(&mut self, range: &Range<usize>) {
>> + self.map.clear(range.start, range.len());
>> + }
>> }
>>
>> /// Represents an unused id in an [`IdPool`].
>> @@ -287,6 +320,42 @@ pub fn acquire(self) -> usize {
>> }
>> }
>>
>> +/// Represents an unused, contiguous area of IDs in an [`IdPool`].
>> +///
>> +/// # Invariants
>> +///
>> +/// `range.start <= range.end <= pool.map.len()`.
>> +#[must_use = "the ID range is not reserved unless acquired"]
>> +pub struct UnusedArea<'pool> {
>> + range: Range<usize>,
>> + pool: &'pool mut IdPool,
>> +}
>
> So, the compilation message refers the "ID range", not the UnusedArea.
> To me, this 'unused' language is confusing. What should I do with the
> area that I just allocated? Drop the 'unused' one and create the 'used'?
>
> Can you rename it to id_range please? Then the API would look more
> consistent, at least to me.
tl;dr: I will remove `UnusedArea` according to your suggestion
`UnusedArea` here mirrors the design of `UnusedId`, which first finds
the unused ID then lets you actually set it whenever you want (first
introduced in f523d110a63b ("rust: id_pool: do not immediately acquire
new ids")). According to f523d110a63b, the reason for this design is to
allow some fallible operations once you know the ID you are getting
before committing to actually allocating it. So `UnusedArea` just
follows the existing design for this API. But, we don't need this
intermediate fallible operation behaviour right now, so I think it's ok
to get rid of it.
>
>> +
>> +impl<'pool> UnusedArea<'pool> {
>> + /// Returns the unused ID range.
>> + ///
>> + /// Be aware that the area has not yet been acquired in the pool. The
>> + /// [`acquire`] method must be called to prevent others from taking it.
>> + ///
>> + /// [`acquire`]: UnusedArea::acquire()
>
> So maybe implement the find_acquire() method? In the caller you
> serialize it with:
>
> let mut ids = self.inner.lock();
>
> Is it possible to pass this down to the suggested find_acquire()? In
> my experience, having non-atomic sequence of find + acquire that
> requires the external locking is the recipe for troubles.
In this case, since we stash a mutable reference to `IdPool` in
`UnusedArea` (just like `UnusedId`), it's not possible to hit any
non-atomic find/acquire issues. The compiler will statically prevent you
from being able to allocate anything else in the `IdPool` as long as the
`UnusedArea` is alive. That is, it's always valid to acquire an
`UnusedArea` (or `UnusedId`). In this case IMO it is more flexible to
let the caller decide how it will do the locking (and that's the
existing design for allocating single IDs here), since it can e.g.
control the granularity. Or maybe it really exists in a single threaded
context and doesn't need locking whatsoever (i.e. can get a mutable ref
to IdPool without locking).
>
>> + #[inline]
>> + #[must_use]
>> + pub fn range(&self) -> Range<usize> {
>> + self.range.clone()
>> + }
>> +
>> + /// Acquires the area.
>> + ///
>> + /// Returns the now-reserved ID range.
>> + #[inline]
>> + pub fn acquire(self) -> Range<usize> {
>> + let Self { range, pool } = self;
>> + // By the type invariants, the range is within bounds.
>> + pool.map.set(range.start, range.end - range.start);
>> + range
>
> From hierarchy perspective, the UnusedArea wraps the Range, and
> passing the Range to the higher layer breaks the hierarchy. If you
> follow my suggestion, the hierarchy will be enforced stricter:
>
> ChannelIdRange -> IdRange-> Range
>
> instead of
>
> ChannelIdArea -> UnusedArea-> Range
> |
> -> Range
>
> Or I misunderstand the concept of the UnusedArea?
>
>> + }
>> +}
>> +
>> impl Default for IdPool {
>> #[inline]
>> fn default() -> Self {
>>
>> --
>> 2.55.0
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v5 5/5] gpu: nova-core: add ChannelIdPool
2026-08-12 8:51 [PATCH v5 0/5] rust: Add support for reserving of ranges of IDs Eliot Courtney
` (3 preceding siblings ...)
2026-08-12 8:51 ` [PATCH v5 4/5] rust: id_pool: add contiguous area allocation Eliot Courtney
@ 2026-08-12 8:51 ` Eliot Courtney
2026-08-12 22:18 ` Yury Norov
4 siblings, 1 reply; 20+ messages in thread
From: Eliot Courtney @ 2026-08-12 8:51 UTC (permalink / raw)
To: Alice Ryhl, Burak Emir, Yury Norov, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, David Airlie, Simona Vetter
Cc: Greg Kroah-Hartman, John Hubbard, Alistair Popple, Timur Tabi,
Zhi Wang, rust-for-linux, linux-kernel, nova-gpu, dri-devel,
Eliot Courtney
Add `ChannelIdPool` which adds automatic tracking and releasing of
channel IDs on top of `IdPool`. This is necessary for apportioning
ranges of channel IDs to be used in e.g. vGPU.
Channel IDs are allocated as a contiguous sequence with a specific
length and sometimes a specific alignment [1] for vGPU. The ID space is
small (limited to 2048) and allocation is not on a hot path, so a
bitmap-backed `IdPool` is a better fit than IDA/xarray (which allocate a
single ID within a range, not a contiguous sequence) or a maple tree
(where aligned allocation needs an alloc_range()+erase() retry loop that
essentially reimplements bitmap_find_next_zero_area()) [2]. It is
also faster than maple tree [3].
Link: https://lore.kernel.org/all/84bc8bd2-e292-4b84-9580-a1b5df4c5bdc@nvidia.com/ # [1]
Link: https://lore.kernel.org/all/20260710-chid-maple-v1-1-4ee869055268@nvidia.com/ # [2]
Link: https://lore.kernel.org/all/20260717053241.916441-1-ynorov@nvidia.com/ # [3]
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
drivers/gpu/nova-core/gpu.rs | 2 +
drivers/gpu/nova-core/gpu/channel.rs | 180 +++++++++++++++++++++++++++++++++++
2 files changed, 182 insertions(+)
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 42a4cd7971fa..66ea697a89f8 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -33,6 +33,8 @@
vgpu::VgpuManager, //
};
+#[cfg_attr(not(CONFIG_KUNIT = "y"), expect(dead_code))]
+mod channel;
mod hal;
macro_rules! define_chipset {
diff --git a/drivers/gpu/nova-core/gpu/channel.rs b/drivers/gpu/nova-core/gpu/channel.rs
new file mode 100644
index 000000000000..b755d2184aee
--- /dev/null
+++ b/drivers/gpu/nova-core/gpu/channel.rs
@@ -0,0 +1,180 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+
+//! Channel ID allocation.
+
+use core::{
+ num::NonZero,
+ ops::{
+ Deref,
+ Range, //
+ }, //
+};
+
+use kernel::{
+ id_pool::IdPool,
+ prelude::*,
+ ptr::Alignment,
+ sync::{
+ new_mutex,
+ Mutex, //
+ }, //
+};
+
+/// Pool for tracking reservations of channel IDs.
+#[pin_data]
+pub(crate) struct ChannelIdPool {
+ #[pin]
+ inner: Mutex<IdPool>,
+ num_chids: usize,
+}
+
+impl ChannelIdPool {
+ /// Creates a pool managing `num_chids` channel IDs.
+ pub(crate) fn new(num_chids: usize) -> impl PinInit<Self, Error> {
+ try_pin_init!(Self {
+ inner <- new_mutex!(IdPool::with_capacity(num_chids, GFP_KERNEL)?),
+ num_chids,
+ })
+ }
+
+ /// Reserves a contiguous area of `count` channel IDs starting at a multiple of `align`,
+ /// returning a guard that releases the area on drop.
+ pub(crate) fn alloc_area(
+ &self,
+ count: NonZero<usize>,
+ align: Alignment,
+ ) -> Result<ChannelIdArea<'_>> {
+ let mut ids = self.inner.lock();
+ let area = ids.find_unused_area(0, count, align).ok_or(ENOSPC)?;
+
+ // If the pool is small, the backing bitmap may be rounded up to a larger size.
+ if area.range().end > self.num_chids {
+ return Err(ENOSPC);
+ }
+ Ok(ChannelIdArea {
+ pool: self,
+ range: area.acquire(),
+ })
+ }
+}
+
+/// A reserved contiguous area of channel IDs.
+///
+/// Releases the whole area back to its [`ChannelIdPool`] when dropped. Releasing locks a
+/// sleeping [`Mutex`], so the area must be dropped in a context that is allowed to sleep.
+#[must_use = "the channel ID area is released immediately when unused"]
+pub(crate) struct ChannelIdArea<'a> {
+ pool: &'a ChannelIdPool,
+ range: Range<usize>,
+}
+
+impl Drop for ChannelIdArea<'_> {
+ fn drop(&mut self) {
+ self.pool.inner.lock().release_area(&self.range);
+ }
+}
+
+impl Deref for ChannelIdArea<'_> {
+ type Target = Range<usize>;
+
+ fn deref(&self) -> &Self::Target {
+ &self.range
+ }
+}
+
+#[kunit_tests(nova_core_channel)]
+mod tests {
+ use super::*;
+
+ const fn nz<const N: usize>() -> NonZero<usize> {
+ const { NonZero::new(N).unwrap() }
+ }
+
+ #[test]
+ fn chid_area() -> Result {
+ let pool = KBox::pin_init(ChannelIdPool::new(2048), GFP_KERNEL)?;
+ let unaligned = Alignment::new::<1>();
+
+ let first = pool.alloc_area(nz::<48>(), unaligned)?;
+ assert_eq!(0, first.start);
+ assert_eq!(48, first.len());
+ assert_eq!(48, first.end);
+
+ let second = pool.alloc_area(nz::<48>(), unaligned)?;
+ assert!(first.end <= second.start || second.end <= first.start);
+
+ let first_start = first.start;
+ drop(first);
+ assert_eq!(first_start, pool.alloc_area(nz::<48>(), unaligned)?.start);
+ Ok(())
+ }
+
+ #[test]
+ fn chid_bounded_by_num_chids() -> Result {
+ let pool = KBox::pin_init(ChannelIdPool::new(4), GFP_KERNEL)?;
+ let unaligned = Alignment::new::<1>();
+
+ {
+ let a = pool.alloc_area(nz::<1>(), unaligned)?;
+ let b = pool.alloc_area(nz::<1>(), unaligned)?;
+ let c = pool.alloc_area(nz::<1>(), unaligned)?;
+ let d = pool.alloc_area(nz::<1>(), unaligned)?;
+ assert_eq!(0, a.start);
+ assert_eq!(1, b.start);
+ assert_eq!(2, c.start);
+ assert_eq!(3, d.start);
+ assert_eq!(
+ Err(ENOSPC),
+ pool.alloc_area(nz::<1>(), unaligned).map(|_| ())
+ );
+ }
+
+ assert_eq!(0, pool.alloc_area(nz::<4>(), unaligned)?.start);
+ assert_eq!(
+ Err(ENOSPC),
+ pool.alloc_area(nz::<5>(), unaligned).map(|_| ())
+ );
+
+ let head = pool.alloc_area(nz::<3>(), unaligned)?;
+ assert_eq!(0, head.start);
+ assert_eq!(
+ Err(ENOSPC),
+ pool.alloc_area(nz::<2>(), unaligned).map(|_| ())
+ );
+ assert_eq!(3, pool.alloc_area(nz::<1>(), unaligned)?.start);
+ Ok(())
+ }
+
+ #[test]
+ fn chid_area_aligned() -> Result {
+ let pool = KBox::pin_init(ChannelIdPool::new(16), GFP_KERNEL)?;
+ let unaligned = Alignment::new::<1>();
+ let align4 = Alignment::new::<4>();
+
+ // Alloc 0 so the first fit for the next area is unaligned.
+ let pad = pool.alloc_area(nz::<1>(), unaligned)?;
+ assert_eq!(0, pad.start);
+
+ let a = pool.alloc_area(nz::<4>(), align4)?;
+ assert_eq!(4, a.start);
+
+ // The area skipped over by the aligned allocation should still be available.
+ let b = pool.alloc_area(nz::<1>(), unaligned)?;
+ assert_eq!(1, b.start);
+
+ let c = pool.alloc_area(nz::<8>(), Alignment::new::<8>())?;
+ assert_eq!(8, c.start);
+
+ // Only 2 IDs left.
+ assert_eq!(Err(ENOSPC), pool.alloc_area(nz::<4>(), align4).map(|_| ()));
+ assert_eq!(
+ Err(ENOSPC),
+ pool.alloc_area(nz::<1>(), Alignment::new::<32>())
+ .map(|_| ())
+ );
+
+ assert_eq!(2, pool.alloc_area(nz::<2>(), unaligned)?.start);
+ Ok(())
+ }
+}
--
2.55.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v5 5/5] gpu: nova-core: add ChannelIdPool
2026-08-12 8:51 ` [PATCH v5 5/5] gpu: nova-core: add ChannelIdPool Eliot Courtney
@ 2026-08-12 22:18 ` Yury Norov
2026-08-13 7:31 ` Eliot Courtney
0 siblings, 1 reply; 20+ messages in thread
From: Yury Norov @ 2026-08-12 22:18 UTC (permalink / raw)
To: Eliot Courtney
Cc: Alice Ryhl, Burak Emir, Yury Norov, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, David Airlie, Simona Vetter,
Greg Kroah-Hartman, John Hubbard, Alistair Popple, Timur Tabi,
Zhi Wang, rust-for-linux, linux-kernel, nova-gpu, dri-devel
On Wed, Aug 12, 2026 at 05:51:25PM +0900, Eliot Courtney wrote:
> Add `ChannelIdPool` which adds automatic tracking and releasing of
> channel IDs on top of `IdPool`. This is necessary for apportioning
> ranges of channel IDs to be used in e.g. vGPU.
>
> Channel IDs are allocated as a contiguous sequence with a specific
> length and sometimes a specific alignment [1] for vGPU. The ID space is
> small (limited to 2048) and allocation is not on a hot path, so a
> bitmap-backed `IdPool` is a better fit than IDA/xarray (which allocate a
> single ID within a range, not a contiguous sequence) or a maple tree
> (where aligned allocation needs an alloc_range()+erase() retry loop that
> essentially reimplements bitmap_find_next_zero_area()) [2]. It is
> also faster than maple tree [3].
>
> Link: https://lore.kernel.org/all/84bc8bd2-e292-4b84-9580-a1b5df4c5bdc@nvidia.com/ # [1]
> Link: https://lore.kernel.org/all/20260710-chid-maple-v1-1-4ee869055268@nvidia.com/ # [2]
> Link: https://lore.kernel.org/all/20260717053241.916441-1-ynorov@nvidia.com/ # [3]
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
> drivers/gpu/nova-core/gpu.rs | 2 +
> drivers/gpu/nova-core/gpu/channel.rs | 180 +++++++++++++++++++++++++++++++++++
> 2 files changed, 182 insertions(+)
>
> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> index 42a4cd7971fa..66ea697a89f8 100644
> --- a/drivers/gpu/nova-core/gpu.rs
> +++ b/drivers/gpu/nova-core/gpu.rs
> @@ -33,6 +33,8 @@
> vgpu::VgpuManager, //
> };
>
> +#[cfg_attr(not(CONFIG_KUNIT = "y"), expect(dead_code))]
> +mod channel;
> mod hal;
>
> macro_rules! define_chipset {
> diff --git a/drivers/gpu/nova-core/gpu/channel.rs b/drivers/gpu/nova-core/gpu/channel.rs
> new file mode 100644
> index 000000000000..b755d2184aee
> --- /dev/null
> +++ b/drivers/gpu/nova-core/gpu/channel.rs
> @@ -0,0 +1,180 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
> +
> +//! Channel ID allocation.
> +
> +use core::{
> + num::NonZero,
> + ops::{
> + Deref,
> + Range, //
> + }, //
> +};
> +
> +use kernel::{
> + id_pool::IdPool,
> + prelude::*,
> + ptr::Alignment,
> + sync::{
> + new_mutex,
> + Mutex, //
> + }, //
> +};
> +
> +/// Pool for tracking reservations of channel IDs.
> +#[pin_data]
> +pub(crate) struct ChannelIdPool {
> + #[pin]
> + inner: Mutex<IdPool>,
> + num_chids: usize,
> +}
> +
> +impl ChannelIdPool {
> + /// Creates a pool managing `num_chids` channel IDs.
> + pub(crate) fn new(num_chids: usize) -> impl PinInit<Self, Error> {
> + try_pin_init!(Self {
> + inner <- new_mutex!(IdPool::with_capacity(num_chids, GFP_KERNEL)?),
> + num_chids,
> + })
> + }
> +
> + /// Reserves a contiguous area of `count` channel IDs starting at a multiple of `align`,
> + /// returning a guard that releases the area on drop.
> + pub(crate) fn alloc_area(
> + &self,
> + count: NonZero<usize>,
OK, here you use NonZero. Please do that in the lowest layer.
> + align: Alignment,
> + ) -> Result<ChannelIdArea<'_>> {
> + let mut ids = self.inner.lock();
> + let area = ids.find_unused_area(0, count, align).ok_or(ENOSPC)?;
> +
> + // If the pool is small, the backing bitmap may be rounded up to a larger size.
Not sure I understand this language. Your ID pool is a fixed-size. Or
do you mean something else?
> + if area.range().end > self.num_chids {
> + return Err(ENOSPC);
> + }
> + Ok(ChannelIdArea {
> + pool: self,
> + range: area.acquire(),
> + })
> + }
> +}
> +
> +/// A reserved contiguous area of channel IDs.
> +///
> +/// Releases the whole area back to its [`ChannelIdPool`] when dropped. Releasing locks a
> +/// sleeping [`Mutex`], so the area must be dropped in a context that is allowed to sleep.
> +#[must_use = "the channel ID area is released immediately when unused"]
> +pub(crate) struct ChannelIdArea<'a> {
> + pool: &'a ChannelIdPool,
> + range: Range<usize>,
> +}
> +
> +impl Drop for ChannelIdArea<'_> {
> + fn drop(&mut self) {
> + self.pool.inner.lock().release_area(&self.range);
> + }
> +}
> +
> +impl Deref for ChannelIdArea<'_> {
> + type Target = Range<usize>;
> +
> + fn deref(&self) -> &Self::Target {
> + &self.range
> + }
> +}
> +
> +#[kunit_tests(nova_core_channel)]
> +mod tests {
> + use super::*;
> +
> + const fn nz<const N: usize>() -> NonZero<usize> {
> + const { NonZero::new(N).unwrap() }
> + }
> +
> + #[test]
> + fn chid_area() -> Result {
> + let pool = KBox::pin_init(ChannelIdPool::new(2048), GFP_KERNEL)?;
> + let unaligned = Alignment::new::<1>();
> +
> + let first = pool.alloc_area(nz::<48>(), unaligned)?;
> + assert_eq!(0, first.start);
> + assert_eq!(48, first.len());
> + assert_eq!(48, first.end);
> +
> + let second = pool.alloc_area(nz::<48>(), unaligned)?;
> + assert!(first.end <= second.start || second.end <= first.start);
> +
> + let first_start = first.start;
> + drop(first);
You test the drop() only once. Can you add more tests? At least, make
sure that 2 allocs followed by 2 drops ends up with an empty pool.
> + assert_eq!(first_start, pool.alloc_area(nz::<48>(), unaligned)?.start);
> + Ok(())
> + }
> +
> + #[test]
> + fn chid_bounded_by_num_chids() -> Result {
> + let pool = KBox::pin_init(ChannelIdPool::new(4), GFP_KERNEL)?;
> + let unaligned = Alignment::new::<1>();
> +
> + {
> + let a = pool.alloc_area(nz::<1>(), unaligned)?;
> + let b = pool.alloc_area(nz::<1>(), unaligned)?;
> + let c = pool.alloc_area(nz::<1>(), unaligned)?;
> + let d = pool.alloc_area(nz::<1>(), unaligned)?;
OK, here your alloc_area() means the find + alloc, and it returns
a Range - not area.
To me it looks like the intermediate UnusedArea layer is excessive.
If you just do find + alloc in this pool.alloc_area(), you seemingly
don't need the UnusedArea.
Can you try without it, please?
> + assert_eq!(0, a.start);
> + assert_eq!(1, b.start);
> + assert_eq!(2, c.start);
> + assert_eq!(3, d.start);
> + assert_eq!(
> + Err(ENOSPC),
> + pool.alloc_area(nz::<1>(), unaligned).map(|_| ())
> + );
> + }
> +
> + assert_eq!(0, pool.alloc_area(nz::<4>(), unaligned)?.start);
> + assert_eq!(
> + Err(ENOSPC),
> + pool.alloc_area(nz::<5>(), unaligned).map(|_| ())
> + );
> +
> + let head = pool.alloc_area(nz::<3>(), unaligned)?;
> + assert_eq!(0, head.start);
> + assert_eq!(
> + Err(ENOSPC),
> + pool.alloc_area(nz::<2>(), unaligned).map(|_| ())
> + );
> + assert_eq!(3, pool.alloc_area(nz::<1>(), unaligned)?.start);
> + Ok(())
> + }
> +
> + #[test]
> + fn chid_area_aligned() -> Result {
> + let pool = KBox::pin_init(ChannelIdPool::new(16), GFP_KERNEL)?;
> + let unaligned = Alignment::new::<1>();
> + let align4 = Alignment::new::<4>();
> +
> + // Alloc 0 so the first fit for the next area is unaligned.
> + let pad = pool.alloc_area(nz::<1>(), unaligned)?;
> + assert_eq!(0, pad.start);
> +
> + let a = pool.alloc_area(nz::<4>(), align4)?;
> + assert_eq!(4, a.start);
> +
> + // The area skipped over by the aligned allocation should still be available.
> + let b = pool.alloc_area(nz::<1>(), unaligned)?;
> + assert_eq!(1, b.start);
> +
> + let c = pool.alloc_area(nz::<8>(), Alignment::new::<8>())?;
Is it possible to make it somehow simpler:
let c = pool.alloc_area(8, 8)?;
All the parameters checking must be a part of implementations, not the
interface.
We had a very similar discussion in the bitfields implementation thread,
and many people in CC list of this thread spent quite a long time to find
a way from:
let color = Rgb::default()
.set_red(Bounded::<u16, _>::new::<0x10>())
.set_green(Bounded::<u16, _>::new::<0x1f>())
.set_blue(Bounded::<u16, _>::new::<0x18>());
to:
let color = Rgb::default().
.set_red(0x10)
.set_green(0x1f)
.set_blue(0x18)
Can you do the same here? Please refer:
https://lore.kernel.org/all/aXCZeVqkDrBWr1uq@yury/
> + assert_eq!(8, c.start);
> +
> + // Only 2 IDs left.
> + assert_eq!(Err(ENOSPC), pool.alloc_area(nz::<4>(), align4).map(|_| ()));
> + assert_eq!(
> + Err(ENOSPC),
> + pool.alloc_area(nz::<1>(), Alignment::new::<32>())
> + .map(|_| ())
> + );
> +
> + assert_eq!(2, pool.alloc_area(nz::<2>(), unaligned)?.start);
> + Ok(())
> + }
> +}
>
> --
> 2.55.0
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 5/5] gpu: nova-core: add ChannelIdPool
2026-08-12 22:18 ` Yury Norov
@ 2026-08-13 7:31 ` Eliot Courtney
2026-08-13 18:32 ` Yury Norov
0 siblings, 1 reply; 20+ messages in thread
From: Eliot Courtney @ 2026-08-13 7:31 UTC (permalink / raw)
To: Yury Norov, Eliot Courtney
Cc: Alice Ryhl, Burak Emir, Yury Norov, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, David Airlie, Simona Vetter,
Greg Kroah-Hartman, John Hubbard, Alistair Popple, Timur Tabi,
Zhi Wang, rust-for-linux, linux-kernel, nova-gpu, dri-devel,
dri-devel
On Thu Aug 13, 2026 at 7:18 AM JST, Yury Norov wrote:
> On Wed, Aug 12, 2026 at 05:51:25PM +0900, Eliot Courtney wrote:
>> Add `ChannelIdPool` which adds automatic tracking and releasing of
>> channel IDs on top of `IdPool`. This is necessary for apportioning
>> ranges of channel IDs to be used in e.g. vGPU.
>>
>> Channel IDs are allocated as a contiguous sequence with a specific
>> length and sometimes a specific alignment [1] for vGPU. The ID space is
>> small (limited to 2048) and allocation is not on a hot path, so a
>> bitmap-backed `IdPool` is a better fit than IDA/xarray (which allocate a
>> single ID within a range, not a contiguous sequence) or a maple tree
>> (where aligned allocation needs an alloc_range()+erase() retry loop that
>> essentially reimplements bitmap_find_next_zero_area()) [2]. It is
>> also faster than maple tree [3].
>>
>> Link: https://lore.kernel.org/all/84bc8bd2-e292-4b84-9580-a1b5df4c5bdc@nvidia.com/ # [1]
>> Link: https://lore.kernel.org/all/20260710-chid-maple-v1-1-4ee869055268@nvidia.com/ # [2]
>> Link: https://lore.kernel.org/all/20260717053241.916441-1-ynorov@nvidia.com/ # [3]
>> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
>> ---
>> drivers/gpu/nova-core/gpu.rs | 2 +
>> drivers/gpu/nova-core/gpu/channel.rs | 180 +++++++++++++++++++++++++++++++++++
>> 2 files changed, 182 insertions(+)
>>
>> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
>> index 42a4cd7971fa..66ea697a89f8 100644
>> --- a/drivers/gpu/nova-core/gpu.rs
>> +++ b/drivers/gpu/nova-core/gpu.rs
>> @@ -33,6 +33,8 @@
>> vgpu::VgpuManager, //
>> };
>>
>> +#[cfg_attr(not(CONFIG_KUNIT = "y"), expect(dead_code))]
>> +mod channel;
>> mod hal;
>>
>> macro_rules! define_chipset {
>> diff --git a/drivers/gpu/nova-core/gpu/channel.rs b/drivers/gpu/nova-core/gpu/channel.rs
>> new file mode 100644
>> index 000000000000..b755d2184aee
>> --- /dev/null
>> +++ b/drivers/gpu/nova-core/gpu/channel.rs
>> @@ -0,0 +1,180 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
>> +
>> +//! Channel ID allocation.
>> +
>> +use core::{
>> + num::NonZero,
>> + ops::{
>> + Deref,
>> + Range, //
>> + }, //
>> +};
>> +
>> +use kernel::{
>> + id_pool::IdPool,
>> + prelude::*,
>> + ptr::Alignment,
>> + sync::{
>> + new_mutex,
>> + Mutex, //
>> + }, //
>> +};
>> +
>> +/// Pool for tracking reservations of channel IDs.
>> +#[pin_data]
>> +pub(crate) struct ChannelIdPool {
>> + #[pin]
>> + inner: Mutex<IdPool>,
>> + num_chids: usize,
>> +}
>> +
>> +impl ChannelIdPool {
>> + /// Creates a pool managing `num_chids` channel IDs.
>> + pub(crate) fn new(num_chids: usize) -> impl PinInit<Self, Error> {
>> + try_pin_init!(Self {
>> + inner <- new_mutex!(IdPool::with_capacity(num_chids, GFP_KERNEL)?),
>> + num_chids,
>> + })
>> + }
>> +
>> + /// Reserves a contiguous area of `count` channel IDs starting at a multiple of `align`,
>> + /// returning a guard that releases the area on drop.
>> + pub(crate) fn alloc_area(
>> + &self,
>> + count: NonZero<usize>,
>
> OK, here you use NonZero. Please do that in the lowest layer.
Will do~
>
>> + align: Alignment,
>> + ) -> Result<ChannelIdArea<'_>> {
>> + let mut ids = self.inner.lock();
>> + let area = ids.find_unused_area(0, count, align).ok_or(ENOSPC)?;
>> +
>> + // If the pool is small, the backing bitmap may be rounded up to a larger size.
>
> Not sure I understand this language. Your ID pool is a fixed-size. Or
> do you mean something else?
Yeah this is a little confusing. The reason this exists is because
`IdPool::with_capacity` will round up the size to
`BitmapVec::MAX_INLINE_LEN`. For binder, there isn't a natural limit to
the ID space IIUC so it's not a problem there.
Anyway, in this case, IdPool can return an area that is outside of the
original capacity you gave to `IdPool::with_capacity`, hence this check.
But, I had a closer look at IdPool::with_capacity, and I can't see a
good reason for why it's doing this adjusting to a min of
`BitmapVec::MAX_INLINE_LEN`. So let me try to instead remove that
behaviour.
>
>> + if area.range().end > self.num_chids {
>> + return Err(ENOSPC);
>> + }
>> + Ok(ChannelIdArea {
>> + pool: self,
>> + range: area.acquire(),
>> + })
>> + }
>> +}
>> +
>> +/// A reserved contiguous area of channel IDs.
>> +///
>> +/// Releases the whole area back to its [`ChannelIdPool`] when dropped. Releasing locks a
>> +/// sleeping [`Mutex`], so the area must be dropped in a context that is allowed to sleep.
>> +#[must_use = "the channel ID area is released immediately when unused"]
>> +pub(crate) struct ChannelIdArea<'a> {
>> + pool: &'a ChannelIdPool,
>> + range: Range<usize>,
>> +}
>> +
>> +impl Drop for ChannelIdArea<'_> {
>> + fn drop(&mut self) {
>> + self.pool.inner.lock().release_area(&self.range);
>> + }
>> +}
>> +
>> +impl Deref for ChannelIdArea<'_> {
>> + type Target = Range<usize>;
>> +
>> + fn deref(&self) -> &Self::Target {
>> + &self.range
>> + }
>> +}
>> +
>> +#[kunit_tests(nova_core_channel)]
>> +mod tests {
>> + use super::*;
>> +
>> + const fn nz<const N: usize>() -> NonZero<usize> {
>> + const { NonZero::new(N).unwrap() }
>> + }
>> +
>> + #[test]
>> + fn chid_area() -> Result {
>> + let pool = KBox::pin_init(ChannelIdPool::new(2048), GFP_KERNEL)?;
>> + let unaligned = Alignment::new::<1>();
>> +
>> + let first = pool.alloc_area(nz::<48>(), unaligned)?;
>> + assert_eq!(0, first.start);
>> + assert_eq!(48, first.len());
>> + assert_eq!(48, first.end);
>> +
>> + let second = pool.alloc_area(nz::<48>(), unaligned)?;
>> + assert!(first.end <= second.start || second.end <= first.start);
>> +
>> + let first_start = first.start;
>> + drop(first);
>
> You test the drop() only once. Can you add more tests? At least, make
> sure that 2 allocs followed by 2 drops ends up with an empty pool.
Yerp good idea. Thanks!
>
>> + assert_eq!(first_start, pool.alloc_area(nz::<48>(), unaligned)?.start);
>> + Ok(())
>> + }
>> +
>> + #[test]
>> + fn chid_bounded_by_num_chids() -> Result {
>> + let pool = KBox::pin_init(ChannelIdPool::new(4), GFP_KERNEL)?;
>> + let unaligned = Alignment::new::<1>();
>> +
>> + {
>> + let a = pool.alloc_area(nz::<1>(), unaligned)?;
>> + let b = pool.alloc_area(nz::<1>(), unaligned)?;
>> + let c = pool.alloc_area(nz::<1>(), unaligned)?;
>> + let d = pool.alloc_area(nz::<1>(), unaligned)?;
>
> OK, here your alloc_area() means the find + alloc, and it returns
> a Range - not area.
>
> To me it looks like the intermediate UnusedArea layer is excessive.
> If you just do find + alloc in this pool.alloc_area(), you seemingly
> don't need the UnusedArea.
>
> Can you try without it, please?
Yes, you're correct that `UnusedArea` layer isn't strictly required. We
are using it once to handle the case when IdPool gives us back something
that is outside of what we originally requested. But we could also
handle this just by bitmap setting the range
`num_chids..pool.capacity()` on creation of `ChannelIdPool`, runtime
checking num_chids >= BitmapVec::MAX_INLINE_LEN, or changing IdPool to
always create a bitmap of the specified capacity (this is what I'll do
unless someone knows a good reason not to).
>
>> + assert_eq!(0, a.start);
>> + assert_eq!(1, b.start);
>> + assert_eq!(2, c.start);
>> + assert_eq!(3, d.start);
>> + assert_eq!(
>> + Err(ENOSPC),
>> + pool.alloc_area(nz::<1>(), unaligned).map(|_| ())
>> + );
>> + }
>> +
>> + assert_eq!(0, pool.alloc_area(nz::<4>(), unaligned)?.start);
>> + assert_eq!(
>> + Err(ENOSPC),
>> + pool.alloc_area(nz::<5>(), unaligned).map(|_| ())
>> + );
>> +
>> + let head = pool.alloc_area(nz::<3>(), unaligned)?;
>> + assert_eq!(0, head.start);
>> + assert_eq!(
>> + Err(ENOSPC),
>> + pool.alloc_area(nz::<2>(), unaligned).map(|_| ())
>> + );
>> + assert_eq!(3, pool.alloc_area(nz::<1>(), unaligned)?.start);
>> + Ok(())
>> + }
>> +
>> + #[test]
>> + fn chid_area_aligned() -> Result {
>> + let pool = KBox::pin_init(ChannelIdPool::new(16), GFP_KERNEL)?;
>> + let unaligned = Alignment::new::<1>();
>> + let align4 = Alignment::new::<4>();
>> +
>> + // Alloc 0 so the first fit for the next area is unaligned.
>> + let pad = pool.alloc_area(nz::<1>(), unaligned)?;
>> + assert_eq!(0, pad.start);
>> +
>> + let a = pool.alloc_area(nz::<4>(), align4)?;
>> + assert_eq!(4, a.start);
>> +
>> + // The area skipped over by the aligned allocation should still be available.
>> + let b = pool.alloc_area(nz::<1>(), unaligned)?;
>> + assert_eq!(1, b.start);
>> +
>> + let c = pool.alloc_area(nz::<8>(), Alignment::new::<8>())?;
>
> Is it possible to make it somehow simpler:
>
> let c = pool.alloc_area(8, 8)?;
>
> All the parameters checking must be a part of implementations, not the
> interface.
>
> We had a very similar discussion in the bitfields implementation thread,
> and many people in CC list of this thread spent quite a long time to find
> a way from:
>
> let color = Rgb::default()
> .set_red(Bounded::<u16, _>::new::<0x10>())
> .set_green(Bounded::<u16, _>::new::<0x1f>())
> .set_blue(Bounded::<u16, _>::new::<0x18>());
>
> to:
>
>
> let color = Rgb::default().
> .set_red(0x10)
> .set_green(0x1f)
> .set_blue(0x18)
>
> Can you do the same here? Please refer:
>
> https://lore.kernel.org/all/aXCZeVqkDrBWr1uq@yury/
I think that taking NonZero and Alignment here obviates the need for
checking the parameters, since they have their own guarantees (and Alice
recommended using Alignment on `Bitmap` too for this reason IIUC). Maybe
I am misundertanding but we spent a few iterations here adding
`Alignment` and `NonZero` on various parameters -- do you mean just
making ChannelIdPool::alloc_area work with a plain integer syntax? It's
possible to just take plain integers here and check, but I don't think
it's necessarily better.
W.r.t. the bitfield stuff, yeah I agree that was a good call since that
syntax was very verbose, and IIUC that was resolved by having e.g.
with_const_red::<0x10>(). The analogous change here would be to provide
const generic args, e.g. alloc_area_const::<size, align>() which could
be plain integers. But, in practice the arguments to alloc_area are
going to be runtime values (outside of tests) that the caller has
strictly more info about. Having the separate types (NonZero, Alignment)
also makes easier to not mix up the order. I can't think of a way to
remove this verbosity without just passing plain integer runtime values,
which IMO is not great.
>
>> + assert_eq!(8, c.start);
>> +
>> + // Only 2 IDs left.
>> + assert_eq!(Err(ENOSPC), pool.alloc_area(nz::<4>(), align4).map(|_| ()));
>> + assert_eq!(
>> + Err(ENOSPC),
>> + pool.alloc_area(nz::<1>(), Alignment::new::<32>())
>> + .map(|_| ())
>> + );
>> +
>> + assert_eq!(2, pool.alloc_area(nz::<2>(), unaligned)?.start);
>> + Ok(())
>> + }
>> +}
>>
>> --
>> 2.55.0
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 5/5] gpu: nova-core: add ChannelIdPool
2026-08-13 7:31 ` Eliot Courtney
@ 2026-08-13 18:32 ` Yury Norov
2026-08-13 20:20 ` Miguel Ojeda
2026-08-13 20:48 ` Danilo Krummrich
0 siblings, 2 replies; 20+ messages in thread
From: Yury Norov @ 2026-08-13 18:32 UTC (permalink / raw)
To: Eliot Courtney
Cc: Alice Ryhl, Burak Emir, Yury Norov, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, David Airlie, Simona Vetter,
Greg Kroah-Hartman, John Hubbard, Alistair Popple, Timur Tabi,
Zhi Wang, rust-for-linux, linux-kernel, nova-gpu, dri-devel,
dri-devel
On Thu, Aug 13, 2026 at 04:31:26PM +0900, Eliot Courtney wrote:
> On Thu Aug 13, 2026 at 7:18 AM JST, Yury Norov wrote:
> > On Wed, Aug 12, 2026 at 05:51:25PM +0900, Eliot Courtney wrote:
...
> >> + let c = pool.alloc_area(nz::<8>(), Alignment::new::<8>())?;
> >
> > Is it possible to make it somehow simpler:
> >
> > let c = pool.alloc_area(8, 8)?;
> >
> > All the parameters checking must be a part of implementations, not the
> > interface.
> >
> > We had a very similar discussion in the bitfields implementation thread,
> > and many people in CC list of this thread spent quite a long time to find
> > a way from:
> >
> > let color = Rgb::default()
> > .set_red(Bounded::<u16, _>::new::<0x10>())
> > .set_green(Bounded::<u16, _>::new::<0x1f>())
> > .set_blue(Bounded::<u16, _>::new::<0x18>());
> >
> > to:
> >
> >
> > let color = Rgb::default().
> > .set_red(0x10)
> > .set_green(0x1f)
> > .set_blue(0x18)
> >
> > Can you do the same here? Please refer:
> >
> > https://lore.kernel.org/all/aXCZeVqkDrBWr1uq@yury/
>
> I think that taking NonZero and Alignment here obviates the need for
> checking the parameters, since they have their own guarantees (and Alice
> recommended using Alignment on `Bitmap` too for this reason IIUC). Maybe
> I am misundertanding but we spent a few iterations here adding
> `Alignment` and `NonZero` on various parameters -- do you mean just
> making ChannelIdPool::alloc_area work with a plain integer syntax? It's
> possible to just take plain integers here and check, but I don't think
> it's necessarily better.
>
> W.r.t. the bitfield stuff, yeah I agree that was a good call since that
> syntax was very verbose, and IIUC that was resolved by having e.g.
> with_const_red::<0x10>(). The analogous change here would be to provide
> const generic args, e.g. alloc_area_const::<size, align>() which could
> be plain integers. But, in practice the arguments to alloc_area are
> going to be runtime values (outside of tests) that the caller has
> strictly more info about. Having the separate types (NonZero, Alignment)
> also makes easier to not mix up the order. I can't think of a way to
> remove this verbosity without just passing plain integer runtime values,
> which IMO is not great.
pub(crate) fn alloc_area(
&self,
count: usize,
align: usize,
) -> Result<ChannelIdArea<'_>> {
let count = NonZero::new(count).ok_or(EINVAL)?;
let align = Alignment::new_checked(align).ok_or(EINVAL)?;
let mut ids = self.inner.lock();
let area = ids.find_unused_area(0, count, align).ok_or(ENOSPC)?;
// If the pool is small, the backing bitmap may be rounded up to a larger size.
if area.range().end > self.num_chids {
return Err(ENOSPC);
}
Ok(ChannelIdArea {
pool: self,
range: area.acquire(),
})
}
let area = pool.alloc_area(8, 4)?;
See the difference? You still check the parameters, but don't make it
the part of interface.
And from practical perspective, your users simply call the function,
not tinkering around your 'safety measures'.
In the next version, if you drop the intermediate UnusedArea layer,
you may want to do a C-like check instead of creating new types,
because here you'll directly call C function. And it's completely OK.
Not OK is complicating interfaces and life of your users.
Thanks,
Yury
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 5/5] gpu: nova-core: add ChannelIdPool
2026-08-13 18:32 ` Yury Norov
@ 2026-08-13 20:20 ` Miguel Ojeda
2026-08-13 20:48 ` Danilo Krummrich
1 sibling, 0 replies; 20+ messages in thread
From: Miguel Ojeda @ 2026-08-13 20:20 UTC (permalink / raw)
To: Yury Norov
Cc: Eliot Courtney, Alice Ryhl, Burak Emir, Yury Norov, Miguel Ojeda,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Danilo Krummrich, Daniel Almeida,
Tamir Duberstein, Alexandre Courbot, Onur Özkan,
David Airlie, Simona Vetter, Greg Kroah-Hartman, John Hubbard,
Alistair Popple, Timur Tabi, Zhi Wang, rust-for-linux,
linux-kernel, nova-gpu, dri-devel, dri-devel
On Thu, Aug 13, 2026 at 8:32 PM Yury Norov <ynorov@nvidia.com> wrote:
>
> Not OK is complicating interfaces and life of your users.
No, we don't want to introduce interfaces with unconstrained integers,
especially so when we have already types that express what we want.
In other words, we don't want to introduce impossible states and error
paths that aren't needed -- we want to rule them out statically. That
is the point of all of what we are doing with Rust.
The unconstrained interface you are proposing is actually the
complicated one, because it means now you have to provide more
documentation about both the allowed input values and the new error
cases. Not only that, you have to provide more tests for all that. And
you have to duplicate information that types like `Alignment` already
explain if you click on them.
And on the callee side, similarly, if someone already has e.g. an
`Alignment`, then we don't want users to be forced to convert back to
an unconstrained integer just to call the function that requires an
alignment to begin with.
Now, if a function is truly something that we want to call with e.g.
integer literals all the time, then maybe it is worth a workaround --
but those cases are rare, and even then it is likely to just be a
wrapper to the normal interface.
Cheers,
Miguel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 5/5] gpu: nova-core: add ChannelIdPool
2026-08-13 18:32 ` Yury Norov
2026-08-13 20:20 ` Miguel Ojeda
@ 2026-08-13 20:48 ` Danilo Krummrich
2026-08-13 20:58 ` Gary Guo
2026-08-13 21:03 ` Yury Norov
1 sibling, 2 replies; 20+ messages in thread
From: Danilo Krummrich @ 2026-08-13 20:48 UTC (permalink / raw)
To: Yury Norov
Cc: Eliot Courtney, Alice Ryhl, Burak Emir, Yury Norov, Miguel Ojeda,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, David Airlie, Simona Vetter,
Greg Kroah-Hartman, John Hubbard, Alistair Popple, Timur Tabi,
Zhi Wang, rust-for-linux, linux-kernel, nova-gpu, dri-devel,
dri-devel
On Thu Aug 13, 2026 at 8:32 PM CEST, Yury Norov wrote:
> pub(crate) fn alloc_area(
> &self,
> count: usize,
> align: usize,
> ) -> Result<ChannelIdArea<'_>> {
> let count = NonZero::new(count).ok_or(EINVAL)?;
> let align = Alignment::new_checked(align).ok_or(EINVAL)?;
>
> let mut ids = self.inner.lock();
> let area = ids.find_unused_area(0, count, align).ok_or(ENOSPC)?;
>
> // If the pool is small, the backing bitmap may be rounded up to a larger size.
> if area.range().end > self.num_chids {
> return Err(ENOSPC);
> }
>
> Ok(ChannelIdArea {
> pool: self,
> range: area.acquire(),
> })
> }
>
> let area = pool.alloc_area(8, 4)?;
>
> See the difference? You still check the parameters, but don't make it
> the part of interface.
Miguel already replied to this, so just briefly adding to this.
We usually want the arguments to already carry the invariants we require. If we
make the arguments unconstrained, we may end up in situations where we already
have types that provide certain guarantees about value constraints and yet we
have to give up on them because the API takes unconstrained arguments and
revalidates.
The code from Eliot does actually already takes advantage of this. In
pool.alloc_area(nz::<8>(), Alignment::new::<8>())?;
both arguments are already validated at compile time, whereas with unconstrained
arguments we're left with a runtime check.
Yes, nz() does not actually validate it statically, but it easily could (and
probably should).
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 5/5] gpu: nova-core: add ChannelIdPool
2026-08-13 20:48 ` Danilo Krummrich
@ 2026-08-13 20:58 ` Gary Guo
2026-08-13 21:38 ` John Hubbard
2026-08-13 21:03 ` Yury Norov
1 sibling, 1 reply; 20+ messages in thread
From: Gary Guo @ 2026-08-13 20:58 UTC (permalink / raw)
To: Danilo Krummrich, Yury Norov
Cc: Eliot Courtney, Alice Ryhl, Burak Emir, Yury Norov, Miguel Ojeda,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, David Airlie, Simona Vetter,
Greg Kroah-Hartman, John Hubbard, Alistair Popple, Timur Tabi,
Zhi Wang, rust-for-linux, linux-kernel, nova-gpu, dri-devel,
dri-devel
On Thu Aug 13, 2026 at 9:48 PM BST, Danilo Krummrich wrote:
> On Thu Aug 13, 2026 at 8:32 PM CEST, Yury Norov wrote:
>> pub(crate) fn alloc_area(
>> &self,
>> count: usize,
>> align: usize,
>> ) -> Result<ChannelIdArea<'_>> {
>> let count = NonZero::new(count).ok_or(EINVAL)?;
>> let align = Alignment::new_checked(align).ok_or(EINVAL)?;
>>
>> let mut ids = self.inner.lock();
>> let area = ids.find_unused_area(0, count, align).ok_or(ENOSPC)?;
>>
>> // If the pool is small, the backing bitmap may be rounded up to a larger size.
>> if area.range().end > self.num_chids {
>> return Err(ENOSPC);
>> }
>>
>> Ok(ChannelIdArea {
>> pool: self,
>> range: area.acquire(),
>> })
>> }
>>
>> let area = pool.alloc_area(8, 4)?;
>>
>> See the difference? You still check the parameters, but don't make it
>> the part of interface.
>
> Miguel already replied to this, so just briefly adding to this.
>
> We usually want the arguments to already carry the invariants we require. If we
> make the arguments unconstrained, we may end up in situations where we already
> have types that provide certain guarantees about value constraints and yet we
> have to give up on them because the API takes unconstrained arguments and
> revalidates.
>
> The code from Eliot does actually already takes advantage of this. In
>
> pool.alloc_area(nz::<8>(), Alignment::new::<8>())?;
I have to say this looks quite ugly, compared to just `.alloc_area(8, 8)`. In
this case I'd go with something aesthetically better...
While keeping invariants with types is generally good, for primitives I'm never
a big fan of `NonZero` and `Bounded`... The turbofishes are just ugly and they
are must harder to use compared to raw primitives without typing a lot of
symbols.
Best,
Gary
>
> both arguments are already validated at compile time, whereas with unconstrained
> arguments we're left with a runtime check.
>
> Yes, nz() does not actually validate it statically, but it easily could (and
> probably should).
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 5/5] gpu: nova-core: add ChannelIdPool
2026-08-13 20:58 ` Gary Guo
@ 2026-08-13 21:38 ` John Hubbard
0 siblings, 0 replies; 20+ messages in thread
From: John Hubbard @ 2026-08-13 21:38 UTC (permalink / raw)
To: Gary Guo, Danilo Krummrich, Yury Norov
Cc: Eliot Courtney, Alice Ryhl, Burak Emir, Yury Norov, Miguel Ojeda,
Boqun Feng, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Daniel Almeida, Tamir Duberstein, Alexandre Courbot,
Onur Özkan, David Airlie, Simona Vetter, Greg Kroah-Hartman,
Alistair Popple, Timur Tabi, Zhi Wang, rust-for-linux,
linux-kernel, nova-gpu, dri-devel, dri-devel
On 8/13/26 1:58 PM, Gary Guo wrote:
> On Thu Aug 13, 2026 at 9:48 PM BST, Danilo Krummrich wrote:
>> On Thu Aug 13, 2026 at 8:32 PM CEST, Yury Norov wrote:
...
>> The code from Eliot does actually already takes advantage of this. In
>>
>> pool.alloc_area(nz::<8>(), Alignment::new::<8>())?;
>
> I have to say this looks quite ugly, compared to just `.alloc_area(8, 8)`. In
> this case I'd go with something aesthetically better...
In addition to aesthetics, a practical concern is that there is a
higher cognitive load to both reading and writing the longer form,
above. Perhaps that is just the price one must pay.
However, for primitive types, I am hearing (even from Miguel) that
there might be a way to get the static checking *and* an easier
interface.
Here's hoping you all can find a way to do that. :)
>
> While keeping invariants with types is generally good, for primitives I'm never
> a big fan of `NonZero` and `Bounded`... The turbofishes are just ugly and they
> are must harder to use compared to raw primitives without typing a lot of
> symbols.
thanks,
--
John Hubbard
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 5/5] gpu: nova-core: add ChannelIdPool
2026-08-13 20:48 ` Danilo Krummrich
2026-08-13 20:58 ` Gary Guo
@ 2026-08-13 21:03 ` Yury Norov
1 sibling, 0 replies; 20+ messages in thread
From: Yury Norov @ 2026-08-13 21:03 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Eliot Courtney, Alice Ryhl, Burak Emir, Yury Norov, Miguel Ojeda,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, David Airlie, Simona Vetter,
Greg Kroah-Hartman, John Hubbard, Alistair Popple, Timur Tabi,
Zhi Wang, rust-for-linux, linux-kernel, nova-gpu, dri-devel,
dri-devel
On Thu, Aug 13, 2026 at 10:48:50PM +0200, Danilo Krummrich wrote:
> On Thu Aug 13, 2026 at 8:32 PM CEST, Yury Norov wrote:
> > pub(crate) fn alloc_area(
> > &self,
> > count: usize,
> > align: usize,
> > ) -> Result<ChannelIdArea<'_>> {
> > let count = NonZero::new(count).ok_or(EINVAL)?;
> > let align = Alignment::new_checked(align).ok_or(EINVAL)?;
> >
> > let mut ids = self.inner.lock();
> > let area = ids.find_unused_area(0, count, align).ok_or(ENOSPC)?;
> >
> > // If the pool is small, the backing bitmap may be rounded up to a larger size.
> > if area.range().end > self.num_chids {
> > return Err(ENOSPC);
> > }
> >
> > Ok(ChannelIdArea {
> > pool: self,
> > range: area.acquire(),
> > })
> > }
> >
> > let area = pool.alloc_area(8, 4)?;
> >
> > See the difference? You still check the parameters, but don't make it
> > the part of interface.
>
> Miguel already replied to this, so just briefly adding to this.
>
> We usually want the arguments to already carry the invariants we require. If we
> make the arguments unconstrained, we may end up in situations where we already
> have types that provide certain guarantees about value constraints and yet we
> have to give up on them because the API takes unconstrained arguments and
> revalidates.
>
> The code from Eliot does actually already takes advantage of this. In
>
> pool.alloc_area(nz::<8>(), Alignment::new::<8>())?;
>
> both arguments are already validated at compile time, whereas with unconstrained
> arguments we're left with a runtime check.
>
> Yes, nz() does not actually validate it statically, but it easily could (and
> probably should).
Alright, I'm not against the static checks, and I don't insist on my
version. My complain is about readability and unnecessary complexity
for end user.
let's find a way to convert this beast:
pool.alloc_area(nz::<8>(), alignment::new::<8>())?;
to something more readable, ideally:
pool.alloc_area(8, 8)?;
We were able to do this for bitfields, and I don't think we should
give up here.
^ permalink raw reply [flat|nested] 20+ messages in thread