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

[Christophe Leroy (CS GROUP)]

Le 26/11/2025 à 17:34, Ryan Roberts a écrit :
> On 26/11/2025 16:07, Ryan Roberts wrote:
> > On 26/11/2025 15:12, David Hildenbrand (Red Hat) wrote:
> > > On 11/26/25 16:08, Lorenzo Stoakes wrote:
> > > > On Wed, Nov 26, 2025 at 03:56:13PM +0100, David Hildenbrand (Red Hat) wrote:
> > > > > On 11/26/25 15:52, Lorenzo Stoakes wrote:
> > > > > > Would the pmdp_get() never get invoked then? Or otherwise wouldn't that end up
> > > > > > requiring a READ_ONCE() further up the stack?
> > > > >
> > > > > See my other reply, I think the pmdp_get() is required because all pud_*
> > > > > functions are just simple stubs.
> > > >
> > > > OK, thought you were saying we should push further down the stack? Or up
> > > > depending on how you view these things :P as in READ_ONCE at leaf?
> > >
> > > I think at leaf because I think the previous ones should essentially be only
> > > used by stubs.
> > >
> > > But I haven't fully digested how this is all working. Or supposed to work.
> > >
> > > I'm trying to chew through the arch/arm/include/asm/pgtable-2level.h example to
> > > see if I can make sense of it,
> >
> > I wonder if we can think about this slightly differently;
> >
> > READ_ONCE() has two important properties:
> >
> >   - It guarrantees that a load will be issued, *even if output is unused*
> >   - It guarrantees that the read will be single-copy-atomic (no tearing)
> >
> > I think for the existing places where READ_ONCE() is used for pagetable reads we
> > only care about:
> >
> >   - It guarrantees that a load will be issued, *if output is used*
> >   - It guarrantees that the read will be single-copy-atomic (no tearing)
> >
> > I think if we can weaken to the "if output is used" property, then the compiler
> > will optimize out all the unneccessary reads.
> >
> > AIUI, a C dereference provides neither of the guarrantees so that's no good.
> >
> > What about non-volatile asm? I'm told (thought need to verify) that for
> > non-volatile asm, the compiler will emit it if the output is used and remove it
> > otherwise. So if the asm contains the required single-copy-atomic, perhaps we
> > are in business?
> >
> > So we would need a new READ_SCA() macro that could default to READ_ONCE() (which
> > is stronger) and arches could opt in to providing a weaker asm version. Then the
> > default pXdp_get() could be READ_SCA(). And this should work for all cases.
> >
> > I think.
>
> I'm not sure this works. It looks like the compiler is free to move non-volatile
> asm sections which might be problematic for places where we are currently using
> READ_ONCE() in lockless algorithms, (e.g. GUP?). We wouldn't want to end up with
> a stale value.
>
> Another idea:
>
> Given the main pattern where we are aiming to optimize out the read is something
> like:
>
> if (!pud_present(*pud))
>
> where for a folded pmd:
>
> static inline int pud_present(pud_t pud)	{ return 1; }
>
> And we will change it to this:
>
> if (!pud_present(pudp_get(pud)))
>
> ...
>
> perhaps we can just define the folded pXd_present(), pXd_none(), pXd_bad(),
> pXd_user() and pXd_leaf() as macros:
>
> #define pud_present(pud)	1
>
> Then the compiler will never even see the pudp_get().

Take care about macros with unused parameters, it can be a problem 
sometimes, see commit 05a4ab823983 ("powerpc/uaccess: fix warning/error 
with access_ok()")

Christophe