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 DB4A6C433E1 for ; Tue, 28 Jul 2020 00:13:15 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 630DB20786 for ; Tue, 28 Jul 2020 00:13:15 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 630DB20786 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 C2DA36B0002; Mon, 27 Jul 2020 20:13:14 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id BDDFC6B0005; Mon, 27 Jul 2020 20:13:14 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id AF4786B0006; Mon, 27 Jul 2020 20:13:14 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0095.hostedemail.com [216.40.44.95]) by kanga.kvack.org (Postfix) with ESMTP id 9B3DA6B0002 for ; Mon, 27 Jul 2020 20:13:14 -0400 (EDT) Received: from smtpin04.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 32BA7181AC9B6 for ; Tue, 28 Jul 2020 00:13:14 +0000 (UTC) X-FDA: 77085559908.04.name44_510863c26f65 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin04.hostedemail.com (Postfix) with ESMTP id 16E8C80019B4 for ; Tue, 28 Jul 2020 00:13:14 +0000 (UTC) X-HE-Tag: name44_510863c26f65 X-Filterd-Recvd-Size: 8135 Received: from out30-45.freemail.mail.aliyun.com (out30-45.freemail.mail.aliyun.com [115.124.30.45]) by imf32.hostedemail.com (Postfix) with ESMTP for ; Tue, 28 Jul 2020 00:13:12 +0000 (UTC) X-Alimail-AntiSpam:AC=PASS;BC=-1|-1;BR=01201311R911e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=e01e01355;MF=xuyu@linux.alibaba.com;NM=1;PH=DS;RN=15;SR=0;TI=SMTPD_---0U40bPNo_1595895185; Received: from xuyu-mbp15.local(mailfrom:xuyu@linux.alibaba.com fp:SMTPD_---0U40bPNo_1595895185) by smtp.aliyun-inc.com(127.0.0.1); Tue, 28 Jul 2020 08:13:06 +0800 Subject: Re: [patch 01/15] mm/memory.c: avoid access flag update TLB flush for retried page fault To: Linus Torvalds , Yang Shi , linux-arch Cc: Catalin Marinas , Andrew Morton , Johannes Weiner , Hillf Danton , Hugh Dickins , Josef Bacik , "Kirill A . Shutemov" , Linux-MM , mm-commits@vger.kernel.org, Will Deacon , Matthew Wilcox References: <20200723211432.b31831a0df3bc2cbdae31b40@linux-foundation.org> <20200724041508.QlTbrHnfh%akpm@linux-foundation.org> <0323de82-cfbd-8506-fa9c-a702703dd654@linux.alibaba.com> <20200727110512.GB25400@gaia> <39560818-463f-da3a-fc9e-3a4a0a082f61@linux.alibaba.com> From: Yu Xu Message-ID: <57d71476-0522-f5dc-6e95-37bd5d41206d@linux.alibaba.com> Date: Tue, 28 Jul 2020 08:13:05 +0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 16E8C80019B4 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/28/20 2:37 AM, Linus Torvalds wrote: > [ Adding linux-arch, just to make other architectures aware of this issue too. > > We have a "flush_tlb_fix_spurious_fault()" thing to take care of the > "TLB may contain stale entries, we can't take the same fault over and > over again" situation. > > On x86, it's a no-op, because x86 doesn't do that. x86 will re-walk > the page tables - or possibly just always invalidate the faulting TLB > entry - before taking a fault, so there can be no long-term stale > TLB's. > > Other architectures may or may not need explicit "invalidate this > TLB entry, because if you make no changes to the page tables, I'll > just otherwise take this fault again. Forever". That is what > "flush_tlb_fix_spurious_fault()" does. > > NOTE! One reason for a stale TLB entry is that another CPU has > already done the change, and is just _about_ to flush the TLB, but the > hardware took the fault before it did so. The code is under the page > table lock, but the hardware fault handler doesn't know or care. So by > the time we get to "flush_tlb_fix_spurious_fault()", we _will_ have > synchronized (because we took the page table lock), and it's entirely > possible that the architecture thus has nothing to do. Make it a > no-op. > > The other reason for a stale TLB entry is if you don't do the > cross-CPU flush for "minor" events that don't matter (ie turning > things dirty, things like that). Rather than flush the TLB, you _want_ > the other CPU to take the fault in the (presumabl;y unlikely) case > that it had that old TLB entry in the first place, and thought _it_ > needed to do mark it dirty. > > Anyway, theres' a reason for "flush_tlb_fix_spurious_fault()", but > not all architectures need it. > > HOWEVER. > > On architectures that don't explicitly define it, it falls back to a > default of "flush_tlb_page()", which sounds sane, but in fact is > completely insane and horribly horribly wrong. > > It's completely insane and horribly wrong, because that fallback > predates the "everybody is SMP" days. On UP, it's fine and sane. > > But on SMP, it's absolutely horrendously bad. Because > flush_tlb_fix_spurious_fault() should not do any cross-CPU > invalidates. > > It looks like arm64 got this nasty performance problem because of > this all, with the cross-CPU invalidates being insanely expensive, and > completely pointless - and easy to hit in some circumstances. > > It looks like powerpc people at least thought about this, and only > do it if there is a coprocessor. Which sounds a bit confused, but I > don't know the rules. > > It looks like a lot of others are ok mainly because they don't do > SMP, or they don't have the kinds of loads where this matters. > > But I wanted to cc the arch mailing list, to make people more aware > of it. And we *should* change the default. It shouldn't be > "flush_tlb_page()". It _should_ be "local_flush_tlb_page()", but we > don't have that, although many architectures implement something like > that as part of their SMP invalidation support ] > > On Mon, Jul 27, 2020 at 11:04 AM Yang Shi wrote: >> >> It looks Linus's patch has better data. It seems sane to me since >> Catalin's patch still needs flush TLB in the shared domain. > > Well, my patch as posted never built at all, I think. > > Looking back at that patch, I used FAULT_FLAG_RETRY. But that's not > the correct name for any of the bits. > > So you must have fixed it. Did you make it use "FAULT_FLAG_TRIED"? > Because that's the right bit - don't flush if this is actually the > second (or more) attempt. Yes, I fixed it with "FAULT_FLAG_TRIED". +static inline bool spurious_protection_fault(unsigned int flags) +{ + if (flags & FAULT_FLAG_TRIED) + return false; + return flags & FAULT_FLAG_WRITE; +} Thanks, Yu > > But I'm a bit worried that you would have used one of the other bits > (FAULT_FLAG_ALLOW_RETRY or FAULT_FLAG_RETRY_NOWAIT), and that would be > wrong. Those get set on the first attempt to say "you _may_ retry", > but they get set on the first one. > > That just shows how much I tested the patch I sent out. It was > whitespace-damaged on purpose, but I still want to check. > > The "FAULT_FLAG_TRIED" bit I believe is reasonable to test. That one > literally says "I've gone through this once already, don't bother with > spurious faults". But I don't think it triggers much in practice. We > seldom actually retry faults, it needs a page that we actually start > IO on (and dropped the mmap lock for) to happen. It wouldn't happen on > the "turn existing page dirty" case, for example. > > The "FAULT_FLAG_WRITE" bit is what we test right now. I think it's > wrong. I think it is a "this happens to work" bit, and cuts down on a > lot of common cases, by simply skipping something that might be needed > but basically never is. > > So I think a lot of this is dodgy. It doesn't matter on x86, and > nobody cared. Because x86 will always re-walk the page tables before > taking an architectural fault (the same way it walks them for > dirty/accessed bit updates - you could think of x86 as doing all the > things everybody else does in software, they just do in the hw walker > micro-fault logic instead). > > A local TLB invalidate of a single virtual address should be basically > free. We're talking single cycles kind of free. The problem here isn't > the flush_tlb_fix_spurious_fault() itself, the problem here is that > arm64 (and pretty much everybody else who uses the default fallback) > does something horribly horribly wrong, and doesn't do the free > version. > > Linus >