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

["Gary Guo" <gary@garyguo.net>]

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).