Message ID | 20231228084642.1765-2-jszhang@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-12527-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7301:6f82:b0:100:9c79:88ff with SMTP id tb2csp1879582dyb; Thu, 28 Dec 2023 00:59:54 -0800 (PST) X-Google-Smtp-Source: AGHT+IH5YtGl1nrT6BK1gW48RD7SZcklAgwp4PCYg2Tjz0Q8US7S9fD8IzSX5Fj2XiD17ZpGofAC X-Received: by 2002:a17:906:18d:b0:a26:ea22:aca with SMTP id 13-20020a170906018d00b00a26ea220acamr2530213ejb.127.1703753994239; Thu, 28 Dec 2023 00:59:54 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1703753994; cv=none; d=google.com; s=arc-20160816; b=vdxZ+kJKMORxYHtmxAkxnBXq0vZm7AjKA7Lq0MTxYg9yQjAEt97zG9ji8LilbxmkIm U0DWdn6EgsXUSs3Cv1PUkjfBOiIj/vdsfazFlVOi2Wgo1pj+jwtRJZuC28qk9eOvnJ06 1r63OMjBOcGg5k0GRChkQKorQXdrLdOA7r6e8ZXP+qBoPExFiLTb7Zp3B9S2dDnC0TxM GQxujgP/qSSAJxp2lu94lMUCdsx/d3X2uUpPnCwwntdtblTIWgC9mZazUfTMSjqor9LI PNWzjQ3KfFOOwo5PFjc7hn2mIlevoULlHYCXLE9rtMX/bbaZG1zBoRaHT6XcFa/fFZGK ykog== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:references:in-reply-to:message-id :date:subject:cc:to:from:dkim-signature; bh=Rbu3zDK4egabfYRNLq5ZDg2lMGMbWX/VoBKV5Lc26JI=; fh=DoOA9XwzitWn/b9G1f1qTy8IVH8KJwoCtLNtKbu9Qp4=; b=UIsqyNof0dWNLrzZ2jIKah6e6fCePeVthIJM8DpKbsBgimabfSdaH6JkzgFVtnRAKP 7R7/FVsy+LRHqaJjcbT8vh3zDaAGxWwliCYC/IxiwxhyQEn27McnvWJgS9U4WccHtDXx GtUsYuVGPw+Nqa4YtVoO+UGSsx4MaxNSRzsSZe5VsUZW9hzxUe8EBXov5q0ynkgwMYsK xaQ4pIfsoWUVAo1kPX2a0CSNB5qnZBihLJl5hiIGeRvRHtWDVeqRK6UbVZv/B3yDVJ3F bgxWPw2GJ0WY5C5rMmim+Tsn0w2xdbm8Qwawjy9FacqZVb0pL4M+jsc3brK9EALdSXEG WD3A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b="VPt/k23K"; spf=pass (google.com: domain of linux-kernel+bounces-12527-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) smtp.mailfrom="linux-kernel+bounces-12527-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from am.mirrors.kernel.org (am.mirrors.kernel.org. [2604:1380:4601:e00::3]) by mx.google.com with ESMTPS id i19-20020a170906115300b00a2365ea58a6si6927533eja.665.2023.12.28.00.59.54 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 28 Dec 2023 00:59:54 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-12527-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b="VPt/k23K"; spf=pass (google.com: domain of linux-kernel+bounces-12527-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) smtp.mailfrom="linux-kernel+bounces-12527-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id D76881F22762 for <ouuuleilei@gmail.com>; Thu, 28 Dec 2023 08:59:53 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id EB34C6FC9; Thu, 28 Dec 2023 08:59:29 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="VPt/k23K" X-Original-To: linux-kernel@vger.kernel.org Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 543306FA1; Thu, 28 Dec 2023 08:59:27 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id EB820C433CA; Thu, 28 Dec 2023 08:59:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1703753966; bh=16+Nu0Wb1JkSQStq2mcU9kdq58a4r2QX4o0XnGMbW1g=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=VPt/k23KQR/ek+Wk4Mb0Ffgqjpr0C4tGoF8uMnFE+csMqgfr093NXmCFQN67kLxmg 2wX671IgLVNFprhYGlOqZT9sQd45M//tMA96Y48PuyxuAUw2YYrGJheIf3jUNzegsO oZr1LwJ+Pd98LSUtkZQ1GQJGY/kKAzRUqhRAQ2LGss8txZ4sD1RLgnpeb+dxBj+bEm lTFy+2WWHg42Kl6nbbR4U7U8bNB7+1AF95XqLf72vf4OQxZI1A08+R/V8QaP95mAv4 W39UhXGY1i/064M112ShThexcH0ieACEEyC3qc6Oesf9LT7f5gDAuaHeHCYJG86mq+ AIf2i7+FdSkcQ== From: Jisheng Zhang <jszhang@kernel.org> To: Will Deacon <will@kernel.org>, "Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com>, Andrew Morton <akpm@linux-foundation.org>, Nick Piggin <npiggin@gmail.com>, Peter Zijlstra <peterz@infradead.org>, Catalin Marinas <catalin.marinas@arm.com>, Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, Albert Ou <aou@eecs.berkeley.edu>, Arnd Bergmann <arnd@arndb.de> Cc: linux-arch@vger.kernel.org, linux-mm@kvack.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org, Nadav Amit <namit@vmware.com>, Andrea Arcangeli <aarcange@redhat.com>, Andy Lutomirski <luto@kernel.org>, Dave Hansen <dave.hansen@linux.intel.com>, Thomas Gleixner <tglx@linutronix.de>, Yu Zhao <yuzhao@google.com>, x86@kernel.org Subject: [PATCH 1/2] mm/tlb: fix fullmm semantics Date: Thu, 28 Dec 2023 16:46:41 +0800 Message-Id: <20231228084642.1765-2-jszhang@kernel.org> X-Mailer: git-send-email 2.40.0 In-Reply-To: <20231228084642.1765-1-jszhang@kernel.org> References: <20231228084642.1765-1-jszhang@kernel.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1786515547814858094 X-GMAIL-MSGID: 1786515547814858094 |
Series |
riscv: tlb: avoid tlb flushing on exit & execve
|
|
Commit Message
Jisheng Zhang
Dec. 28, 2023, 8:46 a.m. UTC
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(-)
Comments
> 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/
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.
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
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
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().
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().
> 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?
On Thu, Jan 04, 2024 at 03:26:43PM +0200, Nadav Amit wrote: > > > > 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? I think Catalin made the same observation here: https://lore.kernel.org/r/ZZWh4c3ZUtadFqD1@arm.com and it does indeed look broken. Will
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; }