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

[Joel Fernandes]

Hi Eliot,

On 4/8/2026 9:26 AM, Eliot Courtney wrote:
> On Tue Apr 7, 2026 at 10:59 PM JST, Joel Fernandes wrote:
>> 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.
> 
> I had a go at this, you can see the branch here [1] - it might not be
> perfect, but I think the shape is directionally good. It's structured so
> the HEAD commit has the diff from the current approach to the
> parametrised approach. The main decision is where to do the type
> erasure, I chose in `Vmm` since it looks like the main top level API for
> this code, but could do `BarUser` instead. I think it's overall better.
> I also think Alex's point about associated types making it easier to use
> the appropriate Bounded type is a good one.
> 
> [1]: https://github.com/Edgeworth/linux/commits/review/nova-mm-v10/
First, thanks for the effort. I looked through this, its pretty much what I
had before when I used traits. I don't think it is better to be honest. In
fact your version is worse, it adds many new types and things like the
following which I did not need before.

To put it mildly, the following suggestion should not be anywhere near my code:

/// Type-erased MMU-specific [`Vmm`] implementations.
enum VmmInner {
    /// `Vmm` implementation for MMU v2.
    V2(VmmImpl<MmuV2>),
    /// `Vmm` implementation for MMU v3.
    V3(VmmImpl<MmuV3>),
}

/// MMU-specific [`Vmm`] implementation.
struct VmmImpl<M: Mmu> {

Seriously, I have to pass on this. :-)

And, you unfortunately seem to have ignored my point about requiring 4 NEW
traits (Mmu, PteOps, PdeOps, DualPdeOps etc), which I did not need before.
So you're making the code much much worse than before actually. We don't
new traits and types pointlessly.

The only positive thing I could take away from your diff is the following
(I thought I had already done that, but I'll double check).

-    fn level_index(&self, level: u64) -> u64 {
+    fn level_index(&self, level: PageTableLevel) -> u64 {

Also you're parametrizing VirtualAddress as well which I did not have before:

-     let va = VirtualAddress::from(vfn);
+     let va = M::va(VirtualAddress::from(vfn));

This is another step back.

> I also think Alex's point about associated types making it easier to use
> the appropriate Bounded type is a good one.

I will reply to Alex thread, separately. I have some good data that should
hopefully convince you and Alex that my approach in this patch is better
(Version struct based dispatch than monomorphization). I would emphasize,
as we all know, that we should make optimizations and changes based on real
data and proper technical arguments so in the spirit of that, I have
collected data with both approaches and I will reply to Alex's email with
all that in there.

Also, the bounded types usage is orthogonal to version-parameterization.
That can be done regardless, we already use bitfield macro in this code and
can use bounded types within that if needed to restrict type creation. So I
don't think we should mix the 2 concepts "bounded types" and
"parameterization".

thanks,

--
Joel Fernandes