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/[email protected]/ [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
From: Nadav Amit <[email protected]>
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 <[email protected]>
Signed-off-by: Jisheng Zhang <[email protected]>
Cc: Andrea Arcangeli <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Yu Zhao <[email protected]>
Cc: Nick Piggin <[email protected]>
Cc: [email protected]
---
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
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 <[email protected]>
---
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
> On Dec 28, 2023, at 10:46 AM, Jisheng Zhang <[email protected]> wrote:
>
> From: Nadav Amit <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Jisheng Zhang <[email protected]>
> Cc: Andrea Arcangeli <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Dave Hansen <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Yu Zhao <[email protected]>
> Cc: Nick Piggin <[email protected]>
> Cc: [email protected]
> ---
> 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/[email protected]/
--
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.
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 <[email protected]>
> ---
> 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
On Sat, Dec 30, 2023 at 11:54:02AM +0200, Nadav Amit wrote:
>
>
> > On Dec 28, 2023, at 10:46 AM, Jisheng Zhang <[email protected]> wrote:
> >
> > From: Nadav Amit <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Jisheng Zhang <[email protected]>
> > Cc: Andrea Arcangeli <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > Cc: Andy Lutomirski <[email protected]>
> > Cc: Dave Hansen <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Cc: Yu Zhao <[email protected]>
> > Cc: Nick Piggin <[email protected]>
> > Cc: [email protected]
> > ---
> > 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/[email protected]/
>
>
> --
> 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.
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 <[email protected]>
> > ---
> > 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
On Thu, Dec 28, 2023 at 04:46:41PM +0800, Jisheng Zhang wrote:
> From: Nadav Amit <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Jisheng Zhang <[email protected]>
> Cc: Andrea Arcangeli <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Dave Hansen <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Yu Zhao <[email protected]>
> Cc: Nick Piggin <[email protected]>
> Cc: [email protected]
> ---
> 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
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 <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Jisheng Zhang <[email protected]>
> > Cc: Andrea Arcangeli <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > Cc: Andy Lutomirski <[email protected]>
> > Cc: Dave Hansen <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Cc: Yu Zhao <[email protected]>
> > Cc: Nick Piggin <[email protected]>
> > Cc: [email protected]
> > ---
> > 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
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
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. ;)
On Wed, Jan 03, 2024 at 12:26:29PM -0800, Dave Hansen wrote:
> 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.
You are right, I did not realise freed_tables is reset in
__tlb_reset_range().
--
Catalin
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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
> On Jan 2, 2024, at 4:41 AM, Jisheng Zhang <[email protected]> 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.
On Thu, Jan 04, 2024 at 03:26:43PM +0200, Nadav Amit wrote:
>
>
> > On Jan 2, 2024, at 4:41 AM, Jisheng Zhang <[email protected]> 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?
I think Catalin made the same observation here:
https://lore.kernel.org/r/[email protected]
and it does indeed look broken.
Will