On Tue Mar 17, 2026 at 3:43 AM JST, Joel Fernandes wrote:
<snip>
>>> +//! ptr::Alignment,
>>> +//! sizes::*, //
>>> +//! };
>>> +//!
>>> +//! // Create a 1GB buddy allocator with 4KB minimum chunk size.
>>> +//! let buddy = GpuBuddy::new(GpuBuddyParams {
>>> +//! base_offset: 0,
>>> +//! physical_memory_size: SZ_1G as u64,
>>> +//! chunk_size: SZ_4K,
>>
>> `chunk_size` is an interesting case. The C API uses a `u64`, but I think
>> we can reasonably consider that we won't ever need chunks larger than
>> 4GB (or can we :O). I'm actually ok with using a `usize` for this one.
>>
>> One of the first things the C code does is throwing an error if it is
>> not a power of 2, so maybe we can even request an `Alignment`?
>>
>> I'm a bit torn as to whether we should use a `u64` to conform with the C
>> API, but doing so would mean we cannot use an `Alignment`...
>
> I prefer to keep it simple and use `usize` for now. I cannot imagine
> chunk_size ever exceeding 4GB, and given our stance on rejecting invalid
> inputs, this sounds reasonable. Regarding `Alignment`, I still prefer
> `usize` here since it makes the caller-side simpler and as you noted the
> C code already does error-checking. Let's revisit if needed once this
> lands.
I would like to insist a bit here re: Alignment. We are not trying to
make the caller side simpler - we are trying to make it correct and to
turn runtime failures into build-time ones as much as possible. This is
a good case for that.
The additional burden, if you can call it so, to the caller is just in
the initial call to `GpuBuddy::new` - i.e. typically once per driver.
The most important API, `alloc_blocks`, will be unaffected - and
actually this one already has one `Alignment` as a parameter, for the
minimal block size! So if anything it would be illogical not to follow
suit on the buddy's `block_size` parameter.
<snip>
>>> +//! let (mut count, mut total) = (0u32, 0usize);
>>> +//! for block in fragmented.iter() {
>>> +//! assert_eq!(block.size(), SZ_4M);
>>> +//! total += block.size();
>>> +//! count += 1;
>>> +//! }
>>
>> Note that we can avoid mutable variables with this:
>>
>> //! let total_size: usize = fragmented.iter()
>> //! .inspect(|block| assert_eq!(block.size(), SZ_4M))
>> //! .map(|block| block.size())
>> //! .sum();
>> //! assert_eq!(total_size, SZ_8M);
>> //! assert_eq!(fragmented.iter().count(), 2);
>>
>> But your call as to whether this is an improvement.
>
> I feel the current for-loop version is slightly more readable,
> especially in a doc example aimed at new users, so I'd like to keep
> it as-is.
Sounds good.
<snip>
>> For this parameter I am pretty sure we want to conform to the C API and
>> use a `u64` - there is no benefit in not doing so, and buffers larger
>> than 4GB *are* a reality nowadays, (maybe not for graphics, but this
>> will also be used in compute scenarios).
>
> Agreed. Though, note this adds 7 more `as` usages, but I guess there's
> nothing we can do till the IntoSafe stuff is moved to core rust, I think.
How so? This parameter is just passed to the C function.
If you are referring to the examples, then yes that's unfortunate but
there are at least two ways where this could be eventually fixed (John's
SZ_* rework and the IntoSafe stuff), so we can update these when either
lands.
