From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qL3Nk-00CCB9-1p for linux-arm-kernel@lists.infradead.org; Sun, 16 Jul 2023 15:09:59 +0000 Date: Sun, 16 Jul 2023 08:09:52 -0700 From: Catalin Marinas Subject: Re: [PATCH v1 11/14] arm64/mm: Wire up PTE_CONT for user mappings Message-ID: References: <20230622144210.2623299-1-ryan.roberts@arm.com> <20230622144210.2623299-12-ryan.roberts@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+lwn-linux-arm-kernel=archive.lwn.net@lists.infradead.org List-Archive: To: Ryan Roberts Cc: Will Deacon , Ard Biesheuvel , Marc Zyngier , Oliver Upton , James Morse , Suzuki K Poulose , Zenghui Yu , Andrey Ryabinin , Alexander Potapenko , Andrey Konovalov , Dmitry Vyukov , Vincenzo Frascino , Andrew Morton , Anshuman Khandual , Matthew Wilcox , Yu Zhao , Mark Rutland , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org On Tue, Jul 04, 2023 at 12:09:31PM +0100, Ryan Roberts wrote: > On 03/07/2023 16:17, Catalin Marinas wrote: > > Hi Ryan, > > > > Some comments below. I did not have time to trim down the quoted text, > > so you may need to scroll through it. > > Thanks for the review! > > Looking at the comments, I think they all relate to implementation. Does that > imply that you are happy with the shape/approach? I can't really tell yet as there are a few dependencies and I haven't applied them to look at the bigger picture. My preference would be to handle the large folio breaking/making in the core code via APIs like set_ptes() and eliminate the loop heuristics in the arm64 code to fold/unfold. Maybe it's not entirely possible I need to look at the bigger picture with all the series applied (and on a bigger screen, writing this reply on a laptop in flight). > Talking with Anshuman yesterday, he suggested putting this behind a new Kconfig > option that defaults to disabled and also adding a command line option to > disable it when compiled in. I think that makes sense for now at least to reduce > risk of performance regression? I'm fine with a Kconfig option (maybe expert) but default enabled, otherwise it won't get enough coverage. AFAICT, the biggest risk of regression is the heuristics for folding/unfolding. In general the overhead should be offset by the reduced TLB pressure but we may find some pathological case where this gets in the way. > > On Thu, Jun 22, 2023 at 03:42:06PM +0100, Ryan Roberts wrote: > >> + /* > >> + * No need to flush here; This is always "more permissive" so we > >> + * can only be _adding_ the access or dirty bit. And since the > >> + * tlb can't cache an entry without the AF set and the dirty bit > >> + * is a SW bit, there can be no confusion. For HW access > >> + * management, we technically only need to update the flag on a > >> + * single pte in the range. But for SW access management, we > >> + * need to update all the ptes to prevent extra faults. > >> + */ > > > > On pre-DBM hardware, a PTE_RDONLY entry (writable from the kernel > > perspective but clean) may be cached in the TLB and we do need flushing. > > I don't follow; The Arm ARM says: > > IPNQBP When an Access flag fault is generated, the translation table entry > causing the fault is not cached in a TLB. > > So the entry can only be in the TLB if AF is already 1. And given the dirty bit > is SW, it shouldn't affect the TLB state. And this function promises to only > change the bits so they are more permissive (so AF=0 -> AF=1, D=0 -> D=1). > > So I'm not sure what case you are describing here? The comment for this function states that it sets the access/dirty flags as well as the write permission. Prior to DBM, the page is marked PTE_RDONLY and we take a fault. This function marks the page dirty by setting the software PTE_DIRTY bit (no need to worry) but also clearing PTE_RDONLY so that a subsequent access won't fault again. We do need the TLBI here since PTE_RDONLY is allowed to be cached in the TLB. Sorry, I did not reply to your other comments (we can talk in person in about a week time). I also noticed you figured the above but I had written it already. -- Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel