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 X-Spam-Level: X-Spam-Status: No, score=-8.6 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 33CD1C433E1 for ; Tue, 28 Jul 2020 10:21:31 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id D923D20792 for ; Tue, 28 Jul 2020 10:21:30 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="KSkcfQ8p" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D923D20792 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 46BE96B0024; Tue, 28 Jul 2020 06:21:30 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 41D306B0025; Tue, 28 Jul 2020 06:21:30 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 331E26B0026; Tue, 28 Jul 2020 06:21:30 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 1E8D86B0024 for ; Tue, 28 Jul 2020 06:21:30 -0400 (EDT) Received: from smtpin22.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id BCFDD180AD81A for ; Tue, 28 Jul 2020 10:21:29 +0000 (UTC) X-FDA: 77087092698.22.cart60_3a0468326f69 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin22.hostedemail.com (Postfix) with ESMTP id 91F2F18038E6A for ; Tue, 28 Jul 2020 10:21:29 +0000 (UTC) X-HE-Tag: cart60_3a0468326f69 X-Filterd-Recvd-Size: 7469 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by imf07.hostedemail.com (Postfix) with ESMTP for ; Tue, 28 Jul 2020 10:21:28 +0000 (UTC) Received: from willie-the-truck (236.31.169.217.in-addr.arpa [217.169.31.236]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 270C820775; Tue, 28 Jul 2020 10:21:26 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1595931688; bh=KFUNXVNI5+UeHlsYjdGkBMdT6cwce8silqrUo0uTwTQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=KSkcfQ8pycM/xKW1MrUYg5B1WTUt1HhtX+V0vzQrIdDFkIhhtrm2Bn3WHVD34l1Gm +ekGuMIlC0mqzD+Vu1iauJt6oOrSt5AGD1qyjbpDidzTKZHwvWJbNMHqWVbIlTHst9 MfDDGjjQ41v6PPESC/Pxo7Dzucln8MEYZGBXj500= Date: Tue, 28 Jul 2020 11:21:23 +0100 From: Will Deacon To: Catalin Marinas Cc: Linus Torvalds , Yang Shi , Andrew Morton , Johannes Weiner , Hillf Danton , Hugh Dickins , Josef Bacik , "Kirill A . Shutemov" , Linux-MM , mm-commits@vger.kernel.org, Matthew Wilcox , xuyu@linux.alibaba.com Subject: Re: [patch 01/15] mm/memory.c: avoid access flag update TLB flush for retried page fault Message-ID: <20200728102122.GA21880@willie-the-truck> References: <20200723211432.b31831a0df3bc2cbdae31b40@linux-foundation.org> <20200724041508.QlTbrHnfh%akpm@linux-foundation.org> <7de20d4a-f86c-8e1f-b238-65f02b560325@linux.alibaba.com> <20200725155841.GA14490@gaia> <20200728092220.GA21800@willie-the-truck> <20200728093910.GB706@gaia> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200728093910.GB706@gaia> User-Agent: Mutt/1.10.1 (2018-07-13) X-Rspamd-Queue-Id: 91F2F18038E6A X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam04 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, Jul 28, 2020 at 10:39:11AM +0100, Catalin Marinas wrote: > On Tue, Jul 28, 2020 at 10:22:20AM +0100, Will Deacon wrote: > > On Sat, Jul 25, 2020 at 04:58:43PM +0100, Catalin Marinas wrote: > > > On Fri, Jul 24, 2020 at 06:29:43PM -0700, Linus Torvalds wrote: > > > > For any architecture that guarantees that a page fault will always > > > > flush the old TLB entry for this kind of situation, that > > > > flush_tlb_fix_spurious_fault() thing can be a no-op. > > > > > > > > So that's why on x86, we just do > > > > > > > > #define flush_tlb_fix_spurious_fault(vma, address) do { } while (0) > > > > > > > > and have no issues. > > > > > > > > Note that it does *not* need to do any cross-CPU flushing or anything > > > > like that. So it's actually wrong (I think) to have that default > > > > fallback for > > > > > > > > #define flush_tlb_fix_spurious_fault(vma, address) > > > > flush_tlb_page(vma, address) > > > > > > > > because flush_tlb_page() is the serious "do cross CPU etc". > > > > > > > > Does the arm64 flush_tlb_page() perhaps do the whole expensive > > > > cross-CPU thing rather than the much cheaper "just local invalidate" > > > > version? > > > > > > I think it makes sense to have a local-only > > > flush_tlb_fix_spurious_fault(), but with ptep_set_access_flags() updated > > > to still issue the full broadcast TLBI. In addition, I added a minor > > > optimisation to avoid the TLB flush if the old pte was not accessible. > > > In a read-access fault case (followed by mkyoung), the TLB wouldn't have > > > cached a non-accessible pte (not sure it makes much difference to Yang's > > > case). Anyway, from ARMv8.1 onwards, the hardware handles the access > > > flag automatically. > > > > > > I'm not sure the first dsb(nshst) below is of much use in this case. If > > > we got a spurious fault, the write to the pte happened on a different > > > CPU (IIUC, we shouldn't return to user with updated ptes without a TLB > > > flush on the same CPU). Anyway, we can refine this if it solves Yang's > > > performance regression. > > > > > > -------------8<----------------------- > > > diff --git a/arch/arm64/include/asm/tlbflush.h b/arch/arm64/include/asm/tlbflush.h > > > index d493174415db..d1401cbad7d4 100644 > > > --- a/arch/arm64/include/asm/tlbflush.h > > > +++ b/arch/arm64/include/asm/tlbflush.h > > > @@ -268,6 +268,20 @@ static inline void flush_tlb_page(struct vm_area_struct *vma, > > > dsb(ish); > > > } > > > > > > +static inline void local_flush_tlb_page(struct vm_area_struct *vma, > > > + unsigned long uaddr) > > > +{ > > > + unsigned long addr = __TLBI_VADDR(uaddr, ASID(vma->vm_mm)); > > > + > > > + dsb(nshst); > > > + __tlbi(vale1, addr); > > > + __tlbi_user(vale1, addr); > > > + dsb(nsh); > > > +} > > > + > > > +#define flush_tlb_fix_spurious_fault(vma, address) \ > > > + local_flush_tlb_page(vma, address) > > > > Why can't we just have flush_tlb_fix_spurious_fault() be a NOP on arm64? > > Possibly, as long as any other optimisations only defer the TLB flushing > for relatively short time (the fault is transient, it will get a > broadcast TLBI eventually). Define relatively short ;) There's always a window where these things can happen, and we try to batch up TLB invalidation to avoid issuing tonnes of messages, so it's a trade-off. The more important thing seems to be that we don't have all those faulting CPUs repeating the TLB invalidation. > Either way, it's worth benchmarking the above patch but with > flush_tlb_fix_spurious_fault() a no-op (we still need flush_tlb_page() > in ptep_set_access_flags()). Xu, Yang, could you please give it a try? (agreed on the flush_tlb_page() in ptep_set_access_flags()) > > Given that the architecture prohibits the TLB from caching invalid entries, > > then software access/dirty is fine without additional flushing. > > The access fault is fine, the TLB has not cached the entry. For a dirty > fault, however, the TLB could cache a read-only mapping, so it does need > flushing. Sorry, yes, you're right. > Question is, do we make the pte dirty anywhere without a > subsequent (broadcast) TLBI? Given that we practically always use broadcast TLBI, then if we do this it could cause the local CPU to get wedged as well. I think we can rely on ptep_set_access_flags() to handle this for us. > > The only > > problematic case I can think of is on the invalid->valid (i.e. map) path, > > where we elide the expensive DSB instruction because (a) most CPUs have a > > walker that can snoop the store buffer and (b) even if they don't, the > > store buffer tends to drain by the time we get back to userspace. Even > > if that was a problem, flush_tlb_fix_spurious_fault() wouldn't be the > > right hook, since the DSB must occur on the CPU that did the pte update. > > I guess the best a CPU can do is attempt the page table walk again, in > the hope that the write buffer on the other CPU eventually drains. It _will_ drain on context-switch as we have a DSB in there, but in practice it will happen way before that. Will