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

[Ryan Roberts <ryan.roberts@arm.com>]

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().

Thanks,
Ryan


> 
>>
>>>
>>> Anyway. I am now designating you the expert at this ;)
>>
>> Oh no. :)
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> IOW, push the READ_ONCE() down to the lowest level so the previous ones
>>>>>> (that will get essentially ignore?) will get folded into the last
>>>>>> READ_ONCE()?
>>>>>>
>>>>>> But my head still hurts and I am focusing on something else concurrently :)
>>>>>
>>>>> Even if we could make this work, I don't love that there's some implicit
>>>>> assumption there that could easily break later on.
>>>>>
>>>>> I'd rather we kept it as stupid/obvious as possible...
>>>>
>>>> Looking at include/asm-generic/pgtable-nopmd.h I am not sure we are talking
>>>> about implicit assumptions here. It's kind-of the design that the pud_t
>>>> values are dummies, so why shoul the pudp_get() give you any guarantees.
>>>>
>>>> At least that's my current understanding, which might be very flawed :)
>>>
>>> I mean I'm waving my hands around like I'm working on an aircraft carrier here
>>> so if you're _sure_ it's _absolutely_ safe then fine :)
>>
>> Well, not yet ... :)
>>
>