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

["David Hildenbrand (Red Hat)" <david@kernel.org>]

On 11/27/25 08:14, David Hildenbrand (Red Hat) wrote:
> On 11/26/25 21:31, David Hildenbrand (Red Hat) wrote:
>> On 11/26/25 17:34, Ryan Roberts wrote:
>>> 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
>>>
>>
>> Let's take a step back and realize that with __PAGETABLE_PMD_FOLDED
>>
>> (a) *pudp does not make any sense
>>
>> For a folded PMD, *pudp == *pmdp and consequently we would actually
>> get a PMD, not a PUD.
>>
>> For this reason all these pud_* helpers ignore the passed value
>> completely. It would be wrong.
>>
>> (b) pmd_offset() does *not* consume a pud but instead a pudp.
>>
>> That makes sense, just imagine what would happen if someone would pass
>> *pudp to that helper (we'd dereference twice ...).
>>
>>
>> So I wonder if we can just teach get_pudp() and friends to ... return
>> true garbage instead of dereferencing something that does not make sense?
>>
>> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
>> index 32e8457ad5352..c95d0d89ab3f1 100644
>> --- a/include/linux/pgtable.h
>> +++ b/include/linux/pgtable.h
>> @@ -351,7 +351,13 @@ static inline pmd_t pmdp_get(pmd_t *pmdp)
>>     #ifndef pudp_get
>>     static inline pud_t pudp_get(pud_t *pudp)
>>     {
>> +#ifdef __PAGETABLE_PMD_FOLDED
>> +       pud_t dummy = { 0 };
>> +
>> +       return dummy;
>> +#else
>>            return READ_ONCE(*pudp);
>> +#endif
>>     }
>>     #endif
>>     
>> set_pud/pud_page/pud_pgtable helper are confusing, I would
>> assume they are essentially unused (like documented for set_put)
>> and only required to keep compilers happy.
> 
> Staring at GUP-fast and perf_get_pgtable_size()---which should better be
> converted to pudp_get() etc--I guess we might have to rework
> p4d_offset_lockless() to do something that doesn't rely on
> passing variables of local variables.
> 
> We might have to enlighten these walkers (and only these) about folded
> page tables such that they don't depend on the result of pudp_get() and
> friends.

Talking to myself (I know), handling this might be as simple as having

diff --git a/include/asm-generic/pgtable-nopmd.h b/include/asm-generic/pgtable-nopmd.h
index 8ffd64e7a24cb..60e5ba02bcf06 100644
--- a/include/asm-generic/pgtable-nopmd.h
+++ b/include/asm-generic/pgtable-nopmd.h
@@ -49,6 +49,13 @@ static inline pmd_t * pmd_offset(pud_t * pud, unsigned long address)
  }
  #define pmd_offset pmd_offset
  
+static inline pmd_t * pmd_offset_lockless(pud_t *pud, puf_t pud, unsigned long address)
+{
+       return (pmd_t *)pud;
+}
+#define pmd_offset_lcokless pmd_offset_lockless
+
+
  #define pmd_val(x)                             (pud_val((x).pud))
  #define __pmd(x)                               ((pmd_t) { __pud(x) } )
  

IOW, just like for pmd_offset() we cast the pointer and don't ever touch the pud.


As a reminder, the default is

#ifndef pmd_offset_lockless
#define pmd_offset_lockless(pudp, pud, address) pmd_offset(&(pud), address)
#endif

(isn't that nasty? :) )

-- 
Cheers

David