Re: [RFC 01/20] mm/tlb: fix fullmm semantics
> On Feb 3, 2021, at 1:44 AM, Will Deacon wrote:
>
> On Tue, Feb 02, 2021 at 01:35:38PM -0800, Nadav Amit wrote:
>>> On Feb 2, 2021, at 3:00 AM, Peter Zijlstra wrote:
>>>
>>> On Tue, Feb 02, 2021 at 01:32:36AM -0800, Nadav Amit wrote:
> On Feb 1, 2021, at 3:36 AM, Peter Zijlstra wrote:
>
>
> https://lkml.kernel.org/r/20210127235347.1402-1-w...@kernel.org
I have seen this series, and applied my patches on it.
Despite Will’s patches, there were still inconsistencies between fullmm
and need_flush_all.
Am I missing something?
>>>
>>> I wasn't aware you were on top. I'll look again.
>>
>> Looking on arm64’s tlb_flush() makes me think that there is currently a bug
>> that this patch fixes. Arm64’s tlb_flush() does:
>>
>> /*
>>* If we're tearing down the address space then we only care about
>>* invalidating the walk-cache, since the ASID allocator won't
>>* reallocate our ASID without invalidating the entire TLB.
>>*/
>> if (tlb->fullmm) {
>> if (!last_level)
>> flush_tlb_mm(tlb->mm);
>> return;
>> }
>>
>> But currently tlb_mmu_finish() can mistakenly set fullmm incorrectly (if
>> mm_tlb_flush_nested() is true), which might skip the TLB flush.
>
> But in that case isn't 'freed_tables' set to 1, so 'last_level' will be
> false and we'll do the flush in the code above?
Indeed. You are right. So no rush.
Re: [RFC 01/20] mm/tlb: fix fullmm semantics
On Tue, Feb 02, 2021 at 01:35:38PM -0800, Nadav Amit wrote:
> > On Feb 2, 2021, at 3:00 AM, Peter Zijlstra wrote:
> >
> > On Tue, Feb 02, 2021 at 01:32:36AM -0800, Nadav Amit wrote:
> >>> On Feb 1, 2021, at 3:36 AM, Peter Zijlstra wrote:
> >>>
> >>>
> >>> https://lkml.kernel.org/r/20210127235347.1402-1-w...@kernel.org
> >>
> >> I have seen this series, and applied my patches on it.
> >>
> >> Despite Will’s patches, there were still inconsistencies between fullmm
> >> and need_flush_all.
> >>
> >> Am I missing something?
> >
> > I wasn't aware you were on top. I'll look again.
>
> Looking on arm64’s tlb_flush() makes me think that there is currently a bug
> that this patch fixes. Arm64’s tlb_flush() does:
>
>/*
> * If we're tearing down the address space then we only care about
> * invalidating the walk-cache, since the ASID allocator won't
> * reallocate our ASID without invalidating the entire TLB.
> */
>if (tlb->fullmm) {
>if (!last_level)
>flush_tlb_mm(tlb->mm);
>return;
>}
>
> But currently tlb_mmu_finish() can mistakenly set fullmm incorrectly (if
> mm_tlb_flush_nested() is true), which might skip the TLB flush.
But in that case isn't 'freed_tables' set to 1, so 'last_level' will be
false and we'll do the flush in the code above?
Will
Re: [RFC 01/20] mm/tlb: fix fullmm semantics
> On Feb 2, 2021, at 3:00 AM, Peter Zijlstra wrote:
>
> On Tue, Feb 02, 2021 at 01:32:36AM -0800, Nadav Amit wrote:
>>> On Feb 1, 2021, at 3:36 AM, Peter Zijlstra wrote:
>>>
>>>
>>> https://lkml.kernel.org/r/20210127235347.1402-1-w...@kernel.org
>>
>> I have seen this series, and applied my patches on it.
>>
>> Despite Will’s patches, there were still inconsistencies between fullmm
>> and need_flush_all.
>>
>> Am I missing something?
>
> I wasn't aware you were on top. I'll look again.
Looking on arm64’s tlb_flush() makes me think that there is currently a bug
that this patch fixes. Arm64’s tlb_flush() does:
/*
* If we're tearing down the address space then we only care about
* invalidating the walk-cache, since the ASID allocator won't
* reallocate our ASID without invalidating the entire TLB.
*/
if (tlb->fullmm) {
if (!last_level)
flush_tlb_mm(tlb->mm);
return;
}
But currently tlb_mmu_finish() can mistakenly set fullmm incorrectly (if
mm_tlb_flush_nested() is true), which might skip the TLB flush.
Lucky for us, arm64 flushes each VMA separately (which as we discussed
separately may not be necessary), so the only PTEs that might not be flushed
are PTEs that are updated concurrently by another thread that also defer
their flushes. It therefore seems that the implications are more on the
correctness of certain syscalls (e.g., madvise(DONT_NEED)) without
implications on security or memory corruptions.
Let me know if you want me to send this patch separately with an updated
commit log for faster inclusion.
Re: [RFC 01/20] mm/tlb: fix fullmm semantics
On Tue, Feb 02, 2021 at 01:32:36AM -0800, Nadav Amit wrote: > > On Feb 1, 2021, at 3:36 AM, Peter Zijlstra wrote: > > > > > > https://lkml.kernel.org/r/20210127235347.1402-1-w...@kernel.org > > I have seen this series, and applied my patches on it. > > Despite Will’s patches, there were still inconsistencies between fullmm > and need_flush_all. > > Am I missing something? I wasn't aware you were on top. I'll look again.
Re: [RFC 01/20] mm/tlb: fix fullmm semantics
> On Feb 1, 2021, at 3:36 AM, Peter Zijlstra wrote: > > > https://lkml.kernel.org/r/20210127235347.1402-1-w...@kernel.org I have seen this series, and applied my patches on it. Despite Will’s patches, there were still inconsistencies between fullmm and need_flush_all. Am I missing something?
Re: [RFC 01/20] mm/tlb: fix fullmm semantics
https://lkml.kernel.org/r/20210127235347.1402-1-w...@kernel.org
Re: [RFC 01/20] mm/tlb: fix fullmm semantics
> On Jan 30, 2021, at 6:57 PM, Andy Lutomirski wrote: > > On Sat, Jan 30, 2021 at 5:19 PM Nadav Amit wrote: >>> On Jan 30, 2021, at 5:02 PM, Andy Lutomirski wrote: >>> >>> On Sat, Jan 30, 2021 at 4:16 PM Nadav Amit wrote: From: Nadav Amit 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. >>> >>> Maybe also rename fullmm? >> >> Possible. How about mm_torn_down? > > Sure. Or mm_exiting, perhaps? mm_exiting indeed sounds better.
Re: [RFC 01/20] mm/tlb: fix fullmm semantics
On Sat, Jan 30, 2021 at 5:19 PM Nadav Amit wrote: > > > On Jan 30, 2021, at 5:02 PM, Andy Lutomirski wrote: > > > > On Sat, Jan 30, 2021 at 4:16 PM Nadav Amit wrote: > >> From: Nadav Amit > >> > >> 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. > > > > Maybe also rename fullmm? > > Possible. How about mm_torn_down? Sure. Or mm_exiting, perhaps? > > I should have also changed the comment in tlb_finish_mmu().
Re: [RFC 01/20] mm/tlb: fix fullmm semantics
> On Jan 30, 2021, at 5:02 PM, Andy Lutomirski wrote: > > On Sat, Jan 30, 2021 at 4:16 PM Nadav Amit wrote: >> From: Nadav Amit >> >> 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. > > Maybe also rename fullmm? Possible. How about mm_torn_down? I should have also changed the comment in tlb_finish_mmu().
Re: [RFC 01/20] mm/tlb: fix fullmm semantics
On Sat, Jan 30, 2021 at 4:16 PM Nadav Amit wrote: > > From: Nadav Amit > > 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. Maybe also rename fullmm?
[RFC 01/20] mm/tlb: fix fullmm semantics
From: Nadav Amit 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. Signed-off-by: Nadav Amit Cc: Andrea Arcangeli Cc: Andrew Morton Cc: Andy Lutomirski Cc: Dave Hansen Cc: Peter Zijlstra Cc: Thomas Gleixner Cc: Will Deacon Cc: Yu Zhao Cc: Nick Piggin Cc: x...@kernel.org --- include/asm-generic/tlb.h | 2 +- mm/mmu_gather.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/asm-generic/tlb.h b/include/asm-generic/tlb.h index 2c68a545ffa7..eea113323468 100644 --- a/include/asm-generic/tlb.h +++ b/include/asm-generic/tlb.h @@ -420,7 +420,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 0dc7149b0c61..5a659d4e59eb 100644 --- a/mm/mmu_gather.c +++ b/mm/mmu_gather.c @@ -323,7 +323,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.25.1
