On 7/13/26 12:49, David Hildenbrand (Arm) wrote:
>> I really do think this series is changing things, not keeping them the same.
> Looking at it from that angle, I tend do agree: it further moves into the
> direction that actually the p4d_present() carries value, and the previous
> pgd_present() is just a dummy.
>
> I can see that some more work is required to untangle the mess we already
> have.
>
> I don't know why pgd_present() was made a dummy function and not
> p4d_present(),
> but I'm sure there is a good reason to it. Changing that now ... omg.
Yeah, the p4d addition added even more wrinkles because it was the first
time we tried runtime folding in addition to the historical compile-time
folding.
There were certainly some ugly corners of it that we never found a nice
way to fix. I know Kiryl tried pretty hard.
But, zooming back out... I don't think this diffstat:
31 files changed, 329 insertions(+), 206 deletions(-)
is nearly justified to eliminate one virtually guaranteed cached memory
read instruction. "Unnecessary", sure. But, worth the diffstat? I'm not
quite there.
Also, I think a series that adds code like this:
> - if (level > 0) {
> + /* Ignore folded levels ... */
> + if (((level == 0) && mm_p4d_folded(st->mm)) ||
> + ((level == 1) && mm_pud_folded(st->mm)) ||
> + ((level == 2) && mm_pmd_folded(st->mm)))
> + return;
> +
> + /* ... and make the actual first level remember the protection. */
> + if (((level == 0)) ||
> + ((level == 1) && mm_p4d_folded(st->mm)) ||
> + ((level == 2) && mm_pud_folded(st->mm)) ||
> + ((level == 3) && mm_pmd_folded(st->mm)))
> + first_level = true;
fails the sniff test of being an effective refactor. It unquestionably
makes _that_ code worse, not better.
If the series was:
Subject: Refactor page table folding in all architectures
and we had a discussion about what the best overall thing to do for all
architectures is, I think we might have a better path forward. It would
also be a smaller series.
To me, the double READ_ONCE() is a non-issue. Having a nice, coherent
page table API in the kernel is much more important to talk about.
