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

[John Hubbard]

On 4/8/26 9:58 AM, Joel Fernandes wrote:
> On 4/8/2026 9:26 AM, Eliot Courtney wrote:
>> On Tue Apr 7, 2026 at 10:59 PM JST, Joel Fernandes wrote:
>>> On 4/7/2026 9:42 AM, Eliot Courtney wrote:
>>>> On Tue Apr 7, 2026 at 6:55 AM JST, Joel Fernandes wrote:
...>> [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.

Hi Joel and all,

I also looked through Eliot's above attempt carefully, and actually
liked it a lot (sorry! haha):

* It cleans up the code. The initial working version was readable, but
  also had lots of noise on the screen: match statements and pairs of
  v2/v3 statements.

  And interestingly, the mmu_version was, in effect, sporadically
  implementing a Trait-based approach. But because it is custom,
  readers don't benefit as much as they would with Traits, which
  tell you immediately how things are structured.

Joel, I am passionately in agreement with your principles: code must
be readable on the screen.

In this case, though, Traits make considerably more readable,
especially if one makes the very reasonable assumption that readers are
thoroughly accustomed to dealing with Rust traits.

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

lol I understand, believe me. But this is short and not too bad, really.

> /// Type-erased MMU-specific [`Vmm`] implementations.

Type erasure remains a semi-exotic thing, IMHO. As such, another
sentence to elaborate on this would be a nice touch.

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

They are not pointless.

However! What I think would be nice is: do a new v11 with approximately
this approach, and then we can beat it into being as readable as 
possible.
 

thanks,
-- 
John Hubbard