Re: [PATCH v10 12/21] gpu: nova-core: mm: Add unified page table entry wrapper enums

[Joel Fernandes]

Hi Eliot,

On 4/7/2026 9:42 AM, Eliot Courtney wrote:
> On Tue Apr 7, 2026 at 6:55 AM JST, Joel Fernandes wrote:
>>>> +    /// Compute upper bound on page table pages needed for `num_virt_pages`.
>>>> +    ///
>>>> +    /// Walks from PTE level up through PDE levels, accumulating the tree.
>>>> +    pub(crate) fn pt_pages_upper_bound(&self, num_virt_pages: usize) -> usize {
>>>> +        let mut total = 0;
>>>> +
>>>> +        // PTE pages at the leaf level.
>>>> +        let pte_epp = self.entries_per_page(self.pte_level());
>>>> +        let mut pages_at_level = num_virt_pages.div_ceil(pte_epp);
>>>> +        total += pages_at_level;
>>>> +
>>>> +        // Walk PDE levels bottom-up (reverse of pde_levels()).
>>>> +        for &level in self.pde_levels().iter().rev() {
>>>> +            let epp = self.entries_per_page(level);
>>>> +
>>>> +            // How many pages at this level do we need to point to
>>>> +            // the previous pages_at_level?
>>>> +            pages_at_level = pages_at_level.div_ceil(epp);
>>>> +            total += pages_at_level;
>>>> +        }
>>>> +
>>>> +        total
>>>> +    }
>>>> +}
>>>> +
>>>
>>> We have a lot of matches on the MMU version here (and below in Pte, Pde,
>>> DualPde). What about making MmuVersion into a trait (e.g. Mmu) with
>>> associated types for Pte, Pde, DualPde which can implement traits
>>> defining their common operations too?
>>
>> I coded this up and it did not look pretty, there's not much LOC savings and the
>> code becomes harder to read because of parametrization of several functions. Also:
> 
> Thanks for looking into it. Sorry to be a bother, but would you have a
> branch around with the code? I'm curious what didn't look good about it.

Sorry but I already mentioned that above, the parameterizing of dozens of
function call sites, 3-4 new traits (because each struct like
Pte/Pde/DualPde etc each need their own trait which different MMU versions
implement) etc. The code because hard to read and readability is the top
critical criteria for me - I am personally strictly against "Lets use shiny
features in language at the cost of making code unreadable". Because that
translates into bugs and nightmare for maintainability.

I don't have the code at the moment, but if you still want to spend on time
on this direction, feel free to share a tree. I am happy to take a look.
>>> Then you can parameterise Vmm/PtWalk on this type.
>>
>> The match still to be done somewhere, so you end up matching on chipset to call
>> the correct parametrized functions versus just passing in the parameter or
>> chipset down, in some cases.
>>
>> For now I am inclined to leave it as is. Also there's a Rust pitfall we all
>> learnt during the turing and other patch reviews, sometimes doing a bunch of
>> matches is good especially if the number of variants are expected to be fixed
>> (in the mm case, version 2 and version 3). Traits have some disadvantages too,
>> example dyn traits have to heap-allocated, parametrizing can increase code size
>> (due to monomorphization) etc.
> 
> Yeah, it's just this is a lot of matches in a lot of places. And we have
> ver2 / ver3 specific code leaking into the general pagetable.rs file. So

That's not a leak, that's by design. pagetable.rs is where the matches are
centralized, most of the code changes here on out should happen outside of
this file.

31 out of 42 matches in the mm code are in pagetable.rs, so it is already
centralized.

> it would be really nice if we could find a way to improve this specific
> aspect. We can reduce the match to happening in just one file. 

Assuming we know what we're improving. ;-)

> You can> avoid heap allocation if you would like by making Vmm an enum,
> for example, and doing the match based dispatch there at the top of the
> API tree, rather than at the bottom where it fans out into a lot more
> locations.

heap allocation is not always free, this code sensitive to dynamic
allocations in the kernel, due to MM reclaim and locking. I would like to
keep it simple.

thanks,

--
Joel Fernandes