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 B773FC54EE9 for ; Wed, 7 Sep 2022 17:09:02 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 108CD6B0072; Wed, 7 Sep 2022 13:09:02 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0BA106B0073; Wed, 7 Sep 2022 13:09:02 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E9CA38D0001; Wed, 7 Sep 2022 13:09:01 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id DDA176B0072 for ; Wed, 7 Sep 2022 13:09:01 -0400 (EDT) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id AD5BFC051C for ; Wed, 7 Sep 2022 17:09:01 +0000 (UTC) X-FDA: 79885924482.01.58F12F3 Received: from mail-pj1-f52.google.com (mail-pj1-f52.google.com [209.85.216.52]) by imf28.hostedemail.com (Postfix) with ESMTP id 4A33AC009E for ; Wed, 7 Sep 2022 17:09:01 +0000 (UTC) Received: by mail-pj1-f52.google.com with SMTP id q15-20020a17090a304f00b002002ac83485so11561395pjl.0 for ; Wed, 07 Sep 2022 10:09:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=pxt/RFuMQbA6Ym259IAf87KLR2eKCHM0qyYhrgeqdgA=; b=RH5Xt/EXEeMIUdrasR8CVGu699IQA3OH5oCU34EUOIzU4cFynQd+8PwbmyWMTpdNZr VUk9Iq+PwuQnYlEBKlp43w1CtVd76Zh9gO2Imotc9PNcxjS2p+PeGUUvRioyma/cfR2C 0Mr9DXIhWGKYOn5R26udEbpItwfTtEBljecSg3fzIn1BbCfIzoEOgJ0SEhXOD766St3m z/ATN+6+qTe9DQz0PEm+wec1p9GCTnIssCjpkmtYYSwzytezexuJB9FNa87M6Dl0rBmM FJBjE5/ScIQSit4HJar4QZjulf0xShEWVPfssUrN83ne+muNDMIfT7kiBw3tUo+WbkNR s8Cg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=pxt/RFuMQbA6Ym259IAf87KLR2eKCHM0qyYhrgeqdgA=; b=sGqOUveesa+TZ8jg2N/c061BL3CLvJhW6Ws0Lbpmf7KmyOYiyoxQ5DwpGXchcuLlKI SWFfkufgc+T/cGO48wvmx4g9zydDXSZzEwpBCO2NPxiFv34Cten5AxXY4vzOlDiKoiUz FMBEm8OkRQJ0Dc4GeX9oqrlm1C/ORZjV/gJ3kOPn+bXlBuhv+5ECzWjO14j9BTy0V16i VRAKLHuAvMSw5iWa8mq+nzrro2XOtvsz2nFh6ihJA5/WRfvrHeQqmNLhva88Zg8NCohY bX4vq7P8K3b2iWjH13RlRgu3IKzWp5ulSSv6aU2Gx43F4ZVlFp6oRltGgPTZXPwVvhWF DSEw== X-Gm-Message-State: ACgBeo3RIxHoG4WH5aeGouv82ndtez+bVEzdftRbRrYiKedNvlaoZo9H YtoJrZxiijqtZC7+SQ7l8FfYuU7NMV6cH0aJiws= X-Google-Smtp-Source: AA6agR60kaYZTbk+Nbqosr/4E9psZ74b5dkLnw8fsaR0M8g6PFBg55BuXU2N/m4LGKhTLP/1ygrx1JLOQPafb5Lv3nE= X-Received: by 2002:a17:902:be03:b0:175:6397:9425 with SMTP id r3-20020a170902be0300b0017563979425mr4688038pls.26.1662570540204; Wed, 07 Sep 2022 10:09:00 -0700 (PDT) MIME-Version: 1.0 References: <20220901222707.477402-1-shy828301@gmail.com> <87ilm2jj4t.fsf@linux.ibm.com> <92fe7b10-cbcf-6fdb-af23-4cb2f314e612@linux.ibm.com> In-Reply-To: <92fe7b10-cbcf-6fdb-af23-4cb2f314e612@linux.ibm.com> From: Yang Shi Date: Wed, 7 Sep 2022 10:08:47 -0700 Message-ID: Subject: Re: [PATCH] mm: gup: fix the fast GUP race against THP collapse To: Aneesh Kumar K V Cc: Peter Xu , david@redhat.com, kirill.shutemov@linux.intel.com, jhubbard@nvidia.com, jgg@nvidia.com, hughd@google.com, akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, linuxppc-dev Content-Type: text/plain; charset="UTF-8" ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1662570541; a=rsa-sha256; cv=none; b=qV6uYB+td4zX9dxgQnMAKbprVx7k/VdlJ+vLkbFyZGGCTblZf/uHNOmDlPX2v0YFAQu+ee Y0e07gf3yWmjM2UqftD6gYMM3WzUVfiAgFWnXjknE7sg7dwbWAJBstmBDeVRjYjYT9TPQW 7ShSEbIeQ64Kf4m92vsONMhs9KAA+mg= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b="RH5Xt/EX"; spf=pass (imf28.hostedemail.com: domain of shy828301@gmail.com designates 209.85.216.52 as permitted sender) smtp.mailfrom=shy828301@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1662570541; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=pxt/RFuMQbA6Ym259IAf87KLR2eKCHM0qyYhrgeqdgA=; b=R0drLQtaq1oEJmT1nV4tx+Z2X4xlGmdw8qzqThfFJ4y1YqxLZZ5q/hX2MLJtyWWmyBgt/8 v8snHk41sJIKX0LZNGagGpQJgwp01ugCsWip2bPXd0DyoUesfKMCEFJ/4rNy35nvN8Ly44 fFSQcx+E19yFr72mwCF3KMSaO2FmSik= X-Rspam-User: X-Rspamd-Queue-Id: 4A33AC009E Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b="RH5Xt/EX"; spf=pass (imf28.hostedemail.com: domain of shy828301@gmail.com designates 209.85.216.52 as permitted sender) smtp.mailfrom=shy828301@gmail.com; dmarc=pass (policy=none) header.from=gmail.com X-Stat-Signature: it1a8b57u4mjijwrx1kktmgb1k4qd4fz X-Rspamd-Server: rspam03 X-HE-Tag: 1662570541-983186 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, Sep 6, 2022 at 9:51 PM Aneesh Kumar K V wrote: > > On 9/7/22 12:37 AM, Yang Shi wrote: > > On Mon, Sep 5, 2022 at 1:56 AM Aneesh Kumar K.V > > wrote: > >> > >> Yang Shi writes: > >> > >>> > >>> On Fri, Sep 2, 2022 at 9:00 AM Peter Xu wrote: > >>>> > >>>> On Thu, Sep 01, 2022 at 04:50:45PM -0700, Yang Shi wrote: > >>>>> On Thu, Sep 1, 2022 at 4:26 PM Peter Xu wrote: > >>>>>> > >>>>>> Hi, Yang, > >>>>>> > >>>>>> On Thu, Sep 01, 2022 at 03:27:07PM -0700, Yang Shi wrote: > >>>>>>> Since general RCU GUP fast was introduced in commit 2667f50e8b81 ("mm: > >>>>>>> introduce a general RCU get_user_pages_fast()"), a TLB flush is no longer > >>>>>>> sufficient to handle concurrent GUP-fast in all cases, it only handles > >>>>>>> traditional IPI-based GUP-fast correctly. > >>>>>> > >>>>>> If TLB flush (or, IPI broadcasts) used to work to protect against gup-fast, > >>>>>> I'm kind of confused why it's not sufficient even if with RCU gup? Isn't > >>>>>> that'll keep working as long as interrupt disabled (which current fast-gup > >>>>>> will still do)? > >>>>> > >>>>> Actually the wording was copied from David's commit log for his > >>>>> PageAnonExclusive fix. My understanding is the IPI broadcast still > >>>>> works, but it may not be supported by all architectures and not > >>>>> preferred anymore. So we should avoid depending on IPI broadcast IIUC. > >>>>> > >>>>>> > >>>>>> IIUC the issue is you suspect not all archs correctly implemented > >>>>>> pmdp_collapse_flush(), or am I wrong? > >>>>> > >>>>> This is a possible fix, please see below for details. > >>>>> > >>>>>> > >>>>>>> On architectures that send > >>>>>>> an IPI broadcast on TLB flush, it works as expected. But on the > >>>>>>> architectures that do not use IPI to broadcast TLB flush, it may have > >>>>>>> the below race: > >>>>>>> > >>>>>>> CPU A CPU B > >>>>>>> THP collapse fast GUP > >>>>>>> gup_pmd_range() <-- see valid pmd > >>>>>>> gup_pte_range() <-- work on pte > >>>>>>> pmdp_collapse_flush() <-- clear pmd and flush > >>>>>>> __collapse_huge_page_isolate() > >>>>>>> check page pinned <-- before GUP bump refcount > >>>>>>> pin the page > >>>>>>> check PTE <-- no change > >>>>>>> __collapse_huge_page_copy() > >>>>>>> copy data to huge page > >>>>>>> ptep_clear() > >>>>>>> install huge pmd for the huge page > >>>>>>> return the stale page > >>>>>>> discard the stale page > >>>>>>> > >>>>>>> The race could be fixed by checking whether PMD is changed or not after > >>>>>>> taking the page pin in fast GUP, just like what it does for PTE. If the > >>>>>>> PMD is changed it means there may be parallel THP collapse, so GUP > >>>>>>> should back off. > >>>>>> > >>>>>> Could the race also be fixed by impl pmdp_collapse_flush() correctly for > >>>>>> the archs that are missing? Do you know which arch(s) is broken with it? > >>>>> > >>>>> Yes, and this was suggested by me in the first place, but per the > >>>>> suggestion from John and David, this is not the preferred way. I think > >>>>> it is because: > >>>>> > >>>>> Firstly, using IPI to serialize against fast GUP is not recommended > >>>>> anymore since fast GUP does check PTE then back off so we should avoid > >>>>> it. > >>>>> Secondly, if checking PMD then backing off could solve the problem, > >>>>> why do we still need broadcast IPI? It doesn't sound performant. > >>>>> > >>>>>> > >>>>>> It's just not clear to me whether this patch is an optimization or a fix, > >>>>>> if it's a fix whether the IPI broadcast in ppc pmdp_collapse_flush() would > >>>>>> still be needed. > >>>>> > >>>>> It is a fix and the fix will make IPI broadcast not useful anymore. > >>>> > >>>> How about another patch to remove the ppc impl too? Then it can be a two > >>>> patches series. > >>> > >>> BTW, I don't think we could remove the ppc implementation since it is > >>> different from the generic pmdp_collapse_flush(), particularly for the > >>> hash part IIUC. > >>> > >>> The generic version calls flush_tlb_range() -> hash__flush_tlb_range() > >>> for hash, but the hash call is actually no-op. The ppc version calls > >>> hash__pmdp_collapse_flush() -> flush_tlb_pmd_range(), which does > >>> something useful. > >>> > >> > >> We should actually rename flush_tlb_pmd_range(). It actually flush the > >> hash page table entries. > >> > >> I will do the below patch for ppc64 to clarify this better > > > > Thanks, Aneesh. It looks more readable. A follow-up question, I think > > we could remove serialize_against_pte_lookup(), which just issues IPI > > broadcast to run a dummy function. This IPI should not be needed > > anymore with my patch. Of course, we need to keep the memory barrier. > > > > > For radix translation yes. For hash we still need that. W.r.t memory barrier, > radix do use radix__flush_tlb_collapsed_pmd() which does a tlb invalidate. > IIUC that will enfocre the required memory barrier there. So you should be able > to just remove > > modified arch/powerpc/mm/book3s64/radix_pgtable.c > @@ -937,15 +937,6 @@ pmd_t radix__pmdp_collapse_flush(struct vm_area_struct *vma, unsigned long addre > pmd = *pmdp; > pmd_clear(pmdp); > > - /* > - * pmdp collapse_flush need to ensure that there are no parallel gup > - * walk after this call. This is needed so that we can have stable > - * page ref count when collapsing a page. We don't allow a collapse page > - * if we have gup taken on the page. We can ensure that by sending IPI > - * because gup walk happens with IRQ disabled. > - */ > - serialize_against_pte_lookup(vma->vm_mm); > - > radix__flush_tlb_collapsed_pmd(vma->vm_mm, address); > > return pmd; > > in your patch. This will also consolidate all the related changes together. Thanks, Aneesh. It may be better to have the ppc cleanup in a separate patch. > > >> > >> diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h b/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h > >> index 8b762f282190..fd30fa20c392 100644 > >> --- a/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h > >> +++ b/arch/powerpc/include/asm/book3s/64/tlbflush-hash.h > >> @@ -112,13 +112,12 @@ static inline void hash__flush_tlb_kernel_range(unsigned long start, > >> > >> struct mmu_gather; > >> extern void hash__tlb_flush(struct mmu_gather *tlb); > >> -void flush_tlb_pmd_range(struct mm_struct *mm, pmd_t *pmd, unsigned long addr); > >> > >> #ifdef CONFIG_PPC_64S_HASH_MMU > >> /* Private function for use by PCI IO mapping code */ > >> extern void __flush_hash_table_range(unsigned long start, unsigned long end); > >> -extern void flush_tlb_pmd_range(struct mm_struct *mm, pmd_t *pmd, > >> - unsigned long addr); > >> +extern void flush_hash_table_pmd_range(struct mm_struct *mm, pmd_t *pmd, > >> + unsigned long addr); > >> #else > >> static inline void __flush_hash_table_range(unsigned long start, unsigned long end) { } > >> #endif > >> diff --git a/arch/powerpc/mm/book3s64/hash_pgtable.c b/arch/powerpc/mm/book3s64/hash_pgtable.c > >> index ae008b9df0e6..f30131933a01 100644 > >> --- a/arch/powerpc/mm/book3s64/hash_pgtable.c > >> +++ b/arch/powerpc/mm/book3s64/hash_pgtable.c > >> @@ -256,7 +256,7 @@ pmd_t hash__pmdp_collapse_flush(struct vm_area_struct *vma, unsigned long addres > >> * the __collapse_huge_page_copy can result in copying > >> * the old content. > >> */ > >> - flush_tlb_pmd_range(vma->vm_mm, &pmd, address); > >> + flush_hash_table_pmd_range(vma->vm_mm, &pmd, address); > >> return pmd; > >> } > >> > >> diff --git a/arch/powerpc/mm/book3s64/hash_tlb.c b/arch/powerpc/mm/book3s64/hash_tlb.c > >> index eb0bccaf221e..a64ea0a7ef96 100644 > >> --- a/arch/powerpc/mm/book3s64/hash_tlb.c > >> +++ b/arch/powerpc/mm/book3s64/hash_tlb.c > >> @@ -221,7 +221,7 @@ void __flush_hash_table_range(unsigned long start, unsigned long end) > >> local_irq_restore(flags); > >> } > >> > >> -void flush_tlb_pmd_range(struct mm_struct *mm, pmd_t *pmd, unsigned long addr) > >> +void flush_hash_table_pmd_range(struct mm_struct *mm, pmd_t *pmd, unsigned long addr) > >> { > >> pte_t *pte; > >> pte_t *start_pte; > >> >