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

Joel Fernandes
2026-04-13T20:04:51Z

Hi Alex,

On 4/9/2026 6:56 AM, Alexandre Courbot wrote:
> On Thu Apr 9, 2026 at 5:19 AM JST, Joel Fernandes wrote:
>> Hi Alex, Eliot, Danilo,
>>
>> Thanks for taking a look. Let me respond to the specific points below.
>>
>> On Wed, 08 Apr 2026, Alexandre Courbot wrote:
>>> After a quick look I'd say that having a trait here would actually be
>>> *good* for correctness and maintainability.
>>>
>>> The current design implies that every operation on a page table (most
>>> likely using the walker) goes through a branching point. Just looking at
>>> `PtWalk::read_pte_at_level`, there are already at least 6
>>> `if version == 2 { } else { }` branches that all resolve to the same
>>> result. Include walking down the PDEs and you have at least a dozen of
>>> these just to resolve a virtual address. I know CPUs are fast, but this
>>> is still wasted cycles for no good reason.
>>
>> I did some measurements and there is no notieceable difference in both
>> approaches. I ran perf and loaded nova with self-tests running. The extra
>> potential branching is lost in the noise. In both cases, loading nova and
>> running the self-tests has ~119.7M branch instructions on my Ampere. The total
>> instruction count is also identical (~615M).
>
> That's expected - as I said, CPUs are fast - and that's also not my
> point. My issue is that we are doing countless tests that all resolve to
> the code path, a code path that is already known during probe time.
> That's a huge code smell.
>
> When we create the GPU, we know whether we will be using v2 or v3 page
> tables. That we need to test that again 12 times per address resolution
> is a design issue, irrespective of performance. There are 24 version
> match sites in patch 12 alone.
>
> And that's precisely a good justification for using monomorphization. v2
> and v3 are technically two different page table implementations (they
> even have their own distinct module in your series), we just use
> generics to factorize the (source) code a bit.

To repeat my point though, the extra tests don't complicate the code or change
performance. The bottleneck is here is MMIO, not CPU cycles (and arguably
testing shows no change in CPU cycles either). So I tend to prefer cleaner code
(which in my view is the few matches in pagetable.rs versus templatizing lots of
code). I do see your point of view though and I Ok with (trying out) the
alternative as well, and it seems to be what mostly everyone wants. It is a
little more boiler plate in higher layers which are not exposed to version
matching, but not the end of the world :).
>> I measured like this:
>> perf stat -e
>> branches,branch-misses,cache-references,cache-misses,instructions,cycles --
>> modprobe nova_core
>>
>> So I think the branching argument is not a strong one. I also did more
>> measurements and the dominant time taken is MMIO. During the map prep and
>> execute, page table walks are done. A TLB flush alone costs ~1.4 microseconds.
>> And PRAMIN BAR0 writes to write the PTE is also about 1 microsecond. Considering
>> this, I don't think the extra branching argument holds (even without branch
>> prediction and speculation).
>>
>> Also some branches cannot be eliminated even with parameterization:
>>
>> if level == self.mmu_version.dual_pde_level() {
>> // 128-bit dual PDE read
>> } else {
>> // Regular 64-bit PDE read
>> }
>>
>> This isn't really a version branch -- it's a structural branch that
>> distinguishes between 64-bit PDE and 128-bit dual PDE entries. Any MMU
>> version with a dual PDE level would need this same distinction.
>
> The dual PDE level should be an associated constant - you still need to
> do the test, but note that you would also do it if there was only a
> single page table version. It's orthogonal to whether we use a trait or
> not here.

Sure, fair enough.

>
>>
>> I also did code-generation size analysis (see diff of code used below):
>>
>> Code generation analysis:
>>
>> Module .ko size: Before: 511,792 bytes After: 524,464 bytes (+2.5%)
>> .text section: Before: 112,620 bytes After: 116,628 bytes (+4,008 bytes)
>>
>> The +4K .text growth is the monomorphization cost: every generic function
>> is compiled twice (once for MmuV2, once for MmuV3).
>
> I would say this is working as intended then.

2.5% is not much for 1 feature but overtime IMO it can add up and become
significant, I think we should keep an eye on size.
>>> If you use a trait here, and make `PtWalk` generic against it, you can
>>> optimize this away. We had a similar situation when we introduced Turing
>>> support and the v2 ucode header, and tried both approaches: the
>>> trait-based one was slightly shorter, and arguably more readable.
>>
>> Actually I was the one who suggested traits for Falcon ucode descriptor if you
>> see this thread [1]. So basically you and Eliot are telling me to do what I
>> suggested in [1]. :-) However, I disagree that it is the right choice for this code.
>>
>> [1] https://lore.kernel.org/all/20251117231028.GA1095236@joelbox2/
>>
>> I think the two cases are quite different in complexity:
>
> Exactly. The complexity is different (this one involves multiple traits
> and associated types) but the pattern is the same - and that's a pattern
> traits are designed to address. If we were supposed to stop applying it
> when things go beyond a certain level of complexity, the conceptors of
> Rust would not have bothered addings things like associated types.
>
> These traits are nothing new, they simply formalize a reality that
> already exists in your code, which is that each version of the page
> table needs to implement a given set of methods. It's already there with
> the version doing dispatches, only it is not articulated clearly to the
> reader. So in that respect, having traits make the code *more* readable
> imho.

Makes the lower layers more readable and the higher layers more complex. Me, I'd
have preferred to keep the version matching stuff to the lower layers /
centralized. Maybe there is still a way to do that, while still using traits.

>>
>> The falcon ucode descriptor is essentially a set of flat field accessors
>> and a few params (imem_sec_load_params, dmem_load_params).
>> The trait has ~10 simple getter methods. There's no multi-level hierarchy,
>> no walker, and no generic propagation.
>>
>> The MMU page table case is structurally different. Making PtWalk generic
>> over an Mmu trait would require:
>>
>> - PtWalk<M: Mmu> (the walker)
>> - Plus all the associated types: M::Pte, M::Pde, M::DualPde each
>> needing their own trait bounds
>>
>> And we would also need:
>> - Vmm<M: Mmu> (which creates PtWalk)
>> - BarUser<M: Mmu> (which creates Vmm)
>>
>> I am also against making Vmm an enum as Eliot suggested:
>> enum Vmm {
>> V2(VmmInner<MmuV2>),
>> V3(VmmInner<MmuV3>),
>> }
>>
>> That moves the version complexity up to the reader. Code complexity IMO should
>> decrease as we go up abstractions, making it easier for users (Vmm/Bar).
>>
>> If you look at the the changes in vmm.rs to handle version dispatch there [2]:
>> Added: +109
>> Removed: -28
>>
>> [2]
>> https://github.com/Edgeworth/linux/commit/3627af550b61256184d589e7ec666c1108971f0e
>>
>> The main benefit of my approach is version-specific dispatch complexity is
>> completely isolated inside MmuVersion thus making the code outside of
>> pagetable.rs much more readable, without having to parametrize anything, and
>> without code size increase. I think that is worth considering.
>>
>>> But the main argument to use a trait here IMO is that it enables
>>> associated types and constants. That's particularly critical since some
>>> equivalent fields have different lengths between v2 and v3. An
>>> associated `Bounded` type for these would force the caller to validate
>>> the length of these fields before calling a non-fallible operation,
>>> which is exactly the level of caution that we want when dealing with
>>> page tables.
>>
>> I think Bounded validation is orthogonal to the dispatch model.
>> We can add Bounded to the current design without restructuring
>> into traits. For example:
>>
>> // In ver2::Pte
>> pub fn new_vram(pfn: Bounded<Pfn, 25>, writable: bool) -> Self { ... }
>>
>> // In ver3::Pte
>> pub fn new_vram(pfn: Bounded<Pfn, 40>, writable: bool) -> Self { ... }
>>
>> The unified Pte enum wrapper already dispatches to the correct
>> version-specific constructor, which would enforce the correct Bounded
>> constraint for that version.
>
> But then what type does the `new_vram` dispatch method take? Generic
> code lets us expose the expected `Bounded` type to the caller, which can
> do the proper validation. This is a small example, but I expect this
> pattern to come up in other parts of the code as well.

Maybe I didn't follow your point, but the caller would do a version match and
pass the correct bounded number of bits, but granted that's uglier if/once we
switched to bounded types here.

>>
>>> In order to fully benefit from it, we will need the bitfield macro from
>>> the `kernel` crate so the PDE/PTE fields can be `Bounded`, I will try to
>>> make it available quickly in a patch that you can depend on.
>>
>> That would be great, and I'd be happy to integrate Bounded validation once
>> the macro is available. I just don't think we need to restructure the
>> dispatch model in order to benefit from it.
>
> I'll finish the series and hopefully send it a bit later today. That's
> another significant rework for the series (sorry about that) but it
> should be worth the effort for the added correctness.

Sure!

thanks,

--
Joel Fernandes