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 3CF88ECAAD3 for ; Thu, 1 Sep 2022 23:51:00 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A33768D0008; Thu, 1 Sep 2022 19:50:59 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 9E0CB8D0002; Thu, 1 Sep 2022 19:50:59 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 834AD8D0008; Thu, 1 Sep 2022 19:50:59 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 6F00A8D0002 for ; Thu, 1 Sep 2022 19:50:59 -0400 (EDT) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 422B916088D for ; Thu, 1 Sep 2022 23:50:59 +0000 (UTC) X-FDA: 79865164638.14.2D8A016 Received: from mail-pf1-f177.google.com (mail-pf1-f177.google.com [209.85.210.177]) by imf03.hostedemail.com (Postfix) with ESMTP id EE2352004D for ; Thu, 1 Sep 2022 23:50:58 +0000 (UTC) Received: by mail-pf1-f177.google.com with SMTP id y141so315888pfb.7 for ; Thu, 01 Sep 2022 16:50:58 -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=rgfh2ddi8n0E0NRuyd4hmH9/qJacY3Iem5NMVyw+DzY=; b=F6m5E4BZTfNRuTo02s1snzGsFaJw2Au+1CHu6ImumiapshQiYnkVbAuEooOafgPMkF miIZPjYe1LUhkfPf6IuTdUDpRGmuGwBevKrexovn439U5OzXIL60qj+V40VHt9BhKer9 B1ui3yqdeEelOVltF11FjrnnzGCzl6ZxDGmCHXCF9WGY5ihIm4CpGfT3IDOvs3OqIpF8 2hHDqrwheGlq2FDDv6LOOGL32T3DWNb/gdaZnrKx7AFnHtBWvaJy8NN2WOq1Akcv+NAj +lJ6ukbtqgf+XeB1L1H2JubBJ/eAU/UxALqdtHHwBKqAJRVq0dSgfKDcG+tFNebulqVi 7skA== 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=rgfh2ddi8n0E0NRuyd4hmH9/qJacY3Iem5NMVyw+DzY=; b=z8LYzCmRLIJzMTxdmgfWST3rqH4SbyYw8CEA1R+/+UUFG8AsafuG42AatBGBM0bTLv z6NJjDHY2kpPWl/qIIyVJ7JCUDR1l/P2EUB56KoHwygWLHjVSu2jjHYdrtIuYcPsFW3r DnBfNixCm7TSjJHucswGW4UP5jb5IOv24VgmjRpnifHeTO9dUeOmclvpQkjrxHOKJbr2 tFSCp7IC47B8AspYDM8+hLR+oa/6SkY/mzb6JqSqE1RvL7RPRZq/uMxlen5kEBqjzwfT OPZqpjT3QQ8iUYc4qdc4hkhZmAdpgqlusL1nrqHFHAaS3pGAiRPCZ0oVvVJmzGqe3ln3 AeWg== X-Gm-Message-State: ACgBeo2z4wrmIN/hVb7rOFIggErKb3InDsKr/2zIiHBeCbiTItcMoAGo bYGRpWQB867pROwIfFnT6G+/chePDsVHwipQuzs= X-Google-Smtp-Source: AA6agR5eLHrYTjwWmVpkp/WauU991H/b0j9j6TTL8fxRaKMNhd3ZO/oRArhpKmBdAGABuHvBKeO/bbFBz9D/FdS07cw= X-Received: by 2002:a63:de01:0:b0:42b:31f7:b6ea with SMTP id f1-20020a63de01000000b0042b31f7b6eamr27634226pgg.587.1662076257912; Thu, 01 Sep 2022 16:50:57 -0700 (PDT) MIME-Version: 1.0 References: <20220901222707.477402-1-shy828301@gmail.com> In-Reply-To: From: Yang Shi Date: Thu, 1 Sep 2022 16:50:45 -0700 Message-ID: Subject: Re: [PATCH] mm: gup: fix the fast GUP race against THP collapse To: Peter Xu Cc: 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 Content-Type: text/plain; charset="UTF-8" ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1662076258; a=rsa-sha256; cv=none; b=ZJ3MpvGcHYzAKbC0tL/6Uds3DhQJkvV1Fmf3GIzLyjak69gKJ5CSfPsDewHJe+TAMB8LmL ysf/JaT+QVSq3UuT6VK1RR9JGNhHRbarFFLQv2t9oGjW9uqq36mM7Q84E2jVEGGfPdTIDw IDKmS2K4uq9aqvBRfIZt89PE+73knnc= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=F6m5E4BZ; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf03.hostedemail.com: domain of shy828301@gmail.com designates 209.85.210.177 as permitted sender) smtp.mailfrom=shy828301@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1662076258; 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=rgfh2ddi8n0E0NRuyd4hmH9/qJacY3Iem5NMVyw+DzY=; b=nMWvjbCHWNZpJEJvbQfw9edH+bEXxYKsFOBC34M4cqPMDmS3tonN0TRd1ZmjOhE4cVhWZo ALvwJ+X/3Cj0IDXBTT/IgayrDecppLtliwYu82cF/IlxPhIWrW7YV7uKJOidpWqrmiAuIM p8J+a3pVe+08Wa1o4JaPt/QnoDM6mA4= X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: EE2352004D Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=F6m5E4BZ; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf03.hostedemail.com: domain of shy828301@gmail.com designates 209.85.210.177 as permitted sender) smtp.mailfrom=shy828301@gmail.com X-Rspam-User: X-Stat-Signature: f8fc64mz85njagzu4huax1dutpsc9kjw X-HE-Tag: 1662076258-276272 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 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. > > Thanks, > > > > > Also update the stale comment about serializing against fast GUP in > > khugepaged. > > > > Fixes: 2667f50e8b81 ("mm: introduce a general RCU get_user_pages_fast()") > > Signed-off-by: Yang Shi > > --- > > mm/gup.c | 30 ++++++++++++++++++++++++------ > > mm/khugepaged.c | 10 ++++++---- > > 2 files changed, 30 insertions(+), 10 deletions(-) > > > > diff --git a/mm/gup.c b/mm/gup.c > > index f3fc1f08d90c..4365b2811269 100644 > > --- a/mm/gup.c > > +++ b/mm/gup.c > > @@ -2380,8 +2380,9 @@ static void __maybe_unused undo_dev_pagemap(int *nr, int nr_start, > > } > > > > #ifdef CONFIG_ARCH_HAS_PTE_SPECIAL > > -static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end, > > - unsigned int flags, struct page **pages, int *nr) > > +static int gup_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr, > > + unsigned long end, unsigned int flags, > > + struct page **pages, int *nr) > > { > > struct dev_pagemap *pgmap = NULL; > > int nr_start = *nr, ret = 0; > > @@ -2423,7 +2424,23 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end, > > goto pte_unmap; > > } > > > > - if (unlikely(pte_val(pte) != pte_val(*ptep))) { > > + /* > > + * THP collapse conceptually does: > > + * 1. Clear and flush PMD > > + * 2. Check the base page refcount > > + * 3. Copy data to huge page > > + * 4. Clear PTE > > + * 5. Discard the base page > > + * > > + * So fast GUP may race with THP collapse then pin and > > + * return an old page since TLB flush is no longer sufficient > > + * to serialize against fast GUP. > > + * > > + * Check PMD, if it is changed just back off since it > > + * means there may be parallel THP collapse. > > + */ > > + if (unlikely(pmd_val(pmd) != pmd_val(*pmdp)) || > > + unlikely(pte_val(pte) != pte_val(*ptep))) { > > gup_put_folio(folio, 1, flags); > > goto pte_unmap; > > } > > @@ -2470,8 +2487,9 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end, > > * get_user_pages_fast_only implementation that can pin pages. Thus it's still > > * useful to have gup_huge_pmd even if we can't operate on ptes. > > */ > > -static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end, > > - unsigned int flags, struct page **pages, int *nr) > > +static int gup_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr, > > + unsigned long end, unsigned int flags, > > + struct page **pages, int *nr) > > { > > return 0; > > } > > @@ -2791,7 +2809,7 @@ static int gup_pmd_range(pud_t *pudp, pud_t pud, unsigned long addr, unsigned lo > > if (!gup_huge_pd(__hugepd(pmd_val(pmd)), addr, > > PMD_SHIFT, next, flags, pages, nr)) > > return 0; > > - } else if (!gup_pte_range(pmd, addr, next, flags, pages, nr)) > > + } else if (!gup_pte_range(pmd, pmdp, addr, next, flags, pages, nr)) > > return 0; > > } while (pmdp++, addr = next, addr != end); > > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > > index 2d74cf01f694..518b49095db3 100644 > > --- a/mm/khugepaged.c > > +++ b/mm/khugepaged.c > > @@ -1049,10 +1049,12 @@ static int collapse_huge_page(struct mm_struct *mm, unsigned long address, > > > > pmd_ptl = pmd_lock(mm, pmd); /* probably unnecessary */ > > /* > > - * After this gup_fast can't run anymore. This also removes > > - * any huge TLB entry from the CPU so we won't allow > > - * huge and small TLB entries for the same virtual address > > - * to avoid the risk of CPU bugs in that area. > > + * This removes any huge TLB entry from the CPU so we won't allow > > + * huge and small TLB entries for the same virtual address to > > + * avoid the risk of CPU bugs in that area. > > + * > > + * Parallel fast GUP is fine since fast GUP will back off when > > + * it detects PMD is changed. > > */ > > _pmd = pmdp_collapse_flush(vma, address, pmd); > > spin_unlock(pmd_ptl); > > -- > > 2.26.3 > > > > -- > Peter Xu >