All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ryan Roberts <ryan.roberts@arm.com>
To: Dev Jain <dev.jain@arm.com>, catalin.marinas@arm.com, will@kernel.org
Cc: anshuman.khandual@arm.com, wangkefeng.wang@huawei.com,
	baohua@kernel.org, pjaroszynski@nvidia.com,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] arm64/mm: Elide TLB flush in certain pte protection transitions
Date: Thu, 18 Sep 2025 13:49:02 +0100	[thread overview]
Message-ID: <fb5818ec-dde9-4d53-ab0f-e28e5c2cab33@arm.com> (raw)
In-Reply-To: <20250918103638.77282-1-dev.jain@arm.com>

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.

> +{
> +	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.

> +		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?

> +
> +	if (!diff)
> +		return false;
> +	return true;

Perhaps just "return !!diff;" here?

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



  parent reply	other threads:[~2025-09-18 12:49 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-09-18 10:36 [PATCH] arm64/mm: Elide TLB flush in certain pte protection transitions Dev Jain
2025-09-18 12:22 ` Kefeng Wang
2025-09-18 12:46   ` Dev Jain
2025-09-18 12:49 ` Ryan Roberts [this message]
2025-09-18 15:04   ` Dev Jain
2025-09-18 15:22     ` Ryan Roberts
2025-09-19  5:15       ` Dev Jain

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fb5818ec-dde9-4d53-ab0f-e28e5c2cab33@arm.com \
    --to=ryan.roberts@arm.com \
    --cc=anshuman.khandual@arm.com \
    --cc=baohua@kernel.org \
    --cc=catalin.marinas@arm.com \
    --cc=dev.jain@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pjaroszynski@nvidia.com \
    --cc=wangkefeng.wang@huawei.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.