Re: [PATCH v5 5/5] gpu: nova-core: add ChannelIdPool

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/[email protected]/ # [1]
> Link: https://lore.kernel.org/all/[email protected]/ # [2]
> Link: https://lore.kernel.org/all/[email protected]/ # [3]
> Signed-off-by: Eliot Courtney <[email protected]>
> ---
>  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