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

[Yury Norov <ynorov@nvidia.com>]

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