Messages in this thread |  | | Date | Tue, 05 May 2026 14:37:42 +0900 | | Subject | Re: [PATCH v12 12/22] gpu: nova-core: mm: Add page table entry operation traits | | From | "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.
|  |