Re: [PATCH v3 06/22] mm: Always use page table accessor functions

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Wed, Nov 26, 2025 at 02:22:13PM +0000, Ryan Roberts wrote:
> On 26/11/2025 13:47, Wei Yang wrote:
> > On Wed, Nov 26, 2025 at 01:03:42PM +0000, Ryan Roberts wrote:
> >> On 26/11/2025 12:35, David Hildenbrand (Red Hat) wrote:
> > [...]
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> I've just come across this patch and wanted to mention that we could also
> >>>>>>> benefit from this improved absraction for some features we are looking at for
> >>>>>>> arm64. As you mention, Anshuman had a go but hit some roadblocks.
> >>>>>>>
> >>>>>>> The main issue is that the compiler was unable to optimize away the
> >>>>>>> READ_ONCE()s
> >>>>>>> for the case where certain levels of the pgtable are folded. But it can
> >>>>>>> optimize
> >>>>>>> the plain C dereferences. There were complaints the the generated code for arm
> >>>>>>> (32) and powerpc was significantly impacted due to having many more
> >>>>>>> (redundant)
> >>>>>>> loads.
> >>>>>>>
> >>>>>>
> >>>>>> We do have mm_pmd_folded()/p4d_folded() etc, could that help to sort
> >>>>>> this out internally?
> >>>>>>
> >>>>>
> >>>>> Just stumbled over the reply from Christope:
> >>>>>
> >>>>> https://lkml.kernel.org/r/0019d675-ce3d-4a5c-89ed-f126c45145c9@kernel.org
> >>>>>
> >>>>> And wonder if we could handle that somehow directly in the pgdp_get() etc.
> >>
> >> I certainly don't like the suggestion of doing the is_folded() test outside the
> >> helper, but if we can push that logic down into pXdp_get() that would be pretty
> >> neat. Anshuman and I did briefly play with the idea of doing a C dereference if
> >> the level is folded and a READ_ONCE() otherwise, all inside each pXdp_get()
> >> helper. Although we never proved it to be correct. I struggle with the model for
> >> folding. Do you want to optimize out all-but-the-highest level's access or
> >> all-but-the-lowest level's access? Makes my head hurt...
> >>
> >>
> >
> > You mean sth like:
> >
> > static inline pmd_t pmdp_get(pmd_t *pmdp)
> > {
> > #ifdef __PAGETABLE_PMD_FOLDED
> > 	return *pmdp;
> > #else
> > 	return READ_ONCE(*pmdp);
> > #endif
> > }
>
> Yes. But I'm not convinced it's correct.
>
> I *think* (but please correct me if I'm wrong) if the PMD is folded, the PUD and
> P4D must also be folded, and you effectively have a 2 level pgtable consisting
> of the PGD table and the PTE table. p4dp_get(), pudp_get() and pmdp_get() are
> all effectively duplicating the load of the pgd entry? So assuming pgdp_get()
> was already called and used READ_ONCE(), you might hope the compiler will just
> drop the other loads and just use the value returned by READ_ONCE(). But I doubt
> there is any guarantee of that and you might be in a situation where pgdp_get()
> never even got called (perhaps you already have the pmd pointer).

Yeah, it kinda sucks to bake that assumption in too even if we can prove it
currently _is_ correct, and it becomes tricky because to somebody observing this
they might well think 'oh so we don't need to think about tearing here' but in
reality we are just assuming somebody already thought about it for us :)

>
> So I don't think it works.
>
> Probably we either have to live with the extra loads or have 2 types of helper.

RoI on arches where we fold PMD maybe make it not such a big problem?

If not then 2 function solution seems the right way.

>
> >
> >>>>
> >>>> I find that kind of gross to be honest. Isn't the whole point of folding that we
> >>>> don't have to think about it...
> >>
> >> Agreed, but if we can put it inside the default helper implementation, that
> >> solves it, I think? An arch has to be careful if they are overriding the
> >> defaults, but it's still well contained.
> >>
> >>>
> >>> If we could adjust generic pgdp_get() and friends to not do a READ_ONCE() once
> >>> folded we might not have to think about that in the callers.
> >>>
> >>> Just an idea, though, not sure if that would fly the way I envision it.
> >>
> >>
> >
>

Yup obviously if we _could_ find a way to bury this down then sure. But having
actual page table walkers have to think about this is a no-go IMO.

Cheers, Lorenzo