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

On Fri Aug 14, 2026 at 6:03 AM JST, Yury Norov wrote:
> 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.


At the moment I can think of two ideas.

1. Provide a nz! macro to create constant non-zero things. Provide some
   constants for common alignments

Potentially, we could use build_assert! for cases it is provably
non-zero but not a literal (although tbh not sure this is a good idea -
at least not do this initially). 

So either: nz!: const { NonZero::new(value).unwrap() }, or, nz!: some
build assert gated construction of NonZero. A general nz! macro could
also help ergonomics for other use cases too.

That looks like this:

pool.alloc_area(nz!(8), Alignment::AL_8)?;

I actually do not like pool.alloc_area(8, 8) because you can't tell what
the parameters mean. Having to write `Alignment` is useful, IMO.

2. The same solution for bitfield, whcih was to provide a separate const
   method that takes the values as const generics. That looks like this:

pool.alloc_area_const::<8, 8>()?;

That said I think this form would mostly only be used in tests.

I personally prefer option #1 because it will make NonZero easier to use
for everyone (I already have a few copies of the nz::<>() function in
unit tests) and because it self-describes what the parameters do. And it
doesn't have any turbofish.

What do you reckon?