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

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

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.

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