> 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?
Okay. I'll try with (4) and let me reply how much we can reuduce
the volume of this patch series.
>
>
> 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)) {
But I think we can get rid of simulation if the x86 allows to modify
(of course, x86 still wants to use PTDUMP) like:
diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
index e32848c7f26d4..aa1277f35773a 100644
--- a/arch/x86/mm/dump_pagetables.c
+++ b/arch/x86/mm/dump_pagetables.c
@@ -29,6 +29,7 @@ struct pg_state {
struct ptdump_state ptdump;
struct mm_struct *mm;
int level;
+ int first_level;
pgprotval_t current_prot;
pgprotval_t effective_prot;
pgprotval_t prot_levels[5];
@@ -254,22 +255,8 @@ static void effective_prot(struct ptdump_state *pt_st, int
level, u64 val)
struct pg_state *st = container_of(pt_st, struct pg_state, ptdump);
pgprotval_t prot = val & PTE_FLAGS_MASK;
pgprotval_t effective;
- bool first_level = false;
- /* 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;
-
- if (!first_level) {
+ if (first_level > st->first_level) {
pgprotval_t higher_prot = st->prot_levels[level - 1];
effective = (higher_prot & prot & (_PAGE_USER | _PAGE_RW)) |
@@ -471,6 +458,15 @@ bool ptdump_walk_pgd_level_core(struct seq_file *m,
.seq = m
};
+ if (mm_pmd_folded (mm))
+ st->first_level = 3;
+ else if (mm_pud_folded (mm))
+ st->first_level = 2;
+ else if (mm_p4d_folded (mm))
+ st->first_level = 1;
+ else
+ st->first_level = 0;
+
ptdump_walk_pgd(&st.ptdump, mm, pgd);
if (!checkwx)
--
Sincerely,
Yeoreum Yun
