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

["Eliot Courtney" <ecourtney@nvidia.com>]

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