Re: [PATCH v12 12/22] gpu: nova-core: mm: Add page table entry operation traits

[Alexandre Courbot]

On Tue May 5, 2026 at 4:28 AM JST, Joel Fernandes wrote:
>
>
> On 5/4/2026 10:31 AM, Alexandre Courbot wrote:
>> On Sun May 3, 2026 at 4:19 AM JST, Joel Fernandes wrote:
>>>> Please reorder things so they land, as much as possible, in their final
>>>> form. In this case this probably means defining the trait *before* the V2
>>>> and V3 page table definitions, so they can implement it from the get-go.
>>>
>>> That is a reasonable approach too, I can try to do that, but it is
>>> misleading to say '270 lines of diff that reviewers will have processed for
>>> nothing' which is nothing but fiction. Please look more carefully, the
>>> patch is iterative on the series.
>> 
>> For context, here is where the 270 lines of diff come from:
>> 
>>  drivers/gpu/nova-core/mm/pagetable/ver2.rs | 150 ++++++++------
>>  drivers/gpu/nova-core/mm/pagetable/ver3.rs | 120 +++++++----
>> 
>> But the number is not important.
>
> It is important, numbers and accuracy are really important things
> especially on the Linux kernel mailing list. Sorry if you feel that is
> inconvenient. And even quoting the 270 is a falsehood, the 270 lines were
> not refactored, only 90 lines or so was.

I am not particularly attached to this 270 number. It represents the
diff that I believe shouldn't be in this commit. Counting the number of
changed lines is also perfectly valid.

But again, that's not the point. Let's set my metric aside: we still
have, by your own account, 90 lines of churn that could be avoided by
the following 3 steps, each of which takes one minute to perform:

- Move the trait definitions of `pagetable.rs` into their own commit.
- Move that new commit before the ones introducing `ver2.rs` and
  `ver3.rs`.
- Squash the relevant parts of the remainder into the commit introducing
  `ver2.rs` or `ver3.rs`.

By doing that, on top of removing 90 lines of immediate follow-up
changes, you have also moved the public interface of the page tables
before their implementation, making all 3 patches easier to process as
reviewers are now introduced to how that code will be used *before* the
implementation details.

That's my closing argument on this topic and I won't insist if you are
not convinced this is worth doing; please act on it, or not, as you see
fit.