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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 913B9C4167B for ; Tue, 28 Nov 2023 20:24:06 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7C81A6B033A; Tue, 28 Nov 2023 15:24:05 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 79FAF6B0346; Tue, 28 Nov 2023 15:24:05 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 63F1D6B034D; Tue, 28 Nov 2023 15:24:05 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 5597B6B033A for ; Tue, 28 Nov 2023 15:24:05 -0500 (EST) Received: from smtpin25.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 307C880356 for ; Tue, 28 Nov 2023 20:24:05 +0000 (UTC) X-FDA: 81508489650.25.14890C5 Received: from mail-vk1-f172.google.com (mail-vk1-f172.google.com [209.85.221.172]) by imf04.hostedemail.com (Postfix) with ESMTP id 5DA834001C for ; Tue, 28 Nov 2023 20:24:03 +0000 (UTC) Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=PRKWoc7M; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf04.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.221.172 as permitted sender) smtp.mailfrom=21cnbao@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1701203043; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=kFSKYAwLLr04eh+vVEp6SSPOAXKBW77ZjLnzCS5PLp4=; b=lE8lIHyYm6gywRXvACUrkbI2mOBObXEsUWtlSVmfQJA+E9+V2XS6GybdlLEpZkcWzbT73Z 5YPzCPGF1+e7U7RCi/OLV5ktoh0ybdK32dIsJMdYhGnlg18S6XIK9XZzR7YaqVyynIQmAl mafSoxCHGvDR/nUjPJEYTVST2W6I+Bk= ARC-Authentication-Results: i=1; imf04.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=PRKWoc7M; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf04.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.221.172 as permitted sender) smtp.mailfrom=21cnbao@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1701203043; a=rsa-sha256; cv=none; b=wLb/eEjthB8o8+4d09cjiBlEppR0S2ZDjOwXB26ptPquRYv35jUlCkZV5oQaZx/6fkiAqg VilWNS+0z1Rwumeedt4Uxe/ApYeHgCb7K9SZx5dmkSXM+DEoax516ojqXY1oWL2yBztRhJ 2NkS1rP0pVq3TLXmdZHE+Y8g6QJ9wzY= Received: by mail-vk1-f172.google.com with SMTP id 71dfb90a1353d-4ac28cab4efso1437950e0c.0 for ; Tue, 28 Nov 2023 12:24:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1701203042; x=1701807842; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=kFSKYAwLLr04eh+vVEp6SSPOAXKBW77ZjLnzCS5PLp4=; b=PRKWoc7MGcP2vdI0JS4dH6ax+wlzMYu98iQ7oLi2aB71kTE3C0I+ZUtoIHdCe2t/87 ZgZYp+EFddB2cvPFkKWKR29XNoQkeeLPrdapXinT9hQAiVmrsscjG5TK+4R3hUmpubGk rX0r+8gdqPmaDiZg5Z5px5W25iThMSf8AHyApnjlzMc+Rk8Wei32Xo/Xo1kWfeVxdRN4 fLgGRYEMXgw864fsTV5zm4PR0GR+eEijuqovJ/zPJvhihlegBO7fq9jKd/LMV+6aYbWJ DcCw+GTJnj3sF8/7fb3oU59NxNKFsC/qc0Qdp0nwbdL6CrTP0SEhnrGlxwF7Wde5Zpns benw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701203042; x=1701807842; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=kFSKYAwLLr04eh+vVEp6SSPOAXKBW77ZjLnzCS5PLp4=; b=lwsMZ+PnKxzpkUZV5d8fIyMgYLTKtMdckDiGsot22ippP0++c4/ZlTlWcCu+1Tw5Cy UZ/wD/zSlgdDkVKly0ByL7ed0BFGCXXyWJcXzYMf6WqInJC0ZyhtAx90XCg+4CPNYxAg eJutnm48GRFvRqQVL2JcCwb8luZBsSpcqT3yC/XlRFlm3qqggkGAShwwvtFmb6rhhkJl PacPPhB2p4LR1vJ0LRfiutzxGuRzq4TvHKjLsoxHUC9rOym3Ne9bxCSWCZWoDPMjMOyl 9JvnAkkmfZ2RosZenIH4LsCxh21S6d8PGXKW1A0Q+JttfAxRQ2jOjRAD3Mnj/Y3cpUmb Vxwg== X-Gm-Message-State: AOJu0YwuM4ySDcGAfPD977dUsukJ+RzwhkgMmPffdJZ8TE6+ODd1z1No FoZetOGJle3BofqsXvdppybKpQB5pOzbth0k7Q0= X-Google-Smtp-Source: AGHT+IGyQAsHSanRpeOW3ExhYoboTHRlINbyUtuzj5/GesB904sSicagSOUcFEkNF1NT0DbQpgpkJkw+c6iw0exRmE0= X-Received: by 2002:a05:6122:16a8:b0:4a4:cd2:5ebe with SMTP id 40-20020a05612216a800b004a40cd25ebemr12311779vkl.12.1701203041863; Tue, 28 Nov 2023 12:24:01 -0800 (PST) MIME-Version: 1.0 References: <20231115163018.1303287-15-ryan.roberts@arm.com> <20231128081742.39204-1-v-songbaohua@oppo.com> <207de995-6d48-41ea-8373-2f9caad9b9c3@arm.com> In-Reply-To: <207de995-6d48-41ea-8373-2f9caad9b9c3@arm.com> From: Barry Song <21cnbao@gmail.com> Date: Wed, 29 Nov 2023 09:23:50 +1300 Message-ID: Subject: Re: [PATCH v2 14/14] arm64/mm: Add ptep_get_and_clear_full() to optimize process teardown To: Ryan Roberts Cc: akpm@linux-foundation.org, andreyknvl@gmail.com, anshuman.khandual@arm.com, ardb@kernel.org, catalin.marinas@arm.com, david@redhat.com, dvyukov@google.com, glider@google.com, james.morse@arm.com, jhubbard@nvidia.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, mark.rutland@arm.com, maz@kernel.org, oliver.upton@linux.dev, ryabinin.a.a@gmail.com, suzuki.poulose@arm.com, vincenzo.frascino@arm.com, wangkefeng.wang@huawei.com, will@kernel.org, willy@infradead.org, yuzenghui@huawei.com, yuzhao@google.com, ziy@nvidia.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 5DA834001C X-Stat-Signature: nho3m69t7fsrewub6crdgmfhdqepcxcb X-Rspam-User: X-HE-Tag: 1701203043-663092 X-HE-Meta: U2FsdGVkX18aKSCi20cFJUApELeHKiGvfHsJGKBW1oVNUefWHyxExoYf8PdrwSebauLribJnL4DMNih8TWX8LYVJKx0c7gQsK+3qZPMNrXNcq3nWchOPAnc/jKXxq8ZDR8DwmfApIssBrU8mKWZoGSvjXhOFdE+Lu/QR0QGQa2Fg+wp3HJDOpOPmwIbiEG4vWEl6GSPwAxQGJ+uLWGu3pC8N/Fj8J0FUtLSoZoqT354e0SO27b8yR08FTCpFOzew94H0nakZ7B/UZiEQO0/etsHsjsFzxB7TWduWIGtckuoRt8VgZ68nIbsKmVb/Yny4qlWAdFPx77BTa1tHE0vDnxWex/mP7FvZYnGe83y2dDXqc9df3K7wDGBUSEGEfQ8tgBPAUQI9OnTnqlDZImLymUiJYYTNFylauygAsr9xsIDbIQgJQtSUZQ5wUS9f0Yj9+gDqWm2+KM5kPUnFIn6ovDjOi0KtSfzp+7MLfzoRABnI2o5UDTPciMVc3hmZUjeRldTpwAcSfeyogbedF3qHEFHd6rQkY4jFKDYoPSOouDSolms33Q3rOi4FyppzHbRBKkQ+qZf2KdrTcrKdq13nME1Hz+2U2y/93XbcRKYLwZsQS3Q9nD/aRizDF5kF8VinxxcYXEaDdtkqYrvEZm0ZJl/oZbe+osdnKc7MBZ3839mdQZ0bljH4JTohNwep1hiKzlNvr7bX4GXG+krAhpp8rRJr7zxU65FcPfGZeaVDCv5J9q7pk40YHA9Leh4L74phJLcv9ng7EwOc5PC+sD776XkPXc4PcMmrgxtfVkcWCtTVTQfkHVH4VoI/JITndtgbEDHVarTuW2PLbq2DeWqYcWE2vo9zz6DVLN1F7/7E90v4OXi26PZtBUFBPv57tMRI7kAKkfq+0eGR/qIcDXpVDXRbLP0orv/n+TXEePEkw5+Q09Vml1PvM6s5LyzDWgXTxALMssL//1GjbqUbbjA JptT6Spe b1rSS5cNnql1LbrqydhGBNkdF48+mELFZQcV7FjVlFH+j5MNPZqY9GCrw1VfPIcwn53Mcbs4zm/lTqK3hHHEVNalAHbMOU1Pc6xQfBxPL6uxJiMwYaAhnfx6vWYAa7Quk2GIKJzWXO8Zy7t12hGRasgtP+cbtxyV6g6OTcm5oCn+QWDBW/8n5pAm1V9mrroJdfAcaSORnmeGF1omTBIsOreZvestNQOJ50uKMPROZ+B8dw5kkBzIVUFMJOpKrtfQ5uXFkl9gj5lF1MIaqDKseN75vZRsXGp7rtQ9t3sTo1MDkjwTJuGFxc1TY3+4/w0F2hgnCCN7csJrcv1YwOGXu8KNicg8iRVeONl5vpgvTfp2/70/e2yZLxh+CrFK8fhRid1PbsVPswwE0lVRE5A32z89fXhxfLCN2GfZwDGRQT+g+H0bO4xSb2mwbYjibK9oA+rRsG4k6XUSsFXNIxw3YRgvJMFr10EF3YYzAvcq0izwhtzkEoYlaXCTba5gbauBBmdNR/DK9IgkgzrBJ0dfX2D0jF9hGo66wPQwSFhjt3Ln+n5NWjr9YjbBYDA== 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: List-Subscribe: List-Unsubscribe: On Wed, Nov 29, 2023 at 12:49=E2=80=AFAM Ryan Roberts wrote: > > On 28/11/2023 08:17, Barry Song wrote: > >> +pte_t contpte_ptep_get_and_clear_full(struct mm_struct *mm, > >> + unsigned long addr, pte_t *ptep) > >> +{ > >> + /* > >> + * When doing a full address space teardown, we can avoid unfoldi= ng the > >> + * contiguous range, and therefore avoid the associated tlbi. Ins= tead, > >> + * just get and clear the pte. The caller is promising to call us= for > >> + * every pte, so every pte in the range will be cleared by the ti= me the > >> + * tlbi is issued. > >> + * > >> + * This approach is not perfect though, as for the duration betwe= en > >> + * returning from the first call to ptep_get_and_clear_full() and= making > >> + * the final call, the contpte block in an intermediate state, wh= ere > >> + * some ptes are cleared and others are still set with the PTE_CO= NT bit. > >> + * If any other APIs are called for the ptes in the contpte block= during > >> + * that time, we have to be very careful. The core code currently > >> + * interleaves calls to ptep_get_and_clear_full() with ptep_get()= and so > >> + * ptep_get() must be careful to ignore the cleared entries when > >> + * accumulating the access and dirty bits - the same goes for > >> + * ptep_get_lockless(). The only other calls we might resonably e= xpect > >> + * are to set markers in the previously cleared ptes. (We shouldn= 't see > >> + * valid entries being set until after the tlbi, at which point w= e are > >> + * no longer in the intermediate state). Since markers are not va= lid, > >> + * this is safe; set_ptes() will see the old, invalid entry and w= ill not > >> + * attempt to unfold. And the new pte is also invalid so it won't > >> + * attempt to fold. We shouldn't see this for the 'full' case any= way. > >> + * > >> + * The last remaining issue is returning the access/dirty bits. T= hat > >> + * info could be present in any of the ptes in the contpte block. > >> + * ptep_get() will gather those bits from across the contpte bloc= k. We > >> + * don't bother doing that here, because we know that the informa= tion is > >> + * used by the core-mm to mark the underlying folio as accessed/d= irty. > >> + * And since the same folio must be underpinning the whole block = (that > >> + * was a requirement for folding in the first place), that inform= ation > >> + * will make it to the folio eventually once all the ptes have be= en > >> + * cleared. This approach means we don't have to play games with > >> + * accumulating and storing the bits. It does mean that any inter= leaved > >> + * calls to ptep_get() may lack correct access/dirty information = if we > >> + * have already cleared the pte that happened to store it. The co= re code > >> + * does not rely on this though. > > > > even without any other threads running and touching those PTEs, this wo= n't survive > > on some hardware. we expose inconsistent CONTPTEs to hardware, this mig= ht result > > No that's not the case; if you read the Arm ARM, the page table is only > considered "misgrogrammed" when *valid* entries within the same contpte b= lock > have different values for the contiguous bit. We are clearing the ptes to= zero > here, which is an *invalid* entry. So if the TLB entry somehow gets inval= idated > (either due to explicit tlbi as you point out below, or due to a concurre= nt TLB > miss which selects our entry for removal to make space for the new incomm= ing > entry), then it gets an access request for an address in our partially cl= eared > contpte block the address will either be: > > A) an address for a pte entry we have already cleared, so its invalid and= it > will fault (and get serialized behind the PTL). > > or > > B) an address for a pte entry we haven't yet cleared, so it will reform a= TLB > entry for the contpte block. But that's ok because the memory still exist= s > because we haven't yet finished clearing the page table and have not yet = issued > the final tlbi. > > > > in crashed firmware even in trustzone, strange&unknown faults to trustz= one we have > > seen on Qualcomm, but for MTK, it seems fine. when you do tlbi on a par= t of PTEs > > with dropped CONT but still some other PTEs have CONT, we make hardware= totally > > confused. > > I suspect this is because in your case you are "misprogramming" the contp= te > block; there are *valid* pte entries within the block that disagree about= the > contiguous bit or about various other fields. In this case some HW TLB de= signs > can do weird things. I suspect in your case, that's resulting in accessin= g bad > memory space and causing an SError, which is trapped by EL3, and the FW i= s > probably just panicking at that point. you are probably right. as we met the SError, we became very very cautious. so anytime when we flush tlb for a CONTPTE, we strictly do it by 1. set all 16 ptes to zero 2. flush the whole 16 ptes in your case, it can be: 1. set pte0 to zero 2. flush pte0 TBH, i have never tried this. but it might be safe according to your description. > > > > > zap_pte_range() has a force_flush when tlbbatch is full: > > > > if (unlikely(__tlb_remove_page(tlb, page, delay= _rmap))) { > > force_flush =3D 1; > > addr +=3D PAGE_SIZE; > > break; > > } > > > > this means you can expose partial tlbi/flush directly to hardware while= some > > other PTEs are still CONT. > > Yes, but that's also possible even if we have a tight loop that clears do= wn the > contpte block; there could still be another core that issues a tlbi while= you're > halfway through that loop, or the HW could happen to evict due to TLB pre= ssure > at any time. The point is, it's safe if you are clearing the pte to an *i= nvalid* > entry. > > > > > on the other hand, contpte_ptep_get_and_clear_full() doesn't need to de= pend > > on fullmm, as long as zap range covers a large folio, we can flush tlbi= for > > those CONTPTEs all together in your contpte_ptep_get_and_clear_full() r= ather > > than clearing one PTE. > > > > Our approach in [1] is we do a flush for all CONTPTEs and go directly t= o the end > > of the large folio: > > > > #ifdef CONFIG_CONT_PTE_HUGEPAGE > > if (pte_cont(ptent)) { > > unsigned long next =3D pte_cont_addr_end(= addr, end); > > > > if (next - addr !=3D HPAGE_CONT_PTE_SIZE)= { > > __split_huge_cont_pte(vma, pte, a= ddr, false, NULL, ptl); > > /* > > * After splitting cont-pte > > * we need to process pte again. > > */ > > goto again_pte; > > } else { > > cont_pte_huge_ptep_get_and_clear(= mm, addr, pte); > > > > tlb_remove_cont_pte_tlb_entry(tlb= , pte, addr); > > if (unlikely(!page)) > > continue; > > > > if (is_huge_zero_page(page)) { > > tlb_remove_page_size(tlb,= page, HPAGE_CONT_PTE_SIZE); > > goto cont_next; > > } > > > > rss[mm_counter(page)] -=3D HPAGE_= CONT_PTE_NR; > > page_remove_rmap(page, true); > > if (unlikely(page_mapcount(page) = < 0)) > > print_bad_pte(vma, addr, = ptent, page); > > > > tlb_remove_page_size(tlb, page, H= PAGE_CONT_PTE_SIZE); > > } > > cont_next: > > /* "do while()" will do "pte++" and "addr= + PAGE_SIZE" */ > > pte +=3D (next - PAGE_SIZE - (addr & PAGE= _MASK))/PAGE_SIZE; > > addr =3D next - PAGE_SIZE; > > continue; > > } > > #endif > > > > this is our "full" counterpart, which clear_flush CONT_PTES pages direc= tly, and > > it never requires tlb->fullmm at all. > > Yes, but you are benefitting from the fact that contpte is exposed to cor= e-mm > and it is special-casing them at this level. I'm trying to avoid that. I am thinking we can even do this while we don't expose CONTPTE. if zap_pte_range meets a large folio and the zap_range covers the whole folio, we can flush all ptes in this folio and jump to the end of this foli= o? i mean if (folio head && range_end > folio_end) { nr =3D folio_nr_page(folio); full_flush_nr_ptes() pte +=3D nr -1; addr +=3D (nr - 1) * basepage size } zap_pte_range is the most frequent behaviour from userspace libc heap as i explained before. libc can call madvise(DONTNEED) the most often. It is crucial to performance. and this way can also help drop your full version by moving to full flushing the whole large folios? and we don't need to depend on fullmm any more? > > I don't think there is any correctness issue here. But there is a problem= with > fragility, as raised by Alistair. I have some ideas on potentially how to= solve > that. I'm going to try to work on it this afternoon and will post if I ge= t some > confidence that it is a real solution. > > Thanks, > Ryan > > > > > static inline pte_t __cont_pte_huge_ptep_get_and_clear_flush(struct mm_= struct *mm, > > unsigned long addr, > > pte_t *ptep, > > bool flush) > > { > > pte_t orig_pte =3D ptep_get(ptep); > > > > CHP_BUG_ON(!pte_cont(orig_pte)); > > CHP_BUG_ON(!IS_ALIGNED(addr, HPAGE_CONT_PTE_SIZE)); > > CHP_BUG_ON(!IS_ALIGNED(pte_pfn(orig_pte), HPAGE_CONT_PTE_NR)); > > > > return get_clear_flush(mm, addr, ptep, PAGE_SIZE, CONT_PTES, flus= h); > > } > > > > [1] https://github.com/OnePlusOSS/android_kernel_oneplus_sm8550/blob/on= eplus/sm8550_u_14.0.0_oneplus11/mm/memory.c#L1539 > > > >> + */ > >> + > >> + return __ptep_get_and_clear(mm, addr, ptep); > >> +} > >> +EXPORT_SYMBOL(contpte_ptep_get_and_clear_full); > >> + > > Thanks Barry >