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=-6.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,UNPARSEABLE_RELAY,USER_AGENT_SANE_1 autolearn=no 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 98BF9C433E1 for ; Mon, 27 Jul 2020 17:52:27 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 4EF122070B for ; Mon, 27 Jul 2020 17:52:27 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4EF122070B Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.alibaba.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id A255D6B0002; Mon, 27 Jul 2020 13:52:26 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 9D5266B0005; Mon, 27 Jul 2020 13:52:26 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8EBF96B0006; Mon, 27 Jul 2020 13:52:26 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0242.hostedemail.com [216.40.44.242]) by kanga.kvack.org (Postfix) with ESMTP id 7ABD76B0002 for ; Mon, 27 Jul 2020 13:52:26 -0400 (EDT) Received: from smtpin03.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id E11FB1EE6 for ; Mon, 27 Jul 2020 17:52:25 +0000 (UTC) X-FDA: 77084600250.03.sock94_1b0989126f63 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin03.hostedemail.com (Postfix) with ESMTP id AD26028A4E9 for ; Mon, 27 Jul 2020 17:52:25 +0000 (UTC) X-HE-Tag: sock94_1b0989126f63 X-Filterd-Recvd-Size: 7370 Received: from out30-57.freemail.mail.aliyun.com (out30-57.freemail.mail.aliyun.com [115.124.30.57]) by imf14.hostedemail.com (Postfix) with ESMTP for ; Mon, 27 Jul 2020 17:52:24 +0000 (UTC) X-Alimail-AntiSpam:AC=PASS;BC=-1|-1;BR=01201311R201e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=e01f04397;MF=yang.shi@linux.alibaba.com;NM=1;PH=DS;RN=13;SR=0;TI=SMTPD_---0U4.gH1y_1595872331; Received: from US-143344MP.local(mailfrom:yang.shi@linux.alibaba.com fp:SMTPD_---0U4.gH1y_1595872331) by smtp.aliyun-inc.com(127.0.0.1); Tue, 28 Jul 2020 01:52:18 +0800 Subject: Re: [patch 01/15] mm/memory.c: avoid access flag update TLB flush for retried page fault To: Linus Torvalds Cc: Andrew Morton , Catalin Marinas , Johannes Weiner , Hillf Danton , Hugh Dickins , Josef Bacik , "Kirill A . Shutemov" , Linux-MM , mm-commits@vger.kernel.org, Will Deacon , Matthew Wilcox , xuyu@linux.alibaba.com References: <20200723211432.b31831a0df3bc2cbdae31b40@linux-foundation.org> <20200724041508.QlTbrHnfh%akpm@linux-foundation.org> <7de20d4a-f86c-8e1f-b238-65f02b560325@linux.alibaba.com> From: Yang Shi Message-ID: <45015c63-e719-a58a-7d07-6c156273a890@linux.alibaba.com> Date: Mon, 27 Jul 2020 10:52:01 -0700 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-Rspamd-Queue-Id: AD26028A4E9 X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam05 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 7/24/20 6:29 PM, Linus Torvalds wrote: > On Fri, Jul 24, 2020 at 5:37 PM Yang Shi wrote: >> A follow-up question about your comment in the previous email "The >> notion of "this is a retry, so let's do nothing" is fundamentally >> wrong.", do you mean it is not safe? > I mean it fails my "smell test". > > The patch didn't just avoid the TLB flush, it avoided all the other > "mark it dirty and young" things too. And that made me go "why would > RETRY be different in this regard"? > > It sounds unsafe, because it basically means that a retry does > something else than the initial page fault handling would do. > > See what worries me and makes me go "that's not safe"? > >> Or since we have pte_same check, we >> should just rely on it to skip unnecessary TLB flush? > Right. That makes me much happier, because if the retry flag is only > used to avoid a TLB flush (when the pte's are identical, of course), > then I feel that the retry path is _logically_ all the same. The page > tables end up looking exactly the same, and the only difference is > whether we do that TLB invalidate for a spurious fault. > > And that, in turn, makes me feel it is safe, because even if it turns > out that "yes, we keep getting a spurious fault because we have some > stale TLB entries", then checking the RETRY bit is fine: we'll do a > full page fault next time around without the retry bit set. > > So that's why I feel that your patch is sketchy and unsafe, but I > don't worry about testing the RETRY bit in that "clear spurious TLB > entries" case. > > See? Yes, I got your point. Thanks for elaborating. > >>> Can somebody flesh out the comment about the >>> "spurious_protection_fault()" thing? Because something like this I >>> wouldn't mind, but I'd like that comment to explain the >>> FAULT_FLAG_WRITE part too. >> I'm not quite familiar with other architectures, my wild guess is >> FAULT_FLAG_WRITE is a cheap way to tell us if this is a .text page or >> not. > Yes. However, I'm not seeing why a text page would be so special. > > IOW, if it's ok to skip the TLB flush fo ra text page, then why isn't > it ok to skip for a normal page? It looks normal page is skipped too unless it is a write fault. The comment might be a little bit misleading. Read fault should just change young bit and typically TLB won't get flushed if just young bit is changed and TLB flush can be deferred again to write fault which may change access permission and/or dirty bit. > > My suspicion is that we have stale TLB entries for potentially > multiple different reasons: > > - software optimizations, where we decide "skip the TLB flush, > because it's expensive and it is likely to never matter". > > I have a _memory_ of us doing this when we have a pure "loosening" > of the protections (IOW, make something writable that wasn't writable > before), but I can't actually find the code. I'm thinking things like > the wp_page_reuse() case. > > - temporarily stale TLB entries because we've _just_updated them on > another CPU, but it hasn't gotten to the actual TLB flush yet. > > By the time we actually get to this point, we'll have serialized > with the page table lock, but the *fault* happened when the CPU saw > the original stale TLB entry, so we took the fault with what is now a > stale TLB entry. > > - actual software bugs where we've not flushed the TLB properly. > > Anyway, the _reason_ for that "flush_tlb_fix_spurious_fault()" is that > some architectures don't flush their TLB on a fault. > > So if you don't flush the TLB when talking a page fault, and you may > have these stale TLB entries around, you'll just keep faulting until > enough other system event happens that just ends up flushing the TLB > sufficiently. > > On an otherwise idle system, that "keep faulting until enough other > system event happens" might be effectively forever. > > 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? > > The "random letter combination" thing that ARM documentation uses for > these things is really confusing, but I think the "is" in "vale1is" > means that it's broadcast to all "inner sharable" - ie CPU cores. > > I get the feeling that on arm64, flush_tlb_fix_spurious_fault() should > either be a no-op, or it should perhaps be a non-broadcasting version > of the TLB invalidates, and use just "vale1" > > Catalin? Will? > > Linus