Re: [RFC PATCH 10/34] x86: mm: carve out the generic compile-time folded pgtable case in effective_prot()

["David Hildenbrand (Arm)" <david@kernel.org>]

On 7/13/26 23:40, Dave Hansen wrote:
> On 7/13/26 14:17, David Hildenbrand (Arm) wrote:
>>> To me, the double READ_ONCE() is a non-issue.
>> To other arch maintainers, it is!
> 
> Ahh, so it seems like Christophe in ppc32 land was looking at this more
> like a regression that needed to get fixed.

Yes, exactly. :)

And I agree that it is something we should be optimizing.

> 
> Christophe, just out of curiosity, was this something that was causing
> you practical problems like measurable performance regressions, or was
> it really just insane code generation that seems unacceptably suboptimal?

I'd be curious about that as well. I mean, looking at the generated code
it's clear that it is suboptimal.


The solution space I see:


(1) Pass the result from e.g., pgdp_get() into p4d_get(), so it can just return 
    the value with folded p4ds.

That requires extreme amounts of churn in core-mm AFAIKS, so I don't see that as feasible.


(2) Rewrite folding code to make p4d_present() be a dummy instead of 
    pgd_present(), and make p4dp_get() return a dummy value.

... a lot of churn across architectures. I'm sure we'll learn soon why it was done 
ike the way it is today in the first place. Something interesting to look at,
certainly, but a bit of a stretch just to optimize reads.


(3) Make folded pgdp_get() use an ordinary read instead of a READ_ONCE / dummy.

I don't like that, because we couldn't catch easily if the value is then
actually used some old/new code.

If pgd_present() etc are supposed to ignore the value, I'd rather have a mechanism
that enforces that the values are actually ignore in pgd_offset() etc as well.


(4) What we do in this series, but instead of forbidding set_pgd(), make it only 
    complain when we are passing in a dummy value.

The expectation is that it would avoid touching most architecture code in patch
12 -> 27 and still prevent mistakes in the future.

@Yeoreum can you play with that and see what the end result would be?


The problem of how to handle ptdump effective_prot remains. Which is unfortunately an
x86 32-bit only problem ;) ... and effective_prot is essentially an x86-only thing.

There are various ways we could handle that, my preferred one would be

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index fb298e2191792..3d56e48fe4151 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -96,7 +96,7 @@ config X86
        select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
        select ARCH_HAS_PMEM_API                if X86_64
        select ARCH_HAS_PREEMPT_LAZY
-       select ARCH_HAS_PTDUMP
+       select ARCH_HAS_PTDUMP                  if X86_64
        select ARCH_HAS_PTE_SPECIAL
        select ARCH_HAS_HW_PTE_YOUNG
        select ARCH_HAS_NONLEAF_PMD_YOUNG       if PGTABLE_LEVELS > 2


Another option would be to keep the behavior unchanged:


diff --git a/mm/ptdump.c b/mm/ptdump.c
index 5851096e6f656..ce2cf5e07ac0a 100644
--- a/mm/ptdump.c
+++ b/mm/ptdump.c
@@ -39,6 +39,9 @@ static int ptdump_pgd_entry(pgd_t *pgd, unsigned long addr,
                return note_kasan_page_table(walk, addr);
 #endif
 
+       if (mm_p4d_folded(walk->mm))
+               return 0;
+
        if (st->effective_prot_pgd)
                st->effective_prot_pgd(st, val);
 
@@ -62,6 +65,13 @@ static int ptdump_p4d_entry(p4d_t *p4d, unsigned long addr,
                return note_kasan_page_table(walk, addr);
 #endif
 
+       if (mm_pud_folded(walk->mm))
+               return 0;
+
+       /* Simulate old behavior: all page table levels exist with folded values. */
+       if (mm_p4d_folded(walk->mm) && (st->effective_prot_pgd))
+               st->effective_prot_pgd(st, __pgd(p4d_val(val)));
+
        if (st->effective_prot_p4d)
                st->effective_prot_p4d(st, val);
 
@@ -85,6 +95,15 @@ static int ptdump_pud_entry(pud_t *pud, unsigned long addr,
                return note_kasan_page_table(walk, addr);
 #endif
 
+       if (mm_pmd_folded(walk->mm))
+               return 0;
+
+       /* Simulate old behavior: all page table levels exist with folded values. */
+       if (mm_pud_folded(walk->mm) && st->effective_prot_pgd)
+               st->effective_prot_pgd(st, __pgd(pud_val(val)));
+       if (mm_p4d_folded(walk->mm) && st->effective_prot_p4d)
+               st->effective_prot_p4d(st, __p4d(pud_val(val)));
+
        if (st->effective_prot_pud)
                st->effective_prot_pud(st, val);
 
@@ -107,6 +126,14 @@ static int ptdump_pmd_entry(pmd_t *pmd, unsigned long addr,
                return note_kasan_page_table(walk, addr);
 #endif
 
+       /* Simulate old behavior: all page table levels exist with folded values. */
+       if (mm_pud_folded(walk->mm) && st->effective_prot_pgd)
+               st->effective_prot_pgd(st, __pgd(pmd_val(val)));
+       if (mm_p4d_folded(walk->mm) && st->effective_prot_p4d)
+               st->effective_prot_p4d(st, __p4d(pmd_val(val)));
+       if (mm_pmd_folded(walk->mm) && st->effective_prot_pud)
+               st->effective_prot_pud(st, __pud(pmd_val(val)));
+
        if (st->effective_prot_pmd)
                st->effective_prot_pmd(st, val);
        if (pmd_leaf(val)) {


Then there would be another option of making x86 simply remembering the first level it got called for.


-- 
Cheers,

David