Re: [PATCH] arm64/mm: Elide TLB flush in certain pte protection transitions

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

On 18/09/2025 16:04, Dev Jain wrote:
> 
> On 18/09/25 6:19 pm, Ryan Roberts wrote:
>> On 18/09/2025 11:36, Dev Jain wrote:
>>> Currently arm64 does an unconditional TLB flush in mprotect(). This is not
>>> required for some cases, for example, when changing from PROT_NONE to
>>> PROT_READ | PROT_WRITE (a real usecase - glibc malloc does this to emulate
>>> growing into the non-main heaps), and unsetting uffd-wp in a range.
>>>
>>> Therefore, implement pte_needs_flush() for arm64, which is already
>>> implemented by some other arches as well.
>>>
>>> Running a userspace program changing permissions back and forth between
>>> PROT_NONE and PROT_READ | PROT_WRITE, and measuring the average time taken
>>> for the none->rw transition, I get a reduction from 3.2 microseconds to
>>> 2.95 microseconds, giving an 8.5% improvement.
>>>
>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>> ---
>>> mm-selftests pass. Based on 6.17-rc6.
>>>
>>>   arch/arm64/include/asm/tlbflush.h | 29 +++++++++++++++++++++++++++++
>>>   1 file changed, 29 insertions(+)
>>>
>>> diff --git a/arch/arm64/include/asm/tlbflush.h b/arch/arm64/include/asm/
>>> tlbflush.h
>>> index 18a5dc0c9a54..4a566d589100 100644
>>> --- a/arch/arm64/include/asm/tlbflush.h
>>> +++ b/arch/arm64/include/asm/tlbflush.h
>>> @@ -524,6 +524,35 @@ static inline void arch_tlbbatch_add_pending(struct
>>> arch_tlbflush_unmap_batch *b
>>>   {
>>>       __flush_tlb_range_nosync(mm, start, end, PAGE_SIZE, true, 3);
>>>   }
>>> +
>>> +static inline bool __pte_flags_need_flush(pteval_t oldval, pteval_t newval)
>> ptdesc_t is the preferred any-level type.
> 
> I keep forgetting this :)
> 
>>
>>> +{
>>> +    pteval_t diff = oldval ^ newval;
>>> +
>>> +    /* invalid to valid transition requires no flush */
>>> +    if (!(oldval & PTE_VALID) || (oldval & PTE_PRESENT_INVALID))
>> Is the PTE_PRESENT_INVALID really required? If the oldval was invalid, there
>> can't be a TLB entry for it, so no flush is required; that's it, I think?
>>
>> In fact, PTE_PRESENT_INVALID is overlaid with PTE_NG; it only means
>> PTE_PRESENT_INVALID when PTE_INVALID=0, so I think this is broken as is. Valid
>> user-space PTEs always have PTE_NG set, so you will never flush.
> 
> Not sure I get you. The condition as I wrote means
> 
> 1. If PTE_VALID is not set, then do not flush.
> 2. If PTE_VALID is set, *and* PTE_PRESENT_INVALID is set, then do not flush.
> 
> So when you say "it only means PTE_PRESENT_INVALID when PTE_INVALID=0", the
> second condition meets that.

Sorry I meant PTE_VALID=0. Your second condition is wrong; PTE_PRESENT_INVALID
is only defined when PTE_VALID=0.

Think about it; the PTE is valid from the HW's perspective if and only if
PTE_VALID=1. So that's the only condition that needs to be checked.


See this comment in the code for more info:

/*
 * PTE_PRESENT_INVALID=1 & PTE_VALID=0 indicates that the pte's fields should be
 * interpreted according to the HW layout by SW but any attempted HW access to
 * the address will result in a fault. pte_present() returns true.
 */
#define PTE_PRESENT_INVALID	(PTE_NG)     /* only when !PTE_VALID */


> 
>>
>>> +        return false;
>>> +
>>> +    /* Transition in the SW bits and access flag requires no flush */
>>> +    diff &= ~(PTE_SWBITS_MASK | PTE_AF);
>> Could you explain your thinking on why PTE_AF changes don't need a flush? I
>> would have thought if we want to clear the access flag, that would definitely
>> require a flush? Otherwise how would the MMU know to set the acccess bit on next
>> access if it already has a TLB entry?
> 
> You are correct, but AFAIK losing access bit information is not fatal, it will only
> mess with page aging. So potentially reclaim will lose some accuracy.

Sure, but it means that your change has a cost; reduced page aging accuracy.
That part of the change should at least be separated into its own commit and
probably backed up with performance numbers. Otherwise, I think we should retain
the original behaviour.


> 
>>
>>> +
>>> +    if (!diff)
>>> +        return false;
>>> +    return true;
>> Perhaps just "return !!diff;" here?
> 
> Sure.
> 
>>
>> Thanks,
>> Ryan
>>
>>
>>> +}
>>> +
>>> +static inline bool pte_needs_flush(pte_t oldpte, pte_t newpte)
>>> +{
>>> +    return __pte_flags_need_flush(pte_val(oldpte), pte_val(newpte));
>>> +}
>>> +#define pte_needs_flush pte_needs_flush
>>> +
>>> +static inline bool huge_pmd_needs_flush(pmd_t oldpmd, pmd_t newpmd)
>>> +{
>>> +    return __pte_flags_need_flush(pmd_val(oldpmd), pmd_val(newpmd));
>>> +}
>>> +#define huge_pmd_needs_flush huge_pmd_needs_flush
>>> +
>>>   #endif
>>>     #endif