[PATCH 0/2] riscv: tlb: avoid tlb flushing on exit & execve

[PATCH 0/2] riscv: tlb: avoid tlb flushing on exit & execve

[Jisheng Zhang]

The mmu_gather code sets fullmm=1 when tearing down the entire address
space for an mm_struct on exit or execve. So if the underlying platform
supports ASID, the tlb flushing can be avoided because the ASID
allocator will never re-allocate a dirty ASID.

But currently, the tlb_finish_mmu() sets fullmm, when in fact it wants
to say that the TLB should be fully flushed.

So patch1 takes one of Nadav's patch from [1] to fix fullmm semantics.
Compared with original patch from[1], the differences are:
a. fixes the fullmm semantics in arm64 too
b. bring back the fullmm optimization back on arm64.

patch2 does the optimization on riscv.

Use the performance of Process creation in unixbench on T-HEAD TH1520
platform is improved by about 4%.

Link: https://lore.kernel.org/linux-mm/20210131001132.3368247-2-namit@vmware.com/ [1]

Jisheng Zhang (1):
  riscv: tlb: avoid tlb flushing if fullmm == 1

Nadav Amit (1):
  mm/tlb: fix fullmm semantics

 arch/arm64/include/asm/tlb.h | 5 ++++-
 arch/riscv/include/asm/tlb.h | 9 +++++++++
 include/asm-generic/tlb.h    | 2 +-
 mm/mmu_gather.c              | 2 +-
 4 files changed, 15 insertions(+), 3 deletions(-)

-- 
2.40.0

[PATCH 1/2] mm/tlb: fix fullmm semantics

[Jisheng Zhang]

From: Nadav Amit <namit@vmware.com>

fullmm in mmu_gather is supposed to indicate that the mm is torn-down
(e.g., on process exit) and can therefore allow certain optimizations.
However, tlb_finish_mmu() sets fullmm, when in fact it want to say that
the TLB should be fully flushed.

Change tlb_finish_mmu() to set need_flush_all and check this flag in
tlb_flush_mmu_tlbonly() when deciding whether a flush is needed.

At the same time, bring the arm64 fullmm on process exit optimization back.

Signed-off-by: Nadav Amit <namit@vmware.com>
Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will@kernel.org>
Cc: Yu Zhao <yuzhao@google.com>
Cc: Nick Piggin <npiggin@gmail.com>
Cc: x86@kernel.org
---
 arch/arm64/include/asm/tlb.h | 5 ++++-
 include/asm-generic/tlb.h    | 2 +-
 mm/mmu_gather.c              | 2 +-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/tlb.h b/arch/arm64/include/asm/tlb.h
index 846c563689a8..6164c5f3b78f 100644
--- a/arch/arm64/include/asm/tlb.h
+++ b/arch/arm64/include/asm/tlb.h
@@ -62,7 +62,10 @@ static inline void tlb_flush(struct mmu_gather *tlb)
 	 * invalidating the walk-cache, since the ASID allocator won't
 	 * reallocate our ASID without invalidating the entire TLB.
 	 */
-	if (tlb->fullmm) {
+	if (tlb->fullmm)
+		return;
+
+	if (tlb->need_flush_all) {
 		if (!last_level)
 			flush_tlb_mm(tlb->mm);
 		return;
diff --git a/include/asm-generic/tlb.h b/include/asm-generic/tlb.h
index 129a3a759976..f2d46357bcbb 100644
--- a/include/asm-generic/tlb.h
+++ b/include/asm-generic/tlb.h
@@ -452,7 +452,7 @@ static inline void tlb_flush_mmu_tlbonly(struct mmu_gather *tlb)
 	 * these bits.
 	 */
 	if (!(tlb->freed_tables || tlb->cleared_ptes || tlb->cleared_pmds ||
-	      tlb->cleared_puds || tlb->cleared_p4ds))
+	      tlb->cleared_puds || tlb->cleared_p4ds || tlb->need_flush_all))
 		return;
 
 	tlb_flush(tlb);
diff --git a/mm/mmu_gather.c b/mm/mmu_gather.c
index 4f559f4ddd21..79298bac3481 100644
--- a/mm/mmu_gather.c
+++ b/mm/mmu_gather.c
@@ -384,7 +384,7 @@ void tlb_finish_mmu(struct mmu_gather *tlb)
 		 * On x86 non-fullmm doesn't yield significant difference
 		 * against fullmm.
 		 */
-		tlb->fullmm = 1;
+		tlb->need_flush_all = 1;
 		__tlb_reset_range(tlb);
 		tlb->freed_tables = 1;
 	}
-- 
2.40.0

[PATCH 2/2] riscv: tlb: avoid tlb flushing if fullmm == 1

[Jisheng Zhang]

The mmu_gather code sets fullmm=1 when tearing down the entire address
space for an mm_struct on exit or execve. So if the underlying platform
supports ASID, the tlb flushing can be avoided because the ASID
allocator will never re-allocate a dirty ASID.

Use the performance of Process creation in unixbench on T-HEAD TH1520
platform is improved by about 4%.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 arch/riscv/include/asm/tlb.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/riscv/include/asm/tlb.h b/arch/riscv/include/asm/tlb.h
index 1eb5682b2af6..35f3c214332e 100644
--- a/arch/riscv/include/asm/tlb.h
+++ b/arch/riscv/include/asm/tlb.h
@@ -12,10 +12,19 @@ static void tlb_flush(struct mmu_gather *tlb);
 
 #define tlb_flush tlb_flush
 #include <asm-generic/tlb.h>
+#include <asm/mmu_context.h>
 
 static inline void tlb_flush(struct mmu_gather *tlb)
 {
 #ifdef CONFIG_MMU
+	/*
+	 * If ASID is supported, the ASID allocator will either invalidate the
+	 * ASID or mark it as used. So we can avoid TLB invalidation when
+	 * pulling down a full mm.
+	 */
+	if (static_branch_likely(&use_asid_allocator) && tlb->fullmm)
+		return;
+
 	if (tlb->fullmm || tlb->need_flush_all)
 		flush_tlb_mm(tlb->mm);
 	else
-- 
2.40.0

Re: [PATCH 1/2] mm/tlb: fix fullmm semantics

[Nadav Amit]

> On Dec 28, 2023, at 10:46 AM, Jisheng Zhang <jszhang@kernel.org> wrote:
> 
> From: Nadav Amit <namit@vmware.com>
> 
> fullmm in mmu_gather is supposed to indicate that the mm is torn-down
> (e.g., on process exit) and can therefore allow certain optimizations.
> However, tlb_finish_mmu() sets fullmm, when in fact it want to say that
> the TLB should be fully flushed.
> 
> Change tlb_finish_mmu() to set need_flush_all and check this flag in
> tlb_flush_mmu_tlbonly() when deciding whether a flush is needed.
> 
> At the same time, bring the arm64 fullmm on process exit optimization back.
> 
> Signed-off-by: Nadav Amit <namit@vmware.com>
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Will Deacon <will@kernel.org>
> Cc: Yu Zhao <yuzhao@google.com>
> Cc: Nick Piggin <npiggin@gmail.com>
> Cc: x86@kernel.org
> ---
> arch/arm64/include/asm/tlb.h | 5 ++++-
> include/asm-generic/tlb.h    | 2 +-
> mm/mmu_gather.c              | 2 +-
> 3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/tlb.h b/arch/arm64/include/asm/tlb.h
> index 846c563689a8..6164c5f3b78f 100644
> --- a/arch/arm64/include/asm/tlb.h
> +++ b/arch/arm64/include/asm/tlb.h
> @@ -62,7 +62,10 @@ static inline void tlb_flush(struct mmu_gather *tlb)
> 	 * invalidating the walk-cache, since the ASID allocator won't
> 	 * reallocate our ASID without invalidating the entire TLB.
> 	 */
> -	if (tlb->fullmm) {
> +	if (tlb->fullmm)
> +		return;
> +
> +	if (tlb->need_flush_all) {
> 		if (!last_level)
> 			flush_tlb_mm(tlb->mm);
> 		return;
> 

Thanks for pulling my patch out of the abyss, but the chunk above
did not come from my old patch.

My knowledge of arm64 is a bit limited, but the code does not seem
to match the comment, so if it is correct (which I strongly doubt),
the comment should be updated.

[1] https://lore.kernel.org/all/20210131001132.3368247-2-namit@vmware.com/


-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

Re: [PATCH 2/2] riscv: tlb: avoid tlb flushing if fullmm == 1

[Alexandre Ghiti]

Hi Jisheng,

On 28/12/2023 09:46, Jisheng Zhang wrote:
> The mmu_gather code sets fullmm=1 when tearing down the entire address
> space for an mm_struct on exit or execve. So if the underlying platform
> supports ASID, the tlb flushing can be avoided because the ASID
> allocator will never re-allocate a dirty ASID.
>
> Use the performance of Process creation in unixbench on T-HEAD TH1520
> platform is improved by about 4%.
>
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>   arch/riscv/include/asm/tlb.h | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/arch/riscv/include/asm/tlb.h b/arch/riscv/include/asm/tlb.h
> index 1eb5682b2af6..35f3c214332e 100644
> --- a/arch/riscv/include/asm/tlb.h
> +++ b/arch/riscv/include/asm/tlb.h
> @@ -12,10 +12,19 @@ static void tlb_flush(struct mmu_gather *tlb);
>   
>   #define tlb_flush tlb_flush
>   #include <asm-generic/tlb.h>
> +#include <asm/mmu_context.h>
>   
>   static inline void tlb_flush(struct mmu_gather *tlb)
>   {
>   #ifdef CONFIG_MMU
> +	/*
> +	 * If ASID is supported, the ASID allocator will either invalidate the
> +	 * ASID or mark it as used. So we can avoid TLB invalidation when
> +	 * pulling down a full mm.
> +	 */


Given the number of bits are limited for the ASID, at some point we'll 
reuse previously allocated ASID so the ASID allocator must make sure to 
invalidate the entries when reusing an ASID: can you point where this is 
done?

Thanks,

Alex


> +	if (static_branch_likely(&use_asid_allocator) && tlb->fullmm)
> +		return;
> +
>   	if (tlb->fullmm || tlb->need_flush_all)
>   		flush_tlb_mm(tlb->mm);
>   	else

Re: [PATCH 1/2] mm/tlb: fix fullmm semantics

[Jisheng Zhang]

On Sat, Dec 30, 2023 at 11:54:02AM +0200, Nadav Amit wrote:
> 
> 
> > On Dec 28, 2023, at 10:46 AM, Jisheng Zhang <jszhang@kernel.org> wrote:
> > 
> > From: Nadav Amit <namit@vmware.com>
> > 
> > fullmm in mmu_gather is supposed to indicate that the mm is torn-down
> > (e.g., on process exit) and can therefore allow certain optimizations.
> > However, tlb_finish_mmu() sets fullmm, when in fact it want to say that
> > the TLB should be fully flushed.
> > 
> > Change tlb_finish_mmu() to set need_flush_all and check this flag in
> > tlb_flush_mmu_tlbonly() when deciding whether a flush is needed.
> > 
> > At the same time, bring the arm64 fullmm on process exit optimization back.
> > 
> > Signed-off-by: Nadav Amit <namit@vmware.com>
> > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > Cc: Andrea Arcangeli <aarcange@redhat.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Andy Lutomirski <luto@kernel.org>
> > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Yu Zhao <yuzhao@google.com>
> > Cc: Nick Piggin <npiggin@gmail.com>
> > Cc: x86@kernel.org
> > ---
> > arch/arm64/include/asm/tlb.h | 5 ++++-
> > include/asm-generic/tlb.h    | 2 +-
> > mm/mmu_gather.c              | 2 +-
> > 3 files changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/tlb.h b/arch/arm64/include/asm/tlb.h
> > index 846c563689a8..6164c5f3b78f 100644
> > --- a/arch/arm64/include/asm/tlb.h
> > +++ b/arch/arm64/include/asm/tlb.h
> > @@ -62,7 +62,10 @@ static inline void tlb_flush(struct mmu_gather *tlb)
> > 	 * invalidating the walk-cache, since the ASID allocator won't
> > 	 * reallocate our ASID without invalidating the entire TLB.
> > 	 */
> > -	if (tlb->fullmm) {
> > +	if (tlb->fullmm)
> > +		return;
> > +
> > +	if (tlb->need_flush_all) {
> > 		if (!last_level)
> > 			flush_tlb_mm(tlb->mm);
> > 		return;
> > 
> 
> Thanks for pulling my patch out of the abyss, but the chunk above
> did not come from my old patch.

I stated this in cover letter msg ;) IMHO, current arm64 uses fullmm as
need_flush_all, so I think we need at least the need_flush_all line.

I'd like to see comments from arm64 experts.

> 
> My knowledge of arm64 is a bit limited, but the code does not seem
> to match the comment, so if it is correct (which I strongly doubt),
> the comment should be updated.

will do if the above change is accepted by arm64

> 
> [1] https://lore.kernel.org/all/20210131001132.3368247-2-namit@vmware.com/
> 
> 
> -- 
> This electronic communication and the information and any files transmitted 
> with it, or attached to it, are confidential and are intended solely for 
> the use of the individual or entity to whom it is addressed and may contain 
> information that is confidential, legally privileged, protected by privacy 
> laws, or otherwise restricted from disclosure to anyone else. If you are 
> not the intended recipient or the person responsible for delivering the 
> e-mail to the intended recipient, you are hereby notified that any use, 
> copying, distributing, dissemination, forwarding, printing, or copying of 
> this e-mail is strictly prohibited. If you received this e-mail in error, 
> please return the e-mail to the sender, delete it from your computer, and 
> destroy any printed copy of it.

Re: [PATCH 2/2] riscv: tlb: avoid tlb flushing if fullmm == 1

[Jisheng Zhang]

On Sat, Dec 30, 2023 at 07:26:11PM +0100, Alexandre Ghiti wrote:
> Hi Jisheng,

Hi Alex,

> 
> On 28/12/2023 09:46, Jisheng Zhang wrote:
> > The mmu_gather code sets fullmm=1 when tearing down the entire address
> > space for an mm_struct on exit or execve. So if the underlying platform
> > supports ASID, the tlb flushing can be avoided because the ASID
> > allocator will never re-allocate a dirty ASID.
> > 
> > Use the performance of Process creation in unixbench on T-HEAD TH1520
> > platform is improved by about 4%.
> > 
> > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > ---
> >   arch/riscv/include/asm/tlb.h | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> > 
> > diff --git a/arch/riscv/include/asm/tlb.h b/arch/riscv/include/asm/tlb.h
> > index 1eb5682b2af6..35f3c214332e 100644
> > --- a/arch/riscv/include/asm/tlb.h
> > +++ b/arch/riscv/include/asm/tlb.h
> > @@ -12,10 +12,19 @@ static void tlb_flush(struct mmu_gather *tlb);
> >   #define tlb_flush tlb_flush
> >   #include <asm-generic/tlb.h>
> > +#include <asm/mmu_context.h>
> >   static inline void tlb_flush(struct mmu_gather *tlb)
> >   {
> >   #ifdef CONFIG_MMU
> > +	/*
> > +	 * If ASID is supported, the ASID allocator will either invalidate the
> > +	 * ASID or mark it as used. So we can avoid TLB invalidation when
> > +	 * pulling down a full mm.
> > +	 */
> 
> 
> Given the number of bits are limited for the ASID, at some point we'll reuse
> previously allocated ASID so the ASID allocator must make sure to invalidate
> the entries when reusing an ASID: can you point where this is done?

Per my understanding of the code, the path would be
set_mm_asid()
  __new_context()
    __flush_context()  // set context_tlb_flush_pending
if (need_flush_tlb)
  local_flush_tlb_all()

Thanks
 
> 
> > +	if (static_branch_likely(&use_asid_allocator) && tlb->fullmm)
> > +		return;
> > +
> >   	if (tlb->fullmm || tlb->need_flush_all)
> >   		flush_tlb_mm(tlb->mm);
> >   	else

Re: [PATCH 1/2] mm/tlb: fix fullmm semantics

[Will Deacon]

On Thu, Dec 28, 2023 at 04:46:41PM +0800, Jisheng Zhang wrote:
> From: Nadav Amit <namit@vmware.com>
> 
> fullmm in mmu_gather is supposed to indicate that the mm is torn-down
> (e.g., on process exit) and can therefore allow certain optimizations.
> However, tlb_finish_mmu() sets fullmm, when in fact it want to say that
> the TLB should be fully flushed.
> 
> Change tlb_finish_mmu() to set need_flush_all and check this flag in
> tlb_flush_mmu_tlbonly() when deciding whether a flush is needed.
> 
> At the same time, bring the arm64 fullmm on process exit optimization back.
> 
> Signed-off-by: Nadav Amit <namit@vmware.com>
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Will Deacon <will@kernel.org>
> Cc: Yu Zhao <yuzhao@google.com>
> Cc: Nick Piggin <npiggin@gmail.com>
> Cc: x86@kernel.org
> ---
>  arch/arm64/include/asm/tlb.h | 5 ++++-
>  include/asm-generic/tlb.h    | 2 +-
>  mm/mmu_gather.c              | 2 +-
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/tlb.h b/arch/arm64/include/asm/tlb.h
> index 846c563689a8..6164c5f3b78f 100644
> --- a/arch/arm64/include/asm/tlb.h
> +++ b/arch/arm64/include/asm/tlb.h
> @@ -62,7 +62,10 @@ static inline void tlb_flush(struct mmu_gather *tlb)
>  	 * invalidating the walk-cache, since the ASID allocator won't
>  	 * reallocate our ASID without invalidating the entire TLB.
>  	 */
> -	if (tlb->fullmm) {
> +	if (tlb->fullmm)
> +		return;
> +
> +	if (tlb->need_flush_all) {
>  		if (!last_level)
>  			flush_tlb_mm(tlb->mm);
>  		return;

Why isn't the 'last_level' check sufficient here? In other words, when do
we perform a !last_level invalidation with 'fullmm' set outside of teardown?

Will

Re: [PATCH 1/2] mm/tlb: fix fullmm semantics

[Will Deacon]

On Wed, Jan 03, 2024 at 05:50:01PM +0000, Will Deacon wrote:
> On Thu, Dec 28, 2023 at 04:46:41PM +0800, Jisheng Zhang wrote:
> > From: Nadav Amit <namit@vmware.com>
> > 
> > fullmm in mmu_gather is supposed to indicate that the mm is torn-down
> > (e.g., on process exit) and can therefore allow certain optimizations.
> > However, tlb_finish_mmu() sets fullmm, when in fact it want to say that
> > the TLB should be fully flushed.
> > 
> > Change tlb_finish_mmu() to set need_flush_all and check this flag in
> > tlb_flush_mmu_tlbonly() when deciding whether a flush is needed.
> > 
> > At the same time, bring the arm64 fullmm on process exit optimization back.
> > 
> > Signed-off-by: Nadav Amit <namit@vmware.com>
> > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > Cc: Andrea Arcangeli <aarcange@redhat.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Andy Lutomirski <luto@kernel.org>
> > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Yu Zhao <yuzhao@google.com>
> > Cc: Nick Piggin <npiggin@gmail.com>
> > Cc: x86@kernel.org
> > ---
> >  arch/arm64/include/asm/tlb.h | 5 ++++-
> >  include/asm-generic/tlb.h    | 2 +-
> >  mm/mmu_gather.c              | 2 +-
> >  3 files changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/tlb.h b/arch/arm64/include/asm/tlb.h
> > index 846c563689a8..6164c5f3b78f 100644
> > --- a/arch/arm64/include/asm/tlb.h
> > +++ b/arch/arm64/include/asm/tlb.h
> > @@ -62,7 +62,10 @@ static inline void tlb_flush(struct mmu_gather *tlb)
> >  	 * invalidating the walk-cache, since the ASID allocator won't
> >  	 * reallocate our ASID without invalidating the entire TLB.
> >  	 */
> > -	if (tlb->fullmm) {
> > +	if (tlb->fullmm)
> > +		return;
> > +
> > +	if (tlb->need_flush_all) {
> >  		if (!last_level)
> >  			flush_tlb_mm(tlb->mm);
> >  		return;
> 
> Why isn't the 'last_level' check sufficient here? In other words, when do
> we perform a !last_level invalidation with 'fullmm' set outside of teardown?

Sorry, logic inversion typo there. I should've said:

  When do we perform a last_level invalidation with 'fullmm' set outside
  of teardown?

I remember this used to be the case for OOM ages ago, but 687cb0884a71
("mm, oom_reaper: gather each vma to prevent leaking TLB entry") sorted
that out.

I'm not against making this clearer and/or more robust, I'm just trying
to understand whether this is fixing a bug (as implied by the subject)
or not.

Will

Re: [PATCH 1/2] mm/tlb: fix fullmm semantics

[Catalin Marinas]

On Thu, Dec 28, 2023 at 04:46:41PM +0800, Jisheng Zhang wrote:
> diff --git a/arch/arm64/include/asm/tlb.h b/arch/arm64/include/asm/tlb.h
> index 846c563689a8..6164c5f3b78f 100644
> --- a/arch/arm64/include/asm/tlb.h
> +++ b/arch/arm64/include/asm/tlb.h
> @@ -62,7 +62,10 @@ static inline void tlb_flush(struct mmu_gather *tlb)
>  	 * invalidating the walk-cache, since the ASID allocator won't
>  	 * reallocate our ASID without invalidating the entire TLB.
>  	 */
> -	if (tlb->fullmm) {
> +	if (tlb->fullmm)
> +		return;
> +
> +	if (tlb->need_flush_all) {
>  		if (!last_level)
>  			flush_tlb_mm(tlb->mm);
>  		return;

I don't think that's correct. IIRC, commit f270ab88fdf2 ("arm64: tlb:
Adjust stride and type of TLBI according to mmu_gather") explicitly
added the !last_level check to invalidate the walk cache (correspondence
between the VA and the page table page rather than the full VA->PA
translation).

> diff --git a/include/asm-generic/tlb.h b/include/asm-generic/tlb.h
> index 129a3a759976..f2d46357bcbb 100644
> --- a/include/asm-generic/tlb.h
> +++ b/include/asm-generic/tlb.h
> @@ -452,7 +452,7 @@ static inline void tlb_flush_mmu_tlbonly(struct mmu_gather *tlb)
>  	 * these bits.
>  	 */
>  	if (!(tlb->freed_tables || tlb->cleared_ptes || tlb->cleared_pmds ||
> -	      tlb->cleared_puds || tlb->cleared_p4ds))
> +	      tlb->cleared_puds || tlb->cleared_p4ds || tlb->need_flush_all))
>  		return;
>  
>  	tlb_flush(tlb);
> diff --git a/mm/mmu_gather.c b/mm/mmu_gather.c
> index 4f559f4ddd21..79298bac3481 100644
> --- a/mm/mmu_gather.c
> +++ b/mm/mmu_gather.c
> @@ -384,7 +384,7 @@ void tlb_finish_mmu(struct mmu_gather *tlb)
>  		 * On x86 non-fullmm doesn't yield significant difference
>  		 * against fullmm.
>  		 */
> -		tlb->fullmm = 1;
> +		tlb->need_flush_all = 1;
>  		__tlb_reset_range(tlb);
>  		tlb->freed_tables = 1;
>  	}

The optimisation here was added about a year later in commit
7a30df49f63a ("mm: mmu_gather: remove __tlb_reset_range() for force
flush"). Do we still need to keep freed_tables = 1 here? I'd say only
__tlb_reset_range().

-- 
Catalin

Re: [PATCH 1/2] mm/tlb: fix fullmm semantics

[Dave Hansen]

On 1/3/24 10:05, Catalin Marinas wrote:
>> --- a/mm/mmu_gather.c
>> +++ b/mm/mmu_gather.c
>> @@ -384,7 +384,7 @@ void tlb_finish_mmu(struct mmu_gather *tlb)
>>  		 * On x86 non-fullmm doesn't yield significant difference
>>  		 * against fullmm.
>>  		 */
>> -		tlb->fullmm = 1;
>> +		tlb->need_flush_all = 1;
>>  		__tlb_reset_range(tlb);
>>  		tlb->freed_tables = 1;
>>  	}
> The optimisation here was added about a year later in commit
> 7a30df49f63a ("mm: mmu_gather: remove __tlb_reset_range() for force
> flush"). Do we still need to keep freed_tables = 1 here? I'd say only
> __tlb_reset_range().

I think the __tlb_reset_range() can be dangerous if it clears
->freed_tables.  On x86 at least, it might lead to skipping the TLB IPI
for CPUs that are in lazy TLB mode.  When those wake back up they might
start using the freed page tables.

Logically, this little hunk of code is just trying to turn the 'tlb'
from a ranged flush into a full one.  Ideally, just setting
'need_flush_all = 1' would be enough.

Is __tlb_reset_range() doing anything useful for other architectures?  I
think it's just destroying perfectly valid metadata on x86. ;)

Re: [PATCH 2/2] riscv: tlb: avoid tlb flushing if fullmm == 1

[Alexandre Ghiti]

On 02/01/2024 04:12, Jisheng Zhang wrote:
> On Sat, Dec 30, 2023 at 07:26:11PM +0100, Alexandre Ghiti wrote:
>> Hi Jisheng,
> Hi Alex,
>
>> On 28/12/2023 09:46, Jisheng Zhang wrote:
>>> The mmu_gather code sets fullmm=1 when tearing down the entire address
>>> space for an mm_struct on exit or execve. So if the underlying platform
>>> supports ASID, the tlb flushing can be avoided because the ASID
>>> allocator will never re-allocate a dirty ASID.
>>>
>>> Use the performance of Process creation in unixbench on T-HEAD TH1520
>>> platform is improved by about 4%.
>>>
>>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
>>> ---
>>>    arch/riscv/include/asm/tlb.h | 9 +++++++++
>>>    1 file changed, 9 insertions(+)
>>>
>>> diff --git a/arch/riscv/include/asm/tlb.h b/arch/riscv/include/asm/tlb.h
>>> index 1eb5682b2af6..35f3c214332e 100644
>>> --- a/arch/riscv/include/asm/tlb.h
>>> +++ b/arch/riscv/include/asm/tlb.h
>>> @@ -12,10 +12,19 @@ static void tlb_flush(struct mmu_gather *tlb);
>>>    #define tlb_flush tlb_flush
>>>    #include <asm-generic/tlb.h>
>>> +#include <asm/mmu_context.h>
>>>    static inline void tlb_flush(struct mmu_gather *tlb)
>>>    {
>>>    #ifdef CONFIG_MMU
>>> +	/*
>>> +	 * If ASID is supported, the ASID allocator will either invalidate the
>>> +	 * ASID or mark it as used. So we can avoid TLB invalidation when
>>> +	 * pulling down a full mm.
>>> +	 */
>>
>> Given the number of bits are limited for the ASID, at some point we'll reuse
>> previously allocated ASID so the ASID allocator must make sure to invalidate
>> the entries when reusing an ASID: can you point where this is done?
> Per my understanding of the code, the path would be
> set_mm_asid()
>    __new_context()
>      __flush_context()  // set context_tlb_flush_pending
> if (need_flush_tlb)
>    local_flush_tlb_all()


Ok thanks, so feel free to add:

Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>

Thanks!

Alex


>
> Thanks
>   
>>> +	if (static_branch_likely(&use_asid_allocator) && tlb->fullmm)
>>> +		return;
>>> +
>>>    	if (tlb->fullmm || tlb->need_flush_all)
>>>    		flush_tlb_mm(tlb->mm);
>>>    	else
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/2] mm/tlb: fix fullmm semantics

[Nadav Amit]

> On Jan 2, 2024, at 4:41 AM, Jisheng Zhang <jszhang@kernel.org> wrote:
> 
> On Sat, Dec 30, 2023 at 11:54:02AM +0200, Nadav Amit wrote:
> 
>> 
>> My knowledge of arm64 is a bit limited, but the code does not seem
>> to match the comment, so if it is correct (which I strongly doubt),
>> the comment should be updated.
> 
> will do if the above change is accepted by arm64

Jisheng, I expected somebody with arm64 knowledge to point it out, and
maybe I am wrong, but I really don’t understand something about the
correctness, if you can please explain.

In the following code:

--- a/arch/arm64/include/asm/tlb.h
+++ b/arch/arm64/include/asm/tlb.h
@@ -62,7 +62,10 @@ static inline void tlb_flush(struct mmu_gather *tlb)
	 * invalidating the walk-cache, since the ASID allocator won't
	 * reallocate our ASID without invalidating the entire TLB.
	 */
-	if (tlb->fullmm) {
+	if (tlb->fullmm)
+		return;

You skip flush if fullmm is on. But if page-tables are freed, you may
want to flush immediately and not wait for ASID to be freed to avoid
speculative page walks; these walks at least on x86 caused a mess.

No?


-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.